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

server_name: offer ServerName::to_str w/ alloc/std features #14

Merged
merged 3 commits into from
Nov 28, 2023

Conversation

cpu
Copy link
Member

@cpu cpu commented Nov 24, 2023

Downstream crates may want to represent a ServerName as a String or &str, e.g. for passing through a ffi boundary to external code (for example, in rustls-ffi, or in rustls-platform-verifier).

This commit implements support for converting a pki_types::IpAddr to a std::net::IpAddr (to use the to_string impl), and adds a ServerName::to_str that can return a Cow<'_, str> for a DnsName or an IpAddr.

@cpu cpu self-assigned this Nov 24, 2023
@cpu cpu requested a review from djc November 24, 2023 20:09
src/server_name.rs Outdated Show resolved Hide resolved
Copy link
Member

@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.

It seems to me that ServerName is an input-only type from the perspective of webpki/rustls so it's not that obvious why it needs non-trivial implementations to change it back to string form. What's the ffi/platform verifier use case, exactly?

src/server_name.rs Outdated Show resolved Hide resolved
@cpu
Copy link
Member Author

cpu commented Nov 28, 2023

It seems to me that ServerName is an input-only type from the perspective of webpki/rustls so it's not that obvious why it needs non-trivial implementations to change it back to string form. What's the ffi/platform verifier use case, exactly?

For rustls-ffi the ServerName is translated to a C string to pass through the FFI boundary to a verification callback. Previously that was using the AsRef<&str> impl that existed on rustls::server::DnsName and to_string from the Std IP address. Those went away with the pki-types migration.

For rustls-platform-verifier the ServerName is translated to a Rust String that can be based to Java code over a FFI boundary to do additional revocation checking. It too was using the DnsName::AsRef and Std IP to_string approach (and also needed an ugly unreachable!).

Since there are two separate uses and the functionality existed before moving to the pki-types arrangement it seemed sensible to implement. Does the above help convince you?

@cpu cpu force-pushed the cpu-servername-to-string branch 2 times, most recently from b56342f to 1a03948 Compare November 28, 2023 21:51
@cpu cpu force-pushed the cpu-servername-to-string branch from 1a03948 to c245500 Compare November 28, 2023 22:05
src/server_name.rs Outdated Show resolved Hide resolved
src/server_name.rs Outdated Show resolved Hide resolved
src/server_name.rs Show resolved Hide resolved
cpu added 2 commits November 28, 2023 17:33
Downstream crates may want to represent a `ServerName` as a `String` or
`&str`, e.g. for passing through a `ffi` boundary to external code.

This commit implements support for converting a `pki_types::IpAddr` to
a `std::net::IpAddr` (to use the `to_string` impl), and adds
a `ServerName::to_str` that can return a `Cow<'_, str>` for a `DnsName`
or an `IpAddr`.
@cpu cpu force-pushed the cpu-servername-to-string branch from c245500 to dbd46ff Compare November 28, 2023 22:33
@cpu cpu enabled auto-merge November 28, 2023 22:34
@cpu cpu added this pull request to the merge queue Nov 28, 2023
Merged via the queue into rustls:main with commit d99520c Nov 28, 2023
11 checks passed
@cpu cpu deleted the cpu-servername-to-string branch November 28, 2023 22:35
@cpu
Copy link
Member Author

cpu commented Nov 28, 2023

Published rustls-pki-types v0.2.3 at registry crates-io

@cpu
Copy link
Member Author

cpu commented Nov 28, 2023

Argh - I broke semver calling this 0.2.3 😢 af0db93 was in main and added a Debug bound on SignatureVerificationAlgorithm.

That makes it incompatible with rustls-webpki 0.102.0-alpha.7 because RingAlgorithm isn't Debug.

I could cut a new 0.2.4 that includes this branch's changes but not af0db93, or we could roll forward and fix webpki. I'm not sure what the best course of action is given this crate is pretty internal to the Rustls ecosystem.

@cpu
Copy link
Member Author

cpu commented Nov 28, 2023

Here's a webpki fix PR: rustls/webpki#212

@djc
Copy link
Member

djc commented Nov 29, 2023

I guess we should add a cargo-semver-checks job? :D I suppose the webpki fix will be good enough for now while this only causes issues for alphas.

@cpu
Copy link
Member Author

cpu commented Nov 29, 2023

I guess we should add a cargo-semver-checks job?

It was there, but CI wasn't running 😭 I should have fixed that before I merged the other work. Silly mistakes.

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.

2 participants