-
Notifications
You must be signed in to change notification settings - Fork 3k
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
ssl: Error server options when no certs #7918
ssl: Error server options when no certs #7918
Conversation
CT Test Results 2 files 66 suites 47m 41s ⏱️ Results for commit 7a07239. ♻️ This comment has been updated with latest results. To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass. See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally. Artifacts// Erlang/OTP Github Action Bot |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I briefly read it and think it is ok.
If it is potentially incompatible behavior, please check how unclear are we about it in docs. Should we improve something, so we can rely on it upon questions arrive?
NoCertOrKeys = Cert == undefined orelse Key == undefined andalso | ||
CertsKeys == undefined, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do something with clarity here. More parenthesis maybe? IMHO this expects too much knowledge and being awake from the reader ;-)
lib/ssl/src/ssl.erl
Outdated
CiphersSet = sets:from_list(Ciphers, [{version,2}]), | ||
case sets:is_disjoint(Anonymous, CiphersSet) of | ||
false -> ok; | ||
true -> option_error(certs_keys, no_cert_or_key_given) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably we should say no_cert_and_key_given as we need both.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or maybe both_cert_and_key_required
When running a ssl server the user must provide cert and key or use an anonymous cipher in tls1.2. Otherwise no connection will succeed. Add an option check so that this is dectected earlier, and gives the user an appropriate error instead of just failing each connection attempt. To keep backwards compatibility the check is only done in handshake, since it is allowed to use an empty (or minimal) option list in ssl:listen and provide the options in handshake later.
51ad60f
to
7a07239
Compare
When running a ssl server the user must provide cert and key or use an anonymous cipher in tlsv1.2. Otherwise no connection will succeed.
Add an option check so that this is detected earlier, and gives the user an appropriate error instead of just failing each connection attempt.
To keep backwards compatibility the check is only done in handshake, since it is allowed to use an empty (or minimal) option list in ssl:listen/2 and provide the options in handshake later.
Solves #7493