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

test: move all test keys/certs under test/fixtures/keys/ #27962

Closed

Conversation

reasonablytall
Copy link
Contributor

A bunch of pre-generated test keypairs/certs were kept under test/fixtures/ with no information on how they were generated. This PR removes those keypairs/certs and creates new definitions to replace them in the makefile under test/fixtures/keys/. Each was subsequently generated, and references in tests to the old files were updated.

All of the keypairs/certs I touched are now entirely re-generate-able, so long as certain parameters in the makefile aren't changed. Even if they are changed, it should now be simple to track down what tests fail and to rectify it.

With these changes, future work like #3759 and what's needed for #27862 should be a lot more manageable.

Many old references used fixture.readSync(...) which I've changed in most cases to fixture.readKey(...) which is set up to pull directly from the test/fixtures/keys/ directory.

I made changes in df43695 to the following benchmarks that use test fixtures. I'm not familiar with the benchmark system, so I would appreciate a check that what I've done is OK.

benchmark/tls/throughput.js
benchmark/tls/tls-connect.js
benchmark/tls/secure-pair.js

I'm really sorry for the size of this PR! Most of it is the newly generated certificates, and the changed references to those certificates. I would be happy to break up this work in whatever way is desired! Most of these commits are pretty atomic, so separating them shouldn't be too much trouble.

The makefile could also probably use some simplifying/cleaning up. If there's any advice or requests in that avenue, I would be happy to implement them.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

doc/api/https.md Outdated Show resolved Hide resolved
@Trott
Copy link
Member

Trott commented May 30, 2019

/ping @nodejs/testing

@Trott
Copy link
Member

Trott commented May 30, 2019

I'm not sure that a Makefile should go in fixtures unless it is part of the test fixture itself. But that's not a blocking comment. Just a question. Should it be worked into the main Makefile instead? (Also, does the fact that it won't work on Windows matter? I guess not since the certs are checked in?)

@reasonablytall
Copy link
Contributor Author

Should it be worked into the main Makefile instead?

I'd be happy to make that change if desired, but it should be noted that I didn't touch fixtures that were already in the key Makefile, and some of those are probably not safely regenerate-able on build if that's what you're looking for. I also think it's useful for there to be a source of truth on the generation parameters of these fixtures near by them.

test/fixtures/keys/Makefile Outdated Show resolved Hide resolved
Copy link
Contributor

@sam-github sam-github left a comment

Choose a reason for hiding this comment

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

Nice improvement, thanks. I made a couple small suggestions for change to consider.

const modSize = 1024;
const certPem = fixtures.readKey('rsa_cert.crt');
const keyPem = fixtures.readKey('rsa_private.pem');
const keySize = 2048;
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for pointing this out. @nodejs/crypto should we do this? Its not strictly a "move". I don't know if we really need to be testing keys so small. 1024 could be historical, or perhaps its important that we don't accidentally break small-key support for existing users.

test/fixtures/keys/Makefile Show resolved Hide resolved
test/fixtures/keys/rsa_cert_foafssl_b.cnf Show resolved Hide resolved
test/parallel/test-crypto-binary-default.js Outdated Show resolved Hide resolved
test/fixtures/keys/Makefile Outdated Show resolved Hide resolved
Copy link
Contributor

@ryzokuken ryzokuken left a comment

Choose a reason for hiding this comment

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

LGTM w/ @sam-github's comments addressed. Thanks for taking the time to do this.

@sam-github
Copy link
Contributor

@Trott The Makefile shouldn't be part of the main Makefile. It's like deps/openssl/config/Makefile --- its not used to build Node.js, its a script (that happens to be in Makefile syntax) that is used occaisonally to maintain sets of files that are checked in to git. Cert regeneration is rarely needed, but we have had to do it on occaison if a cert's end-of-validity was being passed. It serves less as a Makefile, and more as documentation for how the certs were created.

Also, regenerating the certs invalidates lots of tests, which have dependencies on serial numbers, SHA values, timestamps, etc. that are no longer correct afterwards. I've been tempted to add some scripting with OpenSSL at the end of the Makefile to generate some kind of JSON "manifest" for the certs, so that tests could depend on the meta info in the manifest instead of hardcoded values, but the certs are so seldomly regenerated that it hasn't been worth the effort (yet).

@reasonablytall
Copy link
Contributor Author

@sam-github I'm working on your requests -- thanks for the review!

FOAF+SSL (as described here) is a certificate based 'social' authentication protocol. Apparently node supports it, and it is very weakly tested in test/parallel/test-https-foafssl.js. By weak I mean that the test disables the authentication step, and just verifies that the client's certificate can be retrieved from the socket object. That should probably be another issue.

I'll add comments to describe linking to that page :)

@reasonablytall reasonablytall force-pushed the test-fixtures-move-keys branch from 0c9f225 to 61f2f16 Compare May 31, 2019 23:41
@reasonablytall
Copy link
Contributor Author

reasonablytall commented May 31, 2019

Force push was to ensure make lint passes on all commits. Slight diff is because I used a different format this time around.

@reasonablytall reasonablytall force-pushed the test-fixtures-move-keys branch from 61f2f16 to 82b1f81 Compare June 1, 2019 00:04
@reasonablytall
Copy link
Contributor Author

Force push to add detail to the last commit message.

@nodejs-github-bot
Copy link
Collaborator

@sam-github
Copy link
Contributor

sam-github commented Jun 1, 2019

You don't have to justify force pushing. Its your branch, rebase and force-push it as much as you want.

2a3809a would be better to have been comitted as git commit --fixup 4de2305 doc/api/https.md , so that git rebase --autosquash upstream/master --exec "make test" would merge it. I think you did something similar with the commit that fixed a lint error that had been introduced by an earlier commit in this PR?

Having a PR, particularly a large (by lines) one like this, that has lots of well-defined individual commits is nice, but for a PR to have one commit that introduces a change, and then another that reverts it, isn't so good.

Other than the "revert" commit, this looks good to me. I've kicked off a CI run. If it's green, this should be landable.

Thanks for all your work on this.

@Trott
Copy link
Member

Trott commented Jun 1, 2019

@Trott The Makefile shouldn't be part of the main Makefile.

@sam-github I guess the question should have been: Should the Makefile exist somewhere other than fixtures. Like maybe tools/fixtures or something like that? The only reason I'm suggesting it (and I'm not convinced it's a good suggestion, so it's offered tentatively) is because anything that's in fixtures will look like a fixture for tests. And this Makefile is not a fixture. It's a tool to generate fixtures. (I can imagine future-me grepping through tests to try to find fixtures that can be deleted because they're no longer in use, and wanting to delete this Makefile for that reason.)

@sam-github
Copy link
Contributor

I don't care that much where its moved around to. It could get moved to ./tools/, but having the Makefile that generates something be close ot the thing it generates makes it easy to find. If you were looking at the certs/ directory, and wondering how they were made, then saw a Makefile... you'd look in it. If you didn't, maybe you'd find it if it was tucked away somewhere else in the tree.

And like I said, its the same pattern as deps/openssl/config/Makefile. I've no idea where the scripts that import icu and some of the other vendored deps are. Maybe there is a pattern these should follow, too.

So, if someone wants to move Makefile somewhere else and make the case it improves consistency, shrug, OK, but not into the top-level Makefile! :-)

@sam-github
Copy link
Contributor

And also, most importantly, moving the Makefile has nothig to do with this PR. Its at best a follow up.

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

Test failures on Windows....

@Trott
Copy link
Member

Trott commented Jun 1, 2019

Looks like the tests are failing on Windows due to \n vs. \r\n line endings?

Example: https://ci.nodejs.org/job/node-test-binary-windows-2/1144/COMPILED_BY=vs2017,RUNNER=win2016,RUN_SUBSET=0/testReport/junit/(root)/test/parallel_test_crypto_certificate/

Result from test/parallel/test-crypto-certificate.js:

AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
+ actual - expected

+ '-----BEGIN PUBLIC KEY-----MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAt9xYiIonscC3vz/A2ceR7KhZZlDu/5bye53nCVTcKnWd2seY6UAdKersX6njr83Dd5OVe1BW/wJvp5EjWTAGYbFswlNmeD44edEGM939B6Lq+/8iBkrTi8mGN4YCytivE24YI0D4XZMPfkLSpab2y/Hy4DjQKBq1ThZ0UBnK+9IhX37Ju/ZoGYSlTIGIhzyaiYBh7wrZBoPczIEu6et/kN2VnnbRUtkYTF97ggcv5h+hDpUQjQW0ZgOMcTc8n+RkGpIt0/iM/bTjI3Tz/gsFdi6hHcpZgbopPL630296iByyigQCPJVzdusFrQN5DeC+zT/nGypQkZanLb4ZspSx9QIDAQAB-----END PUBLIC KEY-----'
- '-----BEGIN PUBLIC KEY-----\rMIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAt9xYiIonscC3vz/A2ceR\r7KhZZlDu/5bye53nCVTcKnWd2seY6UAdKersX6njr83Dd5OVe1BW/wJvp5EjWTAG\rYbFswlNmeD44edEGM939B6Lq+/8iBkrTi8mGN4YCytivE24YI0D4XZMPfkLSpab2\ry/Hy4DjQKBq1ThZ0UBnK+9IhX37Ju/ZoGYSlTIGIhzyaiYBh7wrZBoPczIEu6et/\rkN2VnnbRUtkYTF97ggcv5h+hDpUQjQW0ZgOMcTc8n+RkGpIt0/iM/bTjI3Tz/gsF\rdi6hHcpZgbopPL630296iByyigQCPJVzdusFrQN5DeC+zT/nGypQkZanLb4ZspSx\r9QIDAQAB\r-----END PUBLIC KEY-----\r'
    at checkMethods (c:\workspace\node-test-binary-windows-2\test\parallel\test-crypto-certificate.js:43:10)
    at Object.<anonymous> (c:\workspace\node-test-binary-windows-2\test\parallel\test-crypto-certificate.js:58:3)
    at Module._compile (internal/modules/cjs/loader.js:781:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:792:10)
    at Module.load (internal/modules/cjs/loader.js:641:32)
    at Function.Module._load (internal/modules/cjs/loader.js:556:12)
    at Function.Module.runMain (internal/modules/cjs/loader.js:844:10)
    at internal/main/run_main_module.js:17:11 {
  generatedMessage: true,
  code: 'ERR_ASSERTION',
  actual: '-----BEGIN PUBLIC ' +
    'KEY-----MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAt9xYiIonscC3vz/A2ceR7KhZZlDu/5bye53nCVTcKnWd2seY6UAdKersX6njr83Dd5OVe1BW/wJvp5EjWTAGYbFswlNmeD44edEGM939B6Lq+/8iBkrTi8mGN4YCytivE24YI0D4XZMPfkLSpab2y/Hy4DjQKBq1ThZ0UBnK+9IhX37Ju/ZoGYSlTIGIhzyaiYBh7wrZBoPczIEu6et/kN2VnnbRUtkYTF97ggcv5h+hDpUQjQW0ZgOMcTc8n+RkGpIt0/iM/bTjI3Tz/gsFdi6hHcpZgbopPL630296iByyigQCPJVzdusFrQN5DeC+zT/nGypQkZanLb4ZspSx9QIDAQAB-----END ' +
    'PUBLIC KEY-----',
  expected: '-----BEGIN PUBLIC KEY-----\r' +
    'MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAt9xYiIonscC3vz/A2ceR\r' +
    '7KhZZlDu/5bye53nCVTcKnWd2seY6UAdKersX6njr83Dd5OVe1BW/wJvp5EjWTAG\r' +
    'YbFswlNmeD44edEGM939B6Lq+/8iBkrTi8mGN4YCytivE24YI0D4XZMPfkLSpab2\r' +
    'y/Hy4DjQKBq1ThZ0UBnK+9IhX37Ju/ZoGYSlTIGIhzyaiYBh7wrZBoPczIEu6et/\r' +
    'kN2VnnbRUtkYTF97ggcv5h+hDpUQjQW0ZgOMcTc8n+RkGpIt0/iM/bTjI3Tz/gsF\r' +
    'di6hHcpZgbopPL630296iByyigQCPJVzdusFrQN5DeC+zT/nGypQkZanLb4ZspSx\r' +
    '9QIDAQAB\r' +
    '-----END PUBLIC KEY-----\r',
  operator: 'strictEqual'
}

