| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
1. clarity cert to match bottom-half in src/node_crypto.cc setCert() calls SSL_CTX_use_certificate_chain() 2. clarity ca is to be used with requestCert Previously, nothing is mentioned about certificate chain, this may be confusing to beginners. They may pass intermediate certificates to ca option. If requestCert is false, this won't cause any problem. As bottom-half will try to find and insert intermediate certificates from ca store.
| strings, or array of `Buffer`s containing the certificate key of the server | ||
| in PEM format. (Required) | ||
| strings, or array of `Buffer`s containing the server's certificate chain | ||
| (server's certificate and intermediates) in PEM format. (Required) |
There was a problem hiding this comment.
The intermediates should be optional and they are preferably placed in ca. IIRC, OpenSSL does accept both ways of adding them (concatenated vs. through ca).
Sorry, something went wrong.
There was a problem hiding this comment.
a) Why is it preferable to place them in ca? b) even if it is preferable, the ability to provide an entire cert chain here, and the order they should be in, must be documented and it isn't c) I'm about to rewrite much of this, the docs are in rough shape, and this is on my list of things to fix
Sorry, something went wrong.
| clients that connect and attempt to verify that certificate. Defaults to | ||
| `false`. | ||
| clients that connect and attempt to verify that certificate against | ||
| certificates provided in `ca` option. Defaults to `false`. |
There was a problem hiding this comment.
Not entirely correct. Node.js bundles Mozilla's root certificate bundle, which if I'm not mistaken are extended by the certificates provided by the ca option.
Sorry, something went wrong.
There was a problem hiding this comment.
The text is arguably correct, if you consider the ca option to have a default, the bundled certs. But its confusing.
@sliverwind the builtin certs aren't extended by the ca option, they are replaced (for this server)
Sorry, something went wrong.
There was a problem hiding this comment.
Okay, then I totally misunderstood that option. If they are to be replaced, it certainly makes sense to provide a cert bundle instead.
Sorry, something went wrong.
| const options = { | ||
| key: fs.readFileSync('server-key.pem'), | ||
| cert: fs.readFileSync('server-cert.pem'), | ||
| cert: fs.readFileSync('server-cert-chain.pem'), |
There was a problem hiding this comment.
I'd rather like to recommend using the ca option here.
Sorry, something went wrong.
There was a problem hiding this comment.
why?
Sorry, something went wrong.
|
Sorry about the delay, we totally missed this one. I guess a bit more research is necessary on your side :) |
Sorry, something went wrong.
| or array of `Buffer`s of trusted certificates in PEM format. If this is | ||
| omitted several well known "root" CAs (like VeriSign) will be used. These | ||
| are used to authorize connections. | ||
| are used to authorize connections. Useful for `requestCert` option. |
There was a problem hiding this comment.
That isn't helpful, you need to describe what it does, and why what it does combines well with other options.
Sorry, something went wrong.
|
@wacky6 I'm -1 on this so far. Some of the text confuses instead of improving. Also, some of the text you improve is cut-n-paste multiple times in the doc, and you changed only one instance of the copies. Also, it leaves still many things about certs undocumented. You can keep at this, if you like, or wait for me to PR a more comprehensive set of changes. I do agree that the current docs are unusably incomplete. |
Sorry, something went wrong.
|
I think I should explain more on how I find this problem, sorry about my English writing. When I set up for TLS client authentication, I use: const tlsOpts = {
key: serverKey,
cert: serverCert,
ca: [ serverIntermediate, clientCA ],
requestCert: true
}In this setup, both server intermediate and client CA are sent to request certs. OpenSSL output looks like this: Certificate chain 0 s:/CN=Server Cert i:/CN=Server Intermediate 1 s:/CN=Server Intermediate i:/CN=Root CA --- Acceptable client certificate CA names /CN=Server Intermediate ## <- this is wrong /CN=Client CA Server Intermediate should not be used here to request client cert. I think we should clarify ca's usage. It is currently dual-purpose. I think it should only be used to provide client CA. |
Sorry, something went wrong.
CA's usage is pretty much undocumented right now, it should be documented, I'm working on that. We can't change how these APIs work (not easily), so we do need to document that the intermediates can go in ca if that works, but we can say how we recommend the APIs be used. I agree with you, ca should be used for trust roots, cert should be used for cert chains. This is common, many (most?) openssl tutorials show how to concatenate the entity cert and its intermediate chain into a single PEM file (or into a single PFX). I'm curious as to why @silverwind thinks untrusted intermediate certs should be preferentially be put into the ca properties, especially since it seems to cause the server's intermediate to be a trust root, if I'm reading the s_client output correctly. |
Sorry, something went wrong.
|
@sam-github no particular reason, I was under the wrong assumption that ca was the intended place for intermediate certificates (It certainly works though). I think we should clearly document that ca is primarily intended for client authentication only, and that bundling the server cert with the CA certs is the recommended way for a regular server. |
Sorry, something went wrong.
|
@silverwind we may have terminology mismaches.
ca is primarily for using privately issued certs, as opposed to certs with public trust roots. node contains the Mozilla collection of trust roots, so mostly its internal infrastructure/microservices that don't use certs purchased from CAs, and issue their own. In this case, client or server, you need to provide the private trust roots with the CA option.
Why recommend that? Doesn't it confuse things? If you look at @wacky6 's example above, wouldn't it boil down to saying something like: When a server has a intermediate certs in its chain, we recommend: - putting it in the `ca` option if you aren't requesting a client cert - putting it in the `cert` option if you are requesting client auth, and need to provide a trusted CA for the client auth That seems more complicated than "put trusted certs in ca, put own cert chain and key in cert/key". Or do you look at it as "all certs that have the CA bit true should be in ca, trusted or not, your cert or not"? |
Sorry, something went wrong.
fixes nodejs#7230 Reviewed-By: James M Snell <jasnell@gmail.com> PR-URL: nodejs/node-v0.x-archive#25591
|
@sam-github @indutny are we certain that intermediate certs placed in a cert array actually work? Right now I'm attempting this: {
cert: [maincert, intermediate],
key: key
}And OpenSSL just gives me this: error:0B080074:x509 certificate routines:X509_check_private_key:key values mismatch
at Object.createSecureContext (_tls_common.js:85:17)
at Server (_tls_wrap.js:769:25)
at new Server (https.js:27:14)
at Object.createServer (https.js:48:10)
On the other hand, this works as expected: {
cert: maincert,
key: key,
ca: [intermediate]
}I'm pretty sure we are either misdocumenting this and intermediates only work in ca (maybe affecting client auth?) or there's a bug. |
Sorry, something went wrong.
|
They definitely work in the cert array. Will try to look into it today or at Monday. |
Sorry, something went wrong.
|
@indutny I'm pretty sure it's broken. Here's a test that uses a valid cert chain from a test CA I created: https://gist.github.com/silverwind/98d7b717617cbc52bce46498e351ec9f It failes with x509 certificate routines:X509_check_private_key:key values mismatch. Once you move int to ca, a openssl s_client -connect localhost:8000 shows the correct chain: Certificate chain 0 s:/C=GB/ST=England/O=Alice Ltd i:/C=GB/ST=England/O=Alice Ltd 1 s:/C=GB/ST=England/O=Alice Ltd i:/C=GB/ST=England/O=Alice Ltd |
Sorry, something went wrong.
|
Is that a valid cert chain? cert 0 and cert 1 have identical subject and issuer unless I am misreading your comment above, so cert 0 is not signed by cert 1. |
Sorry, something went wrong.
|
Yes (I was following https://jamielinux.com/docs/openssl-certificate-authority almost verbatim): $ openssl verify -CAfile root int int: OK $ openssl verify -CAfile <(cat root int) cert cert: OK The issue should also be reproducible with any LetsEncrypt cert. |
Sorry, something went wrong.
% openssl x509 -text < chain.pem
Certificate:
Data:
Version: 3 (0x2)
Serial Number: 4096 (0x1000)
Signature Algorithm: sha256WithRSAEncryption
Issuer: C=GB, ST=England, O=Alice Ltd
Validity
Not Before: Feb 4 20:32:16 2017 GMT
Not After : Feb 14 20:32:16 2018 GMT
Subject: C=GB, ST=England, O=Alice Ltd
That cert is self-signed, and has exactly the same subject/issuer pair as the intermediate cert, and the CA cert, I think the chain just confuses OpenSSL. Tests for cert chains with intermediate certs: #10747 |
Sorry, something went wrong.
I'm certain that the files in the example are not the cause of this key values mismatch error. I can mail you guys a example using a real cert chain signed by LetsEncrypt if you like 😉. |
Sorry, something went wrong.
|
@sam-github Here's an example using your certs. It fails to start with the dreaded key values mismatch. Once you move int to ca, it works and openssl s_client -connect localhost:8000 shows this chain: Certificate chain 0 s:/C=US/ST=CA/L=SF/O=Joyent/OU=Node.js/CN=agent8.example.com i:/C=US/ST=CA/L=SF/O=Joyent/OU=Node.js/CN=ca4/emailAddress=ry@tinyclouds.org 1 s:/C=US/ST=CA/L=SF/O=Joyent/OU=Node.js/CN=ca4/emailAddress=ry@tinyclouds.org i:/C=US/ST=CA/L=SF/O=Joyent/OU=Node.js/CN=ca2/emailAddress=ry@tinyclouds.org |
Sorry, something went wrong.
|
Okay I think this was actually just a misinterpretion of the docs on my side.
With the intermediate in a separate array entry, openssl gets rightly confused for the missing key. So I guess no action needed here. As for this PR: I'll close. Docs have been overhauled in the meanwhile and I guess it just doesn't apply anymore, sorry. |
Sorry, something went wrong.
fixes nodejs#7230 Reviewed-By: James M Snell <jasnell@gmail.com> PR-URL: nodejs/node-v0.x-archive#25565
| Back | FazBrowse Home | New Git URL |
Checklist
Affected core subsystem(s)
doc
Description of change
in src/node_crypto.cc setCert() calls SSL_CTX_use_certificate_chain()