Skip to content

feat(tls): AWS Libcrypto Support#2008

Merged
tottoto merged 31 commits intohyperium:masterfrom
architect-xyz:master
Nov 6, 2024
Merged

feat(tls): AWS Libcrypto Support#2008
tottoto merged 31 commits intohyperium:masterfrom
architect-xyz:master

Conversation

@jenr24-architect
Copy link
Contributor

@jenr24-architect jenr24-architect commented Oct 16, 2024 •
edited
Loading

Motivation

I want to use aws-lc-rs with tonic.

Solution

adds a feature flag tls-aws-lc in parallel to tls, removed tls feature flag dependency from tls-*-roots features

@jenr24-architect jenr24-architect changed the title AWS Libcrypto Support feat: AWS Libcrypto Support Oct 16, 2024
@jenr24-architect jenr24-architect changed the title feat: AWS Libcrypto Support feat(tls): AWS Libcrypto Support Oct 16, 2024
djc
djc reviewed Oct 16, 2024
View reviewed changes
@jenr24-architect jenr24-architect requested a review from djc October 16, 2024 21:51
djc
djc approved these changes Oct 17, 2024
View reviewed changes
Copy link
Contributor

@djc djc left a comment

Choose a reason for hiding this comment

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

Changes look good to me, thanks! Need to address the CI failures.

tottoto
tottoto reviewed Oct 17, 2024
View reviewed changes
Copy link
Collaborator

@tottoto tottoto left a comment

Choose a reason for hiding this comment

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

Thanks. I left some comments.

Could you update the documentation regarding the features.

@LucioFranco
Copy link
Member

LucioFranco commented Oct 18, 2024

@jenr24-architect could you try to force push again to see if we can get CI to kick off again?

@jenr24-architect
Copy link
Contributor Author

jenr24-architect commented Oct 18, 2024

@jenr24-architect could you try to force push again to see if we can get CI to kick off again?

Does it have to be a force push?? It doesn't look like my last push triggered it

tobz
tobz reviewed Oct 22, 2024
View reviewed changes
tonic/Cargo.toml
Comment on lines +36 to +37
tls-native-roots = ["_tls-any", "channel", "dep:rustls-native-certs"]
tls-webpki-roots = ["_tls-any","channel", "dep:webpki-roots"]
Copy link
Collaborator

@tobz tobz Oct 22, 2024

Choose a reason for hiding this comment

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

Just thinking out loud: it strikes me as unfortunate that this the implication here is that because you would only need to access the certificate trust store for doing client things, that we should also enable the channel feature, which is also necessary for client things.

Like in my mind, it would somehow be nicer if this was tls-client-native-roots, etc... to better indicate what the feature was affecting.

Anyways, just thinking out loud. None of this is blocking or needs a response. :)

Copy link
Collaborator

@tottoto tottoto Oct 22, 2024

Choose a reason for hiding this comment

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

I think this is a good idea. However, since the scope is different from this pull request, I think it would be good to discuss it in a separate pull request rather than changing them here.

tobz
tobz requested changes Oct 22, 2024
View reviewed changes
Copy link
Collaborator

@tobz tobz left a comment

Choose a reason for hiding this comment

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

Looks like this needs to be run through cargo fmt and there's the broken intra-doc link stuff to sort out.

@jenr24-architect jenr24-architect requested a review from tobz October 23, 2024 16:08
@tottoto
Copy link
Collaborator

tottoto commented Oct 23, 2024

With this change, users can enable both ring and aws-lc-rs, and when they enable both at the same time, users will need to specify which of these they want to use as the default provider. This setting was already an exposed dependency of rustls as an interface for setting the default provider for rustls, but this was not an issue until now as only ring was supported. I think it would be needed to wrap these configuration APIs in tonic so that users can set which crypto provider to use when both are enabled in order to allow us to use rustls as an internal dependency. It also seems necessary to state that setting the default provider using rustls directly is not intended to be stable for tonic.

@tottoto tottoto self-requested a review October 23, 2024 23:22
@jenr24-architect
Copy link
Contributor Author

jenr24-architect commented Oct 23, 2024

With this change, users can enable both ring and aws-lc-rs, and when they enable both at the same time, users will need to specify which of these they want to use as the default provider. This setting was already an exposed dependency of rustls as an interface for setting the default provider for rustls, but this was not an issue until now as only ring was supported. I think it would be needed to wrap these configuration APIs in tonic so that users can set which crypto provider to use when both are enabled in order to allow us to use rustls as an internal dependency. It also seems necessary to state that setting the default provider using rustls directly is not intended to be stable for tonic.

I had thought about this -- I initially had written some code to add ClientTlsConfig::provider(provider: rustls::CryptoProvider) and ServerTlsConfig::provider(provider: rustls::CryptoProvider) so that I could write both a connect_handles_tls_ring and connect_handles_tls_aws_lc integration test, but the scope of that change started to get out of hand.

I would be willing to add that feature, in either this or a separate PR if we're interested in adding that functionality :)

LucioFranco
LucioFranco approved these changes Oct 25, 2024
View reviewed changes
@jenr24-architect @LucioFranco
Update Cargo.toml
8daf349 
Co-authored-by: Lucio Franco <[email protected]>
@LucioFranco
Copy link
Member

LucioFranco commented Oct 25, 2024

@jenr24-architect looks like there are some more CI failures

tottoto
tottoto approved these changes Oct 25, 2024
View reviewed changes
Copy link
Collaborator

@tottoto tottoto left a comment

Choose a reason for hiding this comment

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

@jenr24-architect

but the scope of that change started to get out of hand.

Makes sense to me. I suppose there are several possible ways to address this, so it seems reasonable to narrow the scope of this pull request.

@tottoto
Copy link
Collaborator

tottoto commented Oct 25, 2024

@LucioFranco The CI failure does not seem to be caused by this pull request's change. I opened #2021 to address it.

@jenr24-architect jenr24-architect mentioned this pull request Oct 29, 2024
Merged
@jenr24-architect
Copy link
Contributor Author

jenr24-architect commented Nov 5, 2024

merged master after #2021 merged, this PR is ready for CI to re-run and to enter the merge queue

@tottoto tottoto enabled auto-merge November 6, 2024 00:55
@tottoto tottoto added this pull request to the merge queue Nov 6, 2024
Merged via the queue into hyperium:master with commit b28b59b Nov 6, 2024
@conradludgate conradludgate mentioned this pull request Nov 7, 2024
Merged
@lima-limon-inc lima-limon-inc mentioned this pull request Apr 4, 2025
Closed
5 tasks
@connyay connyay mentioned this pull request Apr 23, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@LucioFranco LucioFranco LucioFranco approved these changes

@tottoto tottoto tottoto approved these changes

@tobz tobz Awaiting requested review from tobz

+1 more reviewer

@djc djc djc approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@jenr24-architect @LucioFranco @tottoto @djc @tobz