microsoft / msquic

Cross-platform, C implementation of the IETF QUIC protocol, exposed to C, C++, C# and Rust.
MIT License
4.05k stars 530 forks source link

Allow simple configuration of supported TLS1.3 groups #2374

Open baentsch opened 2 years ago

baentsch commented 2 years ago

Describe the feature you'd like supported

TLS1.3 permits announcing algorithm groups used during key exchange via the supported_groups extension.

The feature requested is to permit setting these groups for msquic applications to different values via some simple (command line or environment variable) manner to allow easy adaptation to different group (lists).

msquic currently fixes this list to 4 groups as per this code.

Proposed solution

An example how it may be implemented is the openssl -groups parameter.

Additional context

While the chosen msquic preference list is somewhat understandable from a classic cryptography point of view, some users may want to request stronger classic algorithms, e.g., by leaving away weaker algorithms like "P-256". In the context of quantum safe cryptography where many different algorithms could be chosen by way of specifying different group names, the issue is even more acute.

nibanks commented 2 years ago

This would require updating the QUIC_CREDENTIAL_CONFIG to allow the app to pass the allowable groups down. It'd be nice to support a model similar to how we do QUIC_ALLOWED_CIPHER_SUITE_FLAGS but I think the number of groups might be too big.

baentsch commented 2 years ago

It'd be nice to support a model similar to how we do QUIC_ALLOWED_CIPHER_SUITE_FLAGS but I think the number of groups might be too big.

Agreed, this would be "boundless". But why limit/hard-code the (set of) allowable groups in this way? Why not simply adding a (colon-delimited) string of default groups to QUIC_CREDENTIAL_CONFIG? That way everyone can define the list as short (my preference :) or long as s/he wants. To rephrase: Why not move CXPLAT_TLS_DEFAULT_SSL_CURVES into the QUIC_CREDENTIAL_CONFIG struct (as --then modifiable-- string, not as a set of pre-ordained bits à la "QUIC_ALLOWED_CIPHER_SUITE_FLAGS")?

For completeness, here's the diff to the current code base showing how I implemented this feature for now:

diff --git a/src/platform/tls_openssl.c b/src/platform/tls_openssl.c
index 42d6ee70..6f2ccc82 100644
--- a/src/platform/tls_openssl.c
+++ b/src/platform/tls_openssl.c
@@ -1173,7 +1173,7 @@ CxPlatTlsSecConfigCreate(
     Ret =
         SSL_CTX_set1_groups_list(
             SecurityConfig->SSLCtx,
-            CXPLAT_TLS_DEFAULT_SSL_CURVES);
+            getenv("SSL_DEFAULT_GROUPS")?getenv("SSL_DEFAULT_GROUPS"):CXPLAT_TLS_DEFAULT_SSL_CURVES);
     if (Ret != 1) {
         QuicTraceEvent(
             LibraryErrorStatus,

This env-var based approach is completely in line with --for example-- the SSL_CERT_FILE environment variable to define the behaviour (root of trust in this case) for openssl.

Adding this into the code base of openssl would be another approach, but as msquic is actively setting the groups list, overriding this in the underlying library doesn't seem proper -- assuming the calling library/app (msquic in this case) has a good reason for setting specific groups. Without such reason, it might even be a more simple approach to close out this issue by completely removing the call to SSL_CTX_set1_groups_list to give back control over this configuration setting to the underlying crypto library (openssl in this case).

So, may I ask why msquic opted to fix the current key exchange groups?

nibanks commented 2 years ago

why msquic opted to fix the current key exchange groups?

We've had no request (until now) to expose a knob to control the value.

Why not simply adding a (colon-delimited) string of default groups to QUIC_CREDENTIAL_CONFIG?

Yes, I believe that's the direction we'll have to go, but the complexity is Schannel. The MsQuic API supports multiple TLS providers, and anything we expose at the MsQuic API needs to map to them all. So we'd have to write custom code to map that string list into whatever Schannel takes as input.

baentsch commented 2 years ago

We've had no request (until now) to expose a knob to control the value.

That I understand. My question is more about what was the rationale for msquic to "overrule" the defaults of openssl, i.e., to call SSL_CTX_set1_groups_list at all?

into whatever Schannel takes as input.

Hmm, I just tried to find out how Schannel handles/configures key exchange group (announcements), but had no luck... Initially I even got the impression that TLS1.3 isn't really supported -- but then again it couldn't be used for QUIC at all :) So, I can't really help on this one.

nibanks commented 2 years ago

call SSL_CTX_set1_groups_list at all?

I was under the impression I was supposed to call it. 😄 Probably just a copy/paste from some other code.

Initially I even got the impression that TLS1.3 isn't really supported

It's supported in Windows Server 2022 and Windows 11 (it may eventually be backported to Windows 10, but I'm not sure).

