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

Provide clearer error messages when a TLS pfx does not contain a private key #49562

Closed
pimterry opened this issue Sep 8, 2023 · 2 comments · Fixed by #49566
Closed

Provide clearer error messages when a TLS pfx does not contain a private key #49562

pimterry opened this issue Sep 8, 2023 · 2 comments · Fixed by #49566
Labels
feature request Issues that request new features to be added to Node.js.

Comments

@pimterry
Copy link
Member

pimterry commented Sep 8, 2023

What is the problem this feature will solve?

Save this p12 on disk:
badssl.com-client-cert-only.p12.txt (rename to drop the .txt - required by GH)

This is the client certificate from the test in https://badssl.com/, but with the key removed from the pfx, so it contains only the certificate without its key.

Run this code:

const tls = require('tls');
const fs = require('fs').promises;

const pfx = await fs.readFile('./badssl.com-client-cert-only.p12');

tls.connect({ pfx, passphrase: 'badssl.com' })

This attempts to make a TLS connection, but before the connection attempt is made, it throws:

Uncaught Error: passed a null parameter
    at configSecureContext (node:internal/tls/secure-context:278:15)
    at Object.createSecureContext (node:_tls_common:116:3)
    at Object.connect (node:_tls_wrap:1718:48)

(in Node v20.5.1, and I suspect something equivalent in all modern versions)

As errors go, this is pretty confusing, because there are no null values here at all. Here it's relatively clear that this argument is wrong, but in more complex cases there can be much more TLS setup & configuration involved, and it can get significantly harder to trace down the issue.

What is the feature you are proposing to solve the problem?

Node is correct to fail here - this is clearly a mistake. That said, it would be significantly better Node provided a clear error that told the user which parameter was wrong. In a perfect world, it would tell the user that the private key specifically is missing here.

It seems the current error message comes from within OpenSSL itself, which defines the same message here. That suggests we're not validating this before it's passed through to some OpenSSL API here - if practical & easy it would be good to do so.

At the very least, we do know exactly where we're failing here (the line in question specifically relates to loading this one TLS parameter, using the passphrase to do so), and so at least wrapping this error with a Failed to load PFX: <message> would be a good start to make it clear which parameter is failing.

What alternatives have you considered?

Right now, the only alternative is careful debugging to trace down each value, examine them, and eventually extract the PFX itself and check its contents (and realise that a private key should be present & is missing). That can be very slow and awkward, and it's counter-intuitive since you're looking for null values but there are none!

@pimterry pimterry added the feature request Issues that request new features to be added to Node.js. label Sep 8, 2023
@bnoordhuis
Copy link
Member

You should be able to check if the pkey out parameter is nullptr after the PKCS12_parse() call but before passing it to SSL_CTX_use_PrivateKey(). That "passed a null parameter" error is coming from the latter.

There's this big if statement in SecureContext::LoadPKCS12() that you will want to break up in separate statements because right now it tries to do everything all at once.

@pimterry
Copy link
Member Author

pimterry commented Sep 8, 2023

Ah, here, right? That makes sense, I'll look into that and open a PR for this shortly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants