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.

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

TLS certificate object documentation and support for EC certificates #24358

Closed
wants to merge 3 commits into from

Conversation

sam-github
Copy link
Contributor

@sam-github sam-github commented Nov 14, 2018

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 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 14, 2018
@sam-github
Copy link
Contributor Author

I didn't add support for DH or DSA certs because its untestable until #10747 lands, we don't have any of those kind of certs in our fixtures.

@sam-github sam-github changed the title Tls cert details TLS certificate object documentation and support for EC certificates Nov 14, 2018
@sam-github
Copy link
Contributor Author

@sam-github
Copy link
Contributor Author

@tniessen
Copy link
Member

I am actually not sure how this aligns with my proposal for key objects... IMO it would make more sense to expose properties of the key via a key object, not using the certificate, but on the other hand, I am not sure whether we should expose those fields at all.

Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

LGTM modulo comments.

src/node_crypto.cc Outdated Show resolved Hide resolved
src/node_crypto.cc Outdated Show resolved Hide resolved
src/node_crypto.cc Outdated Show resolved Hide resolved
src/node_crypto.cc Outdated Show resolved Hide resolved
src/node_crypto.cc Outdated Show resolved Hide resolved
doc/api/tls.md Outdated Show resolved Hide resolved
@sam-github
Copy link
Contributor Author

@tniessen re:

I am actually not sure how this aligns with my proposal for key objects... IMO it would make more sense to expose properties of the key via a key object, not using the certificate, but on the other hand, I am not sure whether we should expose those fields at all.

I think it aligns fine with key objects. The key info in the cert is already present, but mostly for debugging. I don't think that has much to do with the keyObjects you are working on. It would be possible to add a keyObject property to the cert object, but the overhead might not make sense for TLS. It could be optional.

More interestingly, I notice your keyObjects don't allow access to any of the key properties, so a key object isn't a replacement for how the cert objects expose the public key material. Perhaps that is deliberate - key material might not be present if the key is on hardware? Even if the key isn't exposed, it seems to me that the key size, and the ec curve/nist name are all properties that would be useful.

If you are considering exposing some information about the asym keys (alg, size, curve, etc) from keyObjects, then it would make sense that the property names you use are aligned with the property names used in cert objects.

Is that something you see a place for? You could do it later, but it would be good if you at least liked the names used here so that using the same names won't cause any pain.

@sam-github
Copy link
Contributor Author

@sam-github
Copy link
Contributor Author

@sam-github sam-github added semver-minor PRs that contain new features and should be released in the next minor version. tls Issues and PRs related to the tls subsystem. doc Issues and PRs related to the documentations. labels Nov 15, 2018
@sam-github
Copy link
Contributor Author

@tniessen PTAL

@sam-github
Copy link
Contributor Author

Copy link
Contributor

@vsemozhetbyt vsemozhetbyt left a comment

Choose a reason for hiding this comment

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

Some nits.

doc/api/tls.md Outdated Show resolved Hide resolved
doc/api/tls.md Outdated Show resolved Hide resolved
doc/api/tls.md Outdated Show resolved Hide resolved
doc/api/tls.md Outdated Show resolved Hide resolved
doc/api/tls.md Outdated Show resolved Hide resolved
@sam-github
Copy link
Contributor Author

@vsemozhetbyt PTAL

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

doc/api/tls.md Outdated Show resolved Hide resolved
Copy link
Contributor

@nornagon nornagon left a comment

Choose a reason for hiding this comment

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

I know this already got merged, I'm commenting here because I'm working on boringssl integration in Electron and wanted to ask some questions :)

CHECK_NULL(pub);
}

if (EC_GROUP_get_asn1_flag(group) != 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this check for asn1_flag is unnecessary? It should be enough to just check nid != 0—if the curve has a name, nid will be non-zero, and if it doesn't, nid will be zero. No need to also check the asn1_flag, right?

(The reason I'm asking is because EC_GROUP_get_asn1_flag doesn't exist in BoringSSL and I'm working on BoringSSL integration downstream in Electron.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The code was directly copied from

if (EC_GROUP_get_asn1_flag(x)) {
/* the curve parameter are given by an asn1 OID */
int nid;
const char *nname;
if (!BIO_indent(bp, off, 128))
goto err;
nid = EC_GROUP_get_curve_name(x);
if (nid == 0)
goto err;
if (BIO_printf(bp, "ASN1 OID: %s", OBJ_nid2sn(nid)) <= 0)
goto err;
if (BIO_printf(bp, "\n") <= 0)
goto err;
nname = EC_curve_nid2nist(nid);
if (nname) {
if (!BIO_indent(bp, off, 128))
goto err;
if (BIO_printf(bp, "NIST CURVE: %s\n", nname) <= 0)
goto err;
}
} else {
but reading through the docs, yes, I suppose just trying to get the nid would work. I'll update this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

see: #25345

OneByteString(env->isolate(), sn)).FromJust();
}
}
if (nid != 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Merge these two conditionals?

refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
PR-URL: nodejs#24358
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
X.509 certs are provided to the user in a parsed object form by a number
of TLS APIs. Include public key info for elliptic curves as well, not
just RSA.
- pubkey: the public key
- bits: the strength of the curve
- asn1Curve: the ASN.1 OID for the curve
- nistCurve: the NIST nickname for the curve, if it has one

PR-URL: nodejs#24358
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
For symmetricality with the EC public key info, and because its useful.

PR-URL: nodejs#24358
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
Notable Changes:

* console,util:
  * `console` functions now handle symbols as defined in the spec.
    nodejs#23708
  * The inspection `depth` default is now back at 2.
    nodejs#24326
* dgram,net:
  * Added ipv6Only option for `net` and `dgram`.
    nodejs#23798
* http:
  * Chosing between the http parser is now possible per runtime flag.
    nodejs#24739
* readline:
  * The `readline` module now supports async iterators.
    nodejs#23916
* repl:
  * The multiline history feature is removed.
    nodejs#24804
* tls:
  * Added min/max protocol version options.
    nodejs#24405
  * The X.509 public key info now includes the RSA bit size and the
    elliptic curve. nodejs#24358
* url:
  * `pathToFileURL()` now supports LF, CR and TAB.
    nodejs#23720
* Windows:
  * Tools are not installed using Boxstarter anymore.
    nodejs#24677
  * The install-tools scripts or now included in the dist.
    nodejs#24233
* Added new collaborator:
  * [antsmartian](https://github.com/antsmartian) - Anto Aravinth.
    nodejs#24655

PR-URL: nodejs#24854
sam-github added a commit to sam-github/node that referenced this pull request Mar 11, 2019
Remove XXX, there has been an EC specific cert property since
nodejs#24358
sam-github added a commit that referenced this pull request Mar 18, 2019
Remove XXX, there has been an EC specific cert property since
#24358

PR-URL: #26598
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
targos pushed a commit to targos/node that referenced this pull request Mar 27, 2019
Remove XXX, there has been an EC specific cert property since
nodejs#24358

PR-URL: nodejs#26598
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
targos pushed a commit that referenced this pull request Mar 27, 2019
Remove XXX, there has been an EC specific cert property since
#24358

PR-URL: #26598
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
srl295 added a commit that referenced this pull request Mar 28, 2019
- docs incorrectly mention issuerCert, should be issuerCertificate

Fix for Commit:
a856406c2dc#diff-f6e3a86962eaf0897ab59e88b418e64fR678

Fix for PR: #24358
targos pushed a commit that referenced this pull request Mar 28, 2019
- docs incorrectly mention issuerCert, should be issuerCertificate

Fix for Commit:
a856406c2dc#diff-f6e3a86962eaf0897ab59e88b418e64fR678

Fix for PR: #24358
targos pushed a commit that referenced this pull request Mar 29, 2019
- docs incorrectly mention issuerCert, should be issuerCertificate

Fix for Commit:
a856406c2dc#diff-f6e3a86962eaf0897ab59e88b418e64fR678

Fix for PR: #24358
targos pushed a commit that referenced this pull request Mar 30, 2019
- docs incorrectly mention issuerCert, should be issuerCertificate

Fix for Commit:
a856406c2dc#diff-f6e3a86962eaf0897ab59e88b418e64fR678

Fix for PR: #24358
BethGriggs pushed a commit that referenced this pull request Apr 5, 2019
- docs incorrectly mention issuerCert, should be issuerCertificate

Fix for Commit:
a856406c2dc#diff-f6e3a86962eaf0897ab59e88b418e64fR678

Fix for PR: #24358
tniessen added a commit that referenced this pull request Jan 13, 2022
PR-URL: #41477
Refs: #24358
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Tierney Cyren <hello@bnb.im>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
targos pushed a commit that referenced this pull request Jan 14, 2022
PR-URL: #41477
Refs: #24358
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Tierney Cyren <hello@bnb.im>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
mawaregetsuka pushed a commit to mawaregetsuka/node that referenced this pull request Jan 17, 2022
PR-URL: nodejs#41477
Refs: nodejs#24358
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Tierney Cyren <hello@bnb.im>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
thedull pushed a commit to thedull/node that referenced this pull request Jan 18, 2022
PR-URL: nodejs#41477
Refs: nodejs#24358
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Tierney Cyren <hello@bnb.im>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Linkgoron pushed a commit to Linkgoron/node that referenced this pull request Jan 31, 2022
PR-URL: nodejs#41477
Refs: nodejs#24358
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Tierney Cyren <hello@bnb.im>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
danielleadams pushed a commit that referenced this pull request Feb 1, 2022
PR-URL: #41477
Refs: #24358
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Tierney Cyren <hello@bnb.im>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. doc Issues and PRs related to the documentations. lib / src Issues and PRs related to general changes in the lib or src directory. semver-minor PRs that contain new features and should be released in the next minor version. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants