Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tighten up SSL_get1_supported_ciphers() docs #4284

Closed
wants to merge 3 commits into from

Conversation

kaduk
Copy link
Contributor

@kaduk kaduk commented Aug 29, 2017

This function is really emulating what would happen in client mode,
and does not necessarily reflect what is usable for a server SSL.
Make this a bit more explicit, and do some wordsmithing while here.

Checklist
  • documentation is added or updated

This function is really emulating what would happen in client mode,
and does not necessarily reflect what is usable for a server SSL.
Make this a bit more explicit, and do some wordsmithing while here.
differ from the list of acceptable ciphers when B<ssl> is the server side
of a connection, as can occur when there is a
hole in the list of supported protocols or the configured certificates and
DH parameters limit the usable ciphers.
Copy link
Contributor

@dot-asm dot-asm Aug 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As non-native English speaker I find this unnecessary complex. Since Single sentence in place of three, that suggests stronger causality between client and server lists than there is, which with implied subject [I mean what's subject in "as can occur"?]...

Per Andy's review

[to be squashed]
[skip ci]
the list of ciphers that would be acceptable when acting as a server.
For example, additional ciphers may be usable by a server if there is
a hole in the list of supported protocols, and some ciphers may not be
usable by a server if there is not a corresponding certificate configured.
Copy link
Contributor

@dot-asm dot-asm Aug 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit uncomfortable commenting on language [beyond "I find it unnecessary complex"], let alone actually approving commits requests. But does "if there is not a corresponding certificate configured" sound right? Wouldn't "if there is no suitable certificate configured" reflect circumstances better? And may I suggest "gap" instead of "hole", perhaps?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am happy to hear your comments on language, and also to respect your reluctance to approve such commits.
Both "suitable" and "gap" sound like improvements to me; thanks.

@richsalz richsalz added the approval: review pending This pull request needs review by a committer label Aug 30, 2017
[to be squashed]
[skip ci]
Copy link
Contributor

@dot-asm dot-asm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I'm reluctant to approve it, it's taking unreasonably long, so here is goes...

@dot-asm
Copy link
Contributor

dot-asm commented Sep 1, 2017

Oh! I expect that commits will be squashed upon merge :-)

@dot-asm dot-asm added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Sep 1, 2017
levitte pushed a commit that referenced this pull request Sep 1, 2017
This function is really emulating what would happen in client mode,
and does not necessarily reflect what is usable for a server SSL.
Make this a bit more explicit, and do some wordsmithing while here.

Reviewed-by: Andy Polyakov <appro@openssl.org>
(Merged from #4284)
@kaduk
Copy link
Contributor Author

kaduk commented Sep 1, 2017

squashed to e65dfa4 and merged; thanks

@kaduk kaduk closed this Sep 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval: done This pull request has the required number of approvals
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants