doc: clarify tls.createServer's {cert, ca} #7230

wacky6 · 2016-06-08T18:57:58Z

Checklist

documentation is changed or added
the commit message follows commit guidelines

Affected core subsystem(s)

doc

Description of change

clarity cert to match bottom-half
in src/node_crypto.cc setCert() calls SSL_CTX_use_certificate_chain()
clarity ca is to be used with requestCert

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.

silverwind · 2016-11-23T18:37:28Z

doc/api/tls.md

-    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)


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).

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

silverwind · 2016-11-23T18:40:06Z

doc/api/tls.md

-    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`.


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.

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)

Okay, then I totally misunderstood that option. If they are to be replaced, it certainly makes sense to provide a cert bundle instead.

silverwind · 2016-11-23T18:40:48Z

doc/api/tls.md

@@ -965,7 +965,7 @@ const fs = require('fs');

 const options = {
  key: fs.readFileSync('server-key.pem'),
-  cert: fs.readFileSync('server-cert.pem'),
+  cert: fs.readFileSync('server-cert-chain.pem'),


I'd rather like to recommend using the ca option here.

silverwind · 2016-11-23T18:41:36Z

Sorry about the delay, we totally missed this one. I guess a bit more research is necessary on your side :)

sam-github · 2016-11-23T21:24:01Z

doc/api/tls.md

  * `ca` {string|string[]|Buffer|Buffer[]} A string, `Buffer`, array of strings,
    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.


That isn't helpful, you need to describe what it does, and why what it does combines well with other options.

sam-github · 2016-11-23T21:30:20Z

@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.

wacky6 · 2016-11-24T07:40:21Z

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.

sam-github · 2016-11-24T17:55:59Z

I think we should clarify ca's usage. It is currently dual-purpose. I think it should only be used to provide client CA.

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.

silverwind · 2016-11-24T18:33:10Z

@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.

sam-github · 2016-11-24T19:13:38Z

@silverwind we may have terminology mismaches.

I think we should clearly document that ca is primarily intended for client authentication only

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.

, and that bundling the server cert with the CA certs is the recommended way for a regular server.

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"?

fixes nodejs#7230 Reviewed-By: James M Snell <jasnell@gmail.com> PR-URL: nodejs/node-v0.x-archive#25591

silverwind · 2017-02-04T16:39:18Z

@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.

indutny · 2017-02-04T17:57:03Z

They definitely work in the cert array. Will try to look into it today or at Monday.

silverwind · 2017-02-04T20:38:54Z

@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

sam-github · 2017-02-05T00:03:13Z

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.

silverwind · 2017-02-05T09:02:20Z

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.

sam-github · 2017-02-13T22:24:59Z

% 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

silverwind · 2017-02-14T16:53:21Z

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.

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 😉.

silverwind · 2017-02-14T17:17:10Z

@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

silverwind · 2017-02-14T18:55:02Z

Okay I think this was actually just a misinterpretion of the docs on my side.

One cert chain should be provided per private key

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.

fixes nodejs#7230 Reviewed-By: James M Snell <jasnell@gmail.com> PR-URL: nodejs/node-v0.x-archive#25565

nodejs-github-bot added the doc Issues and PRs related to the documentations. label Jun 8, 2016

mscdex added the tls Issues and PRs related to the tls subsystem. label Jun 8, 2016

Trott force-pushed the master branch from b0df363 to c5ce7f4 Compare September 21, 2016 00:09

rvagg force-pushed the master branch 2 times, most recently from c133999 to 83c7a88 Compare October 18, 2016 17:02

silverwind reviewed Nov 23, 2016

View reviewed changes

sam-github reviewed Nov 23, 2016

View reviewed changes

sam-github pushed a commit to sam-github/node that referenced this pull request Dec 14, 2016

Documentation update about Buffer initialization

53b6a61

fixes nodejs#7230 Reviewed-By: James M Snell <jasnell@gmail.com> PR-URL: nodejs/node-v0.x-archive#25591

MylesBorins force-pushed the master branch from 8df7ee0 to 54fef67 Compare February 1, 2017 01:00

silverwind mentioned this pull request Feb 14, 2017

Generate more certs/identities, and use them for better multi-cert test coverage #10747

Closed

3 tasks

silverwind closed this Feb 14, 2017

JacksonTian pushed a commit to JacksonTian/node that referenced this pull request May 4, 2017

Documentation update about Buffer initialization

70efdf3

fixes nodejs#7230 Reviewed-By: James M Snell <jasnell@gmail.com> PR-URL: nodejs/node-v0.x-archive#25565

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

doc: clarify tls.createServer's {cert, ca} #7230

doc: clarify tls.createServer's {cert, ca} #7230

wacky6 commented Jun 8, 2016

silverwind Nov 23, 2016

sam-github Nov 23, 2016

silverwind Nov 23, 2016

sam-github Nov 23, 2016

silverwind Nov 24, 2016

silverwind Nov 23, 2016

sam-github Nov 23, 2016

silverwind commented Nov 23, 2016

sam-github Nov 23, 2016

sam-github commented Nov 23, 2016

wacky6 commented Nov 24, 2016

sam-github commented Nov 24, 2016

silverwind commented Nov 24, 2016

sam-github commented Nov 24, 2016

silverwind commented Feb 4, 2017 •

edited

Loading

indutny commented Feb 4, 2017

silverwind commented Feb 4, 2017

sam-github commented Feb 5, 2017

silverwind commented Feb 5, 2017 •

edited

Loading

sam-github commented Feb 13, 2017

silverwind commented Feb 14, 2017

silverwind commented Feb 14, 2017

silverwind commented Feb 14, 2017

doc: clarify tls.createServer's {cert, ca} #7230

doc: clarify tls.createServer's {cert, ca} #7230

Conversation

wacky6 commented Jun 8, 2016

Checklist

Affected core subsystem(s)

Description of change

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

silverwind commented Nov 23, 2016

Choose a reason for hiding this comment

sam-github commented Nov 23, 2016

wacky6 commented Nov 24, 2016

sam-github commented Nov 24, 2016

silverwind commented Nov 24, 2016

sam-github commented Nov 24, 2016

silverwind commented Feb 4, 2017 • edited Loading

indutny commented Feb 4, 2017

silverwind commented Feb 4, 2017

sam-github commented Feb 5, 2017

silverwind commented Feb 5, 2017 • edited Loading

sam-github commented Feb 13, 2017

silverwind commented Feb 14, 2017

silverwind commented Feb 14, 2017

silverwind commented Feb 14, 2017

silverwind commented Feb 4, 2017 •

edited

Loading

silverwind commented Feb 5, 2017 •

edited

Loading