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

connect: don't colon-hex-encode the AuthorityKeyId and SubjectKeyId fields in connect certs #6492

Merged
merged 1 commit into from
Sep 23, 2019

Conversation

rboyer
Copy link
Member

@rboyer rboyer commented Sep 16, 2019

The fields in the certs are meant to hold the original binary
representation of this data, not some ascii-encoded version.

The only time we should be colon-hex-encoding fields is for display
purposes or marshaling through non-TLS mediums (like RPC).

This obviates #6080

  • update SigningKeyID examples on website

@rboyer rboyer requested a review from a team September 16, 2019 21:33
@rboyer rboyer self-assigned this Sep 16, 2019
kID := sha256.Sum256(bs)
return []byte(strings.Replace(fmt.Sprintf("% x", kID), " ", ":", -1)), nil
return kID[:], nil
Copy link
Member

Choose a reason for hiding this comment

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

I didn't know what this is doing, but https://blog.golang.org/go-slices-usage-and-internals knows:

This is also the syntax to create a slice given an array:

x := [3]string{"Лайка", "Белка", "Стрелка"}
s := x[:] // a slice referencing the storage of x

Copy link
Member

@hanshasselberg hanshasselberg left a comment

Choose a reason for hiding this comment

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

Nice work!

I am wondering how this prevents the double encoding though. EncodeSigningKeyID uses HexString and it seems to me double encoding could still happen? On the other hand, there are tests proving me wrong :D.

@rboyer
Copy link
Member Author

rboyer commented Sep 18, 2019

The SigningKeyID is a consul-only thing which is a colon-hex-encoded version of the thing stored in the x509 fields AuthorityKeyId or SubjectKeyId.

In x509 AuthorityKeyId and SubjectKeyId are both opaque bytes that "summarize" the entity they represent in a spec-suggested way (hash some stuff). Since these are part of the x509 spec they already get encoded, so there's no need to encode what goes into them.

If you use a command like openssl x509 -in root.pem -noout -text to pretty-print the contents of a PEM file in order to display those bytes back at you the openssl command will colon-hex encode them for display, which can easily confuse the matter when you are trying to understand what is actually stored in there.

We had been generating the AuthorityKeyId/SubjectKeyId values by hashing some fields and also colon-hex-encoding that before stuffing it into the x509 fields. This meant that when consul generated the SigningKeyID for comparison purposes it was colon-hex-encoding something that was erroneously already colon-hex-encoded.

That said the old way of doing things was at least self-consistent when you stick with one CA provider. If you mixed them (consul + vault, or likely what @tradel was doing) things didn't quite line up.

Now the only time things get colon-hex encoded is for informational display purposes, or for sending around as a consul SigningKeyID field.

Copy link
Member

@banks banks left a comment

Choose a reason for hiding this comment

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

Nice! Good sleuthing RB. Glad we figured that out rather than plaster over it! Does this means the other PR can be closed now?

@rboyer
Copy link
Member Author

rboyer commented Sep 18, 2019

Yep. Closed the other PR.

@rboyer rboyer force-pushed the fix-vault-ca-provider-uri-sans branch from 8ab24e7 to 0c3e22b Compare September 23, 2019 16:28
@rboyer rboyer force-pushed the stop-hex-encoding-connect-tls-fields branch from 0665f40 to 02dcbb1 Compare September 23, 2019 16:28
…ields in connect certs

The fields in the certs are meant to hold the original binary
representation of this data, not some ascii-encoded version.

The only time we should be colon-hex-encoding fields is for display
purposes or marshaling through non-TLS mediums (like RPC).
@rboyer rboyer force-pushed the stop-hex-encoding-connect-tls-fields branch from 02dcbb1 to e2a1c3f Compare September 23, 2019 17:06
@rboyer rboyer changed the base branch from fix-vault-ca-provider-uri-sans to master September 23, 2019 17:07
@rboyer rboyer merged commit af01d39 into master Sep 23, 2019
rboyer added a commit that referenced this pull request Sep 23, 2019
…ields in connect certs (#6492)

The fields in the certs are meant to hold the original binary
representation of this data, not some ascii-encoded version.

The only time we should be colon-hex-encoding fields is for display
purposes or marshaling through non-TLS mediums (like RPC).
@rboyer rboyer deleted the stop-hex-encoding-connect-tls-fields branch September 23, 2019 17:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants