check_signature + SignatureCriterion interface #1452
Conversation
castedo
commented
Nov 22, 2024
castedo
commented
- check_signature method for Commit/Tag as alternative to verify
- SignatureCriterion interface to decouple crypto from git serialization
- single simple InvalidSignature exception generic to whatever criterion
- DRY re-impl for Commit/Tag verify using GpgSignatureCriterion
|
@jelmer Here is a potential start to a very new direction that can have lots of variations. Take your time to consider. I'm not in a rush to merge this. This, or something similar to it, I wager will resolve future headaches and confusion and give dev-users flexibility to make a variety of trade-offs depending on what kind of signature criterion and implementation makes sense for their particular application. Although verify_time is not used by git+gpg verification (I think) it is a full ssh-keygen feature and cgit passes a verify time to ssh-keygen. This PR is attached to the "pr/decouple_sig_check" branch which I will squash and rebase. If you want to fork and modify off the code in this PR, do it from the "decouple_sig_check" feature branch which I will not squash and rebase and will maintain full git history for short-term feature tracking and merging. I am using castedo/git-prepr. |
|
This direction probably enables a fairly easy and low-risk resolution to #1369. The solution can be "implement your own subclass of |
|
@jelmer And feel free to ask questions and suggest alternatives. In this branch I'm adopting and not adopting some Python approaches that are pretty new to me. Like is abc.ABC worth using? Single simple Exceptions rather than a plethora of fine-grained exceptions? I dunno. I've been tending towards whatever is simpler and more minimal. |
|
FYI, I am going to port the example code in #1448 to a SignatureCriterion subclass that I'll save in |
castedo
force-pushed
the
pr/decouple_sig_check
branch
3 times, most recently
from
88b0047 to
6676615
Compare
* check_signature method for Commit/Tag as alternative to verify * SignatureCriterion interface to decouple crypto from git serialization * single simple InvalidSignature exception generic to whatever criterion * DRY re-impl for Commit/Tag verify using GpgSignatureCriterion * ssh-keygen based SignatureCriterion classes in contrib
castedo
force-pushed
the
pr/decouple_sig_check
branch
from
6676615 to
9236005
Compare
|
And then here's an example of two pure-python criteria using sshsiglib: One criterion, |
| # <http://www.gnu.org/licenses/> for a copy of the GNU General Public License | ||
| # and <http://www.apache.org/licenses/LICENSE-2.0> for a copy of the Apache | ||
| # License, Version 2.0. | ||
| # fmt: off |
There was a problem hiding this comment.
A few reasons:
- make it so the CI checks don't fail
- not waste time bikeshedding about useless extreme esthetics of whitespace consistency
- make the code MORE readable rather than LESS readable (ruff format literally makes this code less readable)
Why do you block CI integration tests on ruff formatting? Why do you care?
| @@ -1531,6 +1534,10 @@ def raw_without_sig(self) -> bytes: | |||
| tmp.gpgsig = None | |||
| return tmp.as_raw_string() | |||
|
|
|||
| def check_signature(self, criterion: SignatureCriterion) -> None: | |||
| if self.gpgsig: | |||
There was a problem hiding this comment.
This seems like a bit of an odd interface; the commit stores whether the signature is PGP or SSH, so why does the caller need to care?
There was a problem hiding this comment.
The caller doesn't need to care about PGP vs GPG. They can use a yet-to-be-implemented criterion implementation that checks either kind. The PGP vs SSH info is in the signature which a criterion implementation can check when SignatureCriterion.check is called.
The caller needs to care at a high level what criterion they want for a signature to be "valid". And depending on that high level desire, they then can pick a SignatureCriterion implementation.
For instance, in my situation no GPG is signature is valid, only RSA and ed25519 public keys, and there is no restriction on commit date. Other application will have other criterion for what make a signature "valid".
There was a problem hiding this comment.
Probably makes sense to make this parameter optional and have a built-in minimal criterion that gets used. A built-in minimal criterion would do what is currently done for PGP sigs when there is no trusted keys list and then for SSH it does what SshsigCheckCriterion does here:
https://gitlab.com/perm.pub/sshsiglib/-/blob/8d20c654bc3366989c76ec07d1e2b8d4b738d3b3/contrib/sshsig_criterion.py#L13
There was a problem hiding this comment.
Might be a useful to have an example somewhere of a helper JoinSignatureCriteria which can take an implementation for PGP and an implementation for SSH and calls one of the two depending on the signature. Then users can mix and match depending on what trade-off and requirements they have.
