IP address support #5

ctz · 2022-09-18T15:29:42Z

This is wholly based on the work of @ereslibre - with some testing and fixing the resulting issues found.

TODO list to move this out of draft:

docs for public items
100% line coverage of added code
100% line diff coverage

For #4

codecov-commenter · 2022-09-18T15:31:50Z

Codecov Report

Merging #5 (b68b42f) into main (19226ea) will increase coverage by 9.98%.
The diff coverage is 93.86%.

@@            Coverage Diff             @@
##             main       #5      +/-   ##
==========================================
+ Coverage   76.09%   86.07%   +9.98%     
==========================================
  Files          14       15       +1     
  Lines        1556     2349     +793     
==========================================
+ Hits         1184     2022     +838     
+ Misses        372      327      -45

Impacted Files	Coverage Δ
src/lib.rs	`100.00% <ø> (ø)`
src/name/dns_name.rs	`71.69% <ø> (+2.20%)`	⬆️
src/name/name.rs	`29.62% <29.62%> (ø)`
src/name/verify.rs	`34.00% <66.03%> (+29.85%)`	⬆️
src/name/ip_address.rs	`98.29% <98.21%> (+98.29%)`	⬆️
src/end_entity.rs	`52.17% <100.00%> (+14.07%)`	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

ereslibre · 2022-10-08T07:25:04Z

@ctz I am happy to own the missing tasks so that we can move this further.

What would be the best way to add work on top of this PR in order to check some of those boxes?

Thanks :)

jacobsvante · 2022-10-21T09:34:23Z

Can't wait to see this get merged! Thanks for the great effort @ereslibre!

djc · 2022-10-21T10:12:40Z

@ctz I am happy to own the missing tasks so that we can move this further.

What would be the best way to add work on top of this PR in order to check some of those boxes?

Usually submitting PRs that target another PRs branch works quite well.

webern · 2022-11-01T17:24:29Z

Is 100% line/diff coverage a realistic requirement?

ereslibre · 2022-11-01T19:41:11Z

Is 100% line/diff coverage a realistic requirement?

I expect to be able to cover it. I should be able to open a PR this week.

digitwolf · 2022-11-21T23:35:17Z

Hi, @ereslibre thank you for this change!
Please let me know if you need help.

ereslibre · 2022-11-25T18:41:45Z

I have started to work on #14. If you folks have a recommendation on how to run codecov locally so I can spare some pushes and the back & forth from GitHub that would be amazing. Thanks!

ereslibre · 2022-12-11T10:41:25Z

#14 is ready for review and targeting this branch :)

ereslibre · 2022-12-26T11:27:28Z

I opened #20 that should check the "100% line diff coverage".

ctz · 2022-12-26T14:15:11Z

@djc I have reviewed @ereslibre's changes here, but my commits on this could use a look-over. Then I plan to merge this and #18. Together they provide almost 20 percentage points of testing line coverage.

djc · 2022-12-26T18:18:06Z

These are near the top of my list of things to review once I manage to get the family through Christmas chaos. 🎄

djc · 2022-12-27T21:18:12Z

src/end_entity.rs

@@ -27,6 +27,9 @@ use core::convert::TryFrom;
 ///   certificate is currently valid *for use by a TLS server*.
 /// * `EndEntityCert.verify_is_valid_for_dns_name`: Verify that the server's
 ///   certificate is valid for the host that is being connected to.
+/// * `EndEntityCert.verify_is_valid_for_dns_name_or_ip`: Verify that the server's


Given that (I think) rustls is the only significant user of this API, I kinda feel like it would be good enough to rename the existing method instead of adding another one. Since we rename the function and it will accept a type of a different argument, downstream users will be forced to change the code anyway, and we don't get stuck with additional API surface that will probably go unused.

What do you think?

The main reason why I think it's important is that current users of briansmith/webpki won't start accepting certificates they would have rejected without this PR. But I understand this does not matter much to rustls today. However, if a slightly different version of webpki merges some day and rustls deviated from that, it will need to adapt to the new webpki API.

If that is fine, I see no issue in renaming the method instead of adding a new one.

I suppose I was assuming this would be in a semver-incompatible release, in which case it wouldn't matter, but if this is to go in a semver-compatible release this approach makes sense to me.

So you're saying this PR only makes semver-compatible changes to the public API?

Yes, the contribution to briansmith/webpki was meant to be semver-compatible.

