Project

Profile

Help

Story #1991

As a user, uploaded units which don't pass the signature check are not imported

Added by bmbouter over 5 years ago. Updated over 2 years ago.

Status:
CLOSED - CURRENTRELEASE
Priority:
Urgent
Sprint/Milestone:
-
Start date:
Due date:
% Done:

100%

Estimated time:
Platform Release:
2.10.0
Groomed:
Yes
Sprint Candidate:
Yes
Tags:
Pulp 2
Sprint:
Sprint 6
Quarter:

Description

When units are imported and their signature is not valid, the unit should fail to import. This should be true for both sync and upload. The key which does the validation is configured on the repo.

https://github.com/PulpQE/pulp-smash/issues/347


Checklist


Related issues

Related to RPM Support - Issue #2188: Make GPG signature checking is called "filtering"CLOSED - CURRENTRELEASE<a title="Actions" class="icon-only icon-actions js-contextmenu" href="#">Actions</a>
Has duplicate RPM Support - Issue #2103: Provide documentation and release notes for issue #1156 (signature attribute for RPMs, SRPMs, and DRPMs)CLOSED - DUPLICATE<a title="Actions" class="icon-only icon-actions js-contextmenu" href="#">Actions</a>
Has duplicate RPM Support - Story #217: As a user, I can restrict RPMs by signatureCLOSED - DUPLICATE

<a title="Actions" class="icon-only icon-actions js-contextmenu" href="#">Actions</a>
Blocked by RPM Support - Story #1156: As a user, I can have an "signature" attribute stored for RPMs, SRPMs, and DRPMsCLOSED - CURRENTRELEASE

<a title="Actions" class="icon-only icon-actions js-contextmenu" href="#">Actions</a>
Blocked by Pulp - Issue #2030: importer config is not validated during the updateCLOSED - CURRENTRELEASE<a title="Actions" class="icon-only icon-actions js-contextmenu" href="#">Actions</a>

Associated revisions

Revision 33c40549 View on GitHub
Added by ipanova@redhat.com over 5 years ago

As a user, units which don't pass the signature check are not imported

closes #1991

Revision 33c40549 View on GitHub
Added by ipanova@redhat.com over 5 years ago

As a user, units which don't pass the signature check are not imported

closes #1991

Revision 01b21bef View on GitHub
Added by ipanova@redhat.com over 5 years ago

As a user, units which don't pass the signature check are not imported

closes #1991 https://pulp.plan.io/issues/1991

History

#1 Updated by bmbouter over 5 years ago

  • Related to Story #1156: As a user, I can have an "signature" attribute stored for RPMs, SRPMs, and DRPMs added

#2 Updated by bmbouter over 5 years ago

  • Subject changed from As a user, uploaded units which don't pass the signature check are not imported to As a user of the yum_distributor

This story needs to be rewritten, but one point of clarification first. Does this prevent content from being associated with the repo at all? Or does it prevent content from being published with the repo?

#3 Updated by bmbouter over 5 years ago

  • Subject changed from As a user of the yum_distributor to As a user, uploaded units which don't pass the signature check are not imported

Reverting accidental title change.

#4 Updated by mhrivnak over 5 years ago

I lean more toward preventing content from being associated to the repo, but it would be interesting to get RCM's opinion. Once a bunch of RPMs signed by the wrong key land in a repo, it could be a real pain to identify and remove each one.

#5 Updated by bmbouter over 5 years ago

  • Description updated (diff)

How exactly is the key configured on the repo? Also what about the lazy cases?

#6 Updated by mhrivnak over 5 years ago

The public key would be an importer config setting.

For the lazy case, that's a good question. A simple option is to just not enable this feature unless the download policy is immediate. That's the only way for us to prevent wrongly-signed content from getting added to a repo.

A different option, which I'm not crazy about but will describe anyway, would be to make this more of an advisory feature than an enforcement feature. We could show on each unit whether the signature is good, bad, or unknown, and let the user sort it out as they please. To me this sounds more cumbersome and less useful.

Have any other ideas?

#7 Updated by bmbouter over 5 years ago

  • Related to deleted (Story #1156: As a user, I can have an "signature" attribute stored for RPMs, SRPMs, and DRPMs)

#8 Updated by bmbouter over 5 years ago

  • Blocked by Story #1156: As a user, I can have an "signature" attribute stored for RPMs, SRPMs, and DRPMs added

#9 Updated by mhrivnak over 5 years ago

Here is info about Red Hat's keys: https://access.redhat.com/security/team/key

And Fedora's: https://getfedora.org/keys/

It's important to note that some keys expire, but that is an optional feature of gpg keys. Looking at Red Hat's, it appears they do not expire. Same for Fedora.

None the less, they could get revoked and replaced at some point if compromised, or if they for some reason become unavailable for further use. For those reasons, we should support the ability to add additional keys to an importer at any time.

#10 Updated by ipanova@redhat.com over 5 years ago

  • Checklist item config importer should have 2 new optional parameters: allow_unsigned(boolean) and allow_keys(list added
  • Checklist item document new importer settings added
  • Checklist item add release notes added
  • Checklist item document new feature added

The signature check will be implemented for upload as well as for sync. Due to the fact that signature check is incompatible with lazy sync, some restrictions will be applied, therefore signature check will work only with immediate download policy.

These conditions and restrictions are related just to the sync case:

1) it will not be possible to create repo with on_demand/background policy and with signature check
2) once you created and synced the repository with immediate download policy and signature check, the policy cannot be switched
3) once you created repo with lazy download policy and synced it, you will not be able to change the policy to immediate with signature check
4) allow_keys can be updated with additional keys, allow_unsigned should be immutable
5) once you created repo with immediate download policy( with no signature check) and synced it, you cannot update repo to enable signature check

#11 Updated by bmbouter over 5 years ago

wrote:

The signature check will be implemented for upload as well as for sync. Due to the fact that signature check is incompatible with lazy sync, some restrictions will be applied, therefore signature check will work only with immediate download policy.

These conditions and restrictions are related just to the sync case:

1) it will not be possible to create repo with on_demand/background policy and with signature check

Do you mean s/repo/importer/ ? ^

2) once you created and synced the repository with immediate download policy and signature check, the policy cannot be switched
3) once you created repo with lazy download policy and synced it, you will not be able to change the policy to immediate with signature check
4) allow_keys can be updated with additional keys, allow_unsigned should be immutable

For clarity to ^, you could add a specific statement that allow_keys can not have keys removed after a sync occurs.

5) once you created repo with immediate download policy( with no signature check) and synced it, you cannot update repo to enable signature check

Do you mean s/repo/importer/ ? ^

#12 Updated by bmbouter over 5 years ago

Here is maybe a shorter set of rules for the restrictions:

1. If an importer has been used for a sync or upload and if the allow_keys allow_keys attribute is None, reject the setting of allow_keys
2. If an importer has been used for sync or upload, reject an update for the download policy attribute
3. If an importer has a download policy of not immediate (now or in this update) and allow_keys (now or in this update) is not None, reject the importer update
4. reject updates for allow_unsigned since it is immutable

#13 Updated by ipanova@redhat.com over 5 years ago

  • Status changed from NEW to ASSIGNED
  • Assignee set to ipanova@redhat.com
  • Sprint/Milestone set to 22
  • Groomed changed from No to Yes

#14 Updated by bmbouter over 5 years ago

I was thinking that 'signature_keys' could be a better name for the option than 'allow_keys'.

#15 Updated by ipanova@redhat.com over 5 years ago

  • Blocked by Issue #2030: importer config is not validated during the update added

#16 Updated by mhrivnak over 5 years ago

  • Sprint/Milestone changed from 22 to 23

#17 Updated by Anonymous over 5 years ago

  • Priority changed from Normal to Urgent

#18 Updated by mhrivnak over 5 years ago

  • Sprint/Milestone changed from 23 to 24

#19 Updated by Ichimonji10 over 5 years ago

It seems like ipanova and bmbouter have clarified some questions about this feature. Can those clarifications be rolled back into the original issue description? That'd really help QE test the feature.

What values may go in allow_keys? Is it a list of PGP keys, or some other kind of key? Can I go to https://repos.fedorapeople.org/pulp/pulp/fixtures/, download an RPM, extract it, and find valid value that may be placed in allow_keys?

What happens if allow_keys is an empty list and allow_unsigned is true? Does that mean I can import unsigned RPMs into a repository, but not signed RPMs?

#21 Updated by Ichimonji10 over 5 years ago

Are the same set of restrictions in place regardless of whether one is:

  • importing units via direct upload,
  • syncing in units via a feed, or
  • copying units between repositories?

#22 Updated by amacdona@redhat.com over 5 years ago

  • Has duplicate Issue #2103: Provide documentation and release notes for issue #1156 (signature attribute for RPMs, SRPMs, and DRPMs) added

#23 Updated by mhrivnak over 5 years ago

  • Status changed from ASSIGNED to POST

#25 Updated by ipanova@redhat.com over 5 years ago

  • Checklist item config importer should have 2 new optional parameters: allow_unsigned(boolean) and allow_keys(list set to Done

#26 Updated by ipanova@redhat.com over 5 years ago

  • Checklist item document new importer settings set to Done
  • Checklist item add release notes set to Done
  • Checklist item document new feature set to Done

#27 Updated by ipanova@redhat.com over 5 years ago

  • Status changed from POST to MODIFIED
  • % Done changed from 0 to 100

#28 Updated by ipanova@redhat.com over 5 years ago

  • Platform Release set to 2.10.0

#29 Updated by semyers over 5 years ago

  • Status changed from MODIFIED to 5

#31 Updated by Ichimonji10 over 5 years ago

This feature appears to be implemented. However, the actual implementation uses keys named require_signature and allowed_keys, not allow_unsigned and allow_keys. Can I get an explicit confirmation from the developers that the implementation intentionally differs from the requirements?

#32 Updated by ipanova@redhat.com over 5 years ago

Ichimonji10 wrote:

This feature appears to be implemented. However, the actual implementation uses keys named require_signature and allowed_keys, not allow_unsigned and allow_keys. Can I get an explicit confirmation from the developers that the implementation intentionally differs from the requirements?

Yes, this is also confirmed and described in docs and release notes.

#33 Updated by mhrivnak over 5 years ago

  • Related to Issue #2188: Make GPG signature checking is called "filtering" added

#34 Updated by Ichimonji10 over 5 years ago

I can verify that Pulp correctly checks key IDs for uploaded packages. I can't say anything about synced-in or copied packages.

#35 Updated by Ichimonji10 about 5 years ago

  • Status changed from 5 to ASSIGNED

Test cases which upload packages, sync in packages, and copy packages are now in place. See:

The following test cases are currently failing for the Pulp 2.10.1 nightly builds:

A close look at the failing tests shows that Pulp always syncs in packages, no matter what the require_signature and allowed_keys options are set to.

#36 Updated by ipanova@redhat.com about 5 years ago

I am not sure if the correct way would be to move the state to assigned or rather to open a new issue, because 1) i bet you tested against master, here the target release is 2.10. In 2.10-dev sync with signature filter works as expected( i just checked). so seems like it is a regression which was introduced on master.

#37 Updated by ipanova@redhat.com about 5 years ago

first suspicious thing that comes to my attention is wrong unit count during download : downloading 6 rpms out of 3

 pulp-admin rpm repo sync run --repo-id test

+----------------------------------------------------------------------+
                    Synchronizing Repository [test]
+----------------------------------------------------------------------+

This command may be exited via ctrl+c without affecting the request.

Downloading metadata...
[|]
... completed

Downloading repository content...
[====================================================================================================] 200%
RPMs:       6/3 items
Delta RPMs: 0/0 items

... completed

Individual package errors encountered during sync:

3 packages failed signature filter and were not imported.

#38 Updated by Ichimonji10 about 5 years ago

1) i bet you tested against master, here the target release is 2.10

I'm testing against the 2.10.1 nightly build.

$ ssh $hostname rpm -qa | sort | grep -i pulp
pulp-admin-client-2.10.1-0.1.alpha.git.7.da8dea2.fc24.noarch
pulp-docker-admin-extensions-2.1.1-0.1.alpha.git.9.e48c093.fc24.noarch
pulp-docker-plugins-2.1.1-0.1.alpha.git.9.e48c093.fc24.noarch
pulp-puppet-admin-extensions-2.10.1-0.1.alpha.git.25.b1e70b6.fc24.noarch
pulp-puppet-plugins-2.10.1-0.1.alpha.git.25.b1e70b6.fc24.noarch
pulp-python-admin-extensions-1.1.2-1.fc24.noarch
pulp-python-plugins-1.1.2-1.fc24.noarch
pulp-rpm-admin-extensions-2.10.1-0.1.alpha.git.11.83ec388.fc24.noarch
pulp-rpm-plugins-2.10.1-0.1.alpha.git.11.83ec388.fc24.noarch
pulp-selinux-2.10.1-0.1.alpha.git.7.da8dea2.fc24.noarch
pulp-server-2.10.1-0.1.alpha.git.7.da8dea2.fc24.noarch
python-kombu-3.0.33-6.pulp.fc24.noarch
python-pulp-bindings-2.10.1-0.1.alpha.git.7.da8dea2.fc24.noarch
python-pulp-client-lib-2.10.1-0.1.alpha.git.7.da8dea2.fc24.noarch
python-pulp-common-2.10.1-0.1.alpha.git.7.da8dea2.fc24.noarch
python-pulp-docker-common-2.1.1-0.1.alpha.git.9.e48c093.fc24.noarch
python-pulp-oid_validation-2.10.1-0.1.alpha.git.7.da8dea2.fc24.noarch
python-pulp-puppet-common-2.10.1-0.1.alpha.git.25.b1e70b6.fc24.noarch
python-pulp-python-common-1.1.2-1.fc24.noarch
python-pulp-repoauth-2.10.1-0.1.alpha.git.7.da8dea2.fc24.noarch
python-pulp-rpm-common-2.10.1-0.1.alpha.git.11.83ec388.fc24.noarch
python-pulp-streamer-2.10.1-0.1.alpha.git.7.da8dea2.fc24.noarch

#39 Updated by ipanova@redhat.com about 5 years ago

oh i see you are right, i did not have pulled latest changes.
hm, these changes might have to do with wring unit count to download https://github.com/pulp/pulp_rpm/commit/8e2bfe7120654b65899c510108374212a33d3435

#40 Updated by Ichimonji10 about 5 years ago

  • Status changed from ASSIGNED to 6

I can verify that this feature works correctly on a Pulp 2.10.0 beta system with the following packages installed:

$ ssh $hostname rpm -qa | sort | grep -i pulp
pulp-admin-client-2.10.0-0.5.rc.el7.noarch
pulp-agent-2.10.0-0.5.rc.el7.noarch
pulp-consumer-client-2.10.0-0.5.rc.el7.noarch
pulp-docker-admin-extensions-2.1.0-0.3.rc.el7.noarch
pulp-docker-plugins-2.1.0-0.3.rc.el7.noarch
pulp-ostree-admin-extensions-1.1.3-1.el7.noarch
pulp-ostree-plugins-1.1.3-1.el7.noarch
pulp-puppet-admin-extensions-2.10.0-0.2.rc.el7.noarch
pulp-puppet-consumer-extensions-2.10.0-0.2.rc.el7.noarch
pulp-puppet-handlers-2.10.0-0.2.rc.el7.noarch
pulp-puppet-plugins-2.10.0-0.2.rc.el7.noarch
pulp-python-admin-extensions-1.1.3-1.el7.noarch
pulp-python-plugins-1.1.3-1.el7.noarch
pulp-rpm-admin-extensions-2.10.0-0.5.rc.el7.noarch
pulp-rpm-consumer-extensions-2.10.0-0.5.rc.el7.noarch
pulp-rpm-handlers-2.10.0-0.5.rc.el7.noarch
pulp-rpm-plugins-2.10.0-0.5.rc.el7.noarch
pulp-rpm-yumplugins-2.10.0-0.5.rc.el7.noarch
pulp-selinux-2.10.0-0.5.rc.el7.noarch
pulp-server-2.10.0-0.5.rc.el7.noarch
python-isodate-0.5.0-4.pulp.el7.noarch
python-kombu-3.0.33-6.pulp.el7.noarch
python-pulp-agent-lib-2.10.0-0.5.rc.el7.noarch
python-pulp-bindings-2.10.0-0.5.rc.el7.noarch
python-pulp-client-lib-2.10.0-0.5.rc.el7.noarch
python-pulp-common-2.10.0-0.5.rc.el7.noarch
python-pulp-docker-common-2.1.0-0.3.rc.el7.noarch
python-pulp-oid_validation-2.10.0-0.5.rc.el7.noarch
python-pulp-ostree-common-1.1.3-1.el7.noarch
python-pulp-puppet-common-2.10.0-0.2.rc.el7.noarch
python-pulp-python-common-1.1.3-1.el7.noarch
python-pulp-repoauth-2.10.0-0.5.rc.el7.noarch
python-pulp-rpm-common-2.10.0-0.5.rc.el7.noarch
python-pulp-streamer-2.10.0-0.5.rc.el7.noarch

There are some issues with the 2.10.1 nightlies, and I'll file a separate issue for that.

#41 Updated by semyers about 5 years ago

  • Status changed from 6 to CLOSED - CURRENTRELEASE

#43 Updated by bmbouter over 4 years ago

  • Has duplicate Story #217: As a user, I can restrict RPMs by signature added

#44 Updated by bmbouter over 3 years ago

  • Sprint set to Sprint 6

#45 Updated by bmbouter over 3 years ago

  • Sprint/Milestone deleted (24)

#46 Updated by bmbouter over 2 years ago

  • Tags Pulp 2 added

Please register to edit this issue

Also available in: Atom PDF