Fix leading serial number zeroes not being included in the database#245
Conversation
|
This is a nice shortcut if doing a roundtrip through ASN1 does what we want. Would it be worthwhile to put the marshaling into a separate function and add tests to make sure we always get the expected values? r? @jcjones & @mozkeeler |
|
I think tests for this would be useful, yeah. I'll work on that. |
| } | ||
|
|
||
| // getSerialNumberASN1Value returns the ASN1-encoded serial number for the given certificate, excluding the tag and length. | ||
| func getSerialNumberASN1Value(cert *x509.Certificate) ([]byte, error) { |
There was a problem hiding this comment.
I think this is the function we want to move to the certificate package. I'd make it GetHexSerial and return the hexadecimal string, since this is what we always manipulate.
There was a problem hiding this comment.
Yep, done. Less duplication this way.
jcjones
left a comment
jcjones
left a comment
There was a problem hiding this comment.
I can't really comment on the integration or DB-migration code, but the GetHexASN1Serial and its unit test look good to me. 👍
mozkeeler
left a comment
mozkeeler
left a comment
There was a problem hiding this comment.
In general, this looks like it should work (can't really speak to the DB code). I do think we should confirm the 0-padding works as expected, though (see second comment).
| } | ||
|
|
||
| func GetHexASN1Serial(cert *x509.Certificate) (serial string, err error) { | ||
| m, err := asn1.Marshal(cert.SerialNumber) |
There was a problem hiding this comment.
Hmmm - presumably go's asn1 parser wouldn't parse the certificate if the serial number were encoded incorrectly in the first place? (e.g. if it had too much leading padding?) That's the only situation I can think of where round-tripping this through asn1 wouldn't result in the exact same bytes as encoded in the serial number of the certificate.
There was a problem hiding this comment.
We parse certificates with crypto/x509's ParseCertificate which blows up on a cert with incorrect encoding. Example by @jcjones here https://play.golang.org/p/shRibwS6_4
| if err != nil { | ||
| return | ||
| } | ||
| serial = fmt.Sprintf("%X", rawValue.Bytes) |
There was a problem hiding this comment.
We've confirmed this 0-pads odd-length hex values? (e.g. if rawValue.Bytes is [0c, ff] this will result in "0CFF" and not "CFF"?)
There was a problem hiding this comment.
`%x` - https://go.dev/play/p/Fylce70N2Zl Leading Zeros - mozilla/tls-observatory#245 Signed-off-by: Rhys Evans <rhys.rr@googlemail.com>
4 participants
Fixes #218
This patch changes the way we store the serial number in the database. Currently, we store the hex representation of the serial number. This patch stores the hex representation of the DER-encoded serial number. A few examples of what currently happens and what will happen after the patch:
You can try more examples here: https://play.golang.org/p/9BErIzLKCF
A possible improvement would be batching the updates in
tools/fixserialnumbers.goto reduce the number of round trips.