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_wrap does not forward privateKeyEngine and privateKeyIdentifier to tls.createSecureContext() on Server opening #36322

Closed
BenBaratte opened this issue Nov 30, 2020 · 2 comments
Labels
tls Issues and PRs related to the tls subsystem.

Comments

@BenBaratte
Copy link

  • Version: v14.15.1
  • Platform: Linux XXX 4.4.0-17763-Microsoft building for mips  #1432-Microsoft Mon Aug 18 18:18:00 PST 2020 x86_64 GNU/Linux
  • Subsystem: lib/_tls_wrap.js

What steps will reproduce the bug?

in the options pass to the https.createServer() functions, all parameters are forwarded to tls.createSecureContext().
the issue is that in the _tls_wrap.js, the Server.prototype.setSecureContext() explicitly log the options structure before forwarding it to tls.createSecureContext()
there is 2 options that are managed by the tls.createSecureContext() that are not forward :
privateKeyEngine, privateKeyIdentifier

The impact is that the https server can not use OpenSSL engine to manage the server private key.

here is a basic example :

var fs = require('fs')
var https = require('https')

var engine_path='/usr/lib/x86_64-linux-gnu/engines-1.1/pkcs11.so'

var server = https.createServer({
maxVersion: 'TLSv1.2',
minVersion: 'TLSv1.2',
privateKeyEngine: engine_path,
privateKeyIdentifier : '0',
cert: fs.readFileSync('server.pem'),
ciphers:'ECDHE-ECDSA-AES128-GCM-SHA256',
enableTrace:true,
sigalgs:'ECDSA+SHA256',
ecdhCurve:'P-256:P-384',
})
.listen(3000, function () {
console.log('Example app listening on port 3000! Go to https://localhost:3000/')
})

How often does it reproduce? Is there a required condition?

functional issue, only specific configuration to highlight the issue.

What is the expected behavior?

bad engine id
Failed to enumerate slots
Failed to enumerate slots
PKCS11_get_private_key returned NULL
_tls_common.js:182
c.context.setEngineKey(privateKeyIdentifier, privateKeyEngine);
^

Error: error:80067065:pkcs11 engine:ctx_load_privkey:object not found
at Object.createSecureContext (_tls_common.js:182:19)
at Server.setSecureContext (_tls_wrap.js:1332:27)
at Server (_tls_wrap.js:1181:8)
at new Server (https.js:66:14)
at Object.createServer (https.js:91:10)
at Object. (/home/ben/node-v14.15.1-linux-x64/testproj/express_engine.js:7:20)
at Module._compile (internal/modules/cjs/loader.js:1063:30)
at Object.Module._extensions..js (internal/modules/cjs/loader.js:1092:10)
at Module.load (internal/modules/cjs/loader.js:928:32)
at Function.Module._load (internal/modules/cjs/loader.js:769:14) {
opensslErrorStack: [
'error:26096080:engine routines:ENGINE_load_private_key:failed loading private key'
],
library: 'pkcs11 engine',
function: 'ctx_load_privkey',
reason: 'object not found',
code: 'ERR_OSSL_USER_OBJECT_NOT_FOUND'
}

This error message is expected as the pkcs11 engine is called.

I made this example so everyone can reproduce the issue.

What do you see instead?

Example app listening on port 3000! Go to https://localhost:3000/

The pkcs11_engine is not call and the private key is not configured.

Additional information

Bascially, for https client, the engine can be called by using the option clientCertEngine but for the https server, the engine can't reach due to the parameters filtering.

For my testing, I'm using a proprietary OpenSSL engine and I'm able to authenticate the server thanks to the HSM private key.
This should be working as well with the TPM2 TSS engine is available on a Linux PC.

@targos
Copy link
Member

targos commented Dec 6, 2020

@nodejs/crypto

@targos targos added the tls Issues and PRs related to the tls subsystem. label Dec 6, 2020
@mildsunrise
Copy link
Member

yes, there's a few places in the code (tls.Server#setSecureContext, tls.Server#setOptions and apparently also https.Agent#getName) where we manually handle each parameter, and we forgot to update them in #28973... I'll take a look and open a fix

mildsunrise added a commit to mildsunrise/node that referenced this issue Dec 7, 2020
We have a few places where we individually forward each
parameter to tls.createSecureContext(). In nodejs#28973 and others,
we added new SecureContext options but forgot to keep these
places up to date.

As per https.Agent#getName, I understand that at least
`privateKeyIdentifier` and `privateKeyEngine` should be
added too, since they're a substitute for `key`. I've
also added sigalgs.

Fixes: nodejs#36322
Refs: nodejs#28973
targos pushed a commit that referenced this issue Dec 21, 2020
We have a few places where we individually forward each
parameter to tls.createSecureContext(). In #28973 and others,
we added new SecureContext options but forgot to keep these
places up to date.

As per https.Agent#getName, I understand that at least
`privateKeyIdentifier` and `privateKeyEngine` should be
added too, since they're a substitute for `key`. I've
also added sigalgs.

Fixes: #36322
Refs: #28973

PR-URL: #36416
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
targos pushed a commit that referenced this issue May 1, 2021
We have a few places where we individually forward each
parameter to tls.createSecureContext(). In #28973 and others,
we added new SecureContext options but forgot to keep these
places up to date.

As per https.Agent#getName, I understand that at least
`privateKeyIdentifier` and `privateKeyEngine` should be
added too, since they're a substitute for `key`. I've
also added sigalgs.

Fixes: #36322
Refs: #28973

PR-URL: #36416
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
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 a pull request may close this issue.

3 participants