[Pulp-dev] Unblocking SigningService hash check PR

[Brian Bouterse]

At the pulpcore meeting we noticed the signing service PR [0] had stalled
and instead we needed to work towards the next-steps identified in the
video call and discussed on this thread. I closed that PR with a note
asking that we focus on the next steps we discussed in the meeting (also
linked to [0]). I can help review and guide that contribution if someone is
interested.

[0]: https://github.com/pulp/pulpcore/pull/659#issuecomment-693658607



On Tue, Jun 30, 2020 at 4:48 PM Brian Bouterse <bmbouter at redhat.com> wrote:

> Sorry for the long delay in reply. Thank you for the email. I also want to
> unblock that PR. I put some replies in line.
>
> On Wed, Jun 24, 2020 at 7:03 AM Quirin Pamp <pamp at atix.de> wrote:
>
>> Hi,
>>
>>
>> However we decide to continue with the SigningService topic in the medium
>> and longrun, I wanted to have one more go at unblocking the following PR in
>> the short run:
>>
>>
>> https://github.com/pulp/pulpcore/pull/659
>>
>>
>> Currently, this PR issues a warning whenever the hash of a signing
>> service script has changed on disk (compared to when it was first
>> validated).
>>
>>
>> I think we are all in agreement that this is a bad compromise between
>> doing nothing at all (since the script might have changed for legitimate
>> reasons), and issuing a full on Error in cases where things are broken.
>>
>>
>> Yes I believe all parties agree the warning is the compromise no-one
> likes.
>
>>
>> My proposal is the following:
>>
>>
>> Instead of issuing a warning, a changed hash value on disk would trigger
>> an automatic re-validation of the script on disk.
>>
>> If the validation fails, it will throw a hard error (which would
>> certainly be the correct course of action for a script that does not
>> perform what the SigningService promises).
>>
>> If the validation succeeds, the SigningService is updated with the new
>> hash value, and everything continues as it nothing had happened (we just
>> assume the script was changed for legitimate reasons).
>>
> Overall this sounds ok to me, but I want to confirm, is the validation
> this code does?
> https://github.com/pulp/pulpcore/blob/bce4bee8124502ecd183fc664b904f5af5be97db/pulpcore/app/models/content.py#L453-L490
>
> If so, moving the `public_key` and ^ `signing service` attributes from
> AsciiArmoredDetachedSigningservice in pulp_rpm to SigningService in
> pulpcore would be a good first step. I believe this will need to happen
> first and is consistent with the last discussion of our video call. This
> would be a good first step in its own PR I think.
>
>
>> The only thing I can come up with where this approach might be
>> problematic, is if users want to have different versions of the signing
>> service script on different workers (for some reason).
>>
>> However, in such cases it would still be possible to work around the
>> problem by having a single signing service script call a secondary script
>> that differs on different workers.
>>
> If each script has its own public key it will fail. If each script on each
> worker with a different sha256 shares one public key it would be
> revalidated-and-modified each time it's used, which is strange but ok. Also
> I don't think this use case is really valid so I'm ok with this.
>
>>
>> If you are worried that the possibility of such a workaround defeats the
>> whole purpose of hashing the script in the first place, consider the
>> following:
>>
>> This is not intended as a security feature against some kind of malicious
>> attacker scenario, it is intended to provide some more meaningful error
>> reporting, for operational mistakes.
>> In this context I almost consider it a bonus if Sysadmin users who want
>> to do something rather unusual and complicated (different signing service
>> scripts on different workers) are forced to think about this carefully.
>>
> The use case doesn't resonate with me, but that's ok. If others see value
> in it, it doesn't harm existing use cases, and ya'll are willing to
> contribute it, let's do this.
>
> Here is one idea I want to pitch as a possible counterproposal:  What if
> we don't store the sha256 and instead, we perform validation on the signed
> data all the time? The sha256 is nice, but it won't catch the case when the
> script is interacting with a key server, and that key rotates but the
> script doesn't. In that case the signatures will still be returned, pulp
> will be unaware, no error will occur, yet it should. Validating data in all
> cases I think would get us everything the sha256 script proposes and even
> more.
>
> What do you think about this as an option?
>
>>
>>
>> Where to go from here:
>>
>> If we can get some kind of agreement that we would be willing to merge
>> the version of the above PR that I have proposed, I would ask Manisha to
>> make the relevant changes and they could be reviewed and merged.
>> This would not prevent us from taking SigningServices into an entirely
>> different direction in the future.
>>
> Agreed, but what do you think about moving the public_key and validate
> methods to SigningService first?
>
>
>> thanks,
>> Quirin (quba42)
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/pulp-dev/attachments/20200916/8cc0f893/attachment.htm>