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: use SSL_set_cert_cb for async SNI/OCSP #1464

Closed
wants to merge 2 commits into from
Closed

tls: use SSL_set_cert_cb for async SNI/OCSP #1464

wants to merge 2 commits into from

Conversation

indutny
Copy link
Member

@indutny indutny commented Apr 18, 2015

Do not enable ClientHello parser for async SNI/OCSP. Use new
OpenSSL-1.0.2's API SSL_set_cert_cb to pause the handshake process and
load the cert/OCSP response asynchronously. Hopefuly this will make
whole async SNI/OCSP process much faster and will eventually let us
remove the ClientHello parser itself (which is currently used only for
async session, see #1462 for the discussion of removing it).

NOTE: Ported our code to SSL_CTX_add1_chain_cert to use
SSL_CTX_get0_chain_certs in CertCbDone. Test provided for this
feature.

Fix: #1423
R=@bnoordhuis and @shigeki

cc @iojs/crypto

@mscdex mscdex added the tls Issues and PRs related to the tls subsystem. label Apr 18, 2015
@indutny
Copy link
Member Author

indutny commented Apr 19, 2015

CI: https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/558/ (UPD.1)

@indutny
Copy link
Member Author

indutny commented Apr 19, 2015

CI: 🔵

shigeki
shigeki reviewed Apr 20, 2015
View reviewed changes
lib/_tls_wrap.js
loadSNI(self, servername, function(err, ctx) {
if (err)
return self.destroy(err);
requestOCSP(self, info, ctx, function(err) {
Copy link
Contributor

Choose a reason for hiding this comment

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

ctx is undefined if (!servername || !self._SNICallback) in loadSNI. Do we require SNI to use OCSP stapling?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope, we don't :)

@shigeki
Copy link
Contributor

shigeki commented Apr 20, 2015

I finished my review and put a few comments.

@indutny
Copy link
Member Author

indutny commented Apr 20, 2015

Thank you, @shigeki ! Does it LGTY?

@shigeki
Copy link
Contributor

shigeki commented Apr 20, 2015

Yes LGTM

@indutny
Copy link
Member Author

indutny commented Apr 20, 2015

@bnoordhuis: could you PTAL too?

chrisdickinson and others added 2 commits April 20, 2015 16:02
@chrisdickinson
Working on v1.8.2
2f6986e
@indutny
tls: use SSL_set_cert_cb for async SNI/OCSP
52ec001 
Do not enable ClientHello parser for async SNI/OCSP. Use new
OpenSSL-1.0.2's API `SSL_set_cert_cb` to pause the handshake process and
load the cert/OCSP response asynchronously. Hopefuly this will make
whole async SNI/OCSP process much faster and will eventually let us
remove the ClientHello parser itself (which is currently used only for
async session, see nodejs#1462 for the discussion of removing it).

NOTE: Ported our code to `SSL_CTX_add1_chain_cert` to use
`SSL_CTX_get0_chain_certs` in `CertCbDone`. Test provided for this
feature.

Fix: nodejs#1423
@indutny
Copy link
Member Author

indutny commented May 1, 2015

Will land it in master soon.

indutny added a commit that referenced this pull request May 1, 2015
@indutny
tls: use SSL_set_cert_cb for async SNI/OCSP
550c263 
Do not enable ClientHello parser for async SNI/OCSP. Use new
OpenSSL-1.0.2's API `SSL_set_cert_cb` to pause the handshake process and
load the cert/OCSP response asynchronously. Hopefuly this will make
whole async SNI/OCSP process much faster and will eventually let us
remove the ClientHello parser itself (which is currently used only for
async session, see #1462 for the discussion of removing it).

NOTE: Ported our code to `SSL_CTX_add1_chain_cert` to use
`SSL_CTX_get0_chain_certs` in `CertCbDone`. Test provided for this
feature.

Fix: #1423
PR-URL: #1464
Reviewed-By: Shigeki Ohtsu <[email protected]>
@indutny
Copy link
Member Author

indutny commented May 1, 2015

Landed in 550c263, thank you @shigeki

@indutny indutny closed this May 1, 2015
@indutny indutny deleted the feature/cert-cb branch May 1, 2015 14:57
@rvagg rvagg mentioned this pull request May 2, 2015
Closed
@Fishrock123 Fishrock123 mentioned this pull request May 13, 2015
Closed
Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request May 19, 2015
@indutny @Fishrock123
tls: use SSL_set_cert_cb for async SNI/OCSP
592230a 
Do not enable ClientHello parser for async SNI/OCSP. Use new
OpenSSL-1.0.2's API `SSL_set_cert_cb` to pause the handshake process and
load the cert/OCSP response asynchronously. Hopefuly this will make
whole async SNI/OCSP process much faster and will eventually let us
remove the ClientHello parser itself (which is currently used only for
async session, see nodejs#1462 for the discussion of removing it).

NOTE: Ported our code to `SSL_CTX_add1_chain_cert` to use
`SSL_CTX_get0_chain_certs` in `CertCbDone`. Test provided for this
feature.

Fix: nodejs#1423
PR-URL: nodejs#1464
Reviewed-By: Shigeki Ohtsu <[email protected]>
@rvagg rvagg mentioned this pull request Oct 31, 2016
Closed
sam-github added a commit to sam-github/node that referenced this pull request Dec 27, 2018
@sam-github
tls: remove unused ocsp extension parsing
dd12dc9 
The OCSP info from parsing the TLS ClientHello has not been used since
550c263, remove it.

See: nodejs#1464
sam-github added a commit that referenced this pull request Dec 28, 2018
@sam-github
tls: remove unused ocsp extension parsing
08387b2 
The OCSP info from parsing the TLS ClientHello has not been used since
550c263, remove it.

See: #1464

PR-URL: #25153
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
targos pushed a commit that referenced this pull request Jan 1, 2019
@sam-github @targos
tls: remove unused ocsp extension parsing
acf7802 
The OCSP info from parsing the TLS ClientHello has not been used since
550c263, remove it.

See: #1464

PR-URL: #25153
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
@sam-github @refack
tls: remove unused ocsp extension parsing
5905f65 
The OCSP info from parsing the TLS ClientHello has not been used since
550c263, remove it.

See: nodejs#1464

PR-URL: nodejs#25153
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
sam-github added a commit to sam-github/node that referenced this pull request Apr 29, 2019
@sam-github
tls: remove unused ocsp extension parsing
ae7c74c 
The OCSP info from parsing the TLS ClientHello has not been used since
550c263, remove it.

See: nodejs#1464

PR-URL: nodejs#25153
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
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.

Make use of OpenSSL-1.0.2
4 participants
@indutny @shigeki @mscdex @chrisdickinson