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

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

anitabyte
Copy link

@anitabyte anitabyte commented Sep 27, 2024

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

Copy link

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.

Copy link

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 anitabyte marked this pull request as ready for review September 28, 2024 07:20
@anitabyte
Copy link
Author

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

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

@kyessenov
Copy link
Contributor

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

@anitabyte
Copy link
Author

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.

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.

Support P-384 and P-521 Server ECDSA Certificates
3 participants