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

Load certificates from systems keychain on darwin #8844

Open
wants to merge 1 commit into
base: maint
Choose a base branch
from

Conversation

starbelly
Copy link
Contributor

@starbelly starbelly commented Sep 22, 2024

The systems root keychain contains well know root certificates, yet is non-modifiable. As such, internal CA certificates (both root and intermediate) tend to get installed into the systems keychain in the context of an private organization. Not loading certs from this keychain results in differing behavior from other tools (e.g., openssl, curl, etc.). This commit changes to that so that ssl in conjunction with public key just works in such environments.

Resolves #8813

The systems root keychain contains well know root certificates,
yet is non-modifiable. As such, internal CA certificates (both root
and intermediate) tend to get installed into the systems keychain
in the context of an private organization. Not loading certs
from this keychain results in differing behavior from other tools
(e.g., openssl, curl, etc.). This commit changes to that so that
ssl in conjunction with public key just works in such environments.
Copy link
Contributor

github-actions bot commented Sep 22, 2024

CT Test Results

  2 files   17 suites   5m 15s ⏱️
284 tests 282 ✅ 2 💤 0 ❌
300 runs  298 ✅ 2 💤 0 ❌

Results for commit dcc396a.

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

@starbelly
Copy link
Contributor Author

Note that a test in the appropriate test suite has not been added, I wasn't sure how that would play out since an import call would be needed a may require a password.

To manually test this one simply needs to install a certificate into the systems keychain, startup erl, then verify the presence of the installed cert via public_key:cacerts_get/0.

@starbelly
Copy link
Contributor Author

Note that a test in the appropriate test suite has not been added, I wasn't sure how that would play out since an import call would be needed a may require a password.

To manually test this one simply needs to install a certificate into the systems keychain, startup erl, then verify the presence of the installed cert via public_key:cacerts_get/0.

Confirmed, this can not be easily tested in a suite.

@IngelaAndin IngelaAndin added the team:PS Assigned to OTP team PS label Sep 23, 2024
@dgud dgud self-assigned this Sep 23, 2024
case get_darwin_certs(SystemKeyChainFile) of
{ok, Bin2} ->
decode_result(<<Bin1/binary, Bin2/binary>>);
Err ->
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this fail?
And if so should we just decode the SystemRoot bin which we already got and return that part which was successful?

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 thought about this, the system keychain is available to all users, so if it did fail, your system has serious problems :) That said, if we wanted to proceed anyway, we could log an error and proceed, wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

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

I thought that better safe than sorry, that will keep backward compatibility and since we can't test it, it might be a good idea. And I don't know anything MacOS so I don't know if it always is available on all versions.

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's a solid point, I also want to take a stab at testing this on GH, the runner may act differently than my laptop 😄 No reason to rush this.

@IngelaAndin IngelaAndin added the waiting waiting for changes/input from author label Oct 1, 2024
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 waiting waiting for changes/input from author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants