Open cinderblock opened 5 years ago
Would it make sense to document this feature?
I’m not sure it’s safe to consider secureContext.context
public API, but I think it should be okay to add a public secureContext.addCACert()
wrapper around it?
@nodejs/crypto may have more insight (and more helpful opinions).
I'm not sure a wrapper is the right solution. That feels like the start of extra functions to maintain.
Besides appending to or enumerating the current list, removing is equally useful (as opposed to revoking).
I wonder if adding more documentation would enable a home rolled SecurityContext
with arbitrary rules.
If the default list of CAs were accessible in node, we could do this ourselves without adding extra APIs.
If this is your use case, I believe it is met by https://github.com/nodejs/node/pull/26415
Adding direct access to SecureContext.context is a no-go, we're more likely to deprecate that and then make it inaccessible from user space, re-exporting well defined functions. This allows us to do less error checking in C++, among other things, and just assert on C++ API misuse.
a home rolled SecurityContext
Not sure if that is feasible, only the .context
(a wrapper (EDIT: "container" is a better term) around an OpenSSL SSL_CTX
object) is passed into C++, so its not replaceable in javascript if that is what you hoped to do.
If the default list of CAs were accessible in node, we could do this ourselves without adding extra APIs.
If this is your use case, I believe it is met by #26415
This would solve my use case with documented API, albeit a little more cumbersome than my current approach.
Adding direct access to SecureContext.context is a no-go, we're more likely to deprecate that and then make it inaccessible from user space, re-exporting well defined functions. This allows us to do less error checking in C++, among other things, and just assert on C++ API misuse.
Just to confirm, should we expect the SecureContext.context
API to go away?
a home rolled SecurityContext
Not sure if that is feasible, only the
.context
(a wrapper (EDIT: "container" is a better term) around an OpenSSLSSL_CTX
object) is passed into C++, so its not replaceable in javascript if that is what you hoped to do.
Yeah, that is what my mind wandered to. Figured it was something like that.
should we expect the SecureContext.context API
Let me be abolutely clear: SecureContext.context
is an undocumented implementation detail of SecureContext
. It is not an API.
Because node.js attempts not to break user's code, even code that depends on undocumented implementation details or internal properties, we are unlikely to make breaking changes to SecureContext outside a semver-major, but you use it at your own risk.
The ability to add additional certificate authorities at runtime is an important need for the company I work for. The short version is that the lack of support for Windows' built-in certificate store results in many developers slamming NODE_TLS_REJECT_UNAUTHORIZED=0 into their code as a quick workaround so they can debug and test locally.
@sam-github any thought on what a well designed Node.js API for adding additional CAs would look like?
The most obvious idea would be to let tls.rootCertificates
be mutable or assignable, although its unfortunate design with a field rather than a function makes it a significant challenge.
Alternately, what do you think about tls.addRootCertificate(cert)
? Assuming the API is agreeable, I could look at putting together a PR.
@ebickle Your devs have demonstrated that they are comfortable setting an env variable, so why not set NODE_EXTRA_CA_CERTS instead of NODE_TLS_REJECT_UNAUTHORIZED?
ca: tls.rootCertificates.concat([myCA1, myCA2])
would also work fine.
That said, I personally don't have any problem with a mutable tls.DEFAULT_CA
(similar to the other tls.DEFAULT_
properties), and would prefer that to a method (or set of methods) that mutates tls.rootCertificates
.
The crux of it is that NODE_EXTRA_CA_CERTS cannot be set at runtime, unfortunately.
The longer answer is is that developers run into the TLS verification failure and they're in the mental mode of "get the project working" not "configure my development workstation". They Google around for documentation, give NODE_EXTRA_CA_CERTS a try at runtime and fail, then give up and toss NODE_TLS_REJECT_UNAUTHORIZED into the codebase regardless of how much security training we toss at them. They can't find the solution (at a project-level) within a reasonable amount of time and then move on to get their work done.
In an enterprise-scenario with a very large number of nonhomogeneous developer workstations around the world, getting NODE_EXTRA_CA_CERTS on the dev machines consistently is also an operational challenge.
Appreciate the advice on using tls.rootCertificates
, I'll give that a go and write it up as a pattern to share with devs internally.
tls.DEFAULT_CA
is a good idea but wouldn't directly cover the need to avoid overriding CAs for systems that need to access both public and internal services. Expanding on that idea, what do you think about this?
tls.DEFAULT_CA
- The default value of the ca
option of tls.createSecureContext()
.tls.DEFAULT_OVERRIDE_CA
- the default value of the overrideCA
option of tls.createSecureContext()
overrideCA
option to tls.createSecureContext()
with a default value of true
. When set to false
, the ca
option adds additional trusted CA certificates to the default well-known ones instead of overriding them.Bigger change, but gets directly to the use case of wanting to add private enterprise CAs to Node.js without affecting anything else. By adding it to createSecureContext, the functionality would be consistent across all of Node.js instead of just being patched in here or there.
An alternate solution would be to have an extraCA
option on tls.createSecureContext()
and a tls.DEFAULT_EXTRA_CA
. It's interesting in that it aligns with the environment variable - you could even see internal functionality of the environment variable reading the CA files off disk and setting DEFAULT_EXTRA_CA at startup.
In an enterprise-scenario with a very large number of nonhomogeneous developer workstations around the world, getting NODE_EXTRA_CA_CERTS on the dev machines consistently is also an operational challenge.
But you asked for a js API to add a cert... which means that a cert (probably in PEM) has to exist on the machine (probably checked into the code base) to be passed to that js API....
I'm not seeing how modifying a project's package.json to do start: NODE_EXTRA_CA_CERTS=ca.pem node bin/www
is any more or less of an operational problem than modifying the project's source code so that it does tls./*some js*/(fs.readFileSync('ca.pem'))
. In both cases, the dev needs the enterprise's CA cert.
In a sufficiently enterprise env, I'd expect the start script to be baked into the base Dockerfile.
tls.DEFAULT_CA is a good idea but wouldn't directly cover the need to avoid overriding CAs for systems that need to access both public and internal services.
Please describe why not. It seems completely equivalent to your proposals, but much simpler and consistent with how tls is configured now.
Your initial suggestion was:
what do you think about tls.addRootCertificate(cert)? Assuming the API is agreeable, I could look at putting together a PR.
Assuming that your suggestion supported your use-case (which I did assume!), your example API is identical in behaviour to my suggestion:
tls.DEFAULT_CA.push(cert)
This "override" API proposal doesn't add anything that doesn't exist already, as far as I can see:
Add tls.DEFAULT_OVERRIDE_CA - the default value of the overrideCA option of tls.createSecureContext() Add overrideCA option to tls.createSecureContext() with a default value of true. When set to false, the ca option adds additional trusted CA certificates to the default well-known ones instead of overriding them.
It just allows writing:
tls.createSecureContext({
ca: CERTS,
overrideCA: false // EDIT: sorry, got the boolean wrong just now, false was intended
})
instead of what is possible right now:
tls.createSecureContext({
ca: tls.rootCertificates.concat(CERTS),
})
The tls.DEFAULT_CA.push(cert)
example makes a lot of sense. For some reason I had visualized that field being null by default and only used if set.
How would you see tls.rootCertificates
working with it? rootCertificates
would be the immutable set of the default node.js root certificates plus the ones loaded from NODE_EXTRA_CA_CERTS and DEFAULT_CA
would be initialized at startup (or first use) to a copy of rootCertificates
?
Yes, that's what I'd expect.
I've been doing some initial work on this over the past few weeks.
After digging through the code, I'd recommend against a tls.DEFAULT_CA
option. The biggest issue is that it would introduce a significant performance degradation for all TLS connections; every time a TLS channel is created it would have to initialize a new X509_STORE
rather than reuse the existing root_cert_store
. There's already some GitHub issues around the performance of the CA
option with a small number of certs; increasing the number of certs (e.g. including the default ones) and making it the default would only make the performance significantly worse.
What about adding a setter to tls.rootCertificates
instead?
After validating the input and ensuring the certs can be read as valid X509 objects they can be placed in the root_cert_store
static variable of node_crypto.cc
, similar to how the NODE_EXTRA_CA_CERTS
are handled. The X509_STORE
is reference counted, so all existing references to the old root_cert_store continue working in their SecureContexts while any new ones get the 'set' CAs.
The frozen nature of the tls.rootCertificates
property is advantageous; it would encourage consumers to make a single "update" to the root certificate store instead of multiple small ones.
Separately, while doing a code review of node_crypto.cc
I noticed a few software defects - what's the best way to discuss or handle those? I'd like a sanity check before I toss them in as GitHub issues. Looks as though a few edge cases were missed when NODE_EXTRA_CA_CERTS
and tls.rootCertificates
were added.
every time a TLS channel is created it would have to initialize a new X509_STORE rather than reuse the existing root_cert_store.
The performance issues are a good point. What about cacheing?
If the textual value of DEFAULT_CA is the same as that used for the default X509_STORE, use the default store?
It seems like a general fix to the ca:
peformance problem would be to cache X509_STORE objects using the PEM input as a key. Keep the builtin always cached, keep a cache size of 2 or something small for others (I suspect that usually ca is set to a single string used for all connections).
Separately, while doing a code review of node_crypto.cc I noticed a few software defects
PR fixes for them, or open an issue to ask if they are a problem, or ask in the slack/irc if you'd like a quick conversation. If you think they are security problems, report on hacker one (see our security page).
I just want to add a use case here that's not addressed by the ability to modify global TLS defaults or other approaches discussed above: trusting an extra CA, in addition to both the built-in root CAs and NODE_EXTRA_CA_CERTS CAs etc, for just a single connection.
I'm writing this within a JS library, not the end application, so I do not control Node's configuration, and I need to add an extra CA for a connection in a way that works regardless of options & global state.
In this case:
ca
option isn't usable by itself - it replaces the list entirely.tls.rootCertificates
, some kind of DEFAULT_CA property) doesn't help, since I only want to configure a single connection.tls.rootCertificates
and extending the defaults isn't possible, since it doesn't include NODE_EXTRA_CA_CERTS etc.The 3rd point was noted and fixed in the past (see #32074) but the fix caused issues with --openssl-use-def-ca-store
and was reverted (see #32229), redefining tls.rootCertificates
explicitly as only returning the built-in CA certificates from build time, not the current set of default CAs. AFAICT there is no way to access the default ca
value of a SecureContext right now.
This use case could be supported by one of either:
ca
at runtime, such that it can be extended and passed as an option explicitly when creating a new SecureContext. This would require a new API to access this without the tls.rootCertificates
issues that caused the revert in #32229. That's more difficult, but might be preferable to exposing internals.additionalCAs
) to create a SecureContext with additional CAs, rather than replacement CAs. This looks very practical, just extending the CA setup code to call both addRootCerts
and addCACerts
, instead of just one or the other. I've looked and I can't see a clear downside to that in the underlying implementations (i.e. running this and then this) but I'm not familiar with the implementation details.@pimterry I have added a monkey-patch to the standard library tls
module in https://www.npmjs.com/package/ca-append / https://github.com/dhermes/ca-append-js that adds caAppend
and caReplace
arguments to tls.createSecureContext()
. It works quite well and I have benchmarked it to try to represent the overhead.
@pimterry Glad to see someone else digging into this area again!
I've been wanting to write a proper fix for Node in this area, particularly after my failed attempt with #32074
Ultimately I realized that most of Node.js' "additional certificate handling" is a bit of a bolted on hack (for better or worse). It works, but it doesn't provide a solid foundation for additional enhancements. What I was hoping to do was provide a class/type that provides an abstraction for a certificate store. The "certificate store object" could then be used to add additional certificates (.add
). The constructor (or other methods) could allow the creation of a certificate store based on Node.js' defaults or a blank/fresh one. The certificate store could then be assigned to a specific secureContext or the default one.
It would be an efficient solution that covers most uses cases and provides a solid foundation for future improvements.
I didn't want to propose a change that significant on my own, but if a few others are interested I'm happy to collaborate on a fork to see if a solid proposal comes together.
There has been no activity on this feature request for 5 months and it is unlikely to be implemented. It will be closed 6 months after the last non-automated comment.
For more information on how the project manages feature requests, please consult the feature request management document.
I would still find a solution to this useful, and I'd like to fix it. Would a PR potentially be accepted to:
additionalCa
option to tls.createSecureContext
ca
- throwing if you try to set both)additionalCa
is set, call addRootCerts
followed by addCACerts
here, instead of just calling one or the other.(Happy to bikeshed the name if this is workable - extraCa
could be preferable to match the env var, for example)
I haven't tested this yet, but from a skim everything suggests that should work fine. As far as I'm aware, by doing this as part of normal context setup the performance impact here would be effectively zero (a single if (additionalCa)
check) for existing TLS code, and minimal (very similar to using the ca
option on any TLS context today) for any code that actively uses this feature. It also doesn't involve exposing internals or changing any of the lower-level code at all.
I think this would immediately fix this issue for the individual context case, which was the original motivating problem for @cinderblock at the start of this issue, and would solve my own use case above too.
This doesn't fix the more general "globally trust an extra CA, defined at runtime" issue that @ebickle is facing, but it does at least provide an API to awkwardly work around that by adding additional CAs to every context individually.
Should I put together a PR?
@pimterry I was going to suggest the exact same thing.
I don't feel strongly about the naming, but as an alternative a boolean could be used to avoid exclusive ca
/additionalCa
check. For example, a boolean overrideCA
option (defaulting to true
) could control whether ca
overrides or appends.
It's been awhile since I've looked at code, but we should make sure the new option can flow through from HTTPS agent options as well (new https.Agent([options])
). This will be consistent with the existing behavior and allow global configuration of additional client-side CAs via https.defaultAgent
.
I'm not in a position to know if they'll accept a PR, but this is a strong use case and simple enough code that they likely will. A PR makes sense to me! If you get busy, let me know and I'll code up the PR. Happy to help review too.
Ok thanks @ebickle. No response from the core team yet, but I'll put together a PR later this week and hopefully that'll spark some progress.
Just to update here: I had a first pass at this, but sadly it turns out it's not quite as quick & easy as I'd hoped. You absolutely can call addRootCerts
then addCACerts
during context setup, but when adding to a root-store like this, addCACerts
internally creates a new root cert store (here) from scratch and the logic to do so (here) does not include NODE_EXTRA_CA_CERTS in that store.
That means the proposed small change would indeed work and allow you to use additionalCA
to add a CA without removing root CAs, but because it doesn't include NODE_EXTRA_CA_CERTS it's effectively just equivalent to ca: tls.rootCertificates.concat(additionalCerts)
, which you can do already.
I'll keep digging, I think there's a path through here by changing NewRootCertStore
to pull in the NODE_EXTRA_CA_CERTS
value too, if it was used previously, but that feels much more likely to risk wider implications in very sensitive code, so it needs some thought... Opinions/ideas welcome :smile:
@pimterry The behavior of NewRootCertStore
not adding the NODE_EXTRA_CA_CERTS
is a bug. I've documented it over at https://github.com/nodejs/node/issues/32010
I'm experimenting with solving both issues in one go - I've crypto_context.cc
so that the extra root certificates are stored in root_certs_vector
, being mindful of the extra certs still needing to work when the hard-coded root certificates are not used.
My initial POC works right now, including additionalCA
, but I still need to run through some unit and performance tests. If all goes well, hopefully I'll have some code to share soon.
Oh great work! Glad to see somebody investigating this already. I wasn't sure if that was a bug or intentional due to some problematic implications of changing this, but I'd be very happy to see it fixed :+1:
@pimterry I still need to write some unit tests and do a thorough review, but if you want to experiment take a look at https://github.com/ebickle/node/tree/fix/tls-missing-extra-certificates
I also haven't settled on what naming / design pattern I like best yet, but you can test it out by modifying lib/internal/tls/secure-context.js
like this:
const
at line 127, add additionalCA,
if (ca) { ... }
block on lines 149 through 153, add another block like this:
if (additionalCA) {
addCACerts(context, ArrayIsArray(additionalCA) ? additionalCA : [additionalCA], `${name}.additionalCA`);
}
Then compile (using my fork/branch as the base) and add additionalCA
to the TLS options of a request. Works in my simple little test project.
There has been no activity on this feature request for 5 months and it is unlikely to be implemented. It will be closed 6 months after the last non-automated comment.
For more information on how the project manages feature requests, please consult the feature request management document.
There has been no activity on this feature request and it is being closed. If you feel closing this issue is not the right thing to do, please leave a comment.
For more information on how the project manages feature requests, please consult the feature request management document.
This is not the right thing to do. Can we get a never-stale
?
:(
Good nudge @cinderblock!
Since all that ancient discussion above, I've now joined the Node core team, so I can now help a bit more with actually making this happen :smile:.
I agree this would be extremely useful for some use cases, so I've reopened this & added never-stale.
I think:
extraCA
option to contexts that would add a CA and preserve the existing defaults, including NODE_EXTRA_CA_CERTS
etc that aren't in rootCertificates
)I think if we can get that PR merged (fixing https://github.com/nodejs/node/issues/32010), then this approach should work with very simple changes (AFAICT) and then this issue would be sorted. I don't have a lot of time to dig into this in depth, but I'm happy to help shepherd from Node maintainer side.
Is that all correct? If so, step 1 is fixing the conflicts in https://github.com/nodejs/node/pull/44529 and then finishing up that PR. @ebickle I know this has been a bit of a saga, but any chance you're interested in working on that to wrap this up once & for all?
@pimterry I'm happy to dust off my PR and resolve the conflicts or otherwise get it back up and ready.
I left off with some exploration of other open source tools to see how they integrated OpenSSL directly with the Windows certificate store with the hope that we could do something similar in Node - e.g. using an OS's built in certificates helps resolve the need for adding certificates manually at runtime. I got about halfway done before I realized node.js doesn't have much platform-specific logic outside of libuv, and libuv doesn't have TLS as a feature. I had wanted to think of a good solution for that and then got busy. I'd also played with some ideas for fancier higher level constructs like a "CertificateStore" object in Node.js that could be used at the global/environment level or as a parameter for a TLS agent. I got that about 70% done before I figured I was over-engineering... but if any of that sounds interesting, let me know!
In any event, the plan outlined above sounds good and gets the job done in the shorter/medium term!
[...] I'd also played with some ideas for fancier higher level constructs like a "CertificateStore" object in Node.js that could be used at the global/environment level or as a parameter for a TLS agent. I got that about 70% done before I figured I was over-engineering... but if any of that sounds interesting, let me know!
This all sounds very cool! Definitely interesting to explore, but yes that's much more complicated & it'd definitely require some wider debate since it's quite a major change. Let's start with solving the practical problem and see where we can go from there.
If you can ping me on that PR once the conflicts are resolved and you think it's ready, I'll review and help move it forwards :+1:
In some cases, I've found that I've wanted to add a single CA to the list of trusted CAs that Node.js uses by default. There seems to be no documented way to do this. As it stands, officially, if you want to use non standard CAs, you can, but must also specify all CAs that might have otherwise been loaded automatically.
From the
createSecureContext
documentation:In trying to accomplish this, I've come across a seemingly stable but undocumented API https://github.com/nodejs/node/issues/20432#issuecomment-441514919
It looks to be a real API for a number of reasons:
_
)Would it make sense to document this feature?
Alternatives
If the default list of CAs were accessible in node, we could do this ourselves without adding extra APIs. I have not actually looked at if this is possible or unintentionally exposed by the SecureContext API.
Related Issues
https://github.com/nodejs/node/issues/4464 https://github.com/nodejs/node/issues/20432 https://github.com/nodejs/node/pull/26908#issuecomment-479147423