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

test: add script to create 0-dns-cert.pem #11579

Closed

shigeki wants to merge 3 commits into nodejs:master from shigeki:create_0-dns_cert

Contributor

shigeki commented Feb 27, 2017 •

edited by gibfahn

Loading

0-dns-cert.pem and 0-dns-key.pem were stored in test/fixtures/key
directory, but the cert file cannot be created with the openssl
command via Makefile. make clean removes them but we could not re-create them.

This added a script to create it with using asn1.js and
asn1.js-rfc5280 and moved them out of key directory and put into
test/fixtures/0-dns.

The domains listed in the cert were also changed into example.com and
example.org to show the use for only testing.

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)

test, tls


          test: add script to create 0-dns-cert.pem

0-dns-cert.pem  and 0-dns-key.pem were stored in `test/fixtures/key`
directory, but the cert file cannot be created with the openssl
command via Makefile.

Added a script to create it with using  `asn1.js` and
`asn1.js-rfc5280` and moved them out of key directory and put into
`test/fixtures/0-dns`.

The domains listed in the cert were also changed into example.com and
example.org to show the use for only testing.

nodejs-github-bot added the test label

shigeki added the tls label

shigeki assigned indutny

Contributor Author

shigeki commented Feb 27, 2017

CI is running on https://ci.nodejs.org/job/node-test-pull-request/6602/ .

Contributor Author

shigeki commented Feb 27, 2017

To be the fix of #10228.

jasnell approved these changes

View reviewed changes

Contributor Author

shigeki commented Feb 28, 2017

The ci jobs was somehow removed.
A new job is https://ci.nodejs.org/job/node-test-pull-request/6623/ and all is green.

sam-github suggested changes

View reviewed changes

test/fixtures/0-dns/README.md Outdated

+              $ node ./createCert.js
+              $ openssl x509 -text -in 0-dns-cert.pem
+              (You can not see evel.example.com in subjectAltName field)

Contributor

sam-github Feb 28, 2017

"evil"?

Contributor Author

shigeki Mar 1, 2017

Fixed

test/fixtures/0-dns/createCert.js Outdated

+              const private_key = fs.readFileSync('./0-dns-key.pem');
+              // public key file can be generated from the private key with
+              // openssl rsa -in 0-dns-key.pem -RSAPublicKey_out -outform der \

Contributor

sam-github Feb 28, 2017

you can wrap to 80 columns, and the backslash isn't necessary at end of line

Contributor Author

shigeki Mar 1, 2017

Fixed

test/fixtures/0-dns/createCert.js Outdated

+              const crypto = require('crypto');
+              const rfc5280 = require('asn1.js-rfc5280');
+              const asn1 = require('asn1.js');
+              const BN = asn1.bignum;

Contributor

sam-github Feb 28, 2017

sort requires

Contributor Author

shigeki Mar 1, 2017

Fixed

test/fixtures/0-dns/createCert.js Outdated

		@@ -0,0 +1,75 @@
		'use strict';

Contributor

sam-github Feb 28, 2017

js files would be named create-cert.js

Contributor Author

shigeki Mar 1, 2017

Done

test/fixtures/0-dns/createCert.js Outdated

+              const asn1 = require('asn1.js');
+              const BN = asn1.bignum;
+              const id_at_commonName = [ 2, 5, 4, 3 ];

Contributor

sam-github Feb 28, 2017

inconsistent casing, sometimes snake_case, sometimes camelCase, it looks like test/fixtures doesn't get linted

Contributor Author

shigeki Mar 1, 2017

id_at_commonName is named after the ASN.1 notation in RFC5280 which would come from OID name.

test/fixtures/0-dns/createCert.js Outdated

