src: add x509.fingerprint512 to crypto module

[3nprob]

SHA-512 equivalent of fingerprint256.

I am working on an integration with a protocol where by convention SHA512 fingerprints of certificates are used and I feel this belongs better here than in a module.

[addaleax]

/cc @nodejs/crypto

[tniessen]

Thank you for your contribution, @3nprob!

As far as I know, SHA-512 is highly unusual for fingerprinting certificates, whereas SHA-1 and SHA-256 are very commonly used for this purpose (e.g., by browsers such as Firefox, Chrome, by CAs such as digicert, by existing software such as .NET and Node.js, etc.).

As far as I can tell, SHA-512 has no real advantages over SHA-256 for fingerprinting X509 certificates. There is little to no security benefit. The hashes are even longer and impractical for humans (128 hexadecimal characters). Storing and transmitting longer hashes effectively wastes storage and bandwidth.

(Also, most X509 certificates use SHA-256 as part of their signature algorithm. Therefore, if someone does break SHA-256 at some point in the future, they will be able to spoof X509 certificates anyway, rendering fingerprinting pointless.)

The other concern is that, if SHA-512 is a non-standard way of fingerprinting certificates, there could be ten more PRs tomorrow adding fingerprint* properties for literally all hash functions supported by Node.js/OpenSSL. For example, despite being insecure, MD5 is still commonly used for fingerprinting. It is simple enough for Node.js applications to calculate other hash values using the .raw property of certificate objects, if necessary.

[tniessen]

Making my concerns explicit.

[tniessen]

It is simple enough for Node.js applications to calculate other hash values using the .raw property of certificate objects, if necessary.

For example:

tls.connect({
  host: 'nodejs.org',
  port: 443,
  checkServerIdentity(h, cert) {
    const fingerprint512 = crypto.createHash('sha512').update(cert.raw).digest();
    console.log(fingerprint512);
    return tls.checkServerIdentity(h, cert);
  }
});

[3nprob]

@tniessen For one, it's used for SASL CertFP authentication on major IRC networks like Libera and Freenode. In fact, it's what's provided in NickServ/SaslServ. In practice any IRC user doing certificate auth will need to supply their sha512 fingerprint to nickserv. So any complete IRC client (and some servers) will want this, for example.

It's also used for TOFU on Gemini and is not an uncommon choice for SAML implementations.

I don't think any other hash algorithm comes even close in adoption.

Thanks for the example, it does work for my personal use-case but still think this could be considered for inclusion.

[tniessen]

As far as I know, SHA-512 is highly unusual for fingerprinting certificates

For one, it's used for SASL CertFP authentication on major IRC networks like Libera and Freenode. In fact, it's what's provided in NickServ/SaslServ. In practice any IRC user doing certificate auth will need to supply their sha512 fingerprint to nickserv. So any complete IRC client (and some servers) will want this, for example.
It's also used for TOFU on Gemini and is not an uncommon choice for SAML implementations.

I stand corrected :)

Thanks for the example, it does work for my personal use-case but still think this could be considered for inclusion.

It's still questionable if SHA-512 has any real benefit over SHA-256 and neither IRC nor Gemini are primary domains of Node.js, but since this is apparently being used in the wild, I don't see much harm in adding it as long as it doesn't act as a precedence for adding other digests that are being used by other small domains.

[mildsunrise]

I would also suggest improving the documentation for fingerprint to mention the alternative code. Something like:

cert.fingerprint is equivalent to crypto.createHash('sha1').update(cert.raw).digest('hex').toUpperCase() except for the colon (:) separators.

So that users can easily derive the code for other algorithms they may need.

[tniessen]

I would also suggest improving the documentation for fingerprint to mention the alternative code.

I'll add that in a separate PR, I'd also like to add a few details regarding the format.

[tniessen]

@3nprob However, this does need an update to the YAML block around line 1091 in tls.md (not needed in crypto.md).

[3nprob]

@3nprob However, this does need an update to the YAML block around line 1091 in tls.md (not needed in crypto.md).

I assume you're talking about the example cert? Good catch, just added a bogus fingerprint line

[tniessen]

@3nprob No, I was referring to the YAML block around line 1091, which tracks changes that were made to the API:

changes:
  - version: v11.4.0
    pr-url: https://github.com/nodejs/node/pull/24358
    description: Support Elliptic Curve public key info.

(Unfortunately, GitHub doesn't let me add comments to lines that you did not change in this PR.)

[3nprob]

@3nprob No, I was referring to the YAML block around line 1091, which tracks changes that were made to the API:

changes:
  - version: v11.4.0
    pr-url: https://github.com/nodejs/node/pull/24358
    description: Support Elliptic Curve public key info.

(Unfortunately, GitHub doesn't let me add comments to lines that you did not change in this PR.)

Right! Should be addressed now

[tniessen]

I am not sure why this stalled. @3nprob, would you mind rebasing and fixing the conflict in tls.md? (Feel free to @ ping me, I might miss the notification otherwise.)