Skip to content

ssl: Relax requierments on keyfile #10221

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Sep 23, 2025
Merged

ssl: Relax requierments on keyfile #10221

merged 1 commit into from
Sep 23, 2025

Conversation

@IngelaAndin
Copy link
Contributor

PR-10046 put to hard requierments on keyfile content.

Closes #10217
Closes #10212

@IngelaAndin IngelaAndin requested review from dgud and u3s September 18, 2025 15:11
@IngelaAndin IngelaAndin self-assigned this Sep 18, 2025
@IngelaAndin IngelaAndin added team:PS Assigned to OTP team PS testing currently being tested, tag is used by OTP internal CI labels Sep 18, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Sep 18, 2025 •
edited
Loading

CT Test Results

    2 files     66 suites   26m 52s ⏱️
  814 tests   770 ✅  44 💤 0 ❌
4 154 runs  3 305 ✅ 849 💤 0 ❌

Results for commit c492385.

♻️ 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

@IngelaAndin IngelaAndin mentioned this pull request Sep 18, 2025
Closed
@sg2342
Copy link
Contributor

sg2342 commented Sep 18, 2025

while this solves the unexpected_content error in ssl:listen/3 it is not sufficient to make ssl work with certificate and private key in the same PEM file:

Erlang/OTP 28 [erts-16.1] [source] [64-bit] [smp:16:16] [ds:16:16:10] [async-threads:1] [jit:ns]

Eshell V16.1 (press Ctrl+G to abort, type help(). for help)
1> ssl:start().
ok
2> Self=self().
<0.83.0>
3> spawn(fun() -> Self ! {server, ssl:handshake(element(2, ssl:transport_accept(element(2, ssl:listen(9999, [{certfile, "lib/ssl/test/ssl_api_SUITE_data/cert_and_key.pem"}])))))} end).
<0.111.0>
4> ssl:connect("localhost", 9999, [{verify, verify_none}]).
{error,closed}
5> flush().
Shell got {server,
              {error,
                  {options,
                      {certfile,
                          "lib/ssl/test/ssl_api_SUITE_data/cert_and_key.pem",
                          no_certs}}}}
ok

tracing ssl_pem_cache via [{debug, [trace]}] shows

 *DBG* ssl_pem_cache got cast {cache_pem,
                                 <<"lib/ssl/test/ssl_api_SUITE_data/cert_and_key.pem">>,
                                 [{'Certificate',
                                      <<48,130,15,148,48,130,6,10,160,3,2,1,2,
....
....
                                        62,63,73,109,147,155,160,182,187,210,
                                        218,229,235,238,239,16,31,35,45,110,
                                        151,194,196,223,243,244,0,0,0,0,0,0,0,
                                        0,0,0,0,0,0,23,38,56,67>>,
                                      not_encrypted},
                                  {'PrivateKeyInfo',
                                      <<48,52,2,1,0,48,11,6,9,96,134,72,1,101,
                                        3,4,3,17,4,34,128,32,0,1,2,3,4,5,6,7,8,
                                        9,10,11,12,13,14,15,16,17,18,19,20,21,
                                        22,23,24,25,26,27,28,29,30,31>>,
                                      not_encrypted}]}

followed by

*DBG* ssl_pem_cache new state {state,ssl_pem_cache,1758222778,120000}
*DBG* ssl_pem_cache got cast {cache_pem,
                                 <<"lib/ssl/test/ssl_api_SUITE_data/cert_and_key.pem">>,
                                 [{'PrivateKeyInfo',
                                      <<48,52,2,1,0,48,11,6,9,96,134,72,1,101,
                                        3,4,3,17,4,34,128,32,0,1,2,3,4,5,6,7,8,
                                        9,10,11,12,13,14,15,16,17,18,19,20,21,
                                        22,23,24,25,26,27,28,29,30,31>>,
                                      not_encrypted}]}

so the second cache_pem cast makes it that the pem_cache only contains the key

sg2342
sg2342 reviewed Sep 18, 2025
View reviewed changes
@IngelaAndin IngelaAndin force-pushed the ingela/ssl/handle-key-file/OTP-19780 branch from 7ad5131 to 868c8bc Compare September 18, 2025 20:16
@IngelaAndin
Copy link
Contributor Author

Good catch, but I will perhaps make an other cert key pair to have a test case that can be run on also with older versions of cryptolib.

sg2342
sg2342 reviewed Sep 18, 2025
View reviewed changes
@IngelaAndin
ssl: Relax requierments on keyfile
c492385 
PR-10046 put to hard requierments on keyfile content.

Closes erlang#10217
Closes erlang#10212
@IngelaAndin IngelaAndin force-pushed the ingela/ssl/handle-key-file/OTP-19780 branch from 868c8bc to c492385 Compare September 19, 2025 08:20
u3s
u3s approved these changes Sep 19, 2025
View reviewed changes
lib/ssl/src/ssl_config.erl
PKey =:= 'PrivateKeyInfo'
],
{ok, PemEntries} = ssl_manager:cache_pem_file(KeyFile, DbHandle),
[PemEntry] = key_entry(PemEntries),
Copy link
Contributor

Choose a reason for hiding this comment

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

what if more private keys are provided in same file? you might get longer list and match failure here.
should we somehow prepare for that better? have a testcase?

you got it covered below. should it be covered here as well?
maybe it is validated earlier and can't happen ... am I missing sth?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It only makes sense to have one key. This is executed in connection processes while the other case "below" is executed before any ssl connection processes exist to enable user processes to find out about file errors from the listen call. On the server side more than one key will have been caught by the listen call (tradeoff here between early failure and some extra overhead). On client side only thing that happens is that some extra entries in the cache for a little while, but connect will return same error as previously.

@IngelaAndin IngelaAndin merged commit 12f2f6a into erlang:maint Sep 23, 2025
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@u3s u3s u3s approved these changes

@dgud dgud Awaiting requested review from dgud

+1 more reviewer

@sg2342 sg2342 sg2342 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@IngelaAndin IngelaAndin

Labels

team:PS Assigned to OTP team PS testing currently being tested, tag is used by OTP internal CI

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@IngelaAndin @sg2342 @u3s