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 <bmbou...@redhat.com> Sent: 16 September 2020 22:55 To: Quirin Pamp <p...@atix.de> Cc: pulp-dev@redhat.com <pulp-dev@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 <bmbou...@redhat.com<mailto:bmbou...@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 <p...@atix.de<mailto:p...@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)
_______________________________________________ Pulp-dev mailing list Pulp-dev@redhat.com https://www.redhat.com/mailman/listinfo/pulp-dev
