GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — generate more certs, and more test coverage using them by sam-github · Pull Request #24374 · nodejs/node

Conversation

@sam-github

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

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor
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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@nodejs-github-bot

nodejs-github-bot commented Nov 15, 2018

Copy link
Copy Markdown
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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to Node.js core tests and test infrastructure. label Nov 15, 2018
@sam-github

sam-github commented Nov 15, 2018

Copy link
Copy Markdown
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/

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@sam-github

sam-github commented Nov 15, 2018

Copy link
Copy Markdown
Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

danbev
danbev approved these changes Nov 15, 2018
View reviewed changes
@Trott

Trott commented Nov 15, 2018

Copy link
Copy Markdown
Member

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@sam-github

sam-github commented Nov 15, 2018

Copy link
Copy Markdown
Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Trott

Trott commented Nov 15, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@sam-github
sam-github force-pushed the generate-more-certs branch from 401cb33 to 7e5c877 Compare November 15, 2018 18:21
@sam-github

sam-github commented Nov 15, 2018

Copy link
Copy Markdown
Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bnoordhuis
bnoordhuis approved these changes Nov 15, 2018
View reviewed changes

@bnoordhuis bnoordhuis left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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. :-)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread test/fixtures/keys/agent10.cnf Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread test/fixtures/keys/Makefile Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread test/parallel/test-tls-multi-key.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

fhinkel
fhinkel approved these changes Nov 16, 2018
View reviewed changes
@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
sam-github force-pushed the generate-more-certs branch from 7e5c877 to dcfdfba Compare November 16, 2018 17:59
sam-github added 3 commits November 16, 2018 11:12
@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
sam-github force-pushed the generate-more-certs branch from dcfdfba to 27af1b0 Compare November 16, 2018 19:12
@sam-github

sam-github commented Nov 16, 2018

Copy link
Copy Markdown
Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@sam-github

sam-github commented Nov 16, 2018

Copy link
Copy Markdown
Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@sam-github

sam-github commented Nov 17, 2018

Copy link
Copy Markdown
Contributor Author

Landed in a569686...13a6798

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@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 <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
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 <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
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 <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
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 <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
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 <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
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 <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
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 <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
This was referenced Dec 8, 2018
2018-12-07 Version 11.4.0 (Current) @BridgeAR beevelop/docker-android-nodejs#264
Closed
2018-12-07 Version 11.4.0 (Current) @BridgeAR beevelop/docker-android-nodejs#265
Closed
@codebytere

codebytere commented Jan 12, 2019

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@codebytere codebytere added the backport-requested-v10.x label Jan 12, 2019
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 <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
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 <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
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 <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
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 <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
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 <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
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 <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
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 <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
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 <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
@BethGriggs BethGriggs mentioned this pull request Feb 12, 2019
v10.15.3 proposal #26063
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 <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
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 <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
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 <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
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 <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bnoordhuis bnoordhuis bnoordhuis approved these changes
+2 more reviewers
@fhinkel fhinkel fhinkel approved these changes
@danbev danbev danbev approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

test Issues and PRs related to Node.js core tests and test infrastructure.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

7 participants

@sam-github @nodejs-github-bot @Trott @codebytere @fhinkel @bnoordhuis @danbev
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.