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, and more test coverage using them #24374

Closed
wants to merge 4 commits into from
Closed

generate more certs, and more test coverage using them #24374

wants to merge 4 commits into from

Conversation

sam-github
Copy link
Contributor

@sam-github sam-github commented Nov 15, 2018 •
edited
Loading

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot
Copy link
Collaborator

@sam-github build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1619/pipeline

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Nov 15, 2018
@sam-github
Copy link
Contributor Author

Update of #10747, which was approved by @bnoordhuis and @mhdawson

Tests pass locally, lets see what ci thinks: https://ci.nodejs.org/job/node-test-pull-request/18621/

@sam-github
Copy link
Contributor Author

Mostly passed, some problems with with windows-binary on vs2017... shouldn't be related, but I'm rebuilding.

@Trott
Copy link
Member

Trott commented Nov 15, 2018

Mostly passed, some problems with with windows-binary on vs2017... shouldn't be related, but I'm rebuilding.

There was a single problematic Windows host on CI that I just took offline several minutes ago, so you may need to do one more rebuild to get to green. Sorry.

@sam-github
Copy link
Contributor Author

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

@Trott
Copy link
Member

Trott commented Nov 15, 2018 •
edited
Loading

Resume Build: https://ci.nodejs.org/job/node-test-pull-request/18641/ ✅

@sam-github
Copy link
Contributor Author

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

bnoordhuis
bnoordhuis approved these changes Nov 15, 2018
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.

LGTM. I didn't do a bit-for-bit check of the generated certs though. :-)

test/fixtures/keys/agent10.cnf Outdated Show resolved Hide resolved
test/fixtures/keys/Makefile Outdated Show resolved Hide resolved
test/parallel/test-tls-multi-key.js Show resolved Hide resolved
@sam-github
test: rename agent1-pfx.pem to agent1.pfx
43f5daf 
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
2dc83b7 
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

All pem and pfx regenerated from scratch to test that the Makefile is
actually working as intended.
@sam-github
test: cover tls multi-identity option mixtures
d9cd4c7 
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
27af1b0 
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
Copy link
Contributor Author

Regenerated certs after test: add independent multi-alg crypto identities, with make clean; make; make test passing.

@sam-github
Copy link
Contributor Author

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

@sam-github
Copy link
Contributor Author

Landed in a569686...13a6798

@sam-github sam-github closed this Nov 17, 2018
@sam-github sam-github deleted the generate-more-certs branch November 17, 2018 01:01
sam-github added a commit that referenced this pull request Nov 17, 2018
@sam-github
test: rename agent1-pfx.pem to agent1.pfx
f770106 
PFX is not PEM, its binary DER. Use the same .pfx extension as
test/fixtures/test_cert.pfx does.

PR-URL: #24374
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
sam-github added a commit that referenced this pull request Nov 17, 2018
@sam-github
test: add independent multi-alg crypto identities
ec6b793 
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

All pem and pfx regenerated from scratch to test that the Makefile is
actually working as intended.

PR-URL: #24374
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
sam-github added a commit that referenced this pull request Nov 17, 2018
@sam-github
test: cover tls multi-identity option mixtures
a745b1b 
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).

PR-URL: #24374
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
sam-github added a commit that referenced this pull request Nov 17, 2018
@sam-github
test: confirm tls server suite default is its own
13a6798 
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.

PR-URL: #24374
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
targos pushed a commit that referenced this pull request Nov 18, 2018
@sam-github @targos
test: rename agent1-pfx.pem to agent1.pfx
f6d5221 
PFX is not PEM, its binary DER. Use the same .pfx extension as
test/fixtures/test_cert.pfx does.

PR-URL: #24374
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
targos pushed a commit that referenced this pull request Nov 18, 2018
@sam-github @targos
test: add independent multi-alg crypto identities
fc4e3fb 
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

All pem and pfx regenerated from scratch to test that the Makefile is
actually working as intended.

PR-URL: #24374
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
targos pushed a commit that referenced this pull request Nov 18, 2018
@sam-github @targos
test: cover tls multi-identity option mixtures
790b6f7 
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).

PR-URL: #24374
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
This was referenced Dec 8, 2018
Closed
Closed
@codebytere
Copy link
Member

@sam-github do you think this is worth backporting to v10.x? I added the label, feel free to remove!

sam-github added a commit to sam-github/node that referenced this pull request Feb 4, 2019
@sam-github
test: rename agent1-pfx.pem to agent1.pfx
96fe1e7 
PFX is not PEM, its binary DER. Use the same .pfx extension as
test/fixtures/test_cert.pfx does.

Backport-PR-URL: nodejs#25501
PR-URL: nodejs#24374
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
sam-github added a commit to sam-github/node that referenced this pull request Feb 4, 2019
@sam-github
test: add independent multi-alg crypto identities
18593f0 
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

All pem and pfx regenerated from scratch to test that the Makefile is
actually working as intended.

Backport-PR-URL: nodejs#25501
PR-URL: nodejs#24374
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
sam-github added a commit to sam-github/node that referenced this pull request Feb 4, 2019
@sam-github
test: cover tls multi-identity option mixtures
f1ff355 
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).

Backport-PR-URL: nodejs#25501
PR-URL: nodejs#24374
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
sam-github added a commit to sam-github/node that referenced this pull request Feb 4, 2019
@sam-github
test: confirm tls server suite default is its own
8365239 
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.

Backport-PR-URL: nodejs#25501
PR-URL: nodejs#24374
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
BethGriggs pushed a commit that referenced this pull request Feb 5, 2019
@sam-github @BethGriggs
test: cover tls multi-identity option mixtures
83145ec 
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).

Backport-PR-URL: #25501
PR-URL: #24374
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
BethGriggs pushed a commit that referenced this pull request Feb 5, 2019
@sam-github @BethGriggs
test: add independent multi-alg crypto identities
43dcbbe 
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

All pem and pfx regenerated from scratch to test that the Makefile is
actually working as intended.

Backport-PR-URL: #25501
PR-URL: #24374
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
BethGriggs pushed a commit that referenced this pull request Feb 5, 2019
@sam-github @BethGriggs
test: rename agent1-pfx.pem to agent1.pfx
3c2b40b 
PFX is not PEM, its binary DER. Use the same .pfx extension as
test/fixtures/test_cert.pfx does.

Backport-PR-URL: #25501
PR-URL: #24374
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
BethGriggs pushed a commit that referenced this pull request Feb 5, 2019
@sam-github @BethGriggs
test: confirm tls server suite default is its own
0e63d0a 
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.

Backport-PR-URL: #25501
PR-URL: #24374
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
@BethGriggs BethGriggs mentioned this pull request Feb 12, 2019
Merged
rvagg pushed a commit that referenced this pull request Feb 28, 2019
@sam-github @rvagg
test: cover tls multi-identity option mixtures
0a36fb2 
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).

Backport-PR-URL: #25501
PR-URL: #24374
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
rvagg pushed a commit that referenced this pull request Feb 28, 2019
@sam-github @rvagg
test: add independent multi-alg crypto identities
24d7071 
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

All pem and pfx regenerated from scratch to test that the Makefile is
actually working as intended.

Backport-PR-URL: #25501
PR-URL: #24374
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
rvagg pushed a commit that referenced this pull request Feb 28, 2019
@sam-github @rvagg
test: rename agent1-pfx.pem to agent1.pfx
3e31344 
PFX is not PEM, its binary DER. Use the same .pfx extension as
test/fixtures/test_cert.pfx does.

Backport-PR-URL: #25501
PR-URL: #24374
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
rvagg pushed a commit that referenced this pull request Feb 28, 2019
@sam-github @rvagg
test: confirm tls server suite default is its own
9293b49 
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.

Backport-PR-URL: #25501
PR-URL: #24374
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fhinkel fhinkel fhinkel approved these changes

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@danbev danbev danbev approved these changes

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

Successfully merging this pull request may close these issues.
7 participants
@sam-github @nodejs-github-bot @Trott @codebytere @fhinkel @bnoordhuis @danbev