Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement IP address validation #260

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

ereslibre
Copy link

@ereslibre ereslibre commented Apr 29, 2022

Fixes: #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
Copy link
Author

ereslibre commented Apr 29, 2022

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
Copy link

pato commented May 4, 2022

This would be a great addition!

@ereslibre ereslibre force-pushed the verify-ip-addresses branch from 54bc1e2 to 22c6184 Compare May 5, 2022 07:08
@ereslibre ereslibre marked this pull request as ready for review May 5, 2022 07:08
@ereslibre ereslibre force-pushed the verify-ip-addresses branch 2 times, most recently from 7c31c0d to c530946 Compare May 5, 2022 07:39
@webern
Copy link

webern commented May 7, 2022

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
Copy link

I need this right now

@ereslibre
Copy link
Author

ereslibre commented May 13, 2022

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
Copy link
Contributor

ctz commented May 28, 2022

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 ereslibre force-pushed the verify-ip-addresses branch from 98e0827 to ec674f8 Compare May 30, 2022 19:41
@ereslibre
Copy link
Author

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
Copy link

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
Copy link
Author

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

From my side this is ready to go.

@djc
Copy link

djc commented Sep 20, 2022

@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
Copy link
Author

ereslibre commented Sep 20, 2022

@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
Copy link
Author

ereslibre commented Sep 29, 2022

I will do that tomorrow.

Well, not quite. Updated now and synced with rustls/webpki@main...rustls:webpki:feat-ip-address

@ereslibre ereslibre force-pushed the verify-ip-addresses branch from e9c8e20 to 100d575 Compare October 9, 2022 16:52
ereslibre and others added 7 commits October 9, 2022 19:00
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.

Signed-off-by: Rafael Fernández López <[email protected]>
current_textual_octet is [u8; 3] but it was indexed by an
unbounded count of octets if they matched 1..9.
rfc5952 says both are allowed.
Seems better to convert from ascii to radix-10 at the time that is
known, rather than doing that validation twice (and skipping a digit
as an error handling strategy).
@ereslibre ereslibre force-pushed the verify-ip-addresses branch from 100d575 to 61cd0b2 Compare October 9, 2022 17:01
@djc
Copy link

djc commented Oct 9, 2022

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

@ereslibre
Copy link
Author

ereslibre commented Oct 9, 2022

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

Do you want me to still open a PR despite rustls/webpki#5 is already submitted?

@djc
Copy link

djc commented Oct 10, 2022

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

@mokeyish
Copy link

mokeyish commented Nov 1, 2022

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

@marziply
Copy link

Any update on this?

@ereslibre
Copy link
Author

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 rustls/webpki#5 and rustls/webpki#14

@ereslibre
Copy link
Author

This is now implemented in rustls/webpki, as described in rustls/rustls#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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

webpki/cert: extend verify_is_valid_for to iPAddress SAN
9 participants