-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Commit Signature Verification #8848
Commit Signature Verification #8848
Conversation
Thank you for the PR. This provides an avenue to have verified updates. As you noted, it might need to support multiple and other kinds of keys in practice. |
@tomberek Thanks!
Do you have thoughts on how that should look like? I'm not sure gpg-signatures are equally easily implementable so I'd rather stick to ssh keys for now and regard other fetchers and gpg signature verification (which in my opinion is undeniably desierable, esspecially for prebuild binaries) as out-of-scope. |
4120542
to
ab36171
Compare
ab36171
to
a26e77d
Compare
Triaged in Nix team meeting:
|
fdaf84d
to
9d8163d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot @VwCSXg for this high-quality PR. A few superficial suggestions, but in essence this is solid. Global change requests:
- Please wrap the new behavior in a new experimental feature (e.g.
git-verify-signatures
, feel free to find a suitable name) to leave freedom to iterate on the design before committing to it in the long term. Link to the experimental feature from the documentation of the newfetchGit
parameters (check the manual for samples). - Please add a release note in
rl-next.md
linking to the builtin and the experimental feature.
Approving already, please ping maintainers when you're done with the fixups.
src/libexpr/flake/flake.cc
Outdated
std::vector<nix::fetchers::PublicKey> publicKeys; | ||
parsePublicKeysList(state, attr, publicKeys); | ||
attrs.emplace(state.symbols[attr.name], publicKeys_to_string(publicKeys)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@VwCSXg and me are both not sure if this is a sustainable design to add parameters to the input this way, but there seems no obvious other way. @edolstra @roberth do you think it makes sense or do you have other ideas? Since we're guarding all of this by an experimental feature we can still adjust in the future, so no pressure to answer it now.
b1c0e79
to
6c509e1
Compare
3979a64
to
41cf52d
Compare
I think this is ready now.
|
I'll need another few days to get back to it due to other priorities. If I don't react by end of next week and no one else does the final pass, please ping me @bootrhetoric. |
41cf52d
to
53efc65
Compare
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Committed the documentation nits myself. Very good, many thanks for the collaboration!
Ugh, there are merge conflicts now. Sorry, you'll have to take another close look since one upstream change interferes with yours.
This implements the git input attributes `verifyCommit`, `keytype`, `publicKey` and `publicKeys` as experimental feature `verified-fetches`. `publicKeys` should be a json string. This representation was chosen because all attributes must be of type bool, int or string so they can be included in flake uris (see definition of fetchers::Attr).
This adds publicKeys as an optional fetcher input attribute to flakes and builtins.fetchGit to provide a nix interface for the json-encoded `publicKeys` attribute of the git fetcher. Co-authored-by: Valentin Gagarin <[email protected]>
This adds simple tests of the commit signature verification mechanism of fetchGit and its flake input wrapper. OpenSSH is added to the build dependencies since it's needed to create a key when testing the functionality. It is neither a built- nor a runtime dependency.
34f9d8e
to
2719327
Compare
Doesn't this block migrating to libgit2? In that case, I would be in favor of reverting this since using libgit2 is more of a priority than signature verification. |
Oh, I wasn't aware of your work on this, but I don't see why it would be a block: |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/git-verify-in-band-commit-verification/38991/1 |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/bachelors-thesis-on-expression-update-security/50738/1 |
I don't have a lot of experience developing C++ and am completly new to this project so I'd be really grateful for feedback on this.
Motivation
When fetching a derivation from a git repository it is desirable to verify that the commit was made by the code owner and not altered by the git server.
Commit signatures are a git-native way of verifying this.
This can be useful for
Context
Mainly #2849, although this implementation is only restricted to the
git
fetcher.Since this can also be used for source code signature verification: #613 and https://discourse.nixos.org/t/any-interest-in-checkings-signatures-while-building-packages/8918
This is much simpler and more limited than what NixOS/rfcs#100 suggests since this PR is only about verifying the current commit with a provided public key.
Implementation
To implement this change I added
verifyCommit
,keytype
,publicKey
andpublicKeys
as an additional git input attributes.To avoid having unverified fetches cached and substituted for verified fetches, I added the attributes to
unlockedAttributes
as well aslockedAttributes
. I'm still not certain on how the caching exactly works and it could be improved slightly by having verified fetches store a cache entry for unverified fetches as well, but I'm not sure it is worth the trouble.publicKeys
, as its supposted to unambiguously define a list of public keys, is a parsed as a json string. I did not implement it as a list since those can't be given directly as part of a flake uri. The json can, provided it is percent-encoded. The json string is also used for cache lookups.It can be given to fetchGit and as flake input attribute in the form of a normal list of attributes, which is then turned into a json string by the flake or fetchTree parser. This is immediately reparsed to
std::vector<publicKey>
by the fetch-method ingit.cc
, which feels hacky to me, but I didn't find a better way to do this while still keeping the current seperation of concerns betweenflake.cc
andgit.cc
.To keep this PR small I consider the following out-of-scope
verifyCommit
is implemented)Checklist for maintainers
Maintainers: tick if completed or explain if not relevant
tests/**.sh
src/*/tests
tests/nixos/*
Priorities
Add 👍 to pull requests you find important.