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

blueprint: fix CA cert testing #1096

Merged
merged 1 commit into from
Dec 13, 2024
Merged

blueprint: fix CA cert testing #1096

merged 1 commit into from
Dec 13, 2024

Conversation

lzap
Copy link
Contributor

@lzap lzap commented Dec 9, 2024

SSIA @thozza @mvo5

@achilleas-k achilleas-k self-requested a review December 9, 2024 14:37
@lzap lzap force-pushed the cacert-test branch 5 times, most recently from bce10a4 to 5e96464 Compare December 10, 2024 08:27
thozza
thozza previously approved these changes Dec 10, 2024
Copy link
Member

@thozza thozza left a comment

Choose a reason for hiding this comment

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

Thanks!

@lzap
Copy link
Contributor Author

lzap commented Dec 10, 2024

Finally it is passing, I figured out how to generalize the test. Ready for review.

echo "📗 Checking extracted CA cert file named '${cn}'"
if ! [ -e "/etc/pki/ca-trust/extracted/pem/directory-hash/${cn}.pem" ]; then
echo "Extracted CA file does not exist, directory contents:"
find /etc/pki/ca-trust/extracted/pem/directory-hash
exit 1
fi
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@achilleas-k fyi this test was introduced last week but it was broken (a typo in directory-hash path) and due to the config-map it was not executed properly so it sneaked into main. This fixes the map and the test.

Copy link
Member

Choose a reason for hiding this comment

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

Thank you!

achilleas-k
achilleas-k previously approved these changes Dec 10, 2024
@achilleas-k
Copy link
Member

The test seems to be failing on Fedora 39 and RHEL 9.4. Perhaps a different behaviour in older versions of the tool, @lzap?

@lzap lzap dismissed stale reviews from achilleas-k and thozza via 120b43b December 10, 2024 15:19
@lzap lzap force-pushed the cacert-test branch 2 times, most recently from 120b43b to def14bf Compare December 10, 2024 15:19
@lzap
Copy link
Contributor Author

lzap commented Dec 10, 2024

Ah that is right, amended a small change where I change the sed to sed -E 's/.*CN ?= ?//' because of the key/value pairs having spaces around = in some versions of openssl.

@lzap lzap force-pushed the cacert-test branch 2 times, most recently from 4cb039f to 3692a1f Compare December 10, 2024 17:19
@lzap
Copy link
Contributor Author

lzap commented Dec 10, 2024

Also I have found out that on some (older) systems the directory-hash directory does not exist, so I came with a better test case: it now searches for CN in the bundle PEM file which is concatenated list of all certs with CNs as comments:

grep "Test CA for osbuild" /etc/pki/ca-trust/extracted/pem/tls-ca-bundle.pem

This should work also on older systems.

@lzap
Copy link
Contributor Author

lzap commented Dec 11, 2024

Is green, let's put it on the queue @achilleas-k @thozza if you like the grep approach. Not rebasing, I think queue will do the job now.

Comment on lines 95 to 96
echo "Extracted CA file does not exist, directory contents:"
find /etc/pki/ca-trust/extracted/pem
Copy link
Member

Choose a reason for hiding this comment

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

The grep-ing of tls-ca-bundle.pem makes me wonder if listing the content of /etc/pki/ca-trust/extracted/pem still makes sense? 🤔

Copy link
Member

Choose a reason for hiding this comment

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

So, judging from https://github.com/osbuild/osbuild-composer/pull/4487/files#diff-a0a3d509a4e8903c0bede204d82662223fa270c20360d5bf2dcd2f1d2b68bbcdR264, it should be probably

grep '^#' /etc/pki/ca-trust/extracted/pem/tls-ca-bundle.pem

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I am working on two very similar patches and as I go I improve every one bit by bit. This should put them on par now, rebased.

Copy link
Member

Choose a reason for hiding this comment

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

Thank you for the change. Please also amend the comment above it.

@lzap
Copy link
Contributor Author

lzap commented Dec 12, 2024

Right, done.

Copy link
Member

@thozza thozza left a comment

Choose a reason for hiding this comment

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

Thanks!

@lzap lzap added this pull request to the merge queue Dec 13, 2024
@lzap
Copy link
Contributor Author

lzap commented Dec 13, 2024

I do not see any comments by @achilleas-k the tests were fixed, adding to the merge queue. Thanks again for help with this, now I understand how tests do work in here a little bit better :-D

Merged via the queue into osbuild:main with commit 994201a Dec 13, 2024
19 checks passed
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