IP address support: Address code review comments by ereslibre · Pull Request #21 · rustls/webpki

ereslibre · 2022-12-30T13:45:02Z

Mimic std names IpAddr::V4, IpAddr::V6 for owned addresses, as well as IpAddrRef::V4, IpAddrRef::V6 for referenced ones. Also mimic the exception name, now named AddrParseError instead of InvalidIpAddressError.
Update copyright header on name.rs.
Reduce the number of allocations on ipv6_to_uncompressed_string used to print IPv6 addresses in their uncompressed form.

Other minor changes.

This addresses most of the feedback provided in #5 (review).

codecov-commenter · 2022-12-30T13:49:23Z

Codecov Report

Merging #21 (327e47f) into feat-ip-address (7573333) will decrease coverage by 0.32%.
The diff coverage is 88.29%.

@@                 Coverage Diff                 @@
##           feat-ip-address      #21      +/-   ##
===================================================
- Coverage            86.40%   86.07%   -0.33%     
===================================================
  Files                   15       15              
  Lines                 2375     2349      -26     
===================================================
- Hits                  2052     2022      -30     
- Misses                 323      327       +4

Impacted Files	Coverage Δ
src/lib.rs	`100.00% <ø> (ø)`
src/name/name.rs	`29.62% <35.29%> (-0.38%)`	⬇️
src/name/ip_address.rs	`98.29% <92.54%> (-1.71%)`	⬇️
src/name/verify.rs	`34.00% <95.83%> (-2.24%)`	⬇️
src/end_entity.rs	`52.17% <100.00%> (ø)`

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

djc · 2022-12-30T14:02:24Z

src/name/ip_address.rs

-        .map(|octet| format!("{:02x?}{:02x?}", octet[0], octet[1]))
-        .collect::<Vec<String>>()
-        .join(":")
+    format!(


I suppose this is a fine solution. I might have gone with pre-allocating a String::with_capacity() and then using .write_fmt() to write into it in a loop.

I have no strong opinion on this one.

What would you do with the Result when using write_fmt? ipv6_to_uncompressed_string is meant to be used on an impl From, so it cannot fail.

Just unwrap() it -- after all, format!() is actually doing the same thing and also doing something like unwrapping.

Addressed in 81837a8bf2862df71a8e35f6f8d91dc4e2f5110f

djc · 2022-12-30T14:04:09Z

src/name/verify.rs

+        cert.inner().subject_alt_name,
+        Err(Error::CertNotValidForName),
+        &|name| {
+            #[allow(clippy::single_match)]


Why not do if let GeneralName::IpAddress(presented_id) = name instead of the match with #[allow(clippy::single_match)]?

- Mimic std names `IpAddr::V4`, `IpAddr::V6` for owned addresses, as well as `IpAddrRef::V4`, `IpAddrRef::V6` for referenced ones. Also mimic the exception name, now named `AddrParseError` instead of `InvalidIpAddressError`. - Update copyright header on `name.rs`. - Reduce the number of allocations on `ipv6_to_uncompressed_string` used to print IPv6 addresses in their uncompressed form. Other minor changes.

- Rename `DnsNameOrIpRef` to `SubjectNameRef` - Rename `InvalidDnsNameOrIpError` to `InvalidSubjectNameError`

djc · 2022-12-30T20:47:38Z

src/name/ip_address.rs

+    if dot_count != 3 {
+        return Err(AddrParseError);
+    }
+    Ok(IpAddrRef::V4(ip_address_, ipv4_octets(ip_address_)?))


Sorry, no, I don't think this quite makes sense. Now we still have a routine that's parsing the data separately from the validation. The idea is that we accumulate the output as we validate it, so that the output accumulates from the validated data itself. So for example, in the same place where you currently move to the next octet, write into the output array?

Addressed in 541bc2b5826354db63292fff15bf7bb3b5a2827a.

djc · 2022-12-30T20:48:27Z

src/name/name.rs

-                subject_name,
-                ip_address::ipv6_octets(subject_name).map_err(|_| InvalidSubjectNameError)?,
-            )));
+        if let Ok(ip_address) = ip_address::parse_ipv6_address(subject_name) {


Can do else if here.

It's a return-on-first-match situation, no need for it, but I will add it nevertheless.

Addressed in 416976f

Parse, don't validate

Use `write_fmt` in order to reduce repetition of arguments when formatting an IPv6 address.

ereslibre · 2022-12-31T15:31:08Z

src/name/ip_address.rs

+                octets[octet] =
+                    TryInto::<u8>::try_into(radix10_to_octet(&current_octet[..current_size]))
+                        .expect("invalid character");


I'm adding these TryInto due to clippy being executed with -D clippy::as-conversions. If there is a better/more idiomatic way to shape this code I'm happy adapting.

Same comment applies to the rest of TryInto conversions in this commit.

ereslibre mentioned this pull request Dec 30, 2022

IP address support #5

Merged

3 tasks

ereslibre force-pushed the rustls-feat-ip-address branch from 76b5a77 to 368294b Compare December 30, 2022 13:52

ereslibre changed the title ~~Address code review comments~~ IP address support: Address code review comments Dec 30, 2022

djc approved these changes Dec 30, 2022

View reviewed changes

ereslibre force-pushed the rustls-feat-ip-address branch from 368294b to 1ed3634 Compare December 30, 2022 16:04

Address code review comments

961cfd7

- Rename `DnsNameOrIpRef` to `SubjectNameRef` - Rename `InvalidDnsNameOrIpError` to `InvalidSubjectNameError`

ereslibre force-pushed the rustls-feat-ip-address branch 2 times, most recently from f1b6d66 to fba17e2 Compare December 30, 2022 19:14

ereslibre requested a review from djc December 30, 2022 19:15

djc reviewed Dec 30, 2022

View reviewed changes

ereslibre force-pushed the rustls-feat-ip-address branch 2 times, most recently from 1b24841 to 81837a8 Compare December 31, 2022 12:06

ereslibre added 2 commits December 31, 2022 13:13

Address code review comments

416976f

Parse, don't validate

Address code review comments

40b84b6

Use `write_fmt` in order to reduce repetition of arguments when formatting an IPv6 address.

ereslibre force-pushed the rustls-feat-ip-address branch from 81837a8 to 40b84b6 Compare December 31, 2022 12:13

ereslibre commented Dec 31, 2022

View reviewed changes

ereslibre force-pushed the rustls-feat-ip-address branch from 4f9d583 to 1d30656 Compare December 31, 2022 15:44

clippy: remove as-conversions

327e47f

ereslibre force-pushed the rustls-feat-ip-address branch from 1d30656 to 327e47f Compare December 31, 2022 15:51

ctz merged this pull request into rustls:feat-ip-address Jan 1, 2023

ereslibre deleted the rustls-feat-ip-address branch January 4, 2023 20:24

Conversation

ereslibre commented Dec 30, 2022

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ereslibre Dec 30, 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!

ereslibre Dec 31, 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-commenter commented Dec 30, 2022 •

edited

Loading

ereslibre Dec 30, 2022 •

edited

Loading

ereslibre Dec 31, 2022 •

edited

Loading