Define base encoding in x509.serial_number #2383
Conversation
Narrow the definition of x509.serial_number to be encoded in hexadecimal, otherwise we end up with integrations choosing their own encoding, as noted below, Zeek uses base 16 while the rest of beats is using base 10. Related to elastic/sdh-beats#5089. Reasoning in: elastic/sdh-beats#5089 (comment)
|
Documentation changes preview: https://ecs_bk_2383.docs-preview.app.elstc.co/diff |
| @@ -52,8 +52,8 @@ | |||
| type: keyword | |||
| short: Unique serial number issued by the certificate authority. | |||
| description: > | |||
| Unique serial number issued by the certificate authority. For consistency, if this value is alphanumeric, it should be | |||
| formatted without colons and uppercase characters. | |||
| Unique serial number issued by the certificate authority. For consistency, this should be | |||
There was a problem hiding this comment.
Given this is should, it implies this encoding is still optional, which might not really completely resolve the original problem. Is that what you intend? But if the wording is changed to something stronger, it could be considered a breaking change.
If you want it to be enforced, what do you think of having should in ECS v8, and changing to must in the next major release?
There was a problem hiding this comment.
I agree, that making it a MUST would be breaking for v8 so I think it needs to remain a SHOULD.
About changing it in the next major, sounds like a good idea.
There was a problem hiding this comment.
I'm fine with that, I'm not familiar with the process at all.
For the next release, shouldn't we restrict it even more? I'd prefer to have something that is strict on case, semi-colons and base.
| Unique serial number issued by the certificate authority. For consistency, this should be | ||
| encoded in base 16 and formatted without colons and uppercase characters. |
There was a problem hiding this comment.
"without colons and uppercase characters" is ambiguous. Does it mean "without colons and without uppercase"? Or does it mean "without colons and with uppercase"? 😄
| Unique serial number issued by the certificate authority. For consistency, this should be | |
| encoded in base 16 and formatted without colons and uppercase characters. | |
| Unique serial number issued by the certificate authority. For consistency, this should be | |
| encoded in base 16 and formatted as uppercase characters without colons. |
(edited to leave "base 16" as is)
There was a problem hiding this comment.
Ha! Good point, my brain didn't spot that.
I think it should still be base 16, not base-16, as it's consistent with the other cases that use space, as in:
https://github.com/elastic/ecs/blob/main/schemas/dns.yml#L98
grep -r 'base-' doesn't return any other uses.
There was a problem hiding this comment.
If all agree, I'd bring thid PR to 8.x with the wording changes from @andrewkroh, and then open a second PR to do the breaking/must changes.
There was a problem hiding this comment.
SGTM. If we haven't already created an 8.x branch then we should do that before you merge the second PR.
| @@ -52,8 +52,8 @@ | |||
| type: keyword | |||
| short: Unique serial number issued by the certificate authority. | |||
| description: > | |||
| Unique serial number issued by the certificate authority. For consistency, if this value is alphanumeric, it should be | |||
| formatted without colons and uppercase characters. | |||
| Unique serial number issued by the certificate authority. For consistency, this should be | |||
There was a problem hiding this comment.
I agree, that making it a MUST would be breaking for v8 so I think it needs to remain a SHOULD.
About changing it in the next major, sounds like a good idea.
This proposes enforcing the base encoding of x509.serial_number to be 16.
The current definition is too loose and leaves room for interpretation, base 16 is also more common and it will help users correlate the value with existing tools. This change was prompted by elastic/sdh-beats#5089 where a customer expected to see it in the same format as other tools.
Particularly relevant comment from @andrewkroh here: https://github.com/elastic/sdh-beats/issues/5089#issuecomment-2349046226
Narrow the definition of x509.serial_number to be encoded in hexadecimal, otherwise we end up with integrations choosing their own encoding, as noted below, Zeek uses base 16 while the rest of beats is using base 10.