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

ssl: Error server options when no certs #7918

Merged

Conversation

dgud
Copy link
Contributor

@dgud dgud commented Nov 30, 2023

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

@dgud dgud added the team:PS Assigned to OTP team PS label Nov 30, 2023
@dgud dgud self-assigned this Nov 30, 2023
Copy link
Contributor

github-actions bot commented Nov 30, 2023

CT Test Results

       2 files       66 suites   47m 41s ⏱️
   772 tests    739 ✔️   33 💤 0
3 683 runs  2 926 ✔️ 757 💤 0

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

@dgud dgud added the testing currently being tested, tag is used by OTP internal CI label Dec 1, 2023
@dgud dgud requested a review from u3s December 4, 2023 08:46
@u3s u3s linked an issue Dec 4, 2023 that may be closed by this pull request
Copy link
Contributor

@u3s u3s left a 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?

Comment on lines +2202 to +2203
NoCertOrKeys = Cert == undefined orelse Key == undefined andalso
CertsKeys == undefined,
Copy link
Contributor

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 ;-)

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)
Copy link
Contributor

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.

Copy link
Contributor

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.
@dgud dgud force-pushed the dgud/ssl/server-option-check/GH-7493/OTP-18887 branch from 51ad60f to 7a07239 Compare December 5, 2023 14:44
@dgud dgud merged commit 843e62b into erlang:master Dec 6, 2023
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team:PS Assigned to OTP team PS testing currently being tested, tag is used by OTP internal CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TLS 1.2 handshake fails
3 participants