-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
Conversation
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. |
3617edc
to
9fd7f91
Compare
9fd7f91
to
79694d2
Compare
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM modulo comments.
@tniessen re:
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. |
de8d226
to
ccfe3aa
Compare
ccfe3aa
to
bbf888a
Compare
bbf888a
to
b6056de
Compare
@tniessen PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some nits.
b6056de
to
6d860df
Compare
There was a problem hiding this 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) { |
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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
node/deps/openssl/openssl/crypto/ec/eck_prn.c
Lines 97 to 119 in 9dfbc39
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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Merge these two conditionals?
PR-URL: nodejs#24358 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Tobias Nießen <tniessen@tnie.de>
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>
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>
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
Remove XXX, there has been an EC specific cert property since nodejs#24358
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>
- docs incorrectly mention issuerCert, should be issuerCertificate Fix for Commit: a856406c2dc#diff-f6e3a86962eaf0897ab59e88b418e64fR678 Fix for PR: #24358
- docs incorrectly mention issuerCert, should be issuerCertificate Fix for Commit: a856406c2dc#diff-f6e3a86962eaf0897ab59e88b418e64fR678 Fix for PR: #24358
- docs incorrectly mention issuerCert, should be issuerCertificate Fix for Commit: a856406c2dc#diff-f6e3a86962eaf0897ab59e88b418e64fR678 Fix for PR: #24358
- docs incorrectly mention issuerCert, should be issuerCertificate Fix for Commit: a856406c2dc#diff-f6e3a86962eaf0897ab59e88b418e64fR678 Fix for PR: #24358
- docs incorrectly mention issuerCert, should be issuerCertificate Fix for Commit: a856406c2dc#diff-f6e3a86962eaf0897ab59e88b418e64fR678 Fix for PR: #24358
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>
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>
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>
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes