Add X.509 certificate parsing #1014

bookmoons · 2019-05-03T20:59:34Z

Adds certificate parsing under a new k6/crypto/x509 module. Methods are:
parse getAltNames getIssuer getSubject

Usage in JavaScript is like this:

import x509 from "k6/crypto/x509";
const pem = readCertificate();

const certificate = x509.parse(pem);
// certificate contains { subject, issuer, notBefore, notAfter, .. }

const altNames = x509.getAltNames(pem);
// altNames contains [ "ecxouncil.zz", "inquiries@excouncil.zz", .. ]

const issuer = x509.getIssuer(pem);
// issuer contains { country, organizationName, commonName, .. }

const subject = x509.getSubject(pem);
// subject contains ( country, organizationName, commonName, .. }

Semantics:

Missing values are encoded as empty strings.
Timestamps in notBefore notAfter are provided as ISO8601 strings. I think it's the easiest thing to deal with in JavaScript.

A few little changes:

Renamed countryName to country, because it's meant to hold the ISO country code.
It seems organizationalUnitName can have multiple values, so I've done it as an array of string.
publicKey.e is provided as a number literal.
Binary data is passed as byte arrays, for consistency with the open() interface on binary files. This applies to fingerPrint publicKey.n. fingerPrint is an SHA-1 hash which has a defined byte order. publicKey.n is big endian.

Toward #900.

Holds ISO country code.

Seemingly always displayed prefixed with the main test name. Redundancy is unnecessary. Follows examples in the testing package docs.

bookmoons · 2019-05-10T04:42:10Z

I'm also not sure if the original value returned from x509.ParseCertificate() shouldn't be saved in a private field somewhere in the Certificate struct, so we don't have to reconstitute it if we ever need to use the actual certificate in some of the other crypto operations mentioned in #900

Looked over the remaining operations. I don't think any of them use the certificate, so I think this is OK for now. Maybe it can be added if it ever becomes necessary.

na--

I'm also not sure if the original value returned from x509.ParseCertificate() shouldn't be saved in a private field somewhere in the Certificate struct, so we don't have to reconstitute it if we ever need to use the actual certificate in some of the other crypto operations mentioned in #900

Looked over the remaining operations. I don't think any of them use the certificate, so I think this is OK for now. Maybe it can be added if it ever becomes necessary.

Yeah, agree, totally fine to leave it as it is if we don't use the certificate in the rest of the new APIs - we can easily add it if it's needed in the future.

js/modules/k6/crypto/x509/x509.go

codecov-io · 2019-05-10T19:53:44Z

Codecov Report

Merging #1014 into master will increase coverage by 0.17%.
The diff coverage is 91.2%.

@@            Coverage Diff             @@
##           master    #1014      +/-   ##
==========================================
+ Coverage   72.33%   72.51%   +0.17%     
==========================================
  Files         132      133       +1     
  Lines        9717     9808      +91     
==========================================
+ Hits         7029     7112      +83     
- Misses       2273     2278       +5     
- Partials      415      418       +3

Impacted Files	Coverage Δ
js/modules/k6/crypto/x509/x509.go	`91.2% <91.2%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1621aad...14806cb. Read the comment docs.

codecov-io · 2019-05-10T19:53:44Z

Codecov Report

Merging #1014 into master will increase coverage by 0.33%.
The diff coverage is 98.4%.

@@            Coverage Diff             @@
##           master    #1014      +/-   ##
==========================================
+ Coverage   72.33%   72.66%   +0.33%     
==========================================
  Files         132      133       +1     
  Lines        9717     9842     +125     
==========================================
+ Hits         7029     7152     +123     
- Misses       2273     2274       +1     
- Partials      415      416       +1

Impacted Files	Coverage Δ
js/modules/k6/crypto/x509/x509.go	`98.4% <98.4%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1621aad...44cfa25. Read the comment docs.

bookmoons · 2019-05-10T20:26:11Z

Pushed these changes.

This test-prev-golang build is hitting the network error. Tests from this PR pass.

The lint check is failing, but I can't seem to reproduce it. Running gofmt -s makes no changes to the file. Running the linter locally finds no issues. Not sure what to do about that one.

bookmoons · 2019-05-10T20:28:59Z

Here's the complete structure we ended up with.

