Story #1991
closedAs a user, uploaded units which don't pass the signature check are not imported
100%
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.
Related issues
Updated by bmbouter over 8 years ago
- Related to Story #1156: As a user, I can have an "signature" attribute stored for RPMs, SRPMs, and DRPMs added
Updated by bmbouter over 8 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?
Updated by bmbouter over 8 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.
Updated by mhrivnak over 8 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.
Updated by bmbouter over 8 years ago
- Description updated (diff)
How exactly is the key configured on the repo? Also what about the lazy cases?
Updated by mhrivnak over 8 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?
Updated by bmbouter over 8 years ago
- Related to deleted (Story #1156: As a user, I can have an "signature" attribute stored for RPMs, SRPMs, and DRPMs)
Updated by bmbouter over 8 years ago
- Blocked by Story #1156: As a user, I can have an "signature" attribute stored for RPMs, SRPMs, and DRPMs added
Updated by mhrivnak over 8 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.
Updated by ipanova@redhat.com over 8 years ago
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
Updated by bmbouter over 8 years ago
ipanova@redhat.com 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/ ? ^
Updated by bmbouter over 8 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
Updated by ipanova@redhat.com over 8 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
Updated by bmbouter over 8 years ago
I was thinking that 'signature_keys' could be a better name for the option than 'allow_keys'.
Updated by ipanova@redhat.com over 8 years ago
- Blocked by Issue #2030: importer config is not validated during the update added
Added by ipanova@redhat.com over 8 years ago
Added by ipanova@redhat.com over 8 years ago
Revision 33c40549 | View on GitHub
As a user, units which don't pass the signature check are not imported
closes #1991
Updated by Ichimonji10 over 8 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?
Updated by Ichimonji10 over 8 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?
Updated by amacdona@redhat.com over 8 years ago
- Has duplicate Issue #2103: Provide documentation and release notes for issue #1156 (signature attribute for RPMs, SRPMs, and DRPMs) added
Added by ipanova@redhat.com over 8 years ago
Revision 01b21bef | View on GitHub
As a user, units which don't pass the signature check are not imported
Updated by ipanova@redhat.com over 8 years ago
- Description updated (diff)
Updated by ipanova@redhat.com over 8 years ago
- Status changed from POST to MODIFIED
- % Done changed from 0 to 100
Applied in changeset pulp:pulp|33c40549a73c180ea48f502a073be1b4272fa4fe.
Updated by Ichimonji10 over 8 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?
Updated by ipanova@redhat.com over 8 years ago
Ichimonji10 wrote:
This feature appears to be implemented. However, the actual implementation uses keys named
require_signature
andallowed_keys
, notallow_unsigned
andallow_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.
Updated by mhrivnak over 8 years ago
- Related to Issue #2188: Make GPG signature checking is called "filtering" added
Updated by Ichimonji10 over 8 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.
Updated by Ichimonji10 over 8 years ago
- Status changed from 5 to ASSIGNED
Test cases which upload packages, sync in packages, and copy packages are now in place. See:
- http://pulp-smash.readthedocs.io/en/latest/api/pulp_smash.tests.rpm.api_v2.test_signatures_checked_for_copies.html
- http://pulp-smash.readthedocs.io/en/latest/api/pulp_smash.tests.rpm.api_v2.test_signatures_checked_for_syncs.html
- http://pulp-smash.readthedocs.io/en/latest/api/pulp_smash.tests.rpm.api_v2.test_signatures_checked_for_uploads.html
- http://pulp-smash.readthedocs.io/en/latest/api/pulp_smash.tests.rpm.api_v2.test_signatures_saved_for_packages.html
The following test cases are currently failing for the Pulp 2.10.1 nightly builds:
- http://pulp-smash.readthedocs.io/en/latest/api/pulp_smash.tests.rpm.api_v2.test_signatures_checked_for_syncs.html#pulp_smash.tests.rpm.api_v2.test_signatures_checked_for_syncs.RequireValidKeyTestCase.test_unsigned_packages
- http://pulp-smash.readthedocs.io/en/latest/api/pulp_smash.tests.rpm.api_v2.test_signatures_checked_for_syncs.html#pulp_smash.tests.rpm.api_v2.test_signatures_checked_for_syncs.RequireInvalidKeyTestCase.test_packages
- http://pulp-smash.readthedocs.io/en/latest/api/pulp_smash.tests.rpm.api_v2.test_signatures_checked_for_syncs.html#pulp_smash.tests.rpm.api_v2.test_signatures_checked_for_syncs.RequireAnyKeyTestCase.test_unsigned_packages
- http://pulp-smash.readthedocs.io/en/latest/api/pulp_smash.tests.rpm.api_v2.test_signatures_checked_for_syncs.html#pulp_smash.tests.rpm.api_v2.test_signatures_checked_for_syncs.AllowInvalidKeyTestCase.test_signed_packages
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.
Updated by ipanova@redhat.com over 8 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.
Updated by ipanova@redhat.com over 8 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.
Updated by Ichimonji10 over 8 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
Updated by ipanova@redhat.com over 8 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
Updated by Ichimonji10 over 8 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.
Updated by semyers over 8 years ago
- Status changed from 6 to CLOSED - CURRENTRELEASE
Updated by bmbouter over 7 years ago
- Has duplicate Story #217: As a user, I can restrict RPMs by signature added
As a user, units which don't pass the signature check are not imported
closes #1991