-
Notifications
You must be signed in to change notification settings - Fork 45
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
Use new fixture signing key #1424
Conversation
Thanks for opening this PR! You may need to rebase the branch and wait for a pulpcore release to make these changes available: https://github.com/pulp/pulpcore/pull/4731/files. |
assert decrypted.username == "Pulp QE" | ||
assert decrypted.username == "pulp-fixture-signing-key" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, this will introduce a bad version interdependence issue. If we cannot defer the userid of the key, we should not assert on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test is useful for asserting data stored in Pulp. We verify whether we did not change anything along the way while parsing images. Am I reading it correctly that you want to remove this check?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Depending on the version of pulpcore used, this username will be the new one or the old one. So the best way would be to find a way to get the asserted name from the gpgkey fixture in pulpcore. If that is impossible, i'd really just remove the assertion (or assert only that it's not empty).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fixture doesn't provide that information. It does provide the fingerprint, which IMO is good enough and effectively tests the same thing. Also the "key id" is a subset of the fingerprint IIRC, yeah? So we wouldn't need to test both?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, let's remove it!
[noissue]
@@ -73,9 +73,7 @@ def test_assert_signed_image( | |||
decrypted = gpg.decrypt(raw_s) | |||
|
|||
assert decrypted.key_id == keyid | |||
assert decrypted.fingerprint == fingerprint |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fingerprint comes from the bindings. I believe that should be fine. Maybe I'm missing something again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See previous comment, fingerprint and keyid are effectively the same, as one (I believe keyid) is just the last X bits of the other. Realistically just checking one is the same as checking both as they are both meant to be functionally unique values.
Edit: at least for the purposes of this test. Supposedly both fingerprints and particularly keyids are attackable by brute force, but that's a bit beyond the scope of what we're testing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should also fix nightly checks:
> assert decrypted.username == "Pulp QE"
E AssertionError: assert 'pulp-fixture...pproject.org>' == 'Pulp QE'
E - Pulp QE
E + pulp-fixture-signing-key <[email protected]>
https://github.com/pulp/pulp_container/actions/runs/6952312275/job/18948179340
Backport to 2.16: 💚 backport PR created✅ Backport PR branch: Backported as #1451 🤖 @patchback |
Backport to 2.15: 💚 backport PR created✅ Backport PR branch: Backported as #1458 🤖 @patchback |
[noissue]