Implement IP address validation

[ereslibre]

Fixes: https://github.com/briansmith/webpki/issues/54

Introduce IpAddressRef, DnsNameOrIpRef and the owned type IpAddress.

Introduce a new public function verify_is_valid_for_dns_name_or_ip 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 IpAddressRef into a DnsNameOrIpRef for better ergonomics when calling to verify_cert_dns_name_or_ip.

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 IpAddressRef, DnsNameOrIpRef nor IpAddress 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.

[ereslibre]

As a consumer, an example on rustls prior to this PR we have:

fn verify_server_cert(...) -> Result<ServerCertVerified, Error> {

    ...

    cert.verify_is_valid_for_dns_name(dns_name.0.as_ref())
        .map_err(pki_error)
        .map(|_| ServerCertVerified::assertion())
}

With this change, this can be changed to:

fn verify_server_cert(...) -> Result<ServerCertVerified, Error> {

    ...

    cert.verify_is_valid_for_dns_name_or_ip(Into::<webpki::DnsNameOrIpRef>::into(server_name))
            .map_err(pki_error)
            .map(|_| ServerCertVerified::assertion())
}

This ensures that if verify_is_valid_for_dns_name is still the one being used, only DNS names will be checked. In order to be able to check IP addresses as well the consumer has to use verify_is_valid_for_dns_name_or_ip.

[pato]

This would be a great addition!

[webern]

This would be a great addition!

I agree, I have had to use OpenSSL instead of rustls because of this. I am interested to see the feedback on this PR.

[superbattery]

I need this right now

[ereslibre]

I need this right now

Please, let's keep the discussion on this PR to the point. Brian has already done a lot of work pointing to some hints on how this could be implemented. This feature was not in sight for the initial set of responsibilities of webpki, and it comes with some friction.

Besides, it also comes with a huge responsibility because once introduced this means that consumers expect for it to be maintained and stable over time.

Let's not put more pressure on core library/crates authors, please. I'm sure he got notified and that he will eventually provide feedback on the PR. We know there is some people interested in this feature, but signaling that on new comments that reach the author is not productive and is not helping in introducing this in the project.

Also, I'm sorry if the PR itself is also putting pressure on the crate author, I just wanted to implement the feature after looking at the previous opened PR's by Brian himself and propose something that would not be invasive for current users and hopefully future-proof.

[ctz]

I think a useful addition to this PR would be std-feature-gated construction of IpAddress from std::net::IpAddr. Perhaps a From<> trait implementation.

[ereslibre]

I think a useful addition to this PR would be std-feature-gated construction of IpAddress from std::net::IpAddr. Perhaps a From<> trait implementation.

@ctz: Just updated the PR with the conversion from std::net::IpAddr to IpAddress. Thanks for the idea!

[digitwolf]

Hey guys, so what's blocking the merge of this PR? What are the next steps? Is there anything that needs to be changed? I am also very interested in this and would love to help if needed.

[ereslibre]

Hey guys, so what's blocking the merge of this PR?

From my side this is ready to go.

[djc]

@ereslibre there is now a fork in the rustls org. Would you be interested in resubmitting your changes there so we can review it? We will consider publishing the next version of rustls with a dependency on our forked rustls-webpki.

[ereslibre]

@djc Thank you, I will keep that fork up to date if I happen to add changes here. I am going to cherry pick the commits back to this PR too of the changes made on the rustls fork. I will do that tomorrow.

And thanks to @ctz and @djc for the fixes!

[ereslibre]

I will do that tomorrow.

Well, not quite. Updated now and synced with https://github.com/rustls/webpki/compare/main...rustls:webpki:feat-ip-address

[djc]

@ereslibre I actually meant for you to resubmit this as a PR against that repository. Would you be able to do that?

[ereslibre]

@djc I can do that without any issue, but I see that https://github.com/rustls/webpki/pull/5 is already open. I synced this PR with that.

Do you want me to still open a PR despite https://github.com/rustls/webpki/pull/5 is already submitted?

[djc]

Ah, sorry, missed that! Should cover everything then.

[mokeyish]

Seems like the author doesn't have any github activity this year. Did he give up?

[marziply]

Any update on this?

[ereslibre]

Any update on this?

The rustls project has forked the repo and we are integrating the changes on their fork, that will be consumed by rustls itself.

The work related to this is going on at https://github.com/rustls/webpki/pull/5 and https://github.com/rustls/webpki/pull/14

[ereslibre]

This is now implemented in rustls/webpki, as described in https://github.com/rustls/rustls/issues/184. rustls 0.21.0 will allow to validate IP addresses in certificate SANs.

I will update this PR with many of the relevant changes and fixes pointed out by the rustls community and that got merged into https://github.com/rustls/webpki.