tls: support for ECDSA P-384 and P-521 certificates

[anitabyte]

Commit Message: tls: support for ECDSA P-384 and P-521 certificates (#10855)

Additional Description: Commercial National Security Algorithm Suite (CNSA) requires ECDSA keys be specified with P-384 curves. The assertion that there are no security benefits to curves higher than P-256 is no longer true. This change is intended to limit the adoptable curves to P-384 and P-521.

Risk Level: Medium - removal of limitation of curves to be used for ECDSA certificates, with potential misconfiguration and DoS risks mentioned in previous discourse on the issue. This risk is mitigated in this PR, however, by continuing to expressly limit the type of EC keys accepted to those associated with the P-256, P-384 or P-521 curves and no others.

Testing: Testing using unit and integration tests

Ran build envoy artefact locally with below config:

---
admin:
  address:
    socket_address:
      address: 127.0.0.1
      port_value: 9901
static_resources:
  listeners:
    - name: listener_0
      address:
        socket_address:
          address: 127.0.0.1
          port_value: 10000
      filter_chains:
        - filters:
            - name: envoy.filters.network.http_connection_manager
              typed_config:
                "@type": type.googleapis.com/envoy.extensions.filters.network.http_connection_manager.v3.HttpConnectionManager
                stat_prefix: ingress_http
                codec_type: AUTO
                route_config:
                  name: local_route
                  virtual_hosts:
                    - name: local_service
                      domains:
                        - "*"
                      routes:
                        - match:
                            prefix: /
                          route:
                            cluster: some_service
                http_filters:
                  - name: envoy.filters.http.router
                    typed_config:
                      "@type": type.googleapis.com/envoy.extensions.filters.http.router.v3.Router
          transport_socket:
            name: envoy.transport_sockets.tls
            typed_config:
              "@type": type.googleapis.com/envoy.extensions.transport_sockets.tls.v3.DownstreamTlsContext
              common_tls_context:
                tls_certificates:
                - certificate_chain: {filename: "test/common/tls/test_data/selfsigned_ecdsa_p384_cert.pem"}
                  private_key: {filename: "test/common/tls/test_data/selfsigned_ecdsa_p384_key.pem"}
  clusters:
    - name: some_service
      connect_timeout: 0.25s
      type: STATIC
      lb_policy: ROUND_ROBIN
      load_assignment:
        cluster_name: some_service
        endpoints:
          - lb_endpoints:
              - endpoint:
                  address:
                    socket_address:
                      address: 127.0.0.1
                      port_value: 1234

Ran openssl s_client 127.0.0.1:10000:

Connecting to 127.0.0.1
CONNECTED(00000003)
Can't use SSL_get_servername
depth=0 C=US, ST=California, L=San Francisco, O=Lyft, OU=Lyft Engineering, CN=Test Server
verify error:num=18:self-signed certificate
verify return:1
depth=0 C=US, ST=California, L=San Francisco, O=Lyft, OU=Lyft Engineering, CN=Test Server
verify return:1
---
Certificate chain
 0 s:C=US, ST=California, L=San Francisco, O=Lyft, OU=Lyft Engineering, CN=Test Server
   i:C=US, ST=California, L=San Francisco, O=Lyft, OU=Lyft Engineering, CN=Test Server
   a:PKEY: id-ecPublicKey, 384 (bit); sigalg: ecdsa-with-SHA256
   v:NotBefore: Aug 21 19:14:10 2024 GMT; NotAfter: Aug 21 19:14:10 2026 GMT
---
Server certificate
-----BEGIN CERTIFICATE-----
MIIC0jCCAlegAwIBAgIUUv13YuIFYMJxp1t4z8Z7H0cFdHowCgYIKoZIzj0EAwIw
ejELMAkGA1UEBhMCVVMxEzARBgNVBAgMCkNhbGlmb3JuaWExFjAUBgNVBAcMDVNh
biBGcmFuY2lzY28xDTALBgNVBAoMBEx5ZnQxGTAXBgNVBAsMEEx5ZnQgRW5naW5l
ZXJpbmcxFDASBgNVBAMMC1Rlc3QgU2VydmVyMB4XDTI0MDgyMTE5MTQxMFoXDTI2
MDgyMTE5MTQxMFowejELMAkGA1UEBhMCVVMxEzARBgNVBAgMCkNhbGlmb3JuaWEx
FjAUBgNVBAcMDVNhbiBGcmFuY2lzY28xDTALBgNVBAoMBEx5ZnQxGTAXBgNVBAsM
EEx5ZnQgRW5naW5lZXJpbmcxFDASBgNVBAMMC1Rlc3QgU2VydmVyMHYwEAYHKoZI
zj0CAQYFK4EEACIDYgAEtFQWaGrCFUT70YVGv9IA0H1d/fUGdoATjqAQlgOnzWf4
FcJIqRQ8dGJ0wom/p8b/3MrKpy8wpWBnAo2C9+9owGdOqcqSIFLVV0iaGogKhIAx
7KAjWoMEpal4uNnaYLlCo4GdMIGaMAwGA1UdEwEB/wQCMAAwCwYDVR0PBAQDAgXg
MB0GA1UdJQQWMBQGCCsGAQUFBwMCBggrBgEFBQcDATAeBgNVHREEFzAVghNzZXJ2
ZXIxLmV4YW1wbGUuY29tMB0GA1UdDgQWBBQ23kFgk8ELq1P0xW3R8SYRwJRcyjAf
BgNVHSMEGDAWgBQ23kFgk8ELq1P0xW3R8SYRwJRcyjAKBggqhkjOPQQDAgNpADBm
AjEA6FC5eEaKcV7i9AUuVsIJruDKqLVmSLKzHX+DVxOvaxQcTuKMwtg8AuTq1qq+
MZ8EAjEA3JKxxjQAp2hi2gvSUGXQqk3seETImDNmUdWXmYcohDRM36KKJORqXoui
jD+/8ipt
-----END CERTIFICATE-----
subject=C=US, ST=California, L=San Francisco, O=Lyft, OU=Lyft Engineering, CN=Test Server
issuer=C=US, ST=California, L=San Francisco, O=Lyft, OU=Lyft Engineering, CN=Test Server
---
No client certificate CA names sent
Peer signing digest: SHA384
Peer signature type: ECDSA
Server Temp Key: X25519, 253 bits
---
SSL handshake has read 1062 bytes and written 379 bytes
Verification error: self-signed certificate
---
New, TLSv1.3, Cipher is TLS_AES_256_GCM_SHA384
Server public key is 384 bit
This TLS version forbids renegotiation.
Compression: NONE
Expansion: NONE
No ALPN negotiated
Early data was not sent
Verify return code: 18 (self-signed certificate)
---

Docs Changes: Changes made to reference that P-384 and P-521 certificates now are usable.

Fixes #10855

[repokitteh-read-only]

Hi @anitabyte, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #36369 was opened by anitabyte.

see: more, trace.

[repokitteh-read-only]

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #36369 was opened by anitabyte.

see: more, trace.

[anitabyte]

From CI runs:

Code coverage for source/common/quic is lower than limit of 93.5 (93.4)

I haven't touched Quic in this PR - but will investigate and see if there's anything I can do to resolve.

[sabershahhoseini]

I just have task which needs these curves to be supported. Would be great if it got merged sooner!

[kyessenov]

Does this have an impact on FIPS validation? AFAIR the restrictions on the curves was also due to FIPS validated algorithms.

[anitabyte]

My knowledge of FIPS is limited, but I believe FIPS 186-5 refers to NIST SP800-186, which does include these curves. I don't believe that this would impact FIPS compliance by being merged, especially as we aren't changing defaults.

[anitabyte]

Merged from main; all checks now pass

[kyessenov]

@ggreenway Do you recall why only P-256 curve as supported initially? I think it's the only FIPS one back in the days, was it because of that?

[ggreenway]

@ggreenway Do you recall why only P-256 curve as supported initially? I think it's the only FIPS one back in the days, was it because of that?

#10855 (comment) explains it

[ggreenway]

/wait

[ggreenway]

Does this have an impact on FIPS validation? AFAIR the restrictions on the curves was also due to FIPS validated algorithms.

FIPS is a separate build, and we can put additional restrictions there if needed. I don't know if FIPS says anything about certificate types or not.

[ggreenway]

I removed the fixed issue number from the title because otherwise when this is merged, it'll have two numbers in parentheses where the PR number usually is. The fixed issue number is part of the body of the commit message.

[anitabyte]

Changes requested addressed with some further context in commit messages.

[ggreenway]

This is looking much better. Please add test coverage for the new code, especially the case of a P-521 cert and a client that supports P-256/P-384, and make sure it falls back to RSA.

Also I think this would self-document better if you add a using CurveNIDSupportedVector = std::vector<int>; and using that throughout, so it's clear what the int is.

Because it can currently only ever have 3 elements, it could be an absl::InlinedVector<int, 3> instead.

/wait

[anitabyte]

Integration tests for client not supporting P-256 curves and client support P-256 and P-384 but server configured with a P-521 cert added. Would welcome comments on if there is anything more elegant I can do where I've have to call CBS_Init again to reset the position of a CBS when checking curve capabilities and sigalgs. Also, if the approach I've taken to replicating using CurveNIDSupportedVector everywhere the type is used is good style or not: I had read that exports of aliases wasn't good practice. Thanks again for your patience.

[ggreenway]

/wait

[ggreenway]

Can you elaborate on why this is needed? I don't understand. I would have guessed that setting the signature algorithms on the client would be enough, and leave the curves as the default.

[anitabyte]

This appears to be required as the only time that the curves are set for the contexts is in this line, which uses the ECDH settings to set the supported curves. As the defaults enabled are X25519 and P-256, we don't get a P-384+ context if we don't explicitly enable them for ECDH curves on the server.

[anitabyte]

Technically, the P-256 curve isn't required - the test would pass with P-384 only enabled, but we do need to explicitly enable it.

[ggreenway]

But the curves are used for ECDH to generate ephemeral keys; I don't think they should matter for certificate selection and validation. And for that, I believe only the signature algorithms are used.

[anitabyte]

I think I may have been thinking of something else here: our client for this test explicitly does not support P-256 for ECDH negotiation. We have to add the P-384 curve for the purposes of ECDH so that the client has a curve that it agrees on with the server to be able to do the ECDH negotiation. My earlier point about the defaults giving us only P-256 and X25519 curves available for that purpose stands.

I'll validate if the other tests pass without the specification of specific server curves.

[ggreenway]

At least in tls 1.3, I don't think you should be setting supported curves on either client or server, you should be setting the signing algorithms on the client for what we're trying to test. I'm not sure if tls 1.2 co-mingles these or not.

[anitabyte]

They do: the client without P-256 support does, however, require the specification of a server-side curve that is does support. New commit has been pushed removing the server curve specification from the tests that don't need it.

[anitabyte]

TLSv1.3 specifications of curves have been removed from tests as they are unnecessary. TLSv1.2 requires the explicit definition of the curves in the client: tests fail where they are not specified in the all-curves client tests where they are not specified.

[ggreenway]

LGTM, thanks for all the work on this!

However, with the release today, there's now a conflict on the release notes file. Please resolve and we'll get this merged.

/wait

[ggreenway]

Not sure what happened, but you needed to merge main and resolve the conflict, to make github happy with it.

[anitabyte]

I was trying to take a lighter touch and embarrassed myself. This should be good now, hopefully.

[anitabyte]

There's a formatting problem, I'll fix it now: apologies, I'm not at my usual computer that actually has the toolchain set up.

[anitabyte]

Formatting CI tasks have completed: I don't think there's anything else in the changes that should trip us up.