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

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

Open
MOZGIII opened this issue Apr 20, 2024 · 5 comments
Labels
enhancement New feature or request

Comments

@MOZGIII
Copy link
Contributor

MOZGIII commented Apr 20, 2024

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

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

MOZGIII commented Apr 20, 2024

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

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:

  • It is "different" from the "usual" (well, maybe documentation is enough to clarify that).
  • It is not as well-tested as the "usual". (As you mentioned) It uses an internal (wtransport) validator which is generally less tested/adopted by usual default in the million-downloaded-rustls. And thus, more subject to bugs.
  • It comes from draft-specs which are small-scaled adopted. WebTransport visibility/scale is still so low, it is not possible to compare with other well-tested mechanisms used in the last decades.

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

MOZGIII commented Apr 21, 2024

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

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.

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

No branches or pull requests

2 participants