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
Do not put the subscription-manager password onto the command line. #492
Conversation
Codecov Report
@@ Coverage Diff @@
## main #492 +/- ##
==========================================
+ Coverage 82.46% 83.65% +1.19%
==========================================
Files 16 16
Lines 2258 2319 +61
Branches 383 403 +20
==========================================
+ Hits 1862 1940 +78
+ Misses 331 314 -17
Partials 65 65
Continue to review full report at Codecov.
|
|
This pull request introduces 3 alerts and fixes 1 when merging 18edac5 into 23fe45a - view on LGTM.com new alerts:
fixed alerts:
|
|
I believe the lgtm errors are false positives but I would appreciate it if someone else would take a look as lgtm is flagging them as security issues (revealing of private data). |
| class PexpectSizedWindowSpawn(pexpect.spawn): | ||
| # https://github.com/pexpect/pexpect/issues/134 | ||
| def setwinsize(self, rows, cols): | ||
| super(PexpectSizedWindowSpawn, self).setwinsize(rows, 120) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're removing this workaround in #475 for 0.27 and won't fix it for 0.26
|
@abadger was there any difference between this and the patch? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The LGTM warnings seem like false positives
There were quite a few conflicts (there have been changes from Rodolfo and Andrew which were merged after the patch was created) which I had to resolve. (I have asked both of them to take a look at this PR specifically to see whether their changes are still intact.) |
|
This pull request introduces 3 alerts and fixes 1 when merging 12dc446 into 23fe45a - view on LGTM.com new alerts:
fixed alerts:
|
|
This pull request introduces 3 alerts and fixes 1 when merging c2a1150 into 23fe45a - view on LGTM.com new alerts:
fixed alerts:
|
|
This pull request introduces 3 alerts and fixes 1 when merging 68e7ad8 into 23fe45a - view on LGTM.com new alerts:
fixed alerts:
|
68e7ad8
to
085fb11
Compare
|
This pull request introduces 3 alerts and fixes 1 when merging 085fb11 into 23fe45a - view on LGTM.com new alerts:
fixed alerts:
|
Looking at the changes right now. |
|
This pull request introduces 3 alerts and fixes 1 when merging b26c1c6 into 23fe45a - view on LGTM.com new alerts:
fixed alerts:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@abadger most of the changes I proposed here are more aesthetic ones, thus, feel free to accept or deny them!
In an overral, from what I remember from my code in subscription.py, everything looks fine with the modifications and merges you did, don't think you missed anything!
|
Note: it was not an approval, I selected the |
|
This pull request introduces 3 alerts and fixes 1 when merging 7cbe6a8 into 37b3a00 - view on LGTM.com new alerts:
fixed alerts:
|
|
This pull request introduces 3 alerts and fixes 1 when merging 0bf9ebb into 37b3a00 - view on LGTM.com new alerts:
fixed alerts:
|
0bf9ebb
to
5671a94
Compare
|
This pull request introduces 3 alerts and fixes 1 when merging 5671a94 into 37b3a00 - view on LGTM.com new alerts:
fixed alerts:
|
5671a94
to
e84faff
Compare
|
Rebased to pick up the fixes for ubi8 |
|
This pull request introduces 3 alerts and fixes 1 when merging e84faff into 68bf0ac - view on LGTM.com new alerts:
fixed alerts:
|
|
This pull request introduces 3 alerts and fixes 1 when merging 29ab9b8 into 68bf0ac - view on LGTM.com new alerts:
fixed alerts:
|
|
This pull request introduces 3 alerts and fixes 1 when merging 36e0e98 into a921086 - view on LGTM.com new alerts:
fixed alerts:
|
36e0e98
to
e9816db
Compare
|
This pull request introduces 3 alerts and fixes 1 when merging e9816db into a921086 - view on LGTM.com new alerts:
fixed alerts:
|
|
This pull request introduces 3 alerts and fixes 1 when merging 34fcd31 into a921086 - view on LGTM.com new alerts:
fixed alerts:
|
|
All tests have passed, @bocekm |
|
@abadger +1 for that toolopts fixture. |
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 a comment about activation_key being insecure for now. We can fix this once subscription-manager implements https://issues.redhat.com/browse/ENT-4724 (Eric Gustavsson) Unittest enhancements for the subscription RegisterCommand change: * 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) * Add unittests for utils.run_cmd_in_pty() * Add unittests for the subscription.RegistrationCommand object. * subscription_test.py::test_hide_secrets was expanded to test without equals sign * Flaky test appeared within systeminfo_test.py::test_generate_rpm_va due to changes to tool_opts in other tests. A temporary fix is added until the whole test class is changed from unittest to pytest * Fix failing unittests for RegistrationCommand * 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. * Add integration test Check for cve fixes, add 'psutil' to install-testing-depends playbook * Fix Pexpect.spawn truncating lines on RHEL7. Along with a comment on why the change fixes the bug.
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.
* Use a regular expression to find either "Password: " or "password: " when waiting for subscription-manager to prompt for the rhsm password. * Fix unittests that were using Mock.called_once_with(). That doesn't exist so the Mock() was translating it to a function that did nothing. Instead, use assert_called_once_with() which will assert if the Mock object was not called with the proper parameters.
Now that we know the cve number, use it in the name of the integration test.
Add docstrings for the RegistrationCommand.args properyy and hide_secrets() function
…aks. gitleaks scans the repository for passwords which might have been committed by accident. Adding EXAMPLE to the password string should allow gitleaks to recognize this as test data rather than a valid credential.
Testing farm doesn't have python-devel installed. We need that to install psutil needed as a testing dependency. Edit test_user_response, as one of the child.expect_exact() is not configured right.
Would need to be made a bit more generic before making it available to all tests.
gitleaks continues to warn about passwords in the repo. Use .gitleaks to specify that the passwords in the test file are not valid and should not be warned about.
34fcd31
to
960490a
Compare
|
This pull request introduces 3 alerts and fixes 1 when merging 960490a into 8658831 - view on LGTM.com new alerts:
fixed alerts:
|
…amg#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 <ddiblik@redhat.com>
…amg#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 <ddiblik@redhat.com>
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::
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 a comment about activation_key being insecure for now. We can fix
this once subscription-manager implements https://issues.redhat.com/browse/ENT-4724
(Eric Gustavsson)
Unittest enhancements for the subscription RegisterCommand change:
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::
Most of our code, though, imports like this::
so the tests should do something like this::
Add unittests for utils.run_cmd_in_pty()
Add unittests for the subscription.RegistrationCommand object.
subscription_test.py::test_hide_secrets was expanded to test without
equals sign
Flaky test appeared within systeminfo_test.py::test_generate_rpm_va
due to changes to tool_opts in other tests. A temporary fix is added
until the whole test class is changed from unittest to pytest
Fix failing unittests for RegistrationCommand
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.
an already finished process. So we need to ignore that exception
there.
Add integration test Check for cve fixes, add 'psutil' to
install-testing-depends playbook
Fix Pexpect.spawn truncating lines on RHEL7.
Along with a comment on why the change fixes the bug.
Fixes: https://issues.redhat.com/browse/RHELC-432