FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Clear certificate chain in setKeyMaterial. by mawiwill · Pull Request #987 · netty/netty-tcnative · GitHub

Clear certificate chain in setKeyMaterial. - #987

Merged
normanmaurer merged 5 commits into
netty:mainfrom
mawiwill:mawiwill/idempotent-cert-cb
Jun 10, 2026
Merged

normanmaurer merged 5 commits into
netty:mainfrom
mawiwill:mawiwill/idempotent-cert-cb

Conversation

mawiwill commented Jun 9, 2026
edited
Loading

Copy link
Copy Markdown
Contributor

Fixes #985.

Netty calls SSL.setKeyMaterial in its server-side certificate callback (https://docs.openssl.org/3.6/man3/SSL_CTX_set_cert_cb/ but the same going back to 1.0.2). OpenSSL invokes certificate callbacks after the server receives a ClientHello.

TLS 1.2 TLS sessions usually had one ClientHello, so this callback would usually run once. TLS 1.3, however, might begin a session with multiple ClientHellos as the result of
HelloRetryRequests. SSL.setKeyMaterial adds chained certificates (intermediates and roots) to the per-connection SSL struct with SSL_add1_chain_certs. This function appends certificates, so every time a ClientHello arrives, setKeyMaterial would re-add the same chain. Finicky clients fail if they receive the same intermediate twice.

This change allows netty-tcnative to talk TLS 1.3 with these clients. It clears the SSL struct's certificate chain (that is, everything after the certificate assigned with SSL_use_certificate) on every setKeyMaterial call. Multiple ClientHellos will result in the same key meterial appearing in Certificate responses.

Note that this should improve correctness for TLS 1.2, as well, since renegotiation for e.g. rekeying would have suffered from the same duplicate intermediates issue.

Fixes netty#985.

Netty calls SSL.setKeyMaterial in its server-side certificate
callback (https://docs.openssl.org/3.6/man3/SSL_CTX_set_cert_cb/ but
the same going back to 1.0.2). OpenSSL invokes certificate callbacks
after the server receives a ClientHello.

TLS 1.2 TLS sessions usually had one ClientHello, so this callback
would usually run once. TLS 1.3, however, might begin a session with
multiple ClientHellos as the result of
HelloRetryRequests. SSL.setKeyMaterial adds chained
certificates (intermediates and roots) to the per-connection SSL
struct with SSL_add1_chain_certs. This function appends certificates,
so every time a ClientHello arrives, setKeyMaterial would re-add the
same chain. Finicky clients fail if they receive the same intermediate
twice.

This change allows netty-tcnative to talk TLS 1.3 with these
clients. It clears the SSL struct's certificate chain (that is,
everything after the certificate assigned with SSL_use_certificate) on
every setKeyMaterial call. Multiple ClientHellos will result in the
same key meterial appearing in Certificate responses.

Note that this should improve correctness for TLS 1.2, as well, since
renegotiation for e.g. rekeying would have suffered from the same
duplicate intermediates issue.
mawiwill force-pushed the mawiwill/idempotent-cert-cb branch from 0ce2e0a to ae55677 Compare June 9, 2026 01:10
mawiwill changed the title Clear certificate chain in setKeyMaterial (#985). Clear certificate chain in setKeyMaterial. Jun 9, 2026
#if !defined(OPENSSL_IS_BORINGSSL) && !defined(OPENSSL_IS_AWSLC)
#define tcn_SSL_add1_chain_cert(ssl, x509) SSL_add1_chain_cert(ssl, x509)
#define tcn_SSL_add0_chain_cert(ssl, x509) SSL_add0_chain_cert(ssl, x509)
#define tcn_SSL_clear_chain_certs(ssl) SSL_clear_chain_certs(ssl)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Why do we even need this and can't just call SSL_clear_chain_certs directly ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Only to mimic the code that's there already. I'm happy to remove it!

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

We only use our own defines if we need to use different APIs depending on which version / lib is used which seems to be not the case here and so we should just use SSL_clear_chain_certs directly.

Comment thread openssl-dynamic/src/main/c/ssl.c Outdated
Co-authored-by: Norman Maurer <norman_maurer@apple.com>

Copy link
Copy Markdown
Member

@mawiwill did you sign our ICLA yet ? https://netty.io/s/icla

Copy link
Copy Markdown
Member

@mawiwill PTAL

mawiwill commented Jun 9, 2026

Copy link
Copy Markdown
Contributor Author

ICLA

Just signed the ICLA!

chrisvest left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Just one small nit

Comment thread openssl-dynamic/src/main/c/ssl.c Outdated
Comment thread openssl-dynamic/src/main/c/ssl.c Outdated

mawiwill commented Jun 9, 2026

Copy link
Copy Markdown
Contributor Author

@mawiwill PTAL

This definitely works for me! I have a minor question about support OpenSSL versions, mostly out of curiosity. Thanks for your quick work on the PR, I really appreciate it!

normanmaurer merged commit 02d0d03 into netty:main Jun 10, 2026
10 checks passed

Copy link
Copy Markdown
Member

@mawiwill thanks a lot!

normanmaurer added this to the 2.0.79.Final milestone Jun 10, 2026
dongjoon-hyun added a commit to apache/spark that referenced this pull request Jun 24, 2026
### What changes were proposed in this pull request?

This PR aims to upgrade `netty-tcnative` to 2.0.79.Final.

### Why are the changes needed?

To bring the latest bug fixes. `netty-tcnative` 2.0.79.Final fixes BoringSSL
compatibility issues (incompatible weak symbol declarations and stale
`SSL_CREDENTIAL` support checks for older BoringSSL) and clears the certificate
chain in `setKeyMaterial`.
- https://github.com/netty/netty-tcnative/milestone/116 (2026-06-12)
  - netty/netty-tcnative#987
  - netty/netty-tcnative#986
  - netty/netty-tcnative#980

### Does this PR introduce _any_ user-facing change?

No.

### How was this patch tested?

Pass the CIs.

### Was this patch authored or co-authored using generative AI tooling?

Generated-by: Claude Opus 4.8

Closes #56748 from dongjoon-hyun/SPARK-57668.

Authored-by: Dongjoon Hyun <dongjoon@apache.org>
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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.

HelloRetryRequest with TLS 1.3 can result in duplicate intermediate certs when tcnative is used with OpenSSL

3 participants


Back | FazBrowse Home | New Git URL