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

Add check for sha1 CA certificate #1004

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

ehelms
Copy link
Member

@ehelms ehelms commented Nov 26, 2024

No description provided.

@ehelms ehelms force-pushed the test-sha1 branch 2 times, most recently from 85380ae to 6e35c1e Compare November 26, 2024 21:59
bin/katello-certs-check Outdated Show resolved Hide resolved
bin/katello-certs-check Outdated Show resolved Hide resolved
Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

I just saw http://sven.stormbind.net/blog/posts/pkix_look_into_cert_chains/ as a nice way to inspect a full chain

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

Please also test this where the chain is one certificate signed by SHA256 and one by SHA1. In reality I think the intermediate is more likely to be SHA256 with a SHA1 root. I think your current code only verifies the first certificate (intermediate) and it will miss the root, which the JIRA issue told us was the actual cause.

@ehelms
Copy link
Member Author

ehelms commented Dec 6, 2024

I just saw http://sven.stormbind.net/blog/posts/pkix_look_into_cert_chains/ as a nice way to inspect a full chain

I'll take a look at this method - this isn't the first issue we've had with not checking the full chain as it's not been an easy thing to do.

@ehelms
Copy link
Member Author

ehelms commented Dec 6, 2024

Updated with a bundle and checks on the bundle.

@ehelms ehelms requested review from ekohl and evgeni December 13, 2024 13:57
@ehelms
Copy link
Member Author

ehelms commented Dec 13, 2024

@ekohl @evgeni Can you re-review please?

bin/katello-certs-check Outdated Show resolved Hide resolved
bin/katello-certs-check Outdated Show resolved Hide resolved
@ekohl
Copy link
Member

ekohl commented Dec 13, 2024

Tests aren't happy with my suggestions :)

Co-authored-by: Ewoud Kohl van Wijngaarden <ewoud@kohlvanwijngaarden.nl>
Signed-off-by: Eric D. Helms <ericdhelms@gmail.com>
Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

@evgeni mind giving it a final read through?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants