RHEL-2480: Do not create /root/.gnupg/ directory by accident

[m-horky]

All Pull Requests:

Check all that apply:

• Have you followed the guidelines in our Contributing document, including the instructions about commit messages?
• Is this PR to correct an issue?
• Is this PR an enhancement?

Complete Description of Additions/Changes:

GPG creates a directory $HOME/.gnupg/ every time it performs some operation. When run under root, but not manually (e.g. via subscription-manager Cockpit plugin), it tries to create and write to this directory, which causes SELinux denials.

This patch utilizes the --homedir argument GPG supports in order to move the home directory to a temporary directory for the time of the transaction. After the GPG action is performed, the directory is cleaned up.

You can verify this patch by performing the following actions:

1/ Create your own GPG key pair

~ $ gpg --full-generate-key
~ $ gpg --list-secret-keys --keyid-format long
~ $ (key identifier is on line `sec`, after the slash)
~ $ KEYID=9F2B2565A7CC100E  # (for example)
~ $ gpg --armor --export $KEYID > $HOME/.gnupg/my-key.pub.gpg

2/ Ensure you have created the egg zip file

~/insights-core $ bash build_client_egg.sh

3/ Sign the egg

~/insights-core $ gpg --detach-sign -u $KEYID --armor insights.zip

4/ Replace the official GPG key with you own.
5/ Run insights-client without --no-gpg or BYPASS_GPG=True.

---

Support for additional GPG keys was dropped from this PR. The crypto.py still supports it, this way it will be easier to add possibility to load multiple GPG keys later. See #3852 for example use case.

[m-horky]

Since this has not been merged, I'd like to ask a question here: should this be documented somewhere else than just in the commit message? This feels like something that will be useful.
cc @subpop @ahitacat @abadger

[abadger]

I'm not an insights committer but from past experience working with the gpgme library and, more recently, signature checking in convert2rhel, I support using --homedir. It's the only way that gnupg provides for applications to isolate their signature checking from the user's own gpg setup.

I'll make some comments about the usage of gpg here but the basic questions of whether insights will do this and whether the design fits into the code base will need to be done by someone on the insights team.

[Glutexo]

A little quick note for the steps to reproduce:

Key generation fails if pinentry is not installed.

$ gpg --full-generate-key
…
gpg: agent_genkey failed: No pinentry
Key generation failed: No pinentry

This can be mitigated by, obviously, installing pinentry.

# dnf install pinentry
$ gpg --full-generate-key 
…
public and secret key created and signed.
…

[Glutexo]

The public key that Insights Client uses to verify the egg by default, /etc/insights-client/redhattools.pub.gpg is in the binary OpenPGP format. If you use a GPG key in an armored ASCII format, GPG verification fails:

WARNING: GPG verification failed.  Not loading egg: insights.zip

To fix this, remove the --armor argument from the gpg --export command:

$ gpg --export $KEYID > $HOME/.gnupg/my-key.pub.gpg

[m-horky]

The public key that Insights Client uses to verify the egg by default, /etc/insights-client/redhattools.pub.gpg is in the binary OpenPGP format. If you use a GPG key in an armored ASCII format, GPG verification fails:

WARNING: GPG verification failed.  Not loading egg: insights.zip

@Glutexo On which system did you get this? It worked fine for me.

[Glutexo]

I wanted to verify that I signed the egg with my own key correctly, so I can be sure that the changes from this PR don’t interfere. There are two ways to do so that I want to share here:

• Change the hard-coded InsightsConstants.pub_gpg_path. This requires rebuilding the egg.
• Replace the public key in /etc/insights-client/redhattools.pub.gpg.

The latter is easier, because it doesn’t require rebuilding the egg. Also because the absolute path is composed from a base path and a file name, it is less trivial to point the constant to your key. Because the public key is in an insights-client folder, I’d assume no other software uses it and thus nothing other than the Insights Client should break.

Make sure you backup the original redhattools.pub.gpg. Note that either way of replacing the key makes the Core fail when verifying the official (downloaded and bundled) eggs signed with Red Hat’s real key.

[Glutexo]

I’d like to note that you can omit the --keyid-format long argument for gpg --export and use the long key printed on the second line of the sec section.

$ gpg --list-secret-keys
/home/insights/.gnupg/pubring.kbx
---------------------------------
sec   rsa3072 2023-11-06 [SC]
      BAF1C340B5DEA32A392D8CA8CA7F5625928C2F5D  # ← This is the long key.
uid           [ultimate] Štěpán Tomsa (Insights Client development test) <[email protected]>
ssb   rsa3072 2023-11-06 [E]

In this case, it is BAF1C340B5DEA32A392D8CA8CA7F5625928C2F5D.

[Glutexo]

@m-horky It’s a plain instalation of RHEL 9.2 with GPG (2.3.3), libcrypt (1.10.0) and Insights Client (3.1.7) from the installation DVD. See the commands below: if I export the key with --armor, GPG verification fails; it succeeds if I don’t use the argument.

$ ls -a /etc/insights-client
…
lrwxrwxrwx. 1 root root      49 Nov  7 00:26 redhattools.pub.gpg -> /home/insights/gnupg/insights-client-test.pub.gpg
…
$ gpg --list-secret-keys
…
sec   rsa3072 2023-11-06 [SC]
      BAF1C340B5DEA32A392D8CA8CA7F5625928C2F5D
…
$  gpg --armor --export BAF1C340B5DEA32A392D8CA8CA7F56
25928C2F5D > gnupg/insights-client-test.pub.gpg
$ sudo EGG=insights-core/insights.zip BYPASS_GPG=True insights-client --offline
WARNING: GPG verification failed.  Not loading egg: insights.zip
WARNING: GPG verification failed.  Not loading egg: /etc/insights-client/rpm.egg
$ gpg --export BAF1C340B5DEA32A392D8CA8CA7F56
25928C2F5D > gnupg/insights-client-test.pub.gpg
$ sudo EGG=insights-core/insights.zip BYPASS_GPG=True insights-client --offline
Starting to collect Insights data for localhost.localdomain
…

[Glutexo]

I spotted some questionable parts, that I think need to be fixed or at least defended. Because of that I chose “Request change”.

What I what to challenge, too, is that this pull request introduces quite more changes that could be reviewed and merged separately:

• the actual --homedir argument for gnupg;
• the new EXTRA_GPG_KEY environment variable;
• the new encapsulating crypto module;
• the new tests.

Only the first point actually fixes the original bug. Although I like the other changes, they are more of a refactoring. As I point out in one of my previous comments, it is possible to use an extra key even without adding explicit support for one.

[Glutexo]

Wow! I didn’t know that. Thank you! ❤️

[Glutexo]

I am thinking whether I like to have two separate commits for

• introducing the crypto module (a9c7a97)
• and making it being actually used (05c49b9).

If there was a separate pull request for the refactoring, which I would strongly prefer, the two commits are separated exaclty as they should be. Here, however, having more separate changes in a single PR, I feel slight temptation to use the commits to group changes that form a mergeable patch.

I learned how to filter changes by commits and it is possible to select a range of commits, too. The commit subjects are clear enough to do the filtering easily, so there is no need to change. Just sharing my thoughts out loud.

[Glutexo]

This looks like a dataclass. Dataclasses were introduced in Python 3.7 and Insights Client needs to support Python 2.6, but there are namedtuples that do exactly what this class implements.

Suggested change

	class CommandResult(object):
	"""Output of a command.
	Attributes:
	ok (bool): Result of an operation.
	return_code (int): Return code of the operation.
	stdout (str): Standard output of the command.
	stderr (str): Standard error of the command.
	_command: An optional reference to the object that created the result
	"""
	def __init__(self, ok, return_code, stdout, stderr):
	self.ok = ok
	self.return_code = return_code
	self.stdout = stdout
	self.stderr = stderr
	self._command = None
	def __str__(self):
	return (
	"<{cls} ok={ok} return_code={code} stdout={out} stderr={err}>"
	).format(
	cls=self.__class__.__name__,
	ok=self.ok, code=self.return_code,
	out=self.stdout, err=self.stderr,
	)
	from collections import namedtuple
	CommandResult = namedtuple("CommandResult", ["ok", "return_code", "stdout", "stderr", "command"])

I still doubt, however, that this abstraction is actually needed. Although the structure looks nice and clean, it increases complexity. It may be enough to just work with native Python shell command handling.

Moreover, it looks pretty low-level for something that should live in this specifict crypto module. There are other shell command calls in the codebase that may make use of this structure, if we wanted to keep it.

[m-horky]

I wouldn't call this an increased complexity. It just wraps the data, instead of passing them around in an anonymous dictionary.
I've renamed it to GPGCommandResult to make its existence more clear.

[Glutexo]

GPGCommandResult is definitely better than just plain CommandResult. Crypto ≠ GPG, so the module name is not enough and the GPG prefix is great. 👍🏻

I am not sure about the capitalization: whether to use GPGCommandResult or GpgCommandResult. I researched on the matter and there is no consensus. My personal taste would be Gpg, but lot of big projects use GPG. Let’s find out how acronyms are capitalized in the existing parts of the Client and stick to that.