@addaleax addaleax added test Issues and PRs related to the tests. tls Issues and PRs related to the tls subsystem. labels Jun 2, 2019
@reasonablytall reasonablytall force-pushed the test-fixtures-move-keys branch from 2a3809a to 6bffe65 Compare June 3, 2019 17:05
@reasonablytall
Copy link
Contributor Author

If I'm reading Jenkins right, the offending tests are:

test.parallel/test-crypto-binary-default
test.parallel/test-crypto-certificate
test.parallel/test-crypto-key-objects
test.parallel/test-crypto-rsa-dsa
test.parallel/test-http2-client-upload
test.parallel/test-https-foafssl

I've made changes to these in cd1a9b3 to avoid the \r\n issue, so the tests should now pass. I was able to check all but test-https-foafssl with a windows server VM.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot
Copy link
Collaborator

@sam-github
Copy link
Contributor

wrt. #27962 (comment), it's great that this all passes, but I'm confused as to why, the failures seem wholly unrelated to regenerating the certs.

Were these tests failing before the certs were moved into test/fixtures, but being ignored, and you fixed them? Or did something about the rearranging of the certs cause the tests to start failing?

It matters, because if a PR is a series of commits, the tests should pass between every single commit, so if one of these commits caused them to fail, and then the tests are only fixed by the last commit, git bisect is broken. On the other hand, if they were always failing and marked flaky in CI, then its just an improvement, and can be a single commit at the end of the PR.

