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

sam-github · 2018-11-08T21:57:37Z

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

nodejs-github-bot · 2018-11-08T21:57:38Z

@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

bnoordhuis · 2018-11-09T10:26:51Z

doc/api/tls.md

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

doc/api/tls.md

src/node_crypto.cc

bnoordhuis

Thanks Sam, LGTM.

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

sam-github · 2018-11-09T18:55:55Z

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?

sam-github · 2018-11-09T19:16:07Z

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

bnoordhuis · 2018-11-09T19:35:43Z

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

Yep.

sam-github · 2018-11-10T00:17:08Z

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

doc/api/tls.md

sam-github · 2018-11-13T20:56:49Z

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

James, Anna - thanks for catching the typo.

src/node_crypto.cc

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

sam-github · 2018-11-14T04:43:29Z

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

addaleax · 2019-01-14T11:24:30Z

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

doc/api/tls.md Outdated Show resolved Hide resolved

jasnell reviewed Nov 10, 2018

View reviewed changes

doc/api/tls.md Outdated Show resolved Hide resolved

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

src/node_crypto.cc Outdated Show resolved Hide resolved

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

Uh oh!

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

sam-github Nov 9, 2018 •

edited

Loading