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: Add publicKey to certificate information #1725

Closed
wants to merge 1 commit into from

Conversation

saarons
Copy link

@saarons saarons commented May 18, 2015

Added a publicKey attribute to peer certificate information. The
attribute contains the DER-encoded value of the public key as a buffer.
This will allow applications to retrieve the necessary information for
validating HTTP public key pins according to RFC 7469.

https://tools.ietf.org/html/rfc7469

@brendanashworth brendanashworth added the tls Issues and PRs related to the tls subsystem. label May 18, 2015
@@ -165,6 +165,7 @@ namespace node {
V(priority_string, "priority") \
V(processed_string, "processed") \
V(prototype_string, "prototype") \
V(public_key_string, "publicKey") \
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, the name of this field is Subject Public Key Info which contains not only a public key but also a key algorithm.

Copy link
Author

Choose a reason for hiding this comment

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

I agree, subjectPublicKeyInfo is a much better name for this because that's what we actually want here.

@shigeki
Copy link
Contributor

shigeki commented May 18, 2015

@saarons Could you update the example in tlsSocket.getPeerCertificate ?

@saarons
Copy link
Author

saarons commented May 18, 2015

@shigeki Absolutely, I'll update the example for tlsSocket.getPeerCertificate in the docs to show the presence of this new attribute.

Added `subjectPublicKeyInfo` attribute to the peer certificate object.
The attribute is a buffer containing the DER-encoded value of the
SubjectPublicKeyInfo structure as described in RFC 5280. This will
allow applications to retrieve the SPKI information necessary for
validating HTTP public key pins according to RFC 7469.

https://tools.ietf.org/html/rfc7469
@saarons
Copy link
Author

saarons commented May 18, 2015

@shigeki I've made the changes you've asked for and squashed the commits.

@shigeki
Copy link
Contributor

shigeki commented May 18, 2015

@saarons Thanks. I made a test for https://www.google.com to check if we can obtain a right PKP as

var https = require('https');
var crypto = require('crypto');
var host = 'www.google.com';
var client = https.get({
  host: host,
  path: '/'
}, function(res) {
  var peerCert = res.socket.getPeerCertificate(true);
  var spki = peerCert.issuerCertificate.subjectPublicKeyInfo;
  var sha1 = crypto.createHash('sha1');
  sha1.update(spki);
  var spki_hashe = sha1.digest('base64');
  console.log(host, ',spki_hashe:', spki_hashe);
  res.resume();
});
$ ./iojs ~/pkp_https_client.js
www.google.com ,spki_hashe: Q9rWMO5T+KmAym79hfRqo3mQ4Oo=

This value matches the one embedded in Chrome.

google_pkp

So that's fine. LGTM.

@indutny How is this?

@indutny
Copy link
Member

indutny commented May 18, 2015

I think we already have a raw field in it? Isn't full DER encoding of certificate enough to figure out this data?

@shigeki
Copy link
Contributor

shigeki commented May 18, 2015

IIRC, I think we need asn1 parser to extract spki.

@shigeki
Copy link
Contributor

shigeki commented May 18, 2015

Ah, you say we can make it by user module. Right.

@indutny
Copy link
Member

indutny commented May 18, 2015

Exactly, there are lots of ASN.1 parsers on npm. (https://github.com/indutny/asn1.js ;) )

@shigeki
Copy link
Contributor

shigeki commented May 18, 2015

Yes, I so other properties such as modulus, exponents and whatever except raw are not necessary. I think SPKI has a very good use case to implement HPKP. Why is only SPKI exceptional?

@indutny
Copy link
Member

indutny commented May 18, 2015

@shigeki I'm afraid that modulus and friends are present here for historical reasons too. The raw field was introduced much later than all of them.

The thing about SPKI is that there are lots of extensions and information in X509 certificate. The more we map it to JS object - the more time will take the .getPeerCertificate() call. I doubt that everyone is interested in every possible X509 extension, while the speed change will impact everyone, because tls.connect() is using .getPeerCertificate() for the cert validation.

@indutny
Copy link
Member

indutny commented May 18, 2015

Oh, it is actually even worse than this :) We already export modulus and all information that is required to construct DER-encoded public key. Even without parsing the X509 certificate itself.

@shigeki
Copy link
Contributor

shigeki commented May 18, 2015

I understand the reason and the current situation. From the point of policy to keep core small, we should avoid to add a new feature that can be done within user module.

@saarons Sorry, I missed the point that we have raw cert data and the spki hash for HPKP can be obtained within user module via asn1parser. We should refine certificate properties to expose in future. Thanks for your work.

@shigeki shigeki closed this May 18, 2015
@saarons
Copy link
Author

saarons commented May 18, 2015

@shigeki @indutny Totally understand where you guys are coming from, I was contemplating myself whether I should even write this given that are some alternatives. One of the main reasons why I feel this would be ok, is because fields like modulus and exponent are only really helpful when you're dealing with RSA keys so when you're trying to reconstruct the DER encoding of the public key it only works in that case (which granted is a lot of the time). The other reason I had was because adding a user module to reparse the entire certificate would add more overhead then what I've put forward here.

I understand there are plenty of ASN.1 parsers out there but with your own example @indutny , I can't figure out if I can just straight parse ASN.1 without having to define an Entity. I've looked at X.509 parsers on NPM but they're no better than the info we get from getPeerCertificate. I guess I could just create my own user module that does this one thing, but then I have to worry about it compiling correctly on all platforms and whether I'm linking against the correct version of OpenSSL. There's a lot of ease by just putting it here (which I admit is a bit of a danger in general. We don't want this object to get too crazy).

@bnoordhuis
Copy link
Member

The thing about SPKI is that there are lots of extensions and information in X509 certificate. The more we map it to JS object - the more time will take the .getPeerCertificate() call. I doubt that everyone is interested in every possible X509 extension, while the speed change will impact everyone, because tls.connect() is using .getPeerCertificate() for the cert validation.

While true, it's something that can be worked around by populating the object with getters that retrieve/calculate the values on demand. (Life cycle management would be a bit of a pain, though.)

@shigeki
Copy link
Contributor

shigeki commented May 21, 2015

@saarons FYI, @indutny provides us with a very nice module named asn1.js-rfc3280. RFC3280 has already been obsoluted by RFC 5280 but most of asn1 entities are still valid.

It is a very easy to deal with HPKP as

var https = require('https');
var crypto = require('crypto');
var rfc3280 = require('asn1.js-rfc3280');
var Certificate = rfc3280.Certificate;
var SubjectPublicKeyInfo = rfc3280.SubjectPublicKeyInfo;
var host = 'www.google.com';
var client = https.get({
  port: 443,
  host: host,
  path: '/'
}, function(res) {
  var peerCert = res.socket.getPeerCertificate(true);
  var cert = Certificate.decode(peerCert.issuerCertificate.raw, 'der');
  var spki = cert.tbsCertificate.subjectPublicKeyInfo;
  var spki_der = SubjectPublicKeyInfo.encode(spki, 'der');
  var sha1 = crypto.createHash('sha1');
  sha1.update(spki_der);
  var spki_hashes = sha1.digest('base64');
  console.log(host, ',spki_hashe:', spki_hashes);
  res.resume();
});
$ ~/github/io.js/iojs hpkp_https_client.js
www.google.com ,spki_hashe: Q9rWMO5T+KmAym79hfRqo3mQ4Oo=

Please be sure to use the latest version (2.0.2 for asn1.js and 2.0.0 for asn1.js-rfc3280) since there were a few issues related to AlgorithmIdentifier.parameters. I hope it helps you.

@refack refack mentioned this pull request Oct 18, 2018
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants