GeezerVison

Will rebase onto #10389 once it lands.

Rebased onto #10771 so that tests can pass. Will rebase again once #10771 merges.

@nodejs/crypto

@shigeki @indutny @nodejs/crypto

Are there any matching cert + intermediate + key files in here? Asking so I can test #7230 (comment).

@silverwind 5c700f9

@silverwind bf43a60a383fa5fce1051532c4e499c4b1df4c07 has the tests you are looking for, I believe, but this PR is partially broken at the moment, I am in process of rebasing (master had an agent8 and agent9 added to it, so I need to move my numbered certs forward, which I'm in process of doing right now).

Hmm, can't find a fitting case there, but this should be a failing example:

cert: [agent10-cert, ca4-cert]
key: agent10-key

@silverwind

I think I see what is happening, you are trying to provide the certs a+b+c of a cert chain as cert: [a, b, c]. This is invalid usage, not documented, and doesn't surprise me it doesn't work (though honestly, it wouldn't surprise me if it had worked, either, ossl is a bit weird). Also, I think you haven't noticed that the PEM files built by the Makefile are chains (they have multiple certs in the single PEM file if there are intermediates).

Pre-existing test with pre-existing agent6 cert:

• https://github.com/sam-github/node/blob/d125ee843b015ec9751562945426f620021ba19c/test/parallel/test-tls-multi-key.js#L80-L97

This PR adds tests for mixed algorithm identies, where the mixed algs have intermediate certs:

• node/test/parallel/test-tls-cert-chains-concat.js

  Lines 18 to 21 in ac602a2

  	server: {
  	cert: keys.agent6.cert,
  	key: keys.agent6.key,
  	},

Your example:

cert: [agent10-cert, ca4-cert]
key: agent10-key

is invalid usage. agent10-cert.pem is a CHAIN, it already contains ca4-cert.pem, as did the pre-existing cert that had an intermediate cert, agent6:

• https://github.com/sam-github/node/blob/d125ee843b015ec9751562945426f620021ba19c/test/fixtures/keys/Makefile#L353
• https://github.com/sam-github/node/blob/d125ee843b015ec9751562945426f620021ba19c/test/fixtures/keys/Makefile#L464

Docs say:

Optional cert chains in PEM format. One cert chain should be provided per private key. Each cert chain should consist of the PEM formatted certificate for a provided private key, followed by the PEM formatted intermediate certificates (if any), in order, and not including the root CA (the root CA must be pre-known to the peer, see ca).

Your example above has two identities, one is agent10+ca4, and the other is just ca4. This is weird, because ca4 is a sub-chain of the first identity, and also, because they are both RSA, and OpenSSL doesn't permit multiple identities of the same algorithm, because it doesn't know which one to pick (unless you use SNI callback, when it picks by the server name).

Sorry, for agent10-cert I actually meant just the first cert, not the whole chain in that file. Let's call the individual certs agent10-cert and agent10-int to avoid confusion.

So the interesting part is that it works if I concatenate agent10-cert and agent10-int into a single file, but not if I provide them individually in the form [agent10-cert, agent10-int]. I think this should be fixable.

So the interesting part is that it works if I concatenate agent10-cert and agent10-int into a single file, but not if I provide them individually in the form [agent10-cert, agent10-int]. I think this should be fixable.

Exactly. That is the documented behaviour, so lets not hijack this PR to discuss it further, now that we agree what is happening. Open another issue if you think there is a problem and we can discuss there.

One thing I would note is that it would be extremely helpful to have some documentation around these to help folks understand how and where to use the keys, how and where they are already used, etc.

I agree. I also would like to change file names and cert attributes to see its use and purpose for ease.

@shigeki PTAL

@shigeki any more comments I can address?

@shigeki ping

@shigeki I have responded to all comments, how can I move this forward?

ci: https://ci.nodejs.org/job/node-test-pull-request/9285/

@sam-github would you be so kind and rebase? I guess it is otherwise ready?

New CI https://ci.nodejs.org/job/node-test-pull-request/9990/

@mhdawson @bnoordhuis would you be so kind and reconfirm your LG?

@sam-github the CI is dark red. Seems like some tests need some additional work.