Certificate:

subject
- country - string
- postalCode - string
- stateOrProvinceName - string
- localityName - string
- streetAddress - string
- organizationName - string
- organizationalUnitName - Array
- commonName - string
- names - Array
issuer:
- country - string
- stateOrProvinceName - string
- localityName - string
- organizationName - string
- commonName - string
- names - Array
notBefore - string, ISO8601 timestamp
notAfter - string, ISO8601 timestamp
altNames - Array
signatureAlgorithm - string, SignatureAlgorithm enum
fingerPrint - Array, byte array
publicKeyAlgorithm - string, PublicKeyAlgorithm enum
publicKey - PublicKey, combined structure

RDN: (= Relative Distinguished Name)

type - string
value - string

SignatureAlgorithm:

MD2-RSA
MD5-RSA
SHA1-RSA
SHA256-RSA
SHA384-RSA
SHA512-RSA
DSA-SHA1
DSA-SHA256
ECDSA-SHA1
ECDSA-SHA256
ECDSA-SHA384
ECDSA-SHA512
SHA256-RSAPSS
SHA384-RSAPSS
SHA512-RSAPSS
UnknownSignatureAlgorithm

PublicKeyAlgorithm:

RSA
DSA
ECDSA
UnknownPublicKeyAlgorithm

na--

Sorry for the late review again 😞 This mostly looks good to me, only the PublicKey details I've noted inline are something that I'm unsure about currently.

Regarding the linting issue, I think you might have an old Go version, 1.11.x probably. Try to update to 1.12 and run gofmt -w -s on the file then. Doing it locally produces the following diff:

diff --git a/js/modules/k6/crypto/x509/x509.go b/js/modules/k6/crypto/x509/x509.go
index 0f89f26d..368b74c5 100644
--- a/js/modules/k6/crypto/x509/x509.go
+++ b/js/modules/k6/crypto/x509/x509.go
@@ -176,7 +176,7 @@ func makeSubject(subject pkix.Name) Subject {
                StreetAddress:          first(subject.StreetAddress),
                OrganizationName:       first(subject.Organization),
                OrganizationalUnitName: subject.OrganizationalUnit,
-               Names: makeRdns(subject.Names),
+               Names:                  makeRdns(subject.Names),
        }
 }

js/modules/k6/crypto/x509/x509.go

js/modules/k6/crypto/x509/x509_test.go

bookmoons · 2019-05-15T22:44:21Z

Regarding the linting issue, I think you might have an old Go version

Of course, should have thought of it. The latest made the change.

na--

LGTM

mstoykov

I would prefer if the certificate contents are transferred as []byte from the start. This way in the future if we support non pem certificates we won't need to change it.
Also I would really like it if we can get 100% coverage on this. There are a couple of error paths that are not covered.

js/modules/k6/crypto/x509/x509.go

bookmoons · 2019-05-17T00:45:23Z

Also I would really like it if we can get 100% coverage on this. There are a couple of error paths that are not covered.

I was able to hit all but 1 of these. The last one is not easily within reach. It requires a valid certificate that parses correctly yet contains an unsupported key. But we're supporting all available key types.

Is it possible that one could be left out? If it's important I can reorganize to make parseCertificate() interceptable.

na--

No, LGTM, I think the current state is more than fine 😄

mstoykov

LGTM too

bookmoons · 2019-05-17T06:58:51Z

Thanks for reviewing guys. Will get signing rebased onto this and submit soon.

bookmoons added 30 commits May 1, 2019 22:05

x509: Parse signature algorithm

924987e

x509: Stringify unknown signature algorithm

8658192

x509: Parse certificate subject

b8b29b4

x509: Parse postal code

f5aeaaf

x509: Parse province

26ae498

x509: Parse locality

817aa16

x509: Parse street address

486989f

x509: Parse organization

9729576

x509: Parse organizational units

1a55042

x509: Parse subject common name

62c678e

x509: Qualify subject error messages

6d507e7

x509: Parse issuer country

12ef550

x509: Parse issuer province

66ebce6

x509: Parse issuer locality

5a5c09f

x509: Parse issuer organization

ccbad81

Add missing semicolon

8c6badf

x509: Parse issuer common name

0f2a328

x509: Parse lower bound