We could squash the entire PR into one commit when it lands, but you've broken it into such nice individual commits, I'm reluctant to do that.

Btw, if you have a windows box, you can use git bisect to (relatively!) quickly find what commit introduced a test failure.

@nodejs/releasers @nodejs/lts do you have an opinon on this?

BridgeAR pushed a commit that referenced this pull request Jun 17, 2019
PR-URL: #27962
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
BridgeAR pushed a commit that referenced this pull request Jun 17, 2019
Lots of changes, but mostly just search/replace of
fixtures.readSync(...) to fixtures.readKey([new key]...)

Benchmarks modified to use fixtures.readKey(...):
benchmark/tls/throughput.js
benchmark/tls/tls-connect.js
benchmark/tls/secure-pair.js

Also be sure to review the change to L16 of
test/parallel/test-crypto-sign-verify.js

PR-URL: #27962
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
BridgeAR pushed a commit that referenced this pull request Jun 17, 2019
PR-URL: #27962
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
BridgeAR pushed a commit that referenced this pull request Jun 17, 2019
PR-URL: #27962
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
BridgeAR pushed a commit that referenced this pull request Jun 17, 2019
PR-URL: #27962
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
BridgeAR pushed a commit that referenced this pull request Jun 17, 2019
PR-URL: #27962
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
BridgeAR pushed a commit that referenced this pull request Jun 17, 2019
PR-URL: #27962
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
BridgeAR pushed a commit that referenced this pull request Jun 17, 2019
PR-URL: #27962
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
BridgeAR pushed a commit that referenced this pull request Jun 17, 2019
PR-URL: #27962
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
BridgeAR pushed a commit that referenced this pull request Jun 17, 2019
Converts the whitespace to spaces in the all: ... target for
consistency. The other whitespace has to remain tabs due to how
Makefiles work.

PR-URL: #27962
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
BridgeAR pushed a commit that referenced this pull request Jun 17, 2019
PR-URL: #27962
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
BridgeAR pushed a commit that referenced this pull request Jun 17, 2019
PR-URL: #27962
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
BridgeAR pushed a commit that referenced this pull request Jun 17, 2019
PR-URL: #27962
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
BridgeAR pushed a commit that referenced this pull request Jun 17, 2019
Also adds make'd signatures for use in tests of signing/verification.
All of the moved keys can be regenerated at will without breaking tests
now.

PR-URL: #27962
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
BridgeAR pushed a commit that referenced this pull request Jun 17, 2019
PR-URL: #27962
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
BridgeAR pushed a commit that referenced this pull request Jun 17, 2019
PR-URL: #27962
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
BridgeAR pushed a commit that referenced this pull request Jun 17, 2019
PR-URL: #27962
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
BridgeAR pushed a commit that referenced this pull request Jun 17, 2019
PR-URL: #27962
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
BridgeAR pushed a commit that referenced this pull request Jun 17, 2019
Lots of changes, but mostly just search/replace of
fixtures.readSync(...) to fixtures.readKey([new key]...)

Benchmarks modified to use fixtures.readKey(...):
benchmark/tls/throughput.js
benchmark/tls/tls-connect.js
benchmark/tls/secure-pair.js

Also be sure to review the change to L16 of
test/parallel/test-crypto-sign-verify.js

PR-URL: #27962
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
BridgeAR pushed a commit that referenced this pull request Jun 17, 2019
PR-URL: #27962
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
BridgeAR pushed a commit that referenced this pull request Jun 17, 2019
PR-URL: #27962
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
BridgeAR pushed a commit that referenced this pull request Jun 17, 2019
PR-URL: #27962
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
BridgeAR pushed a commit that referenced this pull request Jun 17, 2019
PR-URL: #27962
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
BridgeAR pushed a commit that referenced this pull request Jun 17, 2019
PR-URL: #27962
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
BridgeAR pushed a commit that referenced this pull request Jun 17, 2019
PR-URL: #27962
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
BridgeAR pushed a commit that referenced this pull request Jun 17, 2019
PR-URL: #27962
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
BridgeAR pushed a commit that referenced this pull request Jun 17, 2019
Converts the whitespace to spaces in the all: ... target for
consistency. The other whitespace has to remain tabs due to how
Makefiles work.

PR-URL: #27962
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
BridgeAR pushed a commit that referenced this pull request Jun 17, 2019
PR-URL: #27962
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@BridgeAR BridgeAR mentioned this pull request Jun 17, 2019
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. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants