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

tls: copy client CAs and cert store on CertCb by indutny · Pull Request #3537 · nodejs/node · GitHub

/ node Public

tls: copy client CAs and cert store on CertCb - #3537

Closed
indutny wants to merge 2 commits into
nodejs:masterfrom
indutny:fix/tls-ca-sni
ConversationCommits (2)ChecksFiles changed
Closed

tls: copy client CAs and cert store on CertCb#3537
indutny wants to merge 2 commits into
nodejs:masterfrom
indutny:fix/tls-ca-sni

Conversation

indutny commented Oct 26, 2015

Copy link
Copy Markdown
Member

Copy client CA certs and cert store when asynchronously selecting
SecureContext during SNICallback.

Fix: #2772

cc @nodejs/crypto

mscdex added the tls Issues and PRs related to the tls subsystem. label Oct 27, 2015
bnoordhuis reviewed Oct 27, 2015
View reviewed changes
Comment thread src/node_crypto.cc Outdated

bnoordhuis Oct 27, 2015

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

You should check the return code here.

indutny Oct 27, 2015

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

Good idea, though it can't fail in current OpenSSL implementation.

bnoordhuis commented Oct 27, 2015

Copy link
Copy Markdown
Member

Left some comments. The commit log could go into more detail into why this change is necessary.

indutny commented Oct 27, 2015

Copy link
Copy Markdown
Member Author

@bnoordhuis pushed fixes, thanks!

indutny commented Oct 27, 2015

Copy link
Copy Markdown
Member Author

CI: https://ci.nodejs.org/job/node-test-pull-request/631/

indutny commented Oct 27, 2015

Copy link
Copy Markdown
Member Author

CI seems to be green, LGTY @bnoordhuis ?

tls: copy client CAs and cert store on CertCb
7f60df5 
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

indutny commented Oct 27, 2015

Copy link
Copy Markdown
Member Author

@bnoordhuis updated commit message too

bnoordhuis reviewed Nov 10, 2015
View reviewed changes
Comment thread src/node_crypto.cc Outdated

bnoordhuis Nov 10, 2015

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

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?

indutny Nov 12, 2015

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

ACK.

indutny commented Nov 12, 2015

Copy link
Copy Markdown
Member Author

All fixed, PTAL @bnoordhuis

indutny commented Nov 12, 2015

Copy link
Copy Markdown
Member Author

CI: https://ci.nodejs.org/job/node-test-pull-request/715/

bnoordhuis commented Nov 13, 2015

Copy link
Copy Markdown
Member

LGTM

indutny commented Nov 13, 2015

Copy link
Copy Markdown
Member Author

Landed in 483a41c, thank you!

indutny closed this Nov 13, 2015
indutny deleted the fix/tls-ca-sni branch November 13, 2015 17:48
indutny added a commit that referenced this pull request Nov 13, 2015
tls: copy client CAs and cert store on CertCb
483a41c 
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 <info@bnoordhuis.nl>
indutny added a commit that referenced this pull request Nov 17, 2015
tls: copy client CAs and cert store on CertCb
05f0549 
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 <info@bnoordhuis.nl>
rvagg mentioned this pull request Dec 17, 2015
Closed
MylesBorins mentioned this pull request Jan 14, 2016
Closed

rvagg commented Jan 15, 2016

Copy link
Copy Markdown
Member

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?

indutny commented Jan 15, 2016

Copy link
Copy Markdown
Member Author

This is a bugfix.

indutny commented Jan 15, 2016

Copy link
Copy Markdown
Member Author

I think it qualifies for backport.

jasnell commented Jan 15, 2016

Copy link
Copy Markdown
Member

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
tls: copy client CAs and cert store on CertCb
dac9e38 
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 <info@bnoordhuis.nl>
MylesBorins pushed a commit that referenced this pull request Feb 11, 2016
tls: copy client CAs and cert store on CertCb
c841d57 
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 <info@bnoordhuis.nl>
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Feb 11, 2016
tls: copy client CAs and cert store on CertCb
5cf132a 
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 <info@bnoordhuis.nl>
MylesBorins mentioned this pull request Feb 11, 2016
Merged
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Feb 15, 2016
tls: copy client CAs and cert store on CertCb
aefb20a 
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 <info@bnoordhuis.nl>
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
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

tls Issues and PRs related to the tls subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

Back | FazBrowse Home | New Git URL