This addressed only the naming. I have more thoughts on the actual code. I‘ll post them separately.

[Glutexo]

My suggestion was to remove that infrastructure from the crypto module as well. The difference is the larger amount of changes, increased complexity and support for a feature that is not being used (yet). The existing extra keys code in the crypto module can be trivially turned into a patch that would be part of a different pull request adding the feature as a whole.

Suggested change

	keys=[constants.pub_gpg_path],
	key=constants.pub_gpg_path,

[Glutexo]

As I mentioned above, I was thinking whether it wouldn’t be nice to be consistent to move constants.pub_gpg_path to the function signature as a default value of a new named argument gpg_key. I concluded, however, that it’s not the way I’d prefer: it would increase the amount of changes and introduce a piece of refactoring that doesn’t belong here. Such refactoring would go in a blind direction of adding an argument that is not used anywhere, just paving the path for the future. That is exactly what I objected to in my previous comment in this thread.

[Glutexo]

Just to verify: is this attribute use the private-marking underscore, because it is not a result of the command, but just an additional context? Your comment in an older conversation suggests so.

[m-horky]

Yes, it's an additional context.

[Glutexo]

GPGCommandResult is definitely better than just plain CommandResult. Crypto ≠ GPG, so the module name is not enough and the GPG prefix is great. 👍🏻

I am not sure about the capitalization: whether to use GPGCommandResult or GpgCommandResult. I researched on the matter and there is no consensus. My personal taste would be Gpg, but lot of big projects use GPG. Let’s find out how acronyms are capitalized in the existing parts of the Client and stick to that.

This addressed only the naming. I have more thoughts on the actual code. I‘ll post them separately.

[Glutexo]

The only symbol imported from crypto is verify_gpg_signed_file, whose name is pretty self-explanatory. It may not be entirely necessary to import the module just for the sake of explicitly stating the call as crypto.verify_gpg_signed_file.

Suggested change

	from . import crypto
	from crypto import verify_gpg_signed_file

[m-horky]

Well, this allows you to mock the function more easily. If we imported the function itself, Python would create a new object instead of a reference, forcing us to track down how it is implemented. This way there is only one source for the existence of the function implementation.
Plus including the module name makes it easier for the reader to understand where the function is coming from and any additional context it may have. You don't get that from a bare function.

Overall, that is subjective, we may not find an agreement on this detail.

[Glutexo]

If only the function was imported, the call would still make perfect sense even without the module name.

