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.

Sign up for GitHub

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

Jump to bottom

Add new stream formatters for cert chain identifiers #35513

Merged
merged 14 commits into from
Aug 15, 2024
Merged

Add new stream formatters for cert chain identifiers #35513

merged 14 commits into from
Aug 15, 2024

Conversation

keithmattix
Copy link
Contributor

Commit Message: Adds stream formatters to print the fingerprints and serial for entire downstream cert chain
Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]: Fixes #35452
[Optional Deprecated:]
[Optional API Considerations:]

@keithmattix
Copy link
Contributor Author

/retest

@soulxu
Copy link
Member

soulxu commented Jul 31, 2024

/assgin @wbpcode @ggreenway

@keithmattix
Copy link
Contributor Author

I think the test failures are a flake we're seeing on main as well (quic socket singleton cleanup).

/retest

@wbpcode
Copy link
Member

wbpcode commented Aug 1, 2024

I am fine to the formatter. But I think the core are these new connection methods. So, will let @ggreenway to review this PR first.

@wbpcode wbpcode requested a review from yanavlasov August 1, 2024 08:07
@wbpcode wbpcode removed their assignment Aug 1, 2024
@wbpcode wbpcode removed the request for review from yanavlasov August 1, 2024 08:07
ggreenway
ggreenway requested changes Aug 1, 2024
View reviewed changes
Copy link
Contributor

@ggreenway ggreenway left a comment

Choose a reason for hiding this comment

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

/wait

docs/root/configuration/observability/access_log/usage.rst Show resolved Hide resolved
docs/root/configuration/observability/access_log/usage.rst Show resolved Hide resolved
docs/root/configuration/observability/access_log/usage.rst Outdated Show resolved Hide resolved
envoy/ssl/connection.h Outdated Show resolved Hide resolved
source/common/tls/connection_info_impl_base.cc Outdated Show resolved Hide resolved
@keithmattix keithmattix force-pushed the formatters/peer-chain-formatters branch from 8a566eb to 59a0bbe Compare August 2, 2024 01:04
@keithmattix keithmattix requested a review from ggreenway August 2, 2024 01:18
@keithmattix
Fix test build
617da09 
Signed-off-by: Keith Mattix II <[email protected]>
@keithmattix
Fixup tests
d973dc7 
Signed-off-by: Keith Mattix II <[email protected]>
@ggreenway ggreenway removed their assignment Aug 2, 2024
@keithmattix
Copy link
Contributor Author

/retest

@keithmattix
Copy link
Contributor Author

/retest

@keithmattix
Copy link
Contributor Author

I have absolutely no idea why tar is failing:

tar: 9fac9f54c87c48c99d33de59dc23ea66_archive.tar: Wrote only 2048 of 10240 bytes

@adisuissa
Copy link
Contributor

/retest

ggreenway
ggreenway previously approved these changes Aug 13, 2024
View reviewed changes
Copy link
Contributor

@ggreenway ggreenway left a comment

Choose a reason for hiding this comment

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

LGTM

@ggreenway
Copy link
Contributor

I have absolutely no idea why tar is failing:

tar: 9fac9f54c87c48c99d33de59dc23ea66_archive.tar: Wrote only 2048 of 10240 bytes

test infrastructure flake. I'm re-running it.

@ggreenway ggreenway enabled auto-merge (squash) August 13, 2024 22:16
@keithmattix
Copy link
Contributor Author

/retest

@keithmattix
Copy link
Contributor Author

/retest

@keithmattix
Add extra test case
4261520 
Signed-off-by: Keith Mattix II <[email protected]>
auto-merge was automatically disabled August 14, 2024 15:03

Head branch was pushed to by a user without write access

@keithmattix
Copy link
Contributor Author

/retest

1 similar comment
@keithmattix
Copy link
Contributor Author

/retest

@ggreenway
Copy link
Contributor

Coverage is too low; retry won't fix this. Please try to improve coverage.

@keithmattix
Copy link
Contributor Author

4261520 was my attempt at approving coverage; the retest I did was for a flake (timeout). I'm looking for other branches to test, but there aren't very many left from what I can see

@ggreenway ggreenway merged commit e0c98ec into envoyproxy:main Aug 15, 2024
48 checks passed
@adisuissa adisuissa mentioned this pull request Aug 21, 2024
Closed
@botengyao botengyao mentioned this pull request Aug 26, 2024
Merged
duxin40 pushed a commit to duxin40/envoy that referenced this pull request Oct 15, 2024
@keithmattix
Add new stream formatters for cert chain identifiers (envoyproxy#35513)
735fe34 
Adds stream formatters to print the fingerprints and
serial for entire downstream cert chain
Fixes envoyproxy#35452

Signed-off-by: Keith Mattix II <[email protected]>
Signed-off-by: duxin40 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ggreenway ggreenway ggreenway approved these changes

Assignees

@ggreenway ggreenway

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

feature/accesslog: Stream formatters for client cert chain auditing
5 participants
@keithmattix @soulxu @wbpcode @adisuissa @ggreenway