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.

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

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

Closed
wants to merge 4 commits into from

Conversation

sam-github
Copy link
Contributor

@sam-github sam-github commented Nov 15, 2018

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

@Trott
Copy link
Member

Trott commented Nov 15, 2018

@sam-github
Copy link
Contributor Author

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
PFX is not PEM, its binary DER. Use the same .pfx extension as
test/fixtures/test_cert.pfx does.
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.
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).
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

@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
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
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
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
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
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
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
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>
@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
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
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
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
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
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
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
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
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
rvagg pushed a commit that referenced this pull request Feb 28, 2019
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
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
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
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
Labels
test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants