On 29.07.21 01:04, Jonathan Tan wrote:
to verify a ssh signature we first call ssh-keygen -Y find-principal to
look up the signing principal by their public key from the
allowedSignersFile. If the key is found then we do a verify. Otherwise
we only validate the signature but can not verify the signers identity.
Is this the same behavior as GPG signing in Git?
Not quite. GPG requires every signers public key to be in the keyring.
But even then, the "UNDEFINED" Trust level is enough to be valid for
commits (but not for merges).
For SSH i did set the unknown keys to UNDEFINED as well and they will
show up as valid but not have a principal to identify them.
This way a project can decide wether to accept unknown keys by setting
the gpg.mintrustlevel. So the default behaviour is different.
The alternative would be to treat unknown keys always as invalid.
Verification uses the gpg.ssh.allowedSignersFile (see ssh-keygen(1) "ALLOWED
SIGNERS") which contains valid public keys and a principal (usually
user@domain). Depending on the environment this file can be managed by
the individual developer or for example generated by the central
repository server from known ssh keys with push access. If the
repository only allows signed commits / pushes then the file can even be
stored inside it.
Storing the allowedSignersFile in the repo is technically possible even
if the repository does not allow signed commits/pushes, right? I would
reword the last sentence as "This file is usually stored outside the
repository, but if the repository only allows signed commits/pushes, the
user might choose to store it in the repository".
yes, thats correct. I have changed the wording.
Using SSH CA Keys with these files is also possible. Add
"cert-authority" as key option between the principal and the key to mark
it as a CA and all keys signed by it as valid for this CA.
Is this functionality provided by SSH? I don't see "cert-authority"
anywhere in the diff below.
I'll add "See "CERTIFICATES" in ssh-keygen(1)."
It is a SSH feature that i just wanted to make people aware of.
Also, I notice that the tests are all provided at the end. I think that
it would be better for the tests to be incrementally provided along with
the commit that introduces the relevant functionality, so it is clearer
to the reviewers how it is supposed to work (and also for us to observe
test coverage).
The problem is that nearly all of the tests use both signing &
verification of signatures. I could move the initial test that creates
all the signed commits but probably not much else.
+ git_gpg_config(var, value, NULL);
Check the return value of git_gpg_config() to see if that config was
processed by that function - if yes, we can return early.
fixed
+static void parse_ssh_output(struct signature_check *sigc)
+{
+ const char *line, *principal, *search;
+
+ /*
+ * ssh-keysign output should be:
+ * Good "git" signature for PRINCIPAL with RSA key SHA256:FINGERPRINT
+ * Good "git" signature for PRINCIPAL WITH WHITESPACE with RSA key SHA256:FINGERPRINT
+ * or for valid but unknown keys:
+ * Good "git" signature with RSA key SHA256:FINGERPRINT
+ */
Is this "ssh-keysign" or "ssh-keygen" output?
ssh-keygen. ssh-keysign is only used for host keys. But the names can
get a bit confusing sometimes. i changed it to ssh-keygen here.
Also, is this output documented to be stable even across locales?
Not really :/ (it currently is not locale specific)
The documentation states to only check the commands exit code. Do we
trust the exit code enough to rely on it for verification?
If so then i can move the main result and only parse the text for the
signer/fingerprint info thats used in log formats. This way only the
logs would break in case the output changes.
I added the output check since the gpg code did so as well:
ret |= !strstr(gpg_stdout.buf, "\n[GNUPG:] GOODSIG ");
+ sigc->result = 'B';
+ sigc->trust_level = TRUST_NEVER;
A discussion of trust levels should also be in the commit message or
user documentation.
+ if (!strcmp(var, "gpg.ssh.revocationFile")) {
The "F" in "revocationFile" has to be lowercase. (If tests were
included, as I suggested above, it might have been easier to catch
this.)
fixed
Thanks
