Add warning for SigningServirce if signing script changes #659
Conversation
|
Attached issue: https://pulp.plan.io/issues/6291 |
Manisha15
force-pushed
the
script_updation
branch
2 times, most recently
from
ae31dfe
to
a5e6435
Compare
| old_name='sha_256', | ||
| new_name='sha256', | ||
| ), | ||
| ] |
There was a problem hiding this comment.
Can you redo the migration as one?
pulpcore/app/models/content.py
Outdated
| @@ -412,6 +414,9 @@ def sign(self, filename): | |||
| stderr=subprocess.PIPE, | |||
| ) | |||
|
|
|||
| if self.sha256 != self.hash_value(self.script): | |||
| warnings.warn('Provided signing script does not match original signing script', Warning) | |||
There was a problem hiding this comment.
What is the purpose of the Warning argument here?
There was a problem hiding this comment.
Warning is a preexisting class, if no argument is used then it is by default UserWarning. For eg,
>>> warnings.warn('Provided signing script does not match original signing script.')
<string>:1: UserWarning: Provided signing script does not match original signing script.
>>>warnings.warn('Provided signing script does not match original signing script.', Warning)
<string>:1: Warning: Provided signing script does not match original signing script.
There was a problem hiding this comment.
Perhaps we should use _logger.warning here? (As is already the case in other places in pulpcore.)
Manisha15
force-pushed
the
script_updation
branch
from
a5e6435
to
6e538e8
Compare
pulpcore/app/models/content.py
Outdated
| @@ -412,6 +414,9 @@ def sign(self, filename): | |||
| stderr=subprocess.PIPE, | |||
| ) | |||
|
|
|||
| if self.sha256 != self.hash_value(self.script): | |||
| warnings.warn('Provided signing script does not match original signing script', Warning) | |||
There was a problem hiding this comment.
Perhaps we should use _logger.warning here? (As is already the case in other places in pulpcore.)
| @@ -441,8 +446,19 @@ def save(self, *args, **kwargs): | |||
| Save a signing service to the database (unless it fails to validate). | |||
| """ | |||
| self.validate() | |||
| self.sha256 = self.hash_value(self.script) | |||
There was a problem hiding this comment.
I think this should be set before the call to self.validate()?
Won't validate call sign which will check the hash and be confused if it has not been set yet?
| @@ -424,7 +429,7 @@ def sign(self, filename): | |||
|
|
|||
| def validate(self): | |||
| """ | |||
| Ensure that the external signing script produces the desired beahviour. | |||
| Ensure that the external signing script produces the desired behaviour. | |||
Manisha15
force-pushed
the
script_updation
branch
3 times, most recently
from
5b9ea24
to
a52257a
Compare
SigningService issues a warning if the signing script has changed on disk fixes #6291 https://pulp.plan.io/issues/6291
Manisha15
force-pushed
the
script_updation
branch
from
a52257a
to
2c8871c
Compare
|
Since the tests have now turned green for the first time, could we move this towards final review? @mdellweg @dkliban As far as I can tell this is now the "minimal consensus" version of this change. There have been voices (@mibanescu) who would prefer a hard error in combination with some kind of update mechanism (?) for existing singing services. (I am personally sympathetic towards a hard error. 😉) However, anything further could still be done in a followup task. Opinions? |
|
This is a very nice piece of work. |
|
Maybe the new behaviour could be: If there is a new script (changed hash), the validate function is called, and if that fails then there is a hard error. If it changed and validates then only a warning? A hard error for a script that fails to validate is surely uncontroversial. Automatically updating the hash in the SigningService object is more problematic, since there may be different versions of the script on different workers. (Which also strikes me as an error state, but one that is more difficult to diagnose). |
|
If we verify the script only immediately before using it, we can just as well let it fail while we use it. I don't see the benefit there. |
|
I'm late to review this, I'm sorry about that. I think there are possible legitimate use cases where the SigningService script could have during normal operation. Here are two examples:
I'm also not sure this is that helpful in increasing the security of Pulp. If an attacker has already compromised the SigningServiceScript itself you probably also have the ability to read the credentials to connect to the database. |
|
Adding WIP label pending pulp-dev discussion. |
|
A while ago we had a meeting about changes needed for the signing service. It was kind of prompted by this PR because we knew we needed to unblock it. The recording is here: https://youtu.be/uecwUFJTWno Also there was some email discussion about next steps here: https://www.redhat.com/archives/pulp-dev/2020-June/msg00062.html and here: https://www.redhat.com/archives/pulp-dev/2020-June/msg00083.html I think we need to do those before we can move forward with this PR. This PR has been open for so long I think we need to close it, but my hope is that we can work towards the next steps discussed on the call. One that is done I think we can resolve this original goal of this PR pretty easily as built-in validation is part of the plan. I'd like to help provide review and a guidance earlier and better than I did with this PR. My apologies for that. |
SigningService issues a warning if the signing script has changed on disk
fixes #6291
https://pulp.plan.io/issues/6291
Please be sure you have read our documentation on creating PRs:
https://docs.pulpproject.org/contributing/pull-request-walkthrough.html