Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Do not put the subscription-manager password onto the command line. (#…
…492) * Do not put the subscription-manager password onto the command line. Fixes CVE-2022-0852 Passing values on the command line is insecure. With this change, the rhsm password is passed interactively to subscription-manager instead of being passed on the commandline when we shell out to it. The structure of this change deserves a bit of description. Previously, we called one function to assemble all of the information needed to invoke subscription-manager and then returned that as a string that could be run as a command line. We called a second function with that string to actually run the command. To send the password interactively, we need to stop adding the password to the string of command line arguments but it still makes sense to keep the code that figures out the password together with the code which finds the other command line args. So it makes sense to keep a single function to do that but return the password and other args separately. We could use a dict, a class, or a tuple as the return value from the function. That doesn't feel too ugly. But then we need to pass that structure into the function which takes care of invoking subscription-manager on the command line and that *does* feel ugly. That function would have to care about the structure of the data we pass in (If a tuple, what is the order? If a dict, what are the field names?, etc). To take care of this, we can make the data structure that we return from assembling the data a class and the function which calls subscription-manager a method of that class because it's quite natural for a method to have knowledge of what attributes the class contains. Hmm... but now that we have a class with behaviours (methods), it starts to feel like we could do some more things. A function that fills in the values of a class, validates that the data is proper, and then returns an instance of that class is really a constructor, right? So it makes sense to move the function which assembles the data and returns the class a constructor. But that particular function isn't very generic: it uses knowledge of our global toolopts.tool_opts to populate the class. So let's write a simple __init__() that takes all of the values needed as separate parameters and then create an alternative constructor (an @classmethod which returns an instance of the class) which gets the data from a ToolOpt, and then calls __init__() with those values and returns the resulting class. Okay, things are much cleaner now, but there's one more thing that we can do. We now have a class that has a constructor to read in data and a single method that we can call to return some results. What do we call an object that can be called to return a result? A function or more generically, in python, a Callable. We can turn this class into a callable by renaming the method which actually invokes subscription-manager __call__(). What we have at the end of all this is a way to create a function which knows about the settings in tool_opts which we can then call to perform our subscription-manager needs:: registration_command = RegistrationCommand.from_tool_opts() return_code = registration_command() OAMG-6551 #done convert2rhel now passes the rhsm password to subscription-manager securely. * Modify the hiding of secret to hide both --password SECRET and --password=SECRET. Currently we only use it with passwords that we are passing in the second form (to subscription-manager) but catching both will future proof us in case we use this function for more things in the future. (Eric Gustavsson) * Note: using generator expressions was tried here but found that they only bind the variable being iterated over at definition time, the rest of the variables are bound when the generator is used. This means that constructing a set of generators in a loop doesn't really work as the loop variables that you use in the generator will have a different value by the time you're done. So a nested for loop and if-else is the way to implement this. * Add global_tool_opts fixture to conftest.py which monkeypatches a fresh ToolOpts into convert2rhel.toolopts.tool_opts. That way tests can modify that without worrying about polluting other tests. * How toolopts is imported in the code makes a difference whether this fixture is sufficient or if you need to do a little more work. If the import is:: from convert2rhel import toolopts do_something(toolopts.tool_opts.username) then your test can just do:: def test_thing_that_uses_toolopts(global_tool_opts): global_tool_opts.username = 'badger' Most of our code, though, imports like this:: # In subscription.py, for instance from convert2rhel.toolopts import tool_opts do_something(tool_opts.username) so the tests should do something like this:: def test_toolopts_differently(global_test_opts, monkeypatch): monkeypatch.setattr(subscription, 'tool_opts', global_tool_opts) * Sometimes a process will close stdout before it is done processing. When that happens, we need to wait() for the process to end before closing the pty. If we don't wait, the process will receive a HUP signal which may end it before it is done. * But on RHEL7, pexpect.wait() will throw an exception if you wait on an already finished process. So we need to ignore that exception there. lgtm is flagging some cases where it thinks we are logging sensitive data. Here's why we're ignoring lgtm: * One case logs the username to the terminal as a hint for the user as to what account is being used. We consider username to not be sensitive. * One case logs the subscription-manager invocation. We run the command line through hide_secrets() to remove private information when we do this. * The last case logs which argument was passed to subscription-manager without a secret attached to it. ie: "--password" is passed to subscription-manager without a password being added afterwards. In this case, the string "--password" will be logged. Testing farm doesn't have python-devel installed. We need that to install psutil needed as a testing dependency. Co-authored-by: Daniel Diblik <[email protected]>
- Loading branch information
