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

tonic depends on rustls API that isn't guaranteed to exist in minimum dependency version #1510

Closed
str4d opened this issue Sep 11, 2023 · 4 comments · Fixed by #1514
Closed

Comments

@str4d
Copy link

str4d commented Sep 11, 2023

Bug Report

Version

tonic 0.10.0

Description

#1443 migrated from the deprecated RootCertStore::add_server_trust_anchors to the new RootCertStore::add_trust_anchors. However, the latter was only released in rustls 0.21.6, while tonic 0.10.0 has an implicit lower version bound on rustls 0.21.0. tonic does not depend directly on rustls, instead using the re-exported version from tokio-rustls 0.24 (from which it obtains its implicit lower version bound).

This hit me because the update from tonic 0.9 to tonic 0.10 didn't need to bump tokio-rustls, so I didn't initially bump any of my other dependencies. It was only when compilation broke and I dug into why that I uncovered this issue.

tonic should instead add an explicit dependency on rustls = "0.21.6" to ensure that the API it is using exists, even if it continues to access it through the tokio-rustls re-export.

I opened rustls/tokio-rustls#18 about the fact that their examples also use this new API without bumping the minimum rustls version; had they done this, the tonic issue would have been incidentally fixed. But sadly that isn't something we can rely on for transitive dependencies (and it would have been entirely valid for tokio-rustls to continue to recommend the deprecated API in their examples and README). So until cargo provides a better way to handle patch updates to transient dependencies, the core fix needs to be here.

@alexheretic
Copy link

alexheretic commented Sep 12, 2023

I don't think in general README example code should require min versions to be correct, that's too strict a requirement on usage snippets.

However, I agree that the actual lib code should compile with the min dependency versions:

tonic should instead add an explicit dependency on rustls = "0.21.6" to ensure that the API it is using exists, even if it continues to access it through the tokio-rustls re-export.

👍 This seems doable, I would also import the trait directly from rustls too.

@LucioFranco
Copy link
Member

I assume this is happening because its not getting updated in your lockfile since a new pull of the project should bring in the latest. But I think you're right we should force the min version as well. I'd take a PR updating that and we can try to get a patch release out. Thanks!

@LucioFranco
Copy link
Member

Ah actually now that I read this again I think this needs to be fixed in tokio-rustls rather than tonic depending on rustls directly, I don't think that is the right solution.

@djc
Copy link
Contributor

djc commented Sep 12, 2023

I don't think there is a bug in tokio-rustls here -- tokio-rustls isn't depending on the new rustls API directly, so there's no reason for it to require newer rustls. If tonic wants to rely on newer rustls it should ensure the rustls version that is being pulled into its dependency graph is new enough -- or stick with the deprecated API for now instead.

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 a pull request may close this issue.

4 participants