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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 4 commits into from
Closed

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

wants to merge 4 commits into from

Conversation

sam-github
Copy link
Contributor

  • test: cover tls multi-identity option mixtures
  • test: confirm tls server suite default is its own
  • test: add independent multi-alg crypto identities
  • test: rename agent1-pfx.pem to agent1.pfx
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

tls

@nodejs-github-bot nodejs-github-bot added test Issues and PRs related to the tests. dont-land-on-v7.x labels Jan 11, 2017
@sam-github
Copy link
Contributor Author

Will rebase onto #10389 once it lands.

@mscdex mscdex added tls Issues and PRs related to the tls subsystem. and removed dont-land-on-v7.x labels Jan 11, 2017
@sam-github
Copy link
Contributor Author

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

@sam-github
Copy link
Contributor Author

@nodejs/crypto

@sam-github
Copy link
Contributor Author

sam-github commented Jan 19, 2017 •
edited
Loading

@shigeki @indutny @nodejs/crypto

bnoordhuis
bnoordhuis requested changes Jan 20, 2017
View reviewed changes
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are ca3, ca4 and ca6 used anywhere?

test/fixtures/keys/Makefile Outdated
cat ca4-cert.pem >> agent8-cert.pem

agent8-verify: agent8-cert.pem ca4-cert.pem
openssl verify -CAfile ca4-cert.pem agent8-cert.pem
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Target should be .PHONY.

test/fixtures/keys/Makefile Outdated
-inkey ec8-key.pem \
-certfile ca6-cert.pem \
-out ec8.pfx \
-password pass:sample
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could make the recipes a little more DRY by using $@, $< and $^ more.

test/fixtures/keys/ca4.cnf Show resolved Hide resolved
test/fixtures/keys/ca5.cnf Show resolved Hide resolved
test/fixtures/keys/ca6.cnf Show resolved Hide resolved
test/parallel/test-tls-honorcipherorder.js Outdated
if (cb) cb();
});
});
const msg = fmt(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you use util.format here? Saves the casual reader from having to look up what fmt() is.

test/parallel/test-tls-honorcipherorder.js Outdated
ciphers: 'AES256-SHA256:AES128-GCM-SHA256:AES128-SHA256:' +
ciphers: 'AES256-SHA256:' +
'AES128-GCM-SHA256:' +
'AES128-SHA256:' +
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stylistic change.

@sam-github sam-github mentioned this pull request Feb 13, 2017
Closed
2 tasks
@silverwind
Copy link
Contributor

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

@sam-github
Copy link
Contributor Author

@silverwind 5c700f9

@sam-github
Copy link
Contributor Author

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

@silverwind
Copy link
Contributor

silverwind commented Feb 14, 2017 •
edited
Loading

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

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

@sam-github
Copy link
Contributor Author

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

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

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:

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

@silverwind
Copy link
Contributor

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.

@sam-github
Copy link
Contributor Author

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.

@shigeki
Copy link
Contributor

shigeki commented Feb 23, 2017

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.

@sam-github sam-github force-pushed the generate-more-certs branch 3 times, most recently from e089bd0 to 3ed5993 Compare March 6, 2017 23:04
@sam-github
Copy link
Contributor Author

@shigeki PTAL

@sam-github sam-github force-pushed the generate-more-certs branch from 3ed5993 to bb8c327 Compare March 20, 2017 15:42
@sam-github
Copy link
Contributor Author

@shigeki any more comments I can address?

@sam-github sam-github force-pushed the generate-more-certs branch from bb8c327 to 912d645 Compare April 17, 2017 20:27
@sam-github
Copy link
Contributor Author

@shigeki ping

@sam-github sam-github force-pushed the generate-more-certs branch from 912d645 to 2089d22 Compare June 14, 2017 16:03
@sam-github
Copy link
Contributor Author

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

@sam-github sam-github force-pushed the generate-more-certs branch from 2089d22 to 9f90b7b Compare July 21, 2017 16:51
@sam-github sam-github dismissed shigeki’s stale review July 21, 2017 16:51

addressed comments, no replies to request for more comments

@sam-github
Copy link
Contributor Author

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

@BridgeAR
Copy link
Member

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

@sam-github sam-github mentioned this pull request Sep 7, 2017
Closed
@BridgeAR
Copy link
Member

BridgeAR commented Sep 8, 2017

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

@BridgeAR
Copy link
Member

BridgeAR commented Sep 8, 2017

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

mhdawson
mhdawson approved these changes Sep 8, 2017
View reviewed changes
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@BridgeAR
Copy link
Member

BridgeAR commented Sep 9, 2017

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

@sam-github
test: rename agent1-pfx.pem to agent1.pfx
89d57b4 
PFX is not PEM, its binary DER. Use the same .pfx extension as
test/fixtures/test_cert.pfx does.
@sam-github
test: add independent multi-alg crypto identities
0bc3166 
agent6 was the only cert that had a chain (an intermediate certificate),
and there were no non-RSA certs other than a single self-signed one.
This makes it impossible to test cert-chain scenarios with multiple
identities which require chains to prove chain completion, and
multi-algorithm because OpenSSL doesn't support multiple identities
unless they are multi-algorithm.

PFX files were also missing for most identities, making it difficult to
test multi-PFX and PFX interactions with cert-chain+key and CA options.

New server cert chains:

- ECC: ca5 signs ca6 signs ec10, CN=agent10.example.com
- RSA: ca2 signs ca4 signs agent10, CN=agent10.example.com

PFX added for:

- agent6
- agent10
- ec10
@sam-github
test: cover tls multi-identity option mixtures
c4d269f 
Prove that cert and key options do not have to be ordered, and that the
pfx option can be used at the same time as the cert/key option
(which was claimed to be impossible by some pre-existing documentation).
@sam-github
test: confirm tls server suite default is its own
64d61e9 
When honorCipherOrder is not explicitly set, it defaults to true, cover
this condition in the test. Also, run all tests in parallel, instead of
sequentially.
@sam-github sam-github closed this Sep 19, 2017
This was referenced Nov 14, 2018
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shigeki shigeki shigeki left review comments

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@mhdawson mhdawson mhdawson approved these changes

Assignees
No one assigned
Labels
test Issues and PRs related to the tests. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@sam-github @silverwind @shigeki @jasnell @BridgeAR @bnoordhuis @mhdawson @mscdex @targos @nodejs-github-bot