Improve OpenPGP keyring test coverage - #7947
Conversation
| data = self.packet() | ||
| # note: because these queries aren't ordered, the result may be nondetermininistic | ||
| for signature in self.openpgp_signatures.filter(**content_filter): | ||
| for signature in self.openpgp_signatures.filter(**content_filter).order_by("pk"): |
There was a problem hiding this comment.
Should we try to order by something different than PK? Problem is we don't really have access to much else.
| "permissions": [ | ||
| ( | ||
| "manage_roles_openpgpdistribution", | ||
| "Can manage roles on openpgp distributions", |
There was a problem hiding this comment.
So irritating that just fixing a typo requires a migration. I will probably try to see if we can fold this one into another one, should another one come along soon.
| ) | ||
| assert keys.count == 1 | ||
| key = keys.results[0] | ||
| assert len(key.fingerprint) == 64 |
There was a problem hiding this comment.
This condition is implicit in the regex on the next line.
Also this looks kind of repetitive maybe you could @pytest.mark.parametrize these upload tests.
There was a problem hiding this comment.
Already ahead of you
| result = verify(bytes=test_data, store=lambda key_ids: served_certs, signature=sig) | ||
| assert result.valid_sigs | ||
|
|
||
| def test_verify_signature_multiple_keys_in_keyring( |
There was a problem hiding this comment.
This could perhaps be consolidated with the previous one, but parameterize it is a bit trickier than the others.
|
@mdellweg Sorry for requesting review prematurely, I wanted to try to get some early feedback before EOD. But yes, the initial state was very rough, and now it's much more cleaned up. |
|
Also I expect some of these tests to fail, since PySequoia isn't quite PQC ready yet. edit: also I found a bug. Either way we have to wait for the next release. |
|
@mdellweg Do we need an access policy on the OpenPGPDistributionViewSet? Or is the "default" one fine? I'm not sure if the commented out |
426c99e to
34c797c
Compare
|
@mdellweg So, here's one issue. Some of the failures are caused because So we could either migrate it to a separate app, or change the implementation of |
- v4/v6 key uploads (RSA, Ed25519) - PQC keys (ML-DSA-65, ML-DSA-87) - content listing/filtering - serialization round-trips via distribution - signature verification using keys served from the keyring - key revocation - idempotent uploads - private key rejection Also fix three bugs: - Typo: distribution permission said "gem" instead of "openpgp" - content_handler_list_directory crashed when no repository version exists - represent() querysets were unordered, producing nondeterministic output Assisted-By: Claude Opus 4.6
A public key packet needs to be before any other packets
And store fingerprints uppercase consistently in the DB.
Add a viewset for repository versions. Assisted-By: Claude Opus 4.6
The `expired` property used short-circuit evaluation (`self.expiration_time and ...`) which returns None when expiration_time is None. The serializer declares `expired` as a non-nullable BooleanField, so None causes a pydantic ValidationError in the generated client bindings. Wrap the expression in bool() so it always returns False when expiration_time is unset. Assisted-By: Claude Opus 4.6
| # Multiple viewsets for the same model (e.g. RepositoryVersionViewSet | ||
| # and OpenPGPKeyringVersionViewSet both use RepositoryVersion). | ||
| # Prefer the base class — subclass viewsets exist for URL routing | ||
| # and custom access policies, not for model-to-viewset reverse lookups. |
There was a problem hiding this comment.
This is theoretically correct (I think), but it's a huge hack that Claude came up with, and I'd prefer to just move the openpgp types to a new app. There's also conflicts with implementing import/export for types in pulpcore.
If we were to actually move forwards with this I'd want to review this very very heavily. I have not really done so yet, I just want to see the impact it has on CI.
There was a problem hiding this comment.
How come this has not been broken all along? Or has it?
This function is used to generate the prn of objects, right, so yes, we need to be be sure the result is correct and stable. I don't even like that this whole function is a heuristic rooted in a brittle convention.
The comment on top of it alone "[...] these tools exist for serializers, which are depended on by viewsets. They're defined here because they're used here, and to avoid odd import dependencies. [...]" has it all. These odd dependencies are real not just for the (python) import machinery.
So to get on to a much more constructive answer:
A (Model)ViewSet points to a model. That is by nature a one-to-many relation. To know the main ViewSet for a model I'd like to be much more explicit. Designing from the top of my head: We could specify a MAIN_VIEWSET_FOR_MODEL: bool class var on the viewset that on registering classes registers itself on the model class (or in a model-to-primary-viewset-registry) and fails if another viewset already took that place. Then we turn this current heuristic into a deprecation warning and later a failure.
If it were only about PRN generation, I'd rather expect each and every model to learn how to turn into a PRN directly without any dependency on DRF machinery.
There was a problem hiding this comment.
How come this has not been broken all along? Or has it?
I think it has been. No indication that anyone has actually used it yet, though I think we may have some use cases in the immediate future.
Yes, I hate most of this infrastructure, if there's a better way to do it I'm all ears. Need to think about the specific proposal though. Let me get through some of the other PQC work and we can wrap back around to this.
When an app registers multiple NamedModelViewSet subclasses for the same model (e.g. RepositoryVersionViewSet and OpenPGPKeyringVersionViewSet both use RepositoryVersion), the lookup previously skipped the model entirely, causing LookupError in the import/export system. Resolve the ambiguity by preferring the base class in the inheritance hierarchy. Subclass viewsets still participate in URL routing and provide their own access policies — they just aren't the canonical answer for model-to-viewset reverse lookups. Assisted-By: Claude Opus 4.6
The field exists on the model but was missing from the serializer's fields tuple, causing an AttributeError in the client bindings when tests accessed it. Assisted-By: Claude Opus 4.6
| @property | ||
| def expired(self): | ||
| return self.expiration_time and timezone.now() > self.created + self.expiration_time | ||
| return bool(self.expiration_time and timezone.now() > self.created + self.expiration_time) |
There was a problem hiding this comment.
For the keys (line 155) we also use True/False/None as "yes / no / unspecified".
2 participants
Also fix three bugs:
Assisted-By: Claude Opus 4.6
📜 Checklist
See: Pull Request Walkthrough