tls: get the local certificate after tls handshake by sam-github · Pull Request #24261 · nodejs/node · GitHub

Add an API to get the local certificate chosen during TLS handshake from
the SSL context.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

@sam-github build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1549/pipeline

Can you be more specific than "some"?

Pretty vague, isn't it! ;-) This is the extent of the current documentation for parsed cert objects, unchanged since 0c42fac51596a68a6be02ea537e5ec03f228844b. Its on my list to fix, but for now I'd like to leave as-is. The wording of these docs are close to identical as possible to the docs for https://nodejs.org/api/tls.html#tls_tlssocket_getpeercertificate_detailed. I'm working on a PR to add EC and DH key info to X509ToObject, and I promise to doc the cert format then, and reorganize the docs, since the cert can show up in a handful of APIs.

/to @bnoordhuis as promised, #24358

Thanks Sam, LGTM.

One thing: you may want to stick in an exports.translatePeerCertificate = exports.translateCertChain to stop it from being semver-major.

I don't want it to be semver-major, I'll put the original name back.

Whats the current way to hide our internal API? Could I move translatePeerCert into internal/ with a better name, and export a deprecated wrapper from _tls_common?

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

Could I move translatePeerCert into internal/ with a better name, and export a deprecated wrapper from _tls_common?

Yep.

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

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

James, Anna - thanks for catching the typo.

Add an API to get the local certificate chosen during TLS handshake from the SSL context. Fix: nodejs#24095

Landed in db35fee

Add an API to get the local certificate chosen during TLS handshake from the SSL context. Fix: #24095 PR-URL: #24261 Fixes: #24095 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>

Add an API to get the local certificate chosen during TLS handshake from the SSL context. Fix: nodejs#24095 PR-URL: nodejs#24261 Fixes: nodejs#24095 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>

Add an API to get the local certificate chosen during TLS handshake from the SSL context. Fix: #24095 PR-URL: #24261 Fixes: #24095 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>

The documentation of `SSL_get_certificate` states that it returns an internal pointer that must not be freed by the caller. Therefore, using a smart pointer to take ownership is incorrect. Refs: https://man.openbsd.org/SSL_get_certificate.3 Refs: nodejs#24261 Fixes: https://github.com/nodejs-private/security/issues/217

If this is backported to LTS, it should go with #25490.

The documentation of `SSL_get_certificate` states that it returns an internal pointer that must not be freed by the caller. Therefore, using a smart pointer to take ownership is incorrect. Refs: https://man.openbsd.org/SSL_get_certificate.3 Refs: #24261 Fixes: https://github.com/nodejs-private/security/issues/217 PR-URL: #25490 Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

The documentation of `SSL_get_certificate` states that it returns an internal pointer that must not be freed by the caller. Therefore, using a smart pointer to take ownership is incorrect. Refs: https://man.openbsd.org/SSL_get_certificate.3 Refs: nodejs/node#24261 Fixes: https://github.com/nodejs-private/security/issues/217 PR-URL: nodejs/node#25490 Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

Add an API to get the local certificate chosen during TLS handshake from the SSL context. Fix: nodejs#24095 PR-URL: nodejs#24261 Fixes: nodejs#24095 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>

The documentation of `SSL_get_certificate` states that it returns an internal pointer that must not be freed by the caller. Therefore, using a smart pointer to take ownership is incorrect. Refs: https://man.openbsd.org/SSL_get_certificate.3 Refs: nodejs#24261 Fixes: https://github.com/nodejs-private/security/issues/217 PR-URL: nodejs#25490 Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Nov 8, 2018

sam-github mentioned this pull request Nov 8, 2018

Ability to check TLS contexts, servernames, altnames #24095

Closed

sam-github added the semver-minor PRs that contain new features and should be released in the next minor version. label Nov 8, 2018

bnoordhuis reviewed Nov 9, 2018

View reviewed changes

sam-github force-pushed the tls-get-cert branch from 9bb96e6 to 74fa19f Compare November 9, 2018 17:54

bnoordhuis approved these changes Nov 9, 2018

View reviewed changes

addaleax approved these changes Nov 10, 2018

View reviewed changes

Comment thread doc/api/tls.md Outdated

jasnell reviewed Nov 10, 2018

View reviewed changes

Comment thread doc/api/tls.md Outdated

sam-github force-pushed the tls-get-cert branch from ecfe565 to a8c6016 Compare November 13, 2018 20:51

danbev approved these changes Nov 14, 2018

View reviewed changes

Comment thread src/node_crypto.cc Outdated

tls: get the local certificate after tls handshake

f8f571f

Add an API to get the local certificate chosen during TLS handshake from the SSL context. Fix: nodejs#24095

sam-github force-pushed the tls-get-cert branch from a8c6016 to f8f571f Compare November 14, 2018 04:41

sam-github closed this Nov 14, 2018

sam-github deleted the tls-get-cert branch November 14, 2018 04:43

BridgeAR mentioned this pull request Nov 15, 2018

Release proposal: v11.2 #24350

Merged

This was referenced Nov 15, 2018

2018-11-15 Version 11.2.0 (Current) @BridgeAR beevelop/docker-nodejs#232

Closed

2018-11-15 Version 11.2.0 (Current) @BridgeAR beevelop/docker-android-nodejs#249

Closed

2018-11-15 Version 11.2.0 (Current) @BridgeAR beevelop/docker-nodejs#233

Closed

This was referenced Nov 16, 2018

2018-11-15 Version 11.2.0 (Current) @BridgeAR beevelop/docker-nodejs#234

Closed

2018-11-15 Version 11.2.0 (Current) @BridgeAR beevelop/docker-nodejs#235

Closed

2018-11-15 Version 11.2.0 (Current) @BridgeAR beevelop/docker-nodejs#236

Closed

addaleax mentioned this pull request Jan 14, 2019

tls: do not free cert in .getCertificate() #25490

Closed

3 tasks

sam-github mentioned this pull request Apr 26, 2019

[v10.x backport] TLS1.3 (and dependent PRs) #27432

Closed

tls: get the local certificate after tls handshake by sam-github · Pull Request #24261 · nodejs/node · GitHub

Uh oh!

Uh oh!

tls: get the local certificate after tls handshake - #24261

tls: get the local certificate after tls handshake#24261sam-github wants to merge 1 commit intonodejs:masternodejs/node:masterfrom sam-github:tls-get-certsam-github/node:tls-get-certCopy head branch name to clipboard

Conversation

sam-github commented Nov 8, 2018

Checklist

Uh oh!

nodejs-github-bot commented Nov 8, 2018

Uh oh!

bnoordhuis Nov 9, 2018

Choose a reason for hiding this comment

Uh oh!

sam-github Nov 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sam-github Nov 14, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bnoordhuis left a comment

Choose a reason for hiding this comment

Uh oh!

sam-github commented Nov 9, 2018

Uh oh!

sam-github commented Nov 9, 2018

Uh oh!

bnoordhuis commented Nov 9, 2018

Uh oh!

sam-github commented Nov 10, 2018

Uh oh!

Uh oh!

Uh oh!

sam-github commented Nov 13, 2018

Uh oh!

Uh oh!

sam-github commented Nov 14, 2018

Uh oh!

addaleax commented Jan 14, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

tls: get the local certificate after tls handshake#24261
sam-github wants to merge 1 commit into
nodejs:masterfrom
sam-github:tls-get-cert

sam-github Nov 9, 2018 •
edited
Loading