794f219

x509: Parse upper bound

686d5fe

x509: Parse alternate names

f989961

x509: Parse fingerprint

7c0f298

x509: Rename countryName country

f2f5452

Holds ISO country code.

x509: Remove subtest name redundancy

e9a09cd

Seemingly always displayed prefixed with the main test name. Redundancy is unnecessary. Follows examples in the testing package docs.

x509: Parse public key algorithm

d3b5f41

x509: Skip on short test

f69cd25

x509: Parse public key exponent

26775d6

x509: Parse public key modulus

288e1e4

x509: Factor test material

59486b3

x509: Add getIssuer

a05c1d1

x509: Add getSubject

ab3c4de

bookmoons added 2 commits May 10, 2019 00:21

x509: Add DSA public key parsing

4e0c899

x509: Add ECDSA public key parsing

84e10db

bookmoons added 2 commits May 10, 2019 00:44

Format code

1d55323

x509: Minimize type assertions

14806cb

na-- reviewed May 10, 2019

View reviewed changes

js/modules/k6/crypto/x509/x509.go Show resolved Hide resolved

js/modules/k6/crypto/x509/x509.go Outdated Show resolved Hide resolved

na-- reviewed May 15, 2019

View reviewed changes

js/modules/k6/crypto/x509/x509.go Outdated Show resolved Hide resolved

bookmoons added 3 commits May 15, 2019 17:53

Correct formatting

dac9da9

x509: Move algorithm into public key

cb3fabe

x509: Abstract public key

3a13e19

golangcibot reviewed May 15, 2019

View reviewed changes

js/modules/k6/crypto/x509/x509_test.go Outdated Show resolved Hide resolved

x509: Allow test material global

67b97b8

na-- approved these changes May 16, 2019

View reviewed changes

mstoykov reviewed May 16, 2019

View reviewed changes

js/modules/k6/crypto/x509/x509.go Outdated Show resolved Hide resolved

bookmoons added 5 commits May 16, 2019 18:14

x509: Accept encoded certificate as []byte

40d513f

x509: Test signature algorithm

f2bbb77

x509: Test make public key

793ee98

x509: Test make certificate

99e08b7

x509: Correct make certificate return

44cfa25

na-- approved these changes May 17, 2019

View reviewed changes

mstoykov approved these changes May 17, 2019

View reviewed changes

na-- merged commit 2999a1e into grafana:master May 17, 2019

olegbespalov mentioned this pull request Aug 29, 2024

Document k6/crypto/x509 module grafana/k6-docs#1709

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add X.509 certificate parsing #1014

Add X.509 certificate parsing #1014

bookmoons commented May 3, 2019 •

edited

Loading

bookmoons commented May 10, 2019

na-- left a comment

codecov-io commented May 10, 2019

codecov-io commented May 10, 2019 •

edited by codecov bot

Loading

bookmoons commented May 10, 2019

bookmoons commented May 10, 2019

na-- left a comment

bookmoons commented May 15, 2019

na-- left a comment

mstoykov left a comment

bookmoons commented May 17, 2019 •

edited

Loading

na-- left a comment

mstoykov left a comment

bookmoons commented May 17, 2019

Add X.509 certificate parsing #1014

Add X.509 certificate parsing #1014

Conversation

bookmoons commented May 3, 2019 • edited Loading

bookmoons commented May 10, 2019

na-- left a comment

Choose a reason for hiding this comment

codecov-io commented May 10, 2019

Codecov Report

codecov-io commented May 10, 2019 • edited by codecov bot Loading

Codecov Report

bookmoons commented May 10, 2019

bookmoons commented May 10, 2019

na-- left a comment

Choose a reason for hiding this comment

bookmoons commented May 15, 2019

na-- left a comment

Choose a reason for hiding this comment

mstoykov left a comment

Choose a reason for hiding this comment

bookmoons commented May 17, 2019 • edited Loading

na-- left a comment

Choose a reason for hiding this comment

mstoykov left a comment

Choose a reason for hiding this comment

bookmoons commented May 17, 2019

bookmoons commented May 3, 2019 •

edited

Loading

codecov-io commented May 10, 2019 •

edited by codecov bot

Loading

bookmoons commented May 17, 2019 •

edited

Loading