Unresolving this: given that we're about the release the first version of rustls-webpki we don't have any semver-compatibility constraints here, in which case I think it's better to replace the old method with the new method (instead of retaining both).

src/name/ip_address.rs

src/name/name.rs

src/name/verify.rs

src/name/ip_address.rs

digitwolf · 2022-12-30T23:50:04Z

src/name/name.rs

+                ip_address::ipv4_octets(dns_name_or_ip).map_err(|_| InvalidDnsNameOrIpError)?,
+            )));
+        }
+        if ip_address::is_valid_ipv6_address(untrusted::Input::from(dns_name_or_ip)) {
+            return Ok(DnsNameOrIpRef::IpAddress(IpAddressRef::IpV6AddressRef(
+                dns_name_or_ip,
+                ip_address::ipv6_octets(dns_name_or_ip).map_err(|_| InvalidDnsNameOrIpError)?,
+            )));
+        }
+        Ok(DnsNameOrIpRef::DnsName(
+            DnsNameRef::try_from_ascii(dns_name_or_ip).map_err(|_| InvalidDnsNameOrIpError)?,


These map_err are swallowing all the error messages.
Would be nice if we could add a string to InvalidDnsNameOrIpError to forward some details or at least add debug! log statements.

What would be a good error information in this case? Note that this approach is trying to parse ipv4, then ipv6, then a dns name.

If someone provides an invalid ipv4 address, but meaning an ipv4 address, it's not trivial to return a meaningful error message, given we later tried ipv6, and then we tried a dns name.

tests/integration.rs

ctz · 2023-01-04T10:07:10Z

Minded to merge this now; wdyt @djc?

djc · 2023-01-04T11:16:14Z

I think it's close, but I'd like to do another pass, will try to do that today. (Sorry, this week is still a school/work holiday so my screen time has been fairly limited.) Meanwhile, I did put a comment in #18 (comment) yesterday which I think is relevant to the API proposed here.

djc

Noted some further feedback. I think it'd be okay to merge, we can address the feedback after merging this if we prefer. Would be good squash the commits from this one as we merge.

djc · 2023-01-03T13:28:40Z

src/name.rs

@@ -19,7 +19,11 @@ pub use dns_name::{DnsNameRef, InvalidDnsNameError};
 #[cfg(feature = "alloc")]
 pub use dns_name::DnsName;

-mod ip_address;
+#[allow(clippy::module_inception)]
+mod name;


Let's rename this to subject_name?

djc · 2023-01-04T13:36:12Z

src/end_entity.rs

@@ -27,6 +27,9 @@ use core::convert::TryFrom;
 ///   certificate is currently valid *for use by a TLS server*.
 /// * `EndEntityCert.verify_is_valid_for_dns_name`: Verify that the server's
 ///   certificate is valid for the host that is being connected to.
+/// * `EndEntityCert.verify_is_valid_for_dns_name_or_ip`: Verify that the server's


Unresolving this: given that we're about the release the first version of rustls-webpki we don't have any semver-compatibility constraints here, in which case I think it's better to replace the old method with the new method (instead of retaining both).

ereslibre · 2023-01-04T19:29:59Z

Let me know if I can help somehow. I think it’s probably a bit more cumbersome for me to address feedback at this time given I have to open a PR and we want to squash some commits at least, if not all.

I am happy to own further work.

Introduce `IpAddrRef`, `SubjectNameRef` and the owned type `IpAddr`. Introduce a new public function `verify_is_valid_for_subject_name` that validates a given host name or IP address against a certificate. IP addresses are only compared against Subject Alternative Names. It's possible to convert the already existing types `DnsNameRef` and `IpAddrRef` into a `SubjectNameRef` for better ergonomics when calling to `verify_is_valid_for_subject_name`. The behavior of `verify_cert_dns_name` has not been altered, and works in the same way as it has done until now, so that if `webpki` gets bumped as a dependency, it won't start accepting certificates that would have been rejected until now without notice. Neither `IpAddrRef`, `SubjectNameRef` nor `IpAddr` can be instantiated directly. They must be instantiated through the `try_from_ascii` and `try_from_ascii_str` public functions. This ensures that instances of these types are correct by construction. IPv6 addresses are only validated and supported in their uncompressed form.

djc · 2023-01-04T22:26:37Z

@ereslibre addressing my feedback from above in new PRs would be great, thanks for all your hard work so far!

ereslibre · 2023-01-06T14:32:32Z

Submitted #24

ctz force-pushed the feat-ip-address branch from e9c8e20 to 100d575 Compare October 9, 2022 14:27

ereslibre mentioned this pull request Oct 9, 2022

Implement IP address validation briansmith/webpki#260

Open

djc mentioned this pull request Nov 3, 2022

Using rustls for machine-to-machine w/ private CA and no DNS rustls/rustls#331

Closed

mokeyish mentioned this pull request Nov 9, 2022

Fix stuck of dns over tls with clear text SNI hickory-dns/hickory-dns#1826

Merged

aadityadhruv mentioned this pull request Nov 10, 2022

Remove sqlx features due to a bug in rustls/webpki failing to resolve IP addresses rwf2/Rocket#2391

Closed

ereslibre mentioned this pull request Nov 25, 2022

Increase test coverage of IP address support #14

Closed

ctz force-pushed the feat-ip-address branch 3 times, most recently from 1e752c1 to fb85e76 Compare December 12, 2022 08:58

ereslibre mentioned this pull request Dec 26, 2022

Increase test coverage #20

Merged

ctz marked this pull request as ready for review December 26, 2022 14:08

ctz requested a review from djc December 26, 2022 14:13

djc reviewed Dec 27, 2022

View reviewed changes

ereslibre mentioned this pull request Dec 30, 2022

IP address support: Address code review comments #21

Merged

digitwolf reviewed Dec 30, 2022

View reviewed changes

tests/integration.rs Show resolved Hide resolved

ctz requested a review from djc January 2, 2023 10:15

djc mentioned this pull request Jan 3, 2023

Improve name constraints testing and fix bugs found #18

Merged

djc approved these changes Jan 4, 2023

View reviewed changes

ereslibre and others added 2 commits January 4, 2023 19:56

Add basic tests for ipv4/ipv6 SANs

f866359

ctz force-pushed the feat-ip-address branch from b68b42f to f866359 Compare January 4, 2023 19:58

ctz merged commit 15f481c into main Jan 4, 2023

ereslibre mentioned this pull request Jan 6, 2023

Address further IP address support feedback #24

Closed

ctz deleted the feat-ip-address branch March 13, 2023 19:01

daxpedda mentioned this pull request Jul 21, 2023

Support connecting to IP addresses quinn-rs/quinn#564

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IP address support #5

IP address support #5

ctz commented Sep 18, 2022 •

edited

Loading

codecov-commenter commented Sep 18, 2022 •

edited

Loading

ereslibre commented Oct 8, 2022

jacobsvante commented Oct 21, 2022

djc commented Oct 21, 2022

webern commented Nov 1, 2022

ereslibre commented Nov 1, 2022

digitwolf commented Nov 21, 2022

ereslibre commented Nov 25, 2022

ereslibre commented Dec 11, 2022

ereslibre commented Dec 26, 2022

ctz commented Dec 26, 2022

djc commented Dec 26, 2022

djc Dec 27, 2022

ereslibre Dec 28, 2022 •

edited

Loading

djc Dec 30, 2022

ereslibre Dec 30, 2022

djc Jan 4, 2023

digitwolf Dec 30, 2022

ereslibre Dec 31, 2022

ctz commented Jan 4, 2023

djc commented Jan 4, 2023

djc left a comment

djc Jan 3, 2023

djc Jan 4, 2023

ereslibre commented Jan 4, 2023

djc commented Jan 4, 2023

ereslibre commented Jan 6, 2023

IP address support #5

IP address support #5

Conversation

ctz commented Sep 18, 2022 • edited Loading

codecov-commenter commented Sep 18, 2022 • edited Loading

Codecov Report

ereslibre commented Oct 8, 2022

jacobsvante commented Oct 21, 2022

djc commented Oct 21, 2022

webern commented Nov 1, 2022

ereslibre commented Nov 1, 2022

digitwolf commented Nov 21, 2022

ereslibre commented Nov 25, 2022

ereslibre commented Dec 11, 2022

ereslibre commented Dec 26, 2022

ctz commented Dec 26, 2022

djc commented Dec 26, 2022

Choose a reason for hiding this comment

ereslibre Dec 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ctz commented Jan 4, 2023

djc commented Jan 4, 2023

djc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ereslibre commented Jan 4, 2023

djc commented Jan 4, 2023

ereslibre commented Jan 6, 2023

ctz commented Sep 18, 2022 •

edited

Loading

codecov-commenter commented Sep 18, 2022 •

edited

Loading

ereslibre Dec 28, 2022 •

edited

Loading