I just tried to find out how Schannel handles/configures key exchange group (announcements), but had no luck

It looks like we'd set key exchange via the TlsParametersCngAlgUsageKeyExchange in the CRYPTO_SETTINGS, similar to how we do cipher suits.

baentsch commented 2 years ago

I was under the impression I was supposed to call it. smile Probably just a copy/paste from some other code.

I don't think so. We could close out the whole discussion (and save some work) by you removing this call and me adding the capability into (oqs-)openssl.

This approach would also resolve the "hidden" issue namely that msquic allows/enforces unnecessarily "weak" (in the eye of the beholder :) crypto (key exchange groups) -- at least when using openssl. It might be "interesting" to check what Schannel does by default in this regard, though....

nibanks commented 2 years ago

Feel free to send a PR to remove that code. If the CI all passes still, that'd probably be fine to merge. I still think it's probably worth while to expose a knob to the app eventually though.

baentsch commented 2 years ago

Feel free to send a PR to remove that code.

Sorry -- I'd have done that already (would've saved a lot of typing :) if I'd been OK with the contribution conditions to this project. Hope this doesn't cause you to look upon our interaction with disappointment or disdain now.

I still think it's probably worth while to expose a knob to the app eventually though.

It definitely is. Until then I keep my env var "solution".

nibanks commented 2 years ago

if I'd been OK with the contribution conditions to this project.

I'd be curious to know what issue(s) you have with the agreement. I can try to forward any feedback you have along to the owners of that. Perhaps it's something that can be modified in the future.

Hope this doesn't cause you to look upon our interaction with disappointment or disdain now.

It does not. 😄 We still appreciate working with you!

baentsch commented 2 years ago

Perhaps it's something that can be modified in the future.

Highly improbable (I've been working for more than 2 decades for a US IT company :). More seriously: I'm missing the "AS-IS" and "no support obligation" language I know from (and like in) the OpenSSL ICLA. But more generally, this conceptually doesn't sound right: In order to do free and voluntary work for a company generating multi-billion-dollar profits one needs to provide guarantees and assurances to that company?? Shouldn't that be the other way around? For example, when working (equally free and voluntary) for the Red Cross I a) don't have to sign such stinging contracts, b) don't get tied to a US court and --believe it or not-- c) get insurance cover from the Red Cross when working for them.

baentsch commented 2 years ago

Rethinking options for the above, might it be an idea for @christianpaquin to take a look at the above (basically remove https://github.com/microsoft/msquic/blob/692be2372f94b82ff2e0567ecbbf631a4bbf0a2c/src/platform/tls_openssl.c#L1173-L1185 and check CI via a PR)? This would ease activation of liboqs algorithms in msquic substantially and the MSFT contract wouldn't need to be signed by someone already employed, right?

christianpaquin commented 2 years ago

Rethinking options for the above, might it be an idea for @christianpaquin to take a look at the above (basically remove

Back in the office and just catching up on that, sorry for the delay. I could for sure help creating PRs for this. Is there further work to be done beyond #2393? @baentsch, we can sync up and discuss the best approach.

baentsch commented 2 years ago

Thanks for the offer @christianpaquin. Together with https://github.com/open-quantum-safe/openssl/pull/354, the above PR https://github.com/microsoft/msquic/pull/2393 (thanks again, @nibanks for that), enables a complete (all OQS algorithms) and automated OQS/(ms)QUIC integration (see https://github.com/open-quantum-safe/oqs-demos/blob/main/quic/Dockerfile-client for all details). As far as I'm concerned, this issue could thus be closed -- unless an "environment variable free" oqs-msquic config setup would be desirable, e.g., for more "msquic performance experimentation" with quantum-safe algorithms. For such experiments participation of people with more QUIC knowledge would be best (e.g. @nibanks , @igorbarshteyn).