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.
There was a problem hiding this comment.
Agree, I'm just a bit confused, we have branches 8.{11,12,13,14}, when you say 8.x does this mean 8.{12,13,14} ?
Do we change ECS definitions (even if should), for current and older releases (<= 8.11).
In other words, where should I branch from?
There was a problem hiding this comment.
This is what other repos like beats/kibana/elasticsearch and doing... We should create an 8.x from main. And then future 8.{16,17,18} branches will come from 8.x. We should backport to 8.x unless the change is a something specifically for 9.0 (like a breaking change).
There was a problem hiding this comment.
Got it, in the meantime @mjwolf volunteered to take care of this.
There was a problem hiding this comment.
I've created an 8.x branch now. I think the best way to handle this is to merge this PR to main first (and I'll handle the backport to the 8.x branch), and then afterwards create another PR with the changes intended for 9.x only that will stay in main only.
There was a problem hiding this comment.
@haesbaert with this PR merged, and backported to 8.x, you can now create another PR with the changes intended for 9.x only. You can target it to main, and a backport-skip label
| @@ -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.
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.
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.