[Pulp-dev] Unblocking SigningService hash check PR

[Brian Bouterse]

I finally got around to outlining the next steps we discussed.

# The two stories that should be done next
https://pulp.plan.io/issues/7700
https://pulp.plan.io/issues/7701

# I closed this story because we don't have a plan for it. We can reopen it
later when we do
https://pulp.plan.io/issues/6291

On Thu, Sep 17, 2020 at 4:38 AM Quirin Pamp <pamp at atix.de> wrote:

> The result of our discussion was to pursue a (somewhat) different
> approach  to the one from the PR.
> It therefore makes sense to close the PR.
>
> I am a bit fuzzy on what the next steps were (adding a public key to the
> SigningService base class?), but I suppose I will just have to re-watch the
> meeting.
> I also get the feeling, continuing SigningService work isn't at the top of
> anyone's To-Do list right now. We will see who blinks first.
> ------------------------------
> *From:* Brian Bouterse <bmbouter at redhat.com>
> *Sent:* 16 September 2020 22:55
> *To:* Quirin Pamp <pamp at atix.de>
> *Cc:* pulp-dev at redhat.com <pulp-dev at redhat.com>
> *Subject:* Re: Unblocking SigningService hash check PR
>
> 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/20201015/8c97e18c/attachment.htm>