-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Conversation
kID := sha256.Sum256(bs) | ||
return []byte(strings.Replace(fmt.Sprintf("% x", kID), " ", ":", -1)), nil | ||
return kID[:], nil |
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.
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
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.
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.
The In x509 If you use a command like 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. |
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.
Nice! Good sleuthing RB. Glad we figured that out rather than plaster over it! Does this means the other PR can be closed now?
a5033c1
to
8ab24e7
Compare
f8543bc
to
0665f40
Compare
Yep. Closed the other PR. |
8ab24e7
to
0c3e22b
Compare
0665f40
to
02dcbb1
Compare
…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).
02dcbb1
to
e2a1c3f
Compare
…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).
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