Skip to content

Avoid automatic TLS crypto provider detection #3104

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

Open
rylev opened this issue Apr 10, 2025 · 6 comments
Open

Avoid automatic TLS crypto provider detection #3104

rylev opened this issue Apr 10, 2025 · 6 comments

Comments

@rylev
Copy link
Collaborator

rylev commented Apr 10, 2025

As can be seen in #2830 and #3103 - rustls has automatic detection of the crypto provider used for TLS crypto operations. This can very easily go wrong in a complex project like Spin where many different dependencies come together to determine which features are enabled.

Instead, we should explicitly set the crypto provider once so that automatic detection does not happen. The only question is where this should happen. The crypto provider is a global variable which should only be set once. Therefore, it probably makes most sense to set it in main and allow each subcrate of Spin to be ignorant of this entire situation.

@lann
Copy link
Collaborator

lann commented Apr 10, 2025

I think we originally opted to use ring because we had build problems with aws-lc-rs (nasm dependency on...windows maybe?). If that decision is still valid then it seems like we should try to fix the underlying dependency tree issue here that is causing both rustls providers to be enabled.

@lann
Copy link
Collaborator

lann commented Apr 10, 2025

It would also be nice if we could build fewer than 3 versions of rustls... 😢

@rylev
Copy link
Collaborator Author

rylev commented Apr 10, 2025

I should have included that as a part of #3103 (specifically 2ef489c), I did the work to remove the aws-lc-rs feature being turned on. I'm not sure though if my change breaks the AWS key-value implementation as we don't have tests for it, and I'm too lazy currently to test it manually.

@michelleN
Copy link
Collaborator

@rylev Does #3103 fully resolve this issue or are there other bits that need to be addressed?

@rylev
Copy link
Collaborator Author

rylev commented Apr 15, 2025

@michelleN no - #3103 prevents a runtime error, but we are still doing automatic crypto provider detection. So things work currently, but we're at the mercy of any crate update messing things up again. This issue is advocating for using rustls::CryptoProvider::install_default to set the specific ring based crypto provider as the default for the process instead of relying on features to infer the default provider.

@itowlson
Copy link
Collaborator

itowlson commented Apr 16, 2025

@rylev Agree it makes sense to do this in main, but, I wonder if we need to carve out a special case for custom triggers, as these don't pass through Spin's main but through rather through FactorsTriggerCommand::run(). Or we could say that this is just a piece of ceremony that custom triggers need to do in their main, but in that case we maybe need to export a helper, because many of our custom triggers don't have a direct reference to rustls or ring.

(Edited because I missed some of what Ryan said right at tbe beginning of the issue. Sigh.)

@michelleN michelleN moved this from Triage Needed to Investigating / Open For Comment in Spin Triage May 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Investigating / Open For Comment
Development

No branches or pull requests

4 participants