search

BiagioFesta / wtransport

Async-friendly WebTransport implementation in Rust
Apache License 2.0
346 stars 19 forks source link

The `with_server_certificate_hashes` on the *client* should not be under a feature flag #168

Open MOZGIII opened 2 months ago

MOZGIII commented 2 months ago

The rationale is simple: for the client, the use of the server certificate hashes is not dangerous, and they are definitely not necessarily self-signed.

It is very much expected for the server certificates to be a part of a PKI - just not part of the standard Web PKI. It is not required for the trust verification at the WebTransport connection, but the application itself can easily get a certificate from somewhere and verify the trust chain, then compute the hash and pass to wtransport for connection. The same concept is a applicable for the Web APIs as well.

BiagioFesta commented 2 months ago

As said in the past, serverCertificateFingerprints had/have security concerns. This does not necessarily means better or worse (or even dangerous) in the strict sense.

It simply implies the authentication mechanism is different than the "standard de-facto" PKI.

The wtransport feature dangerous collects those methods (in the configuration) which, indeed, alter the "standard de-fact" authentication mechanism. The "scary" word "dangerous" is only a simply alert to inform the programmer is doing something "out the standard", and (since we are touching the security field) they should be warned.

It should more more like: feature = be_careful_you_are_using_non_standard_authentication_mechanisms, but I went for "dangerous" as easier to type in the code.

Being a non standard mechanism (like everything the goodness depends how one uses it), we cannot be completely sure about all implications for all use cases around the world. And concerning security better to be conservative.

Concerning the feature self-signed, I agree that having that method behind that particular feature flag does not sound correct.

That was more a technical choice due to the fact the implementation requires cryptographic functionalities and thus, ring dependency and that was mainly needed to generate self signed certificate and hashes.

I am going to see to remove self-signed for sure.

MOZGIII commented 2 months ago

It is a standard mechanism, by the definition of a standard as being a part of an standards specification, which the very spec you referenced is. So I disagree with the non-standard argument to begin with.

Unusual? For the current status quo of the web certificate validation - probably yes. Actually, very much so.

Dangerous? Well, dangerous things usually don't end up as web standards. The very discussion you referenced is resolved with a pretty minor corrections to the standard, that are merely clarifications to the security algorithm. In fact, it would be great if you read through the said thread, because they discuss the notion of this mode of operation being dangerous pretty extensively.

Being a non standard mechanism (like everything the goodness depends how one uses it), we cannot be completely sure about all implications for all use cases around the world. And concerning security better to be conservative.

We don't have to - if this verifier is supposed to work like the WebTransport spec describes - then just implement what the spec requires and let the spec worry about all those use cases in the wild. This is what web browsers will do.

If your concern is an uncertainty with your particular implementation of the verifier, I would expect a different - unaudited feature flag - to shield this feature, if any.

The reason ring requires specifying the dangerous flag when providing custom verifier is due to a lot of fuck-ups seen in the wild when various people decided to implement their own certificate chain verification logic (or lack of). Not verifying the certificate at all is not considered dangerous - only a broken custom implementations are. In this case, however, this is a candidate for a standardised algorithm - so it had received a certain level of due diligence. It is not indeed the standard yet - but in this sense the whole thing (WebTransport itself) is not a standard.

To sum up - this is fine, and enabling a feature is annoying. This is supposed to be a part of the API.

A more important concern is this though:

Web PKI enforces an expiry period requirement on the certificates. This requirement limits the scope of potential key compromise; it also forces server operators to design systems that support and actively perform key rotation. For this reason, WebTransport imposes a similar expiry requirement; as the certificates are expected to be ephemeral or short-lived, the expiry period is limited to two weeks. The two weeks limit is a balance between setting the expiry limit as low as possible to minimize consequences of a key compromise, and maintaining it sufficiently high to accomodate for clock skew across devices, and to lower the costs of synchronizing certificates between the client and the server side.