+              const subject = PrintStr.encode('evil.example.com', 'der');
+              const tbs =
+                { version: 'v3',

Contributor

sam-github Feb 28, 2017

this isn't generally how node indents object literals

Contributor Author

shigeki Mar 1, 2017

Fixed

test/fixtures/0-dns/createCert.js Outdated

+              const cert = {
+                tbsCertificate: tbs,
+                signatureAlgorithm: { algorithm: sha256WithRSAEncryption, parameters: null_},

Contributor

sam-github Feb 28, 2017

lacking before closing } here and a couple lines onwards

Contributor Author

shigeki Mar 1, 2017

Fixed

test/parallel/test-tls-0-dns-altname.js Outdated

@@ @@ -2,6 +2,8 @@ @@
               const common = require('../common');
               const assert = require('assert');
+              // check getPeerCertificate can properly handle '\0' for fix CVE-2009-2408

Contributor

sam-github Feb 28, 2017

Good to have a test description, it should be a capitalized and period terminated sentence.

Contributor Author

shigeki Mar 1, 2017

Fixed.

Contributor

sam-github commented Feb 28, 2017

Nice use of asn1 to craft the cert. I think the js code should follow the same coding conventions we use elswhere, even though test/fixtures seems to be exempted from linting at the moment.


          fixup

fd05b71

shigeki force-pushed the create_0-dns_cert branch from 0920c23 to fd05b71 Compare

March 1, 2017 05:22

Contributor Author

shigeki commented Mar 1, 2017

I made eslint for test/fixtures/0-dns/create-cert.js and only one error of missing common module appeared.

$ ./node tools/eslint/bin/eslint.js --rulesdir=tools/eslint-rules test/fixtures/0-dns/create-cert.js --no-ignore

/home/sotsu/github/node/test/fixtures/0-dns/create-cert.js
  1:1  error  Mandatory module "common" must be loaded  required-modules

✖ 1 problem (1 error, 0 warnings)

I think it is not necessary for the script in fixtures.

gibfahn mentioned this pull request

parallel/test-debug-port-numbers - intermittent failure #11619

Closed

sam-github suggested changes

View reviewed changes

Contributor

sam-github left a comment

Its still a mixture of snake_case and camelCase for vars, but basically LGTM

test/fixtures/0-dns/0-dns-cert.pem Outdated

+              Z4CCF58oC4b7MrfFo1LXW8EdSjfK5ejFse6xZe6WNAahi7vDS7RJEyoq3EeZ4+A0
+              DvrSjCuretoVAC/U7gUxs467yM3ZCujqZ4OANVQF6knRziTJEF5c1aXOzM563FT3
+              ufeH36lO0ImzE+a2g6et3BZDL2PQLmBF3F6clQ==
+              -----END CERTIFICATE-----

Contributor

sam-github Mar 1, 2017

missing newline

Contributor Author

shigeki Mar 2, 2017

Done


          add newline to pem

68cd928

Contributor Author

shigeki commented Mar 2, 2017 •

edited

Loading

camelCase vars are used to follow ASN.1 notation in ~~RFC5289~~ RFC5280.

Contributor Author

shigeki commented Mar 2, 2017

@indutny Please take a look of this if you have time for you are the author of asn.1 and asn1.js-rfc5280.

sam-github approved these changes

View reviewed changes

Contributor Author

shigeki commented Mar 7, 2017

I will land this tomorrow if there are no any comments.

shigeki unassigned indutny

Contributor Author

shigeki commented Mar 10, 2017

Landed in dacaaa5. Thanks.

shigeki closed this

shigeki added a commit that referenced this pull request


          test: add script to create 0-dns-cert.pem

dacaaa5

0-dns-cert.pem  and 0-dns-key.pem were stored in `test/fixtures/key`
directory, but the cert file cannot be created with the openssl
command via Makefile.

Added a script to create it with using  `asn1.js` and
`asn1.js-rfc5280` and moved them out of key directory and put into
`test/fixtures/0-dns`.

The domains listed in the cert were also changed into example.com and
example.org to show the use for only testing.

Fixes: #10228
PR-URL: #11579
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

italoacasas pushed a commit to italoacasas/node that referenced this pull request


          test: add script to create 0-dns-cert.pem

c01c7a4

0-dns-cert.pem  and 0-dns-key.pem were stored in `test/fixtures/key`
directory, but the cert file cannot be created with the openssl
command via Makefile.

Added a script to create it with using  `asn1.js` and
`asn1.js-rfc5280` and moved them out of key directory and put into
`test/fixtures/0-dns`.

The domains listed in the cert were also changed into example.com and
example.org to show the use for only testing.

Fixes: nodejs#10228
PR-URL: nodejs#11579
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

italoacasas mentioned this pull request

7.7.3 Release Proposal #11831

Merged

jungx098 pushed a commit to jungx098/node that referenced this pull request


          test: add script to create 0-dns-cert.pem

f76a167

0-dns-cert.pem  and 0-dns-key.pem were stored in `test/fixtures/key`
directory, but the cert file cannot be created with the openssl
command via Makefile.

Added a script to create it with using  `asn1.js` and
`asn1.js-rfc5280` and moved them out of key directory and put into
`test/fixtures/0-dns`.

The domains listed in the cert were also changed into example.com and
example.org to show the use for only testing.

Fixes: nodejs#10228
PR-URL: nodejs#11579
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

MylesBorins added the land-on-v6.x label

MylesBorins pushed a commit that referenced this pull request


          test: add script to create 0-dns-cert.pem

60da9ae

0-dns-cert.pem  and 0-dns-key.pem were stored in `test/fixtures/key`
directory, but the cert file cannot be created with the openssl
command via Makefile.

Added a script to create it with using  `asn1.js` and
`asn1.js-rfc5280` and moved them out of key directory and put into
`test/fixtures/0-dns`.

The domains listed in the cert were also changed into example.com and
example.org to show the use for only testing.

Fixes: #10228
PR-URL: #11579
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

MylesBorins pushed a commit that referenced this pull request


          test: add script to create 0-dns-cert.pem

aa98150

0-dns-cert.pem  and 0-dns-key.pem were stored in `test/fixtures/key`
directory, but the cert file cannot be created with the openssl
command via Makefile.

Added a script to create it with using  `asn1.js` and
`asn1.js-rfc5280` and moved them out of key directory and put into
`test/fixtures/0-dns`.

The domains listed in the cert were also changed into example.com and
example.org to show the use for only testing.

Fixes: #10228
PR-URL: #11579
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

MylesBorins mentioned this pull request

v6.10.3 proposal #12498

Merged

andrew749 pushed a commit to michielbaird/node that referenced this pull request


          test: add script to create 0-dns-cert.pem

039c984

0-dns-cert.pem  and 0-dns-key.pem were stored in `test/fixtures/key`
directory, but the cert file cannot be created with the openssl
command via Makefile.

Added a script to create it with using  `asn1.js` and
`asn1.js-rfc5280` and moved them out of key directory and put into
`test/fixtures/0-dns`.

The domains listed in the cert were also changed into example.com and
example.org to show the use for only testing.

Fixes: nodejs/node#10228
PR-URL: nodejs/node#11579
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

test tls