tls: copy client CAs and cert store on CertCb#3537
Closed
Uh oh!
There was an error while loading. Please reload this page.
Conversation
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
src/node_crypto.cc Outdated
Member
There was a problem hiding this comment.
You should check the return code here.
MemberAuthor
There was a problem hiding this comment.
Good idea, though it can't fail in current OpenSSL implementation.
Member
bnoordhuis commented Oct 27, 2015
Left some comments. The commit log could go into more detail into why this change is necessary. |
MemberAuthor
indutny commented Oct 27, 2015
@bnoordhuis pushed fixes, thanks! |
MemberAuthor
indutny commented Oct 27, 2015
MemberAuthor
indutny commented Oct 27, 2015
CI seems to be green, LGTY @bnoordhuis ? |
Copy client CA certs and cert store when asynchronously selecting `SecureContext` during `SNICallback`. We already copy private key, certificate, and certificate chain, but the client CA certs were missing. Fix: nodejs#2772
MemberAuthor
indutny commented Oct 27, 2015
@bnoordhuis updated commit message too |
src/node_crypto.cc Outdated
Member
There was a problem hiding this comment.
Can you add a comment explaining that SSL_set_client_CA_list takes ownership of the duplicate? And maybe explain why you copy it from the SSL_CTX to the SSL?
MemberAuthor
indutny commented Nov 12, 2015
All fixed, PTAL @bnoordhuis |
MemberAuthor
indutny commented Nov 12, 2015
Member
bnoordhuis commented Nov 13, 2015
LGTM |
MemberAuthor
indutny commented Nov 13, 2015
Landed in 483a41c, thank you! |
indutny added a commit that referenced this pull request Nov 13, 2015
Copy client CA certs and cert store when asynchronously selecting `SecureContext` during `SNICallback`. We already copy private key, certificate, and certificate chain, but the client CA certs were missing. Fix: #2772 PR-URL: #3537 Reviewed-By: Ben Noordhuis <[email protected]>
indutny added a commit that referenced this pull request Nov 17, 2015
Copy client CA certs and cert store when asynchronously selecting `SecureContext` during `SNICallback`. We already copy private key, certificate, and certificate chain, but the client CA certs were missing. Fix: #2772 PR-URL: #3537 Reviewed-By: Ben Noordhuis <[email protected]>
Closed
Member
rvagg commented Jan 15, 2016
I'm having trouble working out if this is a bugfix or something closer to a semver-minor. @indutny can you make a call on whether this would qualify for backporting to LTS? |
MemberAuthor
indutny commented Jan 15, 2016
This is a bugfix. |
MemberAuthor
indutny commented Jan 15, 2016
I think it qualifies for backport. |
Member
jasnell commented Jan 15, 2016
The line on this one may be rather fuzzy but I tend to agree with @indutny |
MylesBorins pushed a commit that referenced this pull request Jan 28, 2016
Copy client CA certs and cert store when asynchronously selecting `SecureContext` during `SNICallback`. We already copy private key, certificate, and certificate chain, but the client CA certs were missing. Fix: #2772 PR-URL: #3537 Reviewed-By: Ben Noordhuis <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 11, 2016
Copy client CA certs and cert store when asynchronously selecting `SecureContext` during `SNICallback`. We already copy private key, certificate, and certificate chain, but the client CA certs were missing. Fix: #2772 PR-URL: #3537 Reviewed-By: Ben Noordhuis <[email protected]>
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Feb 11, 2016
Copy client CA certs and cert store when asynchronously selecting `SecureContext` during `SNICallback`. We already copy private key, certificate, and certificate chain, but the client CA certs were missing. Fix: nodejs#2772 PR-URL: nodejs#3537 Reviewed-By: Ben Noordhuis <[email protected]>
Merged
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Feb 15, 2016
Copy client CA certs and cert store when asynchronously selecting `SecureContext` during `SNICallback`. We already copy private key, certificate, and certificate chain, but the client CA certs were missing. Fix: nodejs#2772 PR-URL: nodejs#3537 Reviewed-By: Ben Noordhuis <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment
6 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.
Copy client CA certs and cert store when asynchronously selecting
SecureContextduringSNICallback.Fix: #2772
cc @nodejs/crypto