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

Switch to base 16 for TLS serial number in packetbeat in line with ECS changes #41542

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

fearful-symmetry
Copy link
Contributor

@fearful-symmetry fearful-symmetry commented Nov 6, 2024

Proposed commit message

This switches the serial_number field in packetbeat's TLS reporting to use base 16 values instead of base 10:

		"public_key_size":             "2048",
		"serial_number":               "1121E97D5D37348C572C555A3A59B7B65D2B",
		"signature_algorithm":         "SHA256-RSA",

I don't think this is a breaking change? Also note the lack of a 0x indicator. Not sure if we care.

ECS PR here: elastic/ecs#2383

Also note that this maintains the liter bypass for dsa, which is deprecated. Do we still need it?

Checklist

  • My code follows the style guidelines of this project
  • I have added tests that prove my fix is effective or that my feature works
  • I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

Disruptive User Impact

This will alter the reported serial_number for TLS certs from base 10 to base 16.

@fearful-symmetry fearful-symmetry added bug Team:Security-Linux Platform Linux Platform Team in Security Solution backport-8.16 Automated backport with mergify labels Nov 6, 2024
@fearful-symmetry fearful-symmetry self-assigned this Nov 6, 2024
@fearful-symmetry fearful-symmetry requested a review from a team as a code owner November 6, 2024 18:16
@elasticmachine
Copy link
Collaborator

Pinging @elastic/sec-linux-platform (Team:Security-Linux Platform)

@botelastic botelastic bot added needs_team Indicates that the issue/PR needs a Team:* label and removed needs_team Indicates that the issue/PR needs a Team:* label labels Nov 6, 2024
@fearful-symmetry fearful-symmetry changed the title Switch to base 16 for TLS serial number Switch to base 16 for TLS serial number in packetbeat Nov 6, 2024
Copy link
Contributor

mergify bot commented Nov 6, 2024

backport-8.x has been added to help with the transition to the new branch 8.x.
If you don't need it please use backport-skip label and remove the backport-8.x label.

@mergify mergify bot added the backport-8.x Automated backport to the 8.x branch with mergify label Nov 6, 2024
Copy link
Contributor

@nicholasberlin nicholasberlin left a comment

Choose a reason for hiding this comment

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

LGTM if CI agrees

packetbeat/protos/tls/tls_test.go Outdated Show resolved Hide resolved
packetbeat/protos/tls/parse.go Show resolved Hide resolved
Copy link
Member

@andrewkroh andrewkroh left a comment

Choose a reason for hiding this comment

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

Please update the PR description's proposed commit message to reference the related ECS change, as this provides essential context for understanding the behavior modification.

Has the ECS change been released? In my opinion, it is not advisable for Packetbeat to "get ahead" of the schema. First, we should create a release that incorporates the spec change. Additionally, the ECS version reported by Packetbeat (src ref) should be updated to indicate which version it is following, even if it deviates from the other Beats.

We also need to add a changelog entry for this change.

@fearful-symmetry fearful-symmetry changed the title Switch to base 16 for TLS serial number in packetbeat Switch to base 16 for TLS serial number in packetbeat in line with ECS changes Nov 6, 2024
@fearful-symmetry
Copy link
Contributor Author

@andrewkroh so the ECS PR is here: elastic/ecs#2383

But it doesn't look like there's been an actual release of this yet though. Should we wait for the next minor? Is there a release process for ECS we need to trigger? Looks like the last one was a year ago.

@@ -312,7 +312,7 @@ func TestOCSPStatus(t *testing.T) {
"not_after": time.Date(2035, 3, 4, 9, 0, 0, 0, time.UTC),
"public_key_algorithm": "RSA",
"public_key_size": 4096,
"serial_number": "1492448539999078269498416841973088004758827",
"serial_number": "1121E97D5D37348C572C555A3A59B7B65D2B",
Copy link
Member

Choose a reason for hiding this comment

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

Note that all of the fields under tls.detailed.* are not part of ECS. I cannot find any definition of this field within elastic/beats that needs updated, but it is defined at https://github.com/elastic/integrations/blob/7d7358618583e3d34e83e645d8bb7c83eda58253/packages/network_traffic/data_stream/tls/fields/protocol.yml#L210-L212 so that side will also need an update.

I recommend updating that definition at the same time in which you update the ECS version12 for the network_traffic integration (after we have an ECS release).

Footnotes

  1. https://github.com/elastic/integrations/blob/7d7358618583e3d34e83e645d8bb7c83eda58253/packages/network_traffic/_dev/build/build.yml

  2. https://github.com/search?q=repo%3Aelastic%2Fintegrations+path%3A%2F%5Epackages%5C%2Fnetwork_traffic%5C%2Fdata_stream%5C%2F.*default%5C.yml%2F+%2Fecs%5C.version%2F&type=code

@andrewkroh
Copy link
Member

Should we wait for the next minor? Is there a release process for ECS we need to trigger?

According to https://github.com/elastic/ecs/blob/main/CONTRIBUTING.md#ecs-releases-during-the-donation-to-opentelemetry, it will need to be aligned to a stack minor. As for planning to get an actually release made, can you reach out to the #ecs slack channel to inquire about this (maybe we can do it for 8.16.0 so we don't have to wait long).

CHANGELOG.next.asciidoc Outdated Show resolved Hide resolved
@andrewkroh
Copy link
Member

Please update the example in your proposed commit message to reflect that the serial_number values are now uppercase.

Copy link
Contributor

mergify bot commented Nov 12, 2024

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b tls-base16 upstream/tls-base16
git merge upstream/main
git push upstream tls-base16

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-8.x Automated backport to the 8.x branch with mergify backport-8.16 Automated backport with mergify bug Team:Security-Linux Platform Linux Platform Team in Security Solution
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants