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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 2 commits into from
Closed

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

wants to merge 2 commits into from

Conversation

indutny
Copy link
Member

@indutny indutny commented Oct 26, 2015

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

Fix: #2772

cc @nodejs/crypto

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

template <class Base>
int SSLWrap<Base>::SetCACerts(SecureContext* sc) {
SSL_set1_verify_cert_store(ssl_, SSL_CTX_get_cert_store(sc->ctx_));
Copy link
Member

Choose a reason for hiding this comment

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

You should check the return code here.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

@bnoordhuis
Copy link
Member

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

@indutny
Copy link
Member Author

indutny commented Oct 27, 2015

@bnoordhuis pushed fixes, thanks!

@indutny
Copy link
Member Author

indutny commented Oct 27, 2015

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

@indutny
Copy link
Member Author

indutny commented Oct 27, 2015

CI seems to be green, LGTY @bnoordhuis ?

@indutny
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
Copy link
Member Author

indutny commented Oct 27, 2015

@bnoordhuis updated commit message too

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

STACK_OF(X509_NAME)* list = SSL_dup_CA_list(
SSL_CTX_get_client_CA_list(sc->ctx_));
SSL_set_client_CA_list(ssl_, list);
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

ACK.

@indutny
Copy link
Member Author

indutny commented Nov 12, 2015

All fixed, PTAL @bnoordhuis

@indutny
Copy link
Member Author

indutny commented Nov 12, 2015

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

@bnoordhuis
Copy link
Member

LGTM

@indutny
Copy link
Member Author

indutny commented Nov 13, 2015

Landed in 483a41c, thank you!

@indutny indutny closed this Nov 13, 2015
@indutny 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
@indutny
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
@indutny @Fishrock123
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 rvagg mentioned this pull request Dec 17, 2015
Closed
@MylesBorins MylesBorins mentioned this pull request Jan 14, 2016
Closed
@rvagg
Copy link
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?

@indutny
Copy link
Member Author

indutny commented Jan 15, 2016

This is a bugfix.

@indutny
Copy link
Member Author

indutny commented Jan 15, 2016

I think it qualifies for backport.

@jasnell
Copy link
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
@indutny
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
@indutny
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
@indutny
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 MylesBorins mentioned this pull request Feb 11, 2016
Merged
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Feb 15, 2016
@indutny
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>
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
@indutny @bnoordhuis @rvagg @jasnell @mscdex @MylesBorins