Skip to content

tls: use SSL_set_cert_cb for async SNI/OCSP - #1464

Closed
indutny wants to merge 2 commits into
nodejs:masterfrom
indutny:feature/cert-cb
Closed

tls: use SSL_set_cert_cb for async SNI/OCSP#1464
indutny wants to merge 2 commits into
nodejs:masterfrom
indutny:feature/cert-cb

Conversation

@indutny

@indutny indutny commented Apr 18, 2015

Copy link
Copy Markdown
Member

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

indutny commented Apr 19, 2015

Copy link
Copy Markdown
Member Author

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

indutny commented Apr 19, 2015

Copy link
Copy Markdown
Member Author

CI: 🔵
shigeki
shigeki reviewed Apr 20, 2015
View reviewed changes
Comment thread lib/_tls_wrap.js

@shigeki shigeki Apr 20, 2015

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

@indutny indutny Apr 20, 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.

Nope, we don't :)
@shigeki

shigeki commented Apr 20, 2015

Copy link
Copy Markdown
Contributor

I finished my review and put a few comments.
@indutny

indutny commented Apr 20, 2015

Copy link
Copy Markdown
Member Author

Thank you, @shigeki ! Does it LGTY?
@shigeki

shigeki commented Apr 20, 2015

Copy link
Copy Markdown
Contributor

Yes LGTM
@indutny

indutny commented Apr 20, 2015

Copy link
Copy Markdown
Member Author

@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

indutny commented May 1, 2015

Copy link
Copy Markdown
Member Author

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 <ohtsu@iij.ad.jp>
@indutny

indutny commented May 1, 2015

Copy link
Copy Markdown
Member Author

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 <ohtsu@iij.ad.jp>
@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 <anna@addaleax.net>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
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 <anna@addaleax.net>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
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 <anna@addaleax.net>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
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 <anna@addaleax.net>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
@pimterry pimterry mentioned this pull request Jul 29, 2026
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

tls Issues and PRs related to the tls subsystem.

4 participants

@indutny @shigeki @mscdex @chrisdickinson