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
base: master
from

Conversation

Projects
None yet
6 participants
@sam-github
Copy link
Member

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
@sam-github

This comment has been minimized.

Copy link
Member

sam-github commented Nov 14, 2018

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 referenced this pull request Nov 14, 2018

Closed

tls: get the local certificate after tls handshake #24261

4 of 4 tasks complete

@sam-github sam-github force-pushed the sam-github:tls-cert-details branch from 3617edc to 9fd7f91 Nov 14, 2018

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

@sam-github sam-github force-pushed the sam-github:tls-cert-details branch from 9fd7f91 to 79694d2 Nov 14, 2018

@sam-github

This comment has been minimized.

@sam-github sam-github requested review from bnoordhuis and tniessen Nov 14, 2018

@sam-github

This comment has been minimized.

@tniessen

This comment has been minimized.

Copy link
Member

tniessen commented Nov 14, 2018

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.

@bnoordhuis
Copy link
Member

bnoordhuis left a comment

LGTM modulo comments.

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 src/node_crypto.cc Outdated
Show resolved Hide resolved doc/api/tls.md Outdated
@sam-github

This comment has been minimized.

Copy link
Member

sam-github commented Nov 14, 2018

@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 sam-github force-pushed the sam-github:tls-cert-details branch from de8d226 to ccfe3aa Nov 15, 2018

@sam-github

This comment has been minimized.

@sam-github sam-github force-pushed the sam-github:tls-cert-details branch from ccfe3aa to bbf888a Nov 15, 2018

@sam-github

This comment has been minimized.

@sam-github sam-github force-pushed the sam-github:tls-cert-details branch from bbf888a to b6056de Nov 15, 2018

@sam-github

This comment has been minimized.

Copy link
Member

sam-github commented Nov 15, 2018

@tniessen PTAL

@sam-github

This comment has been minimized.

@vsemozhetbyt
Copy link
Member

vsemozhetbyt left a comment

Some nits.

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 doc/api/tls.md Outdated

@sam-github sam-github force-pushed the sam-github:tls-cert-details branch from b6056de to 6d860df Nov 15, 2018

@sam-github

This comment has been minimized.

Copy link
Member

sam-github commented Nov 15, 2018

@bnoordhuis bnoordhuis referenced this pull request Nov 15, 2018

Closed

generate more certs, and more test coverage using them #24374

4 of 4 tasks complete
Show resolved Hide resolved doc/api/tls.md Outdated
tls: include RSA bit size in X.509 public key info
For symmetricality with the EC public key info, and because its useful.

@sam-github sam-github force-pushed the sam-github:tls-cert-details branch from 349d6af to 51f4e0b Nov 20, 2018

@sam-github

This comment has been minimized.

Copy link
Member

sam-github commented Nov 20, 2018

Landed in 08c14d9...0512d68

@sam-github sam-github closed this Nov 20, 2018

@sam-github sam-github deleted the sam-github:tls-cert-details branch Nov 20, 2018

sam-github added a commit that referenced this pull request Nov 20, 2018

doc: describe certificate object properties
PR-URL: #24358
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>

sam-github added a commit that referenced this pull request Nov 20, 2018

tls: include elliptic curve X.509 public key info
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: #24358
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>

sam-github added a commit that referenced this pull request Nov 20, 2018

tls: include RSA bit size in X.509 public key info
For symmetricality with the EC public key info, and because its useful.

PR-URL: #24358
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>

targos added a commit that referenced this pull request Nov 21, 2018

doc: describe certificate object properties
PR-URL: #24358
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>

targos added a commit that referenced this pull request Nov 21, 2018

tls: include elliptic curve X.509 public key info
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: #24358
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>

targos added a commit that referenced this pull request Nov 21, 2018

tls: include RSA bit size in X.509 public key info
For symmetricality with the EC public key info, and because its useful.

PR-URL: #24358
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>

rvagg added a commit that referenced this pull request Nov 28, 2018

doc: describe certificate object properties
PR-URL: #24358
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>

rvagg added a commit that referenced this pull request Nov 28, 2018

tls: include elliptic curve X.509 public key info
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: #24358
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>

rvagg added a commit that referenced this pull request Nov 28, 2018

tls: include RSA bit size in X.509 public key info
For symmetricality with the EC public key info, and because its useful.

PR-URL: #24358
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>

@BridgeAR BridgeAR referenced this pull request Dec 5, 2018

Merged

v11.4.0 proposal #24854

4 of 4 tasks complete

BridgeAR added a commit that referenced this pull request Dec 6, 2018

2018-12-06, Version 11.4.0 (Current)
Notable Changes:

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

PR-URL: #24854

BridgeAR added a commit that referenced this pull request Dec 7, 2018

2018-12-07, Version 11.4.0 (Current)
Notable Changes:

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

PR-URL: #24854

BridgeAR added a commit that referenced this pull request Dec 7, 2018

2018-12-07, Version 11.4.0 (Current)
Notable Changes:

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

PR-URL: #24854

BridgeAR added a commit that referenced this pull request Dec 7, 2018

2018-12-07, Version 11.4.0 (Current)
Notable Changes:

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

PR-URL: #24854

BridgeAR added a commit that referenced this pull request Dec 7, 2018

2018-12-07, Version 11.4.0 (Current)
Notable Changes:

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

PR-URL: #24854
@nornagon
Copy link
Contributor

nornagon left a comment

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) {

This comment has been minimized.

@nornagon

nornagon Dec 19, 2018

Contributor

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.)

This comment has been minimized.

@sam-github

sam-github Jan 4, 2019

Member

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.

This comment has been minimized.

@nornagon

nornagon Jan 4, 2019

Contributor

Thanks!

This comment has been minimized.

@sam-github
OneByteString(env->isolate(), sn)).FromJust();
}
}
if (nid != 0) {

This comment has been minimized.

@nornagon

nornagon Dec 19, 2018

Contributor

Merge these two conditionals?

refack added a commit to refack/node that referenced this pull request Jan 14, 2019

doc: describe certificate object properties
PR-URL: nodejs#24358
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>

refack added a commit to refack/node that referenced this pull request Jan 14, 2019

tls: include elliptic curve X.509 public key info
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 added a commit to refack/node that referenced this pull request Jan 14, 2019

tls: include RSA bit size in X.509 public key info
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 added a commit to refack/node that referenced this pull request Jan 14, 2019

2018-12-07, Version 11.4.0 (Current)
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment