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.

Sign up for GitHub

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

Jump to bottom

gpgcheck security and usability fixes #697

Merged
merged 5 commits into from
Jul 31, 2023
Merged

gpgcheck security and usability fixes #697

merged 5 commits into from
Jul 31, 2023

Conversation

evan-goode
Copy link
Member

@evan-goode evan-goode commented Jul 5, 2023 •
edited
Loading

As enumerated by Panu here: #617 (comment):

  • The gpgcheck option in the main config should not override the per-repo setting.
  • DNF 5 should not print the misleading message "Verifying PGP signatures" to the console when in fact GPG checking is off.
    • This message is not present as of bfedca8, but we may want to print a warning to the console anyway, ideally after the transaction is run so it's visible to the user
  • When key import fails, a more specific error message should be displayed. For the test case in Using a different security policy from DNF4? #617, DNF 4 prints "The certificate is expired: The primary key is not live" whereas DNF 5 just shows "Failed to import public key". The underlying issue is that DNF 5 swallows output from rpm-sequoia, mentioned here: Flaky gpg CI test #457.
  • Transaction should stop immediately after key import fails

Resolves #617.

@evan-goode evan-goode marked this pull request as ready for review July 10, 2023 20:19
@evan-goode
Copy link
Member Author

As for 9bc0ea2, I'm not sure adding the "Warning: skipped PGP checks" to signature_problems is the best place for it, if a transaction succeeded, it seems reasonable that signature_problems should be empty. But the warning should appear after the transaction so it's clear to the user, so it probably has to be stored somewhere on the transaction. Maybe a pgp_skipped_pkgs set or a get_transaction_warnings on Transaction?

@evan-goode evan-goode self-assigned this Jul 11, 2023
@j-mracek
Copy link
Contributor

It looks like that many tests fails on output check (stderr) where new warning is present (Warning: skipped PGP checks for 1 package(s).).

@praiskup praiskup mentioned this pull request Jul 19, 2023
Closed
@evan-goode
Copy link
Member Author

It looks like that many tests fails on output check (stderr) where new warning is present (Warning: skipped PGP checks for 1 package(s).).

Yeah, I want to get some feedback to make sure these changes are reasonable before rewriting all those tests to include the warning message.

The most important changes in this PR are 5ab4eec (Don't allow main gpgcheck=0 to override repo config) and 01c9867 (Include RPM logs in KeyImportError).

We may not want the warning when skipping GPG checks, which would certainly require a lot of changes to the tests.

m-blaha
m-blaha reviewed Jul 19, 2023
View reviewed changes
libdnf5/base/transaction.cpp
@@ -981,7 +979,7 @@ bool Transaction::Impl::check_gpg_signatures() {
signature_problems.push_back(
err_msg + import_repo_keys_result_to_string(ImportRepoKeysResult::ALREADY_PRESENT));
result = false;
continue;
break;
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure about all these breaks. Doesn't it mean that we will return to the user only the first signature problem instead of accumulating all of them?

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, the benefit is that the key import error will not be buried by dozens of failure messages for the individual packages. But maybe it is best to show all of the errors. Here is what the two different outputs look like: #617 (comment).

Copy link
Member

Choose a reason for hiding this comment

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

Oh, right. For larger transaction the new behaviour is definitely better because the root cause of the issue is more clearly visible.

m-blaha
m-blaha approved these changes Jul 19, 2023
View reviewed changes
Copy link
Member

@m-blaha m-blaha left a comment

Choose a reason for hiding this comment

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

Please, prepare a PR with tests adjustments before merging this, so it could be properly tested.

@j-mracek
Copy link
Contributor

I like the change, but I didn't spend enough time to provide an approval.

@j-mracek
Copy link
Contributor

When a patch for CI-dnf tests will be available I think we can merge it. Or from the second direction - the absence of patch of CI blocks a merge of this PR.

evan-goode added a commit to evan-goode/ci-dnf-stack that referenced this pull request Jul 21, 2023
@evan-goode
Add 'Warning: skipped PGP checks...' to expected outputs
d21013b 
Requires rpm-software-management/dnf5#697
Part of rpm-software-management/dnf5#617
@evan-goode evan-goode mentioned this pull request Jul 21, 2023
Merged
@evan-goode
Copy link
Member Author

When a patch for CI-dnf tests will be available I think we can merge it. Or from the second direction - the absence of patch of CI blocks a merge of this PR.

Thanks. CI PR here: rpm-software-management/ci-dnf-stack#1343

@evan-goode
Don't allow main gpgcheck=0 to override repo config
77146c2 
The main gpgcheck option should not override the per-repo setting. The
repo gpgcheck option is a child of the main gpgcheck option, so setting
the main gpgcheck option will still work, but we want the repo gpgcheck
option to have a higher priority.

The repo-specific gpgcheck option is checked by RpmSignature::check_package_signature.

Resolves one of the problems described in rpm-software-management#617.
@evan-goode
Display warning message when any PGP checks skipped
ba557d3
@evan-goode
Abort PGP checking immediately if any checks fail
09ab532
@evan-goode
Include RPM logs in KeyImportError
0b8eba0
@evan-goode
gpgcheck: fix unit tests after adding warning and SKIPPED
a1a0331
evan-goode added a commit to evan-goode/ci-dnf-stack that referenced this pull request Jul 26, 2023
@evan-goode
Add 'Warning: skipped PGP checks...' to expected outputs
34a0ce9 
Requires rpm-software-management/dnf5#697
Part of rpm-software-management/dnf5#617
@m-blaha m-blaha added this pull request to the merge queue Jul 31, 2023
Merged via the queue into rpm-software-management:main with commit 43eb2cd Jul 31, 2023
5 of 6 checks passed
m-blaha pushed a commit to rpm-software-management/ci-dnf-stack that referenced this pull request Aug 1, 2023
@evan-goode @m-blaha
Add 'Warning: skipped PGP checks...' to expected outputs
5fcdc98 
Requires rpm-software-management/dnf5#697
Part of rpm-software-management/dnf5#617
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@m-blaha m-blaha m-blaha approved these changes

@j-mracek j-mracek j-mracek approved these changes

Assignees

@evan-goode evan-goode

Labels
None yet
Projects
DNF team
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Using a different security policy from DNF4?
3 participants
@evan-goode @j-mracek @m-blaha