Verify git commit signatures

[hiddeco]

Fixes #1683

This feature adds the --git-verify-signatures flag to the daemon.
When this flag is set the daemon will verify the signatures of the tag
and all commits it is working with, ensuring no unauthorized
modifications are synchronized.

To ensure the daemon always synchronizes a verified state a ratchet
mechanism was introduced to the loop. This mechanism moves the sync
HEAD to the latest valid revision after repository refreshes. During
sync runs this revision gets checked out and is applied on to the
cluster.

During modification actions, either automated or instructed by fluxctl
commands, the HEAD of the working clone is compared to the latest valid
revision. If these mismatch the commit is blocked and an error is
returned as the daemon can not be sure it is committing on top of a
verified state.

[squaremo]

This is shaping up well! I am not convinced by the imagePollLock -- see what you think of my reasoning in the comments ...

[jimmyjones2]

Hey @hiddeco, thanks for your work on this. I presume the admin would configure the operator with their trusted GPG keys?

How do you plan on handling the case when trust is revoked, eg. when someone leaves or their key is compromised? However you'd still want to trust their commits up to a certain point. In normal operation I guess this wouldn't be an issue, as flux would have checked for valid signatures at the time and moved passed that commit and wouldn't ever check again retrospectively. However if you wanted to redeloy from scratch, it would try and verify all commits from the first and fail. Could there be an option to only verify commits from a certain point to handle this and the scenario where someone want to migrate to enforcing signatures without rewriting their history to be all signed?

[hiddeco]

I presume the admin would configure the operator with their trusted GPG keys?

Correct, any key imported to Flux (or configured by setting the GNUPGHOME environment variable) is seen as trusted.

How do you plan on handling the case when trust is revoked, eg. when someone leaves or their key is compromised?

This is a good question, and I see two possible solutions

1. Is letting a human apply the insecure state and create a signed git tag, the daemon will then start synchronization from this point and all is well.

   I like this approach because it is secure and leaves no space for attack vectors. I dislike this approach because it requires manual interaction (although this process could be automated).

2. Is adding a flag like you suggested, to instruct the daemon to start verifying commits after a certain revision. This is super friendly to our users but also way easier to abuse.

If you put those two on a scale I am not sure what outweighs as the best option. Although the purist in me is telling me to go for option 1.

[2opremio]

refactor the gigantic doSync() method in to smaller elements

❤️

[jimmyjones2]

1. Is letting a human apply the insecure state and create a signed git tag, the daemon will then start synchronization from this point and all is well.

I like this approach because it is secure and leaves no space for attack vectors. I dislike this approach because it requires manual interaction (although this process could be automated).

I like this too. Considering people will already in the business of signing commits, doesn't seem that much more to ask to tag where to start from, especially as it'll be rare thing to need to do.

[hiddeco]

Gave my head some rest from this after having seen 3 revisions of the solution and I think we are almost there.

The ratchet mechanism introduced detaches the sync itself from the verification part and gives us one point of truth for validated state we are working with, which is really nice (thanks @squaremo for your input on this).

Besides a rebase, there are two things I would like to tackle before this feature is complete:

• Some listing commands need to work with the verified HEAD instead of the HEAD from the repository.
• Documentation, and especially documentation about disaster recovery and working with revoked and/or compromised keys. The outline for this is point 1 in my comment above.

[squaremo]

Comments, having followed the instructions:

The instructions are thorough and high quality. There's a couple of bits where it's not clear which key ID is supposed to be used (my understanding is that it's any of them -- but this isn't pointed out, only implied).

When I got to the end and wanted to kick start syncing with verified signatures. I signed the sync tag and pushed that, but the sync process still seems stuck at the first ever commit. I'm not clear on whether I did something wrong, or have the wrong expectation.

[hiddeco]

Tests are failing due to the Git version in the circleci/golang:1.10 image being too old (--format was introduced in Git 2.12) 😞

[squaremo]

For the record: I've happy with how this works and is explained in the docs, but haven't been through the implementation with a fine-toothed comb yet

[squaremo]

Overall this is in good shape, and thorough. I think type Sync struct may be more trouble than it's worth -- see if you agree with my comment on that.

I need to check on the possible issue with empty commit notifications that you mentioned IRL; other than that, I think only tidies (or counter arguments) are required here.

[hiddeco]

@squaremo addressed all your comments and verified (and fixed) my own theory in 4941138.

[squaremo]

Getting into nitty-gritty now -- Ii think the code is almost correct as it stands (there's one error check missing). I'm going to build it and run it, again

[squaremo]

I am happy this is explained well, works as explained, and the code is sound ⛰️. I would suggest git rebase -i to straighten out some of the twists and turns (and to bring it up to date with respect to master).