Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

Git commit signing #1394

Merged
merged 19 commits into from
Mar 4, 2019
Merged

Git commit signing #1394

merged 19 commits into from
Mar 4, 2019

Conversation

scjudd
Copy link
Contributor

@scjudd scjudd commented Sep 26, 2018

This adds support for flux to sign git commits made to the central config repo. This is important for security-minded users who want to make sure that all commits to the config repo come from an authenticated source, and is the first step towards the larger goal of optionally having flux only apply changes if the latest config has a valid signature from a trusted key.

git/gittest/repo.go Outdated Show resolved Hide resolved
Copy link
Member

@squaremo squaremo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a really thorough job and consequently, easy to review. Thank you.

I have entered a couple of queries as comments, but honestly I am happy for this to be merged and to work any of that stuff out afterwards. See what you think of the comments @scjudd; I'll aim to merge next week, either way.

docker/flux-entrypoint.sh Outdated Show resolved Hide resolved
docker/flux-entrypoint.sh Outdated Show resolved Hide resolved
@squaremo squaremo added the blocked-rebase PR needs a rebase before it can be reviewed/merged label Dec 4, 2018
@hiddeco
Copy link
Member

hiddeco commented Dec 4, 2018

@scjudd, this PR has been stale for a while now and we would like to get it merged!

Are you still interested in finishing it up or do you want me to take over the remaining part? In case of the latter, can you please flip the setting which lets us push to the PR branch? Thanks in advance 🏅

@squaremo
Copy link
Member

squaremo commented Dec 4, 2018

Rebased -- almost all the conflicts were trivial (unrelated changes that happened to touch adjacent lines). The exceptions:

  • in git/operations.go, there's a new argument introduced here for environment entries, while in master, there were changes to how environment entries were prepared. I've made those mesh again, in
    fe8046e.
  • I elided the changelog update, because in the meantime I decided it was better for people to just suggest the entry in their PR and for it to be collated later. The suggested entry is this:
- Flux is now capable of signing commits made to the user git repo
  [Git commit signing](https://github.com/weaveworks/flux/blob/master/site/git-commit-signing.md)

@squaremo
Copy link
Member

squaremo commented Dec 4, 2018

I'm implementing the "inline the GPG key import into fluxd" option; working in git-signing in this repo for now (i.e., it won't appear here yet)

@scjudd
Copy link
Contributor Author

scjudd commented Dec 4, 2018

Please feel free to finish this up, sorry for not being able to contribute recently! Thank you all!

@squaremo squaremo added blocked and removed blocked-rebase PR needs a rebase before it can be reviewed/merged labels Dec 11, 2018
@squaremo squaremo removed the blocked label Feb 12, 2019
@hiddeco hiddeco force-pushed the git-signing branch 5 times, most recently from ccfeab6 to 39b220b Compare February 14, 2019 14:32
@hiddeco hiddeco requested review from squaremo and 2opremio February 14, 2019 15:17
Makefile Outdated Show resolved Hide resolved
cmd/fluxd/main.go Show resolved Hide resolved
git/operations.go Outdated Show resolved Hide resolved
gpg/gpg.go Show resolved Hide resolved
gpg/gpgtest/gpg.go Show resolved Hide resolved
Copy link
Member

@squaremo squaremo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry to take a while to get to this. The exec struct thing (see review comment) I think is worth changing. The package aliases will most likely be reformatted away by another commit -- were they introduced by tooling?

git/operations.go Outdated Show resolved Hide resolved
daemon/daemon.go Outdated Show resolved Hide resolved
@hiddeco hiddeco force-pushed the git-signing branch 4 times, most recently from 15e80fd to c0e34ad Compare February 28, 2019 16:22
@squaremo squaremo requested a review from 2opremio February 28, 2019 16:36
@squaremo
Copy link
Member

Looks good -- I am evaluating at a second or third derivative, now -- Fons, mind having a fresh look?

@2opremio
Copy link
Contributor

Will do! I am off today though.

git/operations.go Outdated Show resolved Hide resolved
git/working.go Outdated Show resolved Hide resolved
@2opremio
Copy link
Contributor

2opremio commented Mar 1, 2019 via email

scjudd and others added 19 commits March 2, 2019 00:11
This is needed in order to set GNUPGHOME during testing.
Currently needed only for testing, since we will be creating a temporary
GPG keyring and will need to be able to signal that it should be used
for signed commits.
We will be testing that this value equals the expected signing key.
Accidentally left this in from a previous iteration. Derp.
The working checkout already holds the configuration for git, which
includes the key that should be used when doing signing operations with
git.

Default to the one in the configuration but leave the option open to
pass along a different key for specific operations.
Besides code reduction this gives more control to the user as they can
now set an alternative path where we import their precious keys. In
theory they could even skip the import and just configure the GNUPGHOME
to a directory pre-loaded with keys.
Copy link
Contributor

@2opremio 2opremio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@

@hiddeco hiddeco merged commit 8c82f10 into fluxcd:master Mar 4, 2019
@hiddeco
Copy link
Member

hiddeco commented Mar 4, 2019

@scjudd it only took us 5 months but this has finally landed in master. Thanks a lot for your initial work on this, much appreciated 💯 🥇

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants