Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[RFC 0100] Sign commits #100
[RFC 0100] Sign commits #100
Changes from 3 commits
d491405
1bacb5b
2ed58e5
8647e1c
b86b32b
63cf9b7
cf51b1f
20609d3
05e1385
54236bd
a30e134
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
How do we handle expired keys? What does that mean for us? Do we accept that as "accident" that can happen and if so when would we reject commits that have expired keys on them but recently pushed to master?
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.
Expired keys can't be used for verifying trust at all. As I explained in another GitHub conversation, such accidents would mean the history needs to be rewritten, but since nobody would update to the untrusted commit, there is very little risk of breakage for downstream users.
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.
Does gpg not have the issue that it can expire? What happen if someone tries to build an old repository?
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.
All this logic is only relevant in the case of updating the commit used. If you specify a commit that's what you're going to get.
If you're trying to update from a repository with expired keys, then of course as you'd expect you do not get the same guarantees. How it will be handled depends on the exact implementation, you could e.g. simply print a warning and go on with the algorithm as usual, or you could err and require that the user manually find a new commit without expired keys.
I don't think the exact design is that important, since this is very much an edge case.
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.
I as a committer would refuse to follow this practice purely on the basis that it only serves an experiment that isn't standardized yet. If on the other hand we reach agreement with the Flakes RFC (pending) I wouldn't mind following this practice if this RFC turns out to be accepted by the active committer community.
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.
Will we produce documentation & tooling that allows 3rd-party repositories to implement the same system? IMO it would be cool if this ends up in a huge uptick of signed commits but I have my doubts. Purely due to GPG being not "the favorite thing" anymore.
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.
Yes, absolutely, that is my intention. I also think GPG is bad, however, we rely on PGP, not GPG. Sequoia will likely support signing commits soon (and if not I will implement it myself), after which none of us would have to use GPG hopefully.
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.
Yet it should give more information on how to handle the situation. Perhaps a pointer to a specification of the check that is being carried out (with optional manual steps for verification)?
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.
I agree that there could be more information, but I'm wondering what a good solution is. Dumping the raw information on the average user isn't a good idea IMO, since many likely won't understand it.
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.
The way you are showing the error is just purely technical right now. What does a nontechnical person care about signatures when trying to upgrade their system? If you look at how rustc does errors it (very often) comes with a pointer on how to find out more about the current situation. In our case that would be a references to the NixOS/Nixpkgs/... manual that describes actionable steps to take in this case. There will also be errors in our implementation so that situation should be handled well.
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.
You are right, thanks.
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.
I've changed this subtly, so that rather than giving you an error, it will just use the latest trusted commit. WIth this system the user doesn't need to know much. Even if an untrusted commit is pushed by accident, nobody would use it, so you could rewrite the untrusted history without any issues, meaning that downstream things will Just Work:tm:.