(from https://w3c.github.io/webtransport/#certificate-hashes)

So far, wtransport provides pretty poor support for key/certificate rotation, which I has to work around with the low-level quinn access. This is fine for me because I know what I'm doing - but I'm sure a lot of people out there don't, and the API for the server available today does not push people to do it properly. It is probably not the right place for the wtransport to implement this, as the rustls ecosystem has a lot of ready-made tools that could be used off the shelf with wtransport today.

I'd argue that, if anything, with_identity should be under a feature flag. But if you ask me, in regard to "if anything" - this should not require a feature flag either. There should just be a big red alert in the doc for the with_identity telling people this will only work for two weeks if the client is connecting with the server cert hashes.

And maybe some sort of support for providing a new certificate / keypair.

I use my https://github.com/MOZGIII/rustls-cert-utils for a lot of servers that are powered by rustls - wtransport, quinn, hyper - and it integrates nicely with my typical certificate management infrastructure of Kubernetes + cert-manager flow. In some cases there is a need for something exotic of course, but this covers 90% of my cases. There are other tools like this too - like various options for ACME flows for getting a certificate, sometimes even on-the-fly.

BiagioFesta commented 2 months ago

I am not sure we can consider WebTransport or W3C spec a "standard" at the moment, do we? I was tempted to consider those as drafts, pretty new, pretty in evolution.

Anyway, nobody said serverCertificateFingerprints is dangerous, or worse, or better authentication mechanism.

Actually, I though to have been quite clear on that point, quoting myself:

[...] This does not necessarily means better or worse (or even dangerous) in the strict sense.

Thus I agree, the naming of the feature "dangerous" might be miss-leading from the users perspective.

Nevertheless, I was tempted to say that users should be aware of the usage of an authentication mechanism that can have potentially an impact on their system security as:

A feature flag (with a better name perhaps) might give more visibility on those criticality, but I don't have a strong opinion on that.

So far, wtransport provides pretty poor support for key/certificate rotation [...]

This is instead a different nevertheless interesting point. Can you be more specific on that. What API quinn provides for that, wtransport does not?

MOZGIII commented 2 months ago

Oh damn, I'm sorry, somehow misread your message completely 👍

So far, wtransport provides pretty poor support for key/certificate rotation [...]

This is instead a different nevertheless interesting point. Can you be more specific on that. What API quinn provides for that, wtransport does not?

I was referring to the with_identity at the server - however, it seems like the Endpoint::reload_config solves this issue. This is an API that doesn't exist at quinn really. In fact, upon close inspection, quinn has none of those builder APIs.

I was thinking that the with_identity setup could actually have a direct support for using keys from some sort of source that can reload them - but then it is not really needed since we have all the rustls ecosystem goodies.

Actually, I think the presence a rich builder APIs around the server and client config is what throws me off. If they actually were absent and would require users to plug in external solutions without wtransport offering its own implementations - it would be more likely that the users pick something that provides more flows.

It is quite awkward to bring this point up, cause it is sort of asking for less API surface. Especially since I often find it useful myself for the toy stuff - one doesn't need production setup for everything. So those APIs are quite useful. It is just with them you have to manually add a reload loop around your server, and it is not like you can skip it or forget about it. But also, adding the API for taking care of reloading the config to wtransport would just add to the API "bloat" that builders create. So, idk, there's no clean solution here. Nvm I guess?

BiagioFesta commented 2 months ago

Yeah that's an interesting point. Indeed, Endpoint::reload_config is mainly there because of certificates reload (as also the doc explicetly mentioned).

Certificate rotation/reload is a common scenario for many server applications. As also you mentioned, existing solutions (integrated with rustls) and/or daemon utilities for automatic certificate handling fill this gaps quite nicely (coding impact is minimal I guess, and it allows user to integrate their best-case solution).

wtransport provides a minimal layer of integration with rustls (via tls module). This modules exposes a minimal but very user-friendly interface, focusing those users who have minimal knowledge on TLS stack (and thus, keeping reasonable good defaults).

Therefore, having an working-out-of-the-box solution directly accessible from wtransport for certificate rotation might be a valuable point for the library. Definitely something we might consider in the future developments.