Suggested change

	result = crypto.verify_gpg_signed_file(
	result = verify_gpg_signed_file(

[Glutexo]

Although the constructor is as straightforward as it can be, it is still a lot of code for something as simple as a data structure.

We can’t use a Data Class, because they are not supported in Python 2.6. A namedtuple would, however do the trick on a single line:

GPGCommandResult = namedtuple("GPGCommandResult", ["ok", "return_code", "stdout", "stderr", "_command"])

There is however no way to have a different argument name (command without an underscore) and attribute name (_command with an underscore).

[Glutexo]

This is what a namedtuple could look like:

from collections import namedtuple
GPGCommandResult = namedtuple("GPGCommandResult", ["ok", "return_code", "stdout", "stderr", "command"])
result = GPGCommandResult(True, 0, "out", "", "echo out")

You can see that a single line definition (plus one import) do exactly what the entire class does.

[m-horky]

But we'd lose the documentation of the fields. The class construct allows us to properly describe what each value means, without having to rely on the name. Sure, the names are clear and descriptive, but the docstring provides type information and potentially additional context you just won't get from a named tuple.

[Glutexo]

It is possible to add the field documentation using inheritance:

class GPGCommandResult(namedtuple("GPGCommandResult", ["ok", "return_code", "stdout", "stderr", "command"]))
    """Output of a GPGCommand.

    Attributes:
        ok (bool): Result of an operation.
        return_code (int): Return code of the operation.
        stdout (str): Standard output of the command.
        stderr (str): Standard error of the command.
        command (str | None):
            An optional reference to the GPGCommand object that created the result
"""

Unfortunately, the __doc__ attribute is not writable in Python 2, so it’s not possible to do

GPGCommandResult.__doc__ = """Output of a GPGCommand.

    Attributes:
        ok (bool): Result of an operation.
        return_code (int): Return code of the operation.
        stdout (str): Standard output of the command.
        stderr (str): Standard error of the command.
        command (str | None):
            An optional reference to the GPGCommand object that created the result

It is rather clumsy, but still pretty straightforward and I’d say, Pythonic.

Here’s a nice article on named tuples on Real Python: https://realpython.com/python-namedtuple/

However, the Client codebase doesn’t contain many dataclass-like types and even any argument documentation for functions and methods is scarce. That’s not to say it’s a bad idea to have it (although I am not a big fan of typing), just omitting it here wouldn’t make it any worse.

[Glutexo]

Adding missing docstrings, unifying and standardizing it format, and documenting function arguments and return values can be done as a separate effort.

[Glutexo]

Even though this is only a data structure, there is still a lot of complexity.

[m-horky]

It's not that complex, it's just a format string ;)

[Glutexo]

A namedtuple comes with __str__ and __repr__ providing almost exactly the same string.

>>> from collections import namedtuple
>>> GPGCommandResult = namedtuple("GPGCommandResult", ["ok", "return_code", "stdout", "stderr", "command"])
>>> result = GPGCommandResult(True, 0, "out", "", "echo out")
>>> print "Result: %s" % (result,)
Result: GPGCommandResult(True, 0, 'out', '', 'echo out')

Although the format string itself is not that complex, the complexity I was refering to is the fact that it is completely redundant. An 8-line method of a custom class inevitably increases the code complexity, even though is as simple as this one. If you encounter it, you still need to read it and understand what it does and why it is there.

[m-horky]

Today's small update: I've fixed the "type hint" of GPGCommandResult._command (used to be incorrectly str).

[m-horky]

(Nothing changed, I only rebased it over the latest master)

[Glutexo]

Adding missing docstrings, unifying and standardizing it format, and documenting function arguments and return values can be done as a separate effort.

[Glutexo]

A description like this is not very helpful. It doesn’t seem to add any additional context. It may be still nice to have a docstring for every class, I’m just responding to your comment challenging that namedtuples can’t have (syntactically nice) docstrings.

The nomenclature doesn’t match here. The class name says “result”, but the documentation says “output”. “Output” sounds more like a string, like in “standard output”. Consistent either way is better, “result” is better than “output”.

Suggested change

	"""Output of a GPGCommand.
	"""Result of a GPGCommand.

GPGCommand is a class; GPGCommandResult is a result (or an output) of its evaluate method. It is, however, its only public method, so it makes some sense. Still, it doesn’t describe what the class represents, only where its instances are constructed. “Output of a GPG command” may fit better, still it’s not clear what an output (or a result) actually is and why it needs a data structure.

Suggested change

	"""Output of a GPGCommand.
	"""Result of a GPG command: its return code, standard and error output and optionally the original command. Returned by GPGCommand.evaluate.

This is just a non-perfect suggestion. It sounds redundant, but the whole docstring is, for something as simple as a dataclass. It should rather express why the class is there.

[m-horky]

It's fair the first line does not provide any more information over the class name. However, it is a good practice to add a one-liner describing the class, and this seems enough. Further information is available in the full docstring.

[Glutexo]

Since this is the only public method and it runs the command, wouldn’t it make sense to leverage the __call__ method?

class GPGCommand(object):
    …
    def __call__(self):
        """Run the command.
        …
        """
        …

gpg = GPGCommand(["--verify", signature, file], keys)
result = gpg()

[m-horky]

That's not something that is common in the code. I'd rather be explicit.

[Glutexo]

Is this attribute necessary? It is only filled once with return_code == 0. I see it’s accessed a few times, so it would be redundant to repeat the comparison logic. Also the attribute is nicely semantic. ❤️ Setting it manually, however, feels misplaced.

I suggest either computing the value in the constructor, or adding a @property method.

[m-horky]

Yes, however the evaluation may not be only done based on return code, some GPG command may require further inspection of the captured output, for example. These may not be the same.

[Glutexo]

The only thing I was really worried about was the command that didn’t have its arguments properly escaped. This is now fixed and every other comments I have only target the code style. I give my ✅ then, so we can merge.

As could be seen from my comments, I find the code too complex in regard to how simple fix it delivers. A minimal, almost one-line, patch could do the trick. And I believe anything else is a refactoring or tech debt crushing that is a separate effort.

[Glutexo]

@ptoscano, @ahitacat Did I remember correctly that you said that this pull request should not be merged right now? I can click the button, just assure me we want it. Thanks!

[ahitacat]

Yes, let's wait for QE verification before merging it. I have added the label that we used when the PR are in that status.

[m-horky]

This PR has been preverified and is ready to be merged, @xiangce.

[Glutexo]

Apparently, I don’t even have the necessary rights to merge. I remember having them.