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.

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

installFromYum: give more detailed error messages on gpg errors #71

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ydirson
Copy link
Contributor

@ydirson ydirson commented Oct 11, 2023

This is a complement to the gpg-checking feature enabled for 8.3.

This is a continuation of #68 which got closed when xs8 was destroyed and cannot be reopened (thx github).

Covers:

  1. repo_gpgcheck:
    a. wrong system clock putting gpg key creation in the future, causing a yum crash (nothing special happens if the date of the signature is in the future ¯\_(ツ)_/¯)
    b. other yum crashes due to uncaught gpg exceptions (if any)
    c. lack of repomd signature (while repo_gpgcheck is in force)
    d. signature done by other key than the one in ISO ("repomd.xml signature could not be verified" ¯\_(ツ)_/¯)
  2. gpgcheck:
    a. RPM signed with unknown key
    b. unsigned RPM referenced by unsigned repomd (no-repo-gpgcheck)
    c. RPM re-signed with unknown key, unsigned repomd (no-repo-gpgcheck)
    d. RPM overwritten with another RPM signed with known key (diagnosed through hash but, same diag as 2.c)
    e. delsigned/resigned/etc RPM, unchanged repomd (same diag as 2.c/d)

Does not cover notably:

  • unsigned RPM referenced by (re)signed repomd

In some cases Yum does not give an error, but dies because of an uncaught exception, which makes this check quite brittle, but in the worst case if messages change, we still fallback to the original "Error installing packages" message.

Covers:
1. repo_gpgcheck:
  a. wrong system clock putting gpg key creation in the future, causing a
     yum crash (nothing special happens if the date of the signature is in
     the future ¯\_(ツ)_/¯)
  b. other yum crashes due to uncaught gpg exceptions (if any)
  c. lack of repomd signature (while repo_gpgcheck is in force)
  d. signature done by other key than the one in ISO ("repomd.xml signature
     could not be verified" ¯\_(ツ)_/¯)
2. gpgcheck:
  a. RPM signed with unknown key
  b. unsigned RPM referenced by unsigned repomd (no-repo-gpgcheck)
  c. RPM re-signed with unknown key, unsigned repomd (no-repo-gpgcheck)
  d. RPM overwritten with another RPM signed with known key (diagnosed
     through hash but, same diag as 2.c)
  e. delsigned/resigned/etc RPM, unchanged repomd (same diag as 2.c/d)

Does not cover notably:
  - unsigned RPM referenced by (re)signed repomd

In some cases Yum does not give an error, but dies because of an
uncaught exception, which makes this check quite brittle, but in the
worst case if messages change, we still fallback to the original
"Error installing packages" message.

Signed-off-by: Yann Dirson <[email protected]>
@@ -821,12 +821,59 @@ def installFromYum(targets, mounts, progress_callback, cachedir):
rv = p.wait()
stderr.seek(0)
stderr = stderr.read()
gpg_uncaught_error = 0
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the purpose of defining these variables and then raising the ErrorInstallingPackage exception later - could we not simply raise the exception directly at the point we find something in the log?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I cannot recall precisely, but:

  • (likely original reason) to keep the structure of the existing code; notably, there was already an exception raised with a generic message, and all that needed to be changed was the message
  • in line with that, it allows to keep the "yum exited" log, which would have to be duplicated, or wrapped with the exception in a helper (and delegating the raise to a helper function causes its own problems)
  • additionally it allows to make sure to use the most pertinent message, regardless of the matching order, which in turn is constrained by the order in which strings appear. Maybe not really necessary for this set of messages, but considering this is ad-hoc parsing of a program's output, including parsing of its crashes, it may be useful to retain this flexibility

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why instead of using a lot of flags variable you don't just set a string variable errmsg to save the specific error and use it? You could initialize it at default generic message and change accordingly based on the specific error.

@ydirson ydirson requested a review from alexbrett June 20, 2024 13:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants