[ssl] Server side handshaker factory stores a map of key signers#42002
Closed
rockspore wants to merge 14 commits into
Closed
[ssl] Server side handshaker factory stores a map of key signers#42002rockspore wants to merge 14 commits into
rockspore wants to merge 14 commits into
Conversation
---- DO NOT SUBMIT. This PR is for testing purposes only. [cl/893799990](http://cl/893799990) PiperOrigin-RevId: 893799990
Automated fix for refs/heads/server-signer-fix
matthewstevenson88
left a comment
Contributor
There was a problem hiding this comment.
Thanks for the PR! A couple questions to make sure I understand before completing the full review.
rockspore
commented
Apr 7, 2026
rockspore
left a comment
Contributor
Author
There was a problem hiding this comment.
Thanks for reviewing.
matthewstevenson88
left a comment
Contributor
There was a problem hiding this comment.
Please also fix the failing sanity tests.
Automated fix for refs/heads/server-signer-fix
| // Verifies that the server can successfully offload signing to an asynchronous | ||
| // custom signer. | ||
| TEST_F(TlsPrivateKeyOffloadTest, OffloadWithCustomKeySignerAsync) { | ||
| TEST_F(TlsPrivateKeyOffloadTest, OffloadWithCustomKeySignerAsyncWithSniMatch) { |
Contributor
There was a problem hiding this comment.
Sorry I forgot to make the analogous comment here: should we also check that the peer name matches the SNI in the e2e tests?
Contributor
Author
There was a problem hiding this comment.
Note that we should not use the TSI-layer helper function here any more, so checking the SNI match is a bit clumsy but I added it.
JFYI, the bad signer added to the other cert that's not supposed to be picked also ensures the test goes the right way.
Contributor
There was a problem hiding this comment.
Do we need the TSI-layer helper? We could just extract the analogous property from the auth context and compare to the hardcoded string.
Don't want to block on this, I think the PR is generally fine, but leaving this open to get your thoughts.
Contributor
Author
There was a problem hiding this comment.
Sorry I already added what you suggested. I just wanted to say that there were multiple SANs so we had to try matching each, and SAN can have wildcard. So it's less elegant than the unit test where we call a helper to get a boolean straighforwardly.
matthewstevenson88
approved these changes
Apr 8, 2026
markdroth
approved these changes
Apr 9, 2026
markdroth
left a comment
Member
There was a problem hiding this comment.
The fix looks good! My comments are mostly things to improve readability.
Please let me know if you have any questions. Thanks!
rockspore
commented
Apr 9, 2026
rockspore
left a comment
Contributor
Author
There was a problem hiding this comment.
Thanks for the review, Mark!
markdroth
approved these changes
Apr 9, 2026
rockspore
added a commit
to rockspore/grpc
that referenced
this pull request
Apr 9, 2026
copybara-service Bot
pushed a commit
that referenced
this pull request
May 1, 2026
… correctly. It was removed in #42002 and caused the `result` to be ignored after the ssl contexts get configured. ---- DO NOT SUBMIT. This PR is for testing purposes only. [cl/908809008](http://cl/908809008) PiperOrigin-RevId: 908809008
copybara-service Bot
pushed a commit
that referenced
this pull request
May 4, 2026
… correctly. It was removed in #42002 and caused the `result` to be ignored after the ssl contexts get configured. ---- DO NOT SUBMIT. This PR is for testing purposes only. [cl/908809008](http://cl/908809008) PiperOrigin-RevId: 908809008
copybara-service Bot
pushed a commit
that referenced
this pull request
May 5, 2026
… correctly. It was removed in #42002 and caused the `result` to be ignored after the ssl contexts get configured. ---- DO NOT SUBMIT. This PR is for testing purposes only. [cl/908809008](http://cl/908809008) PiperOrigin-RevId: 908809008
copybara-service Bot
pushed a commit
that referenced
this pull request
May 5, 2026
… correctly. It was removed in #42002 and caused the `result` to be ignored after the ssl contexts get configured. ---- DO NOT SUBMIT. This PR is for testing purposes only. [cl/908809008](http://cl/908809008) PiperOrigin-RevId: 908809008
copybara-service Bot
pushed a commit
that referenced
this pull request
May 6, 2026
It was removed in #42002 and caused the `result` to be ignored after the ssl contexts get configured. PiperOrigin-RevId: 911317681
asheshvidyut
pushed a commit
to a-detiste/grpc
that referenced
this pull request
Jun 10, 2026
…c#42002) The server takes a vector of key/cert pairs for SNI. So it could have the same number of private key signers. Right now it keeps only one, which is incorrect. This is a bug fix so it could be worth a patch release as suggested by @gtcooke94. One open question is whether we should store the `key_signer` in the handshaker. As of now , I still find the code to be easier to read and manage by doing that, especially when we introduce the `key_signer` from the certificate selector later on. But I acknowledge in this case, key signers are 1-1 mapped to SSL_CTXs and there is some overhead by copying it to each handshaker and the ownership also gets less clear. Closes grpc#42002 COPYBARA_INTEGRATE_REVIEW=grpc#42002 from rockspore:server-signer-fix 971bfa7 PiperOrigin-RevId: 908273778
asheshvidyut
pushed a commit
to a-detiste/grpc
that referenced
this pull request
Jun 10, 2026
It was removed in grpc#42002 and caused the `result` to be ignored after the ssl contexts get configured. PiperOrigin-RevId: 911317681
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
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The server takes a vector of key/cert pairs for SNI. So it could have the same number of private key signers. Right now it keeps only one, which is incorrect.
This is a bug fix so it could be worth a patch release as suggested by @gtcooke94.
One open question is whether we should store the
key_signerin the handshaker. As of now , I still find the code to be easier to read and manage by doing that, especially when we introduce thekey_signerfrom the certificate selector later on. But I acknowledge in this case, key signers are 1-1 mapped to SSL_CTXs and there is some overhead by copying it to each handshaker and the ownership also gets less clear.