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

Secure SSL defaults for HTTPC for OTP-25+ #153

Merged
merged 1 commit into from
Nov 19, 2024

Conversation

essen
Copy link
Contributor

@essen essen commented Nov 18, 2024

Fixes #62

Tests have not been ran at the time of PR open but I did run Erlang.mk tests using this code and they passed (although not many dep fetches happen via Hex in the test suite).

Comment on lines 34 to 36
%% Add safe defaults if possible.
_ = code:load_file(public_key),
HasCacertsGet = erlang:function_exported(public_key, cacerts_get, 0),
Copy link
Member

@wojtekmach wojtekmach Nov 18, 2024

Choose a reason for hiding this comment

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

this (or hex_core in general) is definitely not a hot path but I think I was told going through code server at runtime is best avoided if alternatives exists, WDYT about doing:

case erlang:system_info(otp_release) >= "25" of
  true ->
  false ->
end;

btw, should we add public_key to

{applications, [kernel, stdlib]},
?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since we are going to use ssl, the public_key module WILL be loaded. We are just loading it a little earlier.

You might need to add inets and perhaps ssl as well. Perhaps other applications are also missing. But this only really matters if using hex_core in a release.

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 will change it to a try/catch as that is simpler.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@essen essen force-pushed the secure-ssl-defaults branch from 1352f5c to 4b5e340 Compare November 19, 2024 15:24
@wojtekmach wojtekmach merged commit e57b4fb into hexpm:main Nov 19, 2024
7 of 8 checks passed
@wojtekmach
Copy link
Member

Thank you!

@essen
Copy link
Contributor Author

essen commented Nov 19, 2024

Thank you! When the next release gets published I can merge my branch making Erlang.mk use hex_core also for fetching packages (instead of just for publishing).

@essen essen deleted the secure-ssl-defaults branch November 19, 2024 15:34
@wojtekmach
Copy link
Member

I wanted to do one more thing before next release which is something like this: beam-telemetry/telemetry#134. I'm hoping to do that within a week, otherwise I'd cut new release with what we have which I think is just your patch. Thanks again, if you run any issues feel free to post here or create new issues, I'm on EEF slack and elixir/erlang forum too.

@essen
Copy link
Contributor Author

essen commented Nov 19, 2024

No worries, I remembered that I can just target the current commit, so I did that and will pin to the next release whenever it comes out.

@wojtekmach
Copy link
Member

hex_core v0.11.0 is out with this change, thank you again for the PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Safe SSL defaults for built-in HTTP adapter
2 participants