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.

Sign up for GitHub

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

Jump to bottom

feat: use webpki as rustls roots on non-desktop platforms #1402

Merged
merged 2 commits into from
Dec 5, 2024
Merged

feat: use webpki as rustls roots on non-desktop platforms #1402

merged 2 commits into from
Dec 5, 2024

Conversation

SilverMira
Copy link
Contributor

@SilverMira SilverMira commented Nov 24, 2024 •
edited
Loading

Automatically choose to use rustls-webpki if the platform doesn't support rustls-native-certs (ie: non-desktop platforms).

Main motivation is rustls-native-certs is not supported by all platforms, and therefore HTTPs calls will error with 0 native roots

Partially fixes: #1399

@SilverMira
Copy link
Contributor Author

Looks like CI is checking for librespot-core with --no-default-features, but we need at least either rustls-native-roots or rustls-webpki-roots to be enabled, never both (upstream dependency fails to build if we specify both). And since there's interdependency from other crates to core and is also checked in CI, looks like fixing CI is not simple

@SilverMira
Copy link
Contributor Author

SilverMira commented Nov 24, 2024 •
edited
Loading

Question is whether this should be exposed as feature flags or should we just quietly default to rustls-webpki on other platforms that rustls-native-certs don't support (other than Windows/Linux/Mac), Doing it the feature flags way makes librespot-core not compilable with --no-default-features, so apart from just breaking CI, it's also a breaking change downstream. Is there any value for allowing users on desktop platforms to choose rustls-webpki?

@kingosticks
Copy link
Contributor

I don't recall anyone asking for it, a feature is likely not necessary.

@SilverMira
feat: use webpki as rustls roots on non-desktop platforms
9d1050c 
Silently switch over to using `rustls-webpki` when building for
target_os that is not Windows/Linux/Mac because `rustls-native-certs`
doesn't support them.

Ideally we should use `rustls-platform-verifier` as it's now the
recommended crate even on `rustls-native-certs` repository, since it
chooses the right implementation for the platform. But currently it
doesn't seem like `hyper-proxy2` or `tokio-tungstenite` doesn't support
them yet.
@SilverMira SilverMira changed the title feat: allow to use webpki as roots for rustls feat: use webpki as rustls roots on non-desktop platforms Nov 25, 2024
@SilverMira
Copy link
Contributor Author

Sounds reasonable, now it's implemented as target-specific feature only

@roderickvd
Copy link
Member

Thanks, if you would write a CHANGELOG message then we can merge this.

@roderickvd roderickvd merged commit 705e68e into librespot-org:dev Dec 5, 2024
4 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Certificate issues when connecting session in Android app
3 participants
@SilverMira @kingosticks @roderickvd