IP address support by ctz · Pull Request #5 · rustls/webpki

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

 ///   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).

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

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

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

Let's rename this to subject_name?

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

 ///   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

Comment thread tests/integration.rs

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

Uh oh!

Conversation

ctz commented Sep 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Sep 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ereslibre commented Oct 8, 2022

Uh oh!

jacobsvante commented Oct 21, 2022

Uh oh!

djc commented Oct 21, 2022

Uh oh!

webern commented Nov 1, 2022

Uh oh!

ereslibre commented Nov 1, 2022

Uh oh!

digitwolf commented Nov 21, 2022

Uh oh!

ereslibre commented Nov 25, 2022

Uh oh!

ereslibre commented Dec 11, 2022

Uh oh!

ereslibre commented Dec 26, 2022

Uh oh!

ctz commented Dec 26, 2022

Uh oh!

djc commented Dec 26, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ereslibre Dec 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ctz commented Jan 4, 2023

Uh oh!

djc commented Jan 4, 2023

Uh oh!

djc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ereslibre commented Jan 4, 2023

Uh oh!

djc commented Jan 4, 2023

Uh oh!

ereslibre commented Jan 6, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

ctz commented Sep 18, 2022 •

edited

Loading

codecov-commenter commented Sep 18, 2022 •

edited

Loading

ereslibre Dec 28, 2022 •

edited

Loading