-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Support signature verification of signed merges from unsigned branches #2704
Comments
Short comment as I will be off from today on until after new year, but wanted to give this some attention so you know it has been looked at (by the person who wrote the initial signature verification implementation). First: thanks a lot for diving into the code and getting familiar with it before making the request. I think your enhancement request is valid, and can see why you want to have this. As this feature goes hand-in-hand with security, I need to give your proposed options some thought and map out what the impact of the difference between both of them is. To be continued... |
Finally had some time to dive into this @jstevans, thanks for waiting. I read a good long read about this topic, where your cheap proposal is also described. I think this implementation would be secure enough for what you want to achieve, and leave most of the heavy lifting to Git itself which saves us from technical complexity. I am however interested in why you think the other proposal is better, or what additional advantages it would offer. Given the fact that any manipulation of the merge commit would make it invalid. In addition to this, the feature should be opt-in, and either behind a boolean flag or as a 'validation mode' option (good luck coining it). |
Thanks for your attention, @hiddeco! In my case, either option would work. I'm not aware of a typical git scenario that's only supported by the better option -- perhaps it would have been better if I'd called it the general option. :) I think I agree that I prefer the cheaper option. The only case I can think for the better option is that, when hand-crafting commit objects, it would be nice not to have to figure out which parent commit chain is fully-signed. |
In that case do not bother but just pick the mode flag, this gives someone else the ability to add an additional mode if they deem it necessary for their use-case (without us ending up with a thousand boolean flags), and it keeps us free (for now) from complexity. |
Add a new flag that puts the `--gitVerifySignatures` behavior into one of two modes: * "all" (default) uses the existing `--gitVerifySignatures` behavior * "first-parent" uses the behavior described in issue fluxcd#2704, wherein only each first-parent of each commit from the tip of the flux branch are considered when verifying GPG signatures
Add a new flag that puts the `--gitVerifySignatures` behavior into one of two modes: * "all" (default) uses the existing `--gitVerifySignatures` behavior * "first-parent" uses the behavior described in issue fluxcd#2704, wherein only each first-parent of each commit from the tip of the flux branch are considered when verifying GPG signatures
Describe the feature
fluxd
when configured with--git-verify-signatures
accepts a signed commitA
as long as there is a completely-signed merge-base between that commit and flux's current revision.What would the new user story look like?
master
branch is unsigned.no-fast-forward
) merge commit onto ourrelease
branch.Expected behavior
fluxd
would allow that merge commit by reducing the current constraint from "All commits betweenHEAD..(fluxd_current_revision)
" to...--first-parent
when callinggit log
<<repo.CommitsBetween()
<<latestValidRevision
contains only signed commitsfluxd
's current revision. This could be done by reconstructing the git tree in code, and doing the walk fromHEAD
tofluxd_current_revision
while treating unsigned commits as dead-ends.The text was updated successfully, but these errors were encountered: