verification: enable ip subj. validation on android. #28
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cpu
force-pushed
the
cpu-15-android-ip-subj-verification
branch
from
7e1f5c5
to
bd4668b
Compare
djc
reviewed
Aug 31, 2023
cpu
force-pushed
the
cpu-15-android-ip-subj-verification
branch
2 times, most recently
from
ea7097b
to
043c62a
Compare
djc
approved these changes
Aug 31, 2023
complexspaces
requested changes
Aug 31, 2023
There was a problem hiding this comment.
This ended up much easier then I expected based on when I last tried this, so this is great to see. I wonder if your first point about compressed IPv6 addresses was what was messing this up in the last attempt, since I think that I did try roundtripping it through string parsing.
While the Android verifier defers to the platform verifier for most certificate validation options, it relies on `webpki` for ensuring that a certificate is valid for a given subject name. Since Webpki v0.100.x it's been possible to use `webpki` to validate IP address subject names, and so we want this capability to be enabled for the Android verifier. This commit updates the Android `verifier` to accomplish this. Additionally it enables the mock IP address subject test cases in the mock test suite to ensure things work as expected. In order to support this change the function signature of the inner `Verifier.verify_certificate` fn has to change from accepting a `&str` server name, to also accepting the `&rustls::ServerName` that the trait based `ServerCertVerifier.verify_server_cert` was already accepting. There are two main reasons for this: 1. If we try to pull out a `String` to pass forward from the `rustls::ServerName::IpAddress(&IpAddr)` using `IpAddr.to_string()` we'll get back a "compressed" address for IPv6 addresses. This is problematic when later trying to convert to a `webpki::IpAddrRef` for the validation call using `IpAddrRef::try_from_ascii_str`, because it doesn't support the compressed form. 2. By passing through the `rustls::ServerName` directly we can defer the actual process of interacting with `webpki` to the newly exposed `rustls::client::verify_server_name` fn offered with the `dangerous_configuration` feature. This will ensure the logic for name validation is applied consistently between Rustls and the platform verifier.
cpu
force-pushed
the
cpu-15-android-ip-subj-verification
branch
from
043c62a
to
a02ff32
Compare
complexspaces
approved these changes
Aug 31, 2023
|
Thanks for all the help :-) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
While the Android verifier defers to the platform verifier for most certificate validation options, it relies on
webpkifor ensuring that a certificate is valid for a given subject name. Since Webpki v0.100.x it's been possible to usewebpkito validate IP address subject names, and so we want this capability to be enabled for the Android verifier.This commit updates the Android
verifierto accomplish this. Additionally it enables the mock IP address subject test cases in the mock test suite to ensure things work as expected.In order to support this change the function signature of the inner
Verifier.verify_certificatefn has to change from accepting a&strserver name, to also accepting the&rustls::ServerNamethat the trait basedServerCertVerifier.verify_server_certwas already accepting. There are two main reasons for this:Stringto pass forward from therustls::ServerName::IpAddress(&IpAddr)usingIpAddr.to_string()we'll get back a "compressed" address for IPv6 addresses. This is problematic when later trying to convert to awebpki::IpAddrReffor the validation call usingIpAddrRef::try_from_ascii_str, because it doesn't support the compressed form.rustls::ServerNamedirectly we can defer the actual process of interacting withwebpkito the newly exposedrustls::client::verify_server_namefn offered with thedangerous_configurationfeature. This will ensure the logic for name validation is applied consistently between Rustls and the platform verifier. It also allows removing the Android specificpki_name_errorhelper, and the Android target's usage of the generalcrate::verification::invalid_certificatehelper.Resolves #15