Add SSH signature verification for git commits#1141
Conversation
96523af to
1561774
Compare
|
@bb-Ricardo please run |
eedb46c to
048e862
Compare
e247a34 to
5e6ab4d
Compare
|
Hi, was just wondering if this PR is sufficient or if anything else is needed? |
stefanprodan
left a comment
There was a problem hiding this comment.
Tests are failing with open signatures/... no such file or directory please run make test-git before pushing commits.
|
sorry for that, now the tests passed locally. |
83338dc to
51ceefd
Compare
|
@hiddeco - would you mind to have a look at this PR again? Thank you. |
pjbgf
left a comment
There was a problem hiding this comment.
@bb-Ricardo thanks for working on this. Overall LGTM, however I'd remove any references to DSA as per thread below.
|
@bb-Ricardo please use rebase not merge. Undo the last merge, and properly rebase your fork, GH has a button for this if you go to your forked repo. |
b6f0ece to
bd9abd8
Compare
Yes, sorry. I used the seemingly convenient button GitHub offered in this PR. I removed all occurrences of GPG DSA keys in the tests and test fixtures. |
bd9abd8 to
fa037d1
Compare
fa037d1 to
0811c24
Compare
|
Hi, was wondering if anything else is needed. Thank you |
0811c24 to
af44b2c
Compare
- adds new package git/signatures - adds validation of SSH signed commits to ssh_signature.go - moves GPG signature validation to gpg_signature.go - adds text fixtures for all SSH and GPG key types including commits and signatures - adds tests for all key/signature combinations - adds wrapper for "Verify(keyRings ...string)" function Signed-off-by: Ricardo Bartels <ricardo.bartels@telekom.de>
Signed-off-by: Ricardo Bartels <ricardo.bartels@telekom.de>
Signed-off-by: Ricardo Bartels <ricardo.bartels@telekom.de>
Signed-off-by: Ricardo Bartels <ricardo.bartels@telekom.de>
Signed-off-by: Ricardo Bartels <ricardo.bartels@telekom.de>
Signed-off-by: Ricardo Bartels <ricardo.bartels@telekom.de>
Signed-off-by: Ricardo Bartels <ricardo.bartels@telekom.de>
Signed-off-by: Ricardo Bartels <ricardo.bartels@telekom.de>
…tureType' Signed-off-by: Ricardo Bartels <ricardo.bartels@telekom.de>
Co-authored-by: Paulo Gomes <paulo.gomes.uk@gmail.com> Signed-off-by: Ricardo <ricardo@bitchbrothers.com> Signed-off-by: Ricardo Bartels <ricardo.bartels@telekom.de>
Signed-off-by: Ricardo Bartels <ricardo.bartels@telekom.de>
Signed-off-by: Ricardo Bartels <ricardo.bartels@telekom.de>
Signed-off-by: Ricardo Bartels <ricardo.bartels@telekom.de>
af44b2c to
074c9de
Compare
| err := sshsig.Verify(bytes.NewReader(payload), sig, pubKey, sig.HashAlgorithm, SSHSignatureNamespace) | ||
| if err == nil { |
There was a problem hiding this comment.
The sshsig library has certain error sentinels that can be quite informative (e.g. sshsig.ErrPublicKeyMismatch / ErrNamespaceMismatch / ErrUnsupportedHashAlgorithm. VerifyPGPSignature). This information is now swallowed instead of being made available to the user if all keys fail.
| keyring, err := openpgp.ReadArmoredKeyRing(reader) | ||
| if err != nil { | ||
| return "", fmt.Errorf("unable to read armored key ring: %w", err) | ||
| } |
There was a problem hiding this comment.
I believe this is a bug: if given one or more keyrings, it will error out on the first invalid one instead of continuing to try the other ones it has been given.
There was a problem hiding this comment.
good point, this was just code moved from here: https://github.com/fluxcd/pkg/pull/1141/changes#diff-154234144c1a6e0ce7e676a0a16c2dc92f405ca7935772abdc8b66f7446ca7d4L218-L229
will improve the behaviour to not return on first broken invalid keyring, but instead keep trying to verify signature with any valid keyring left in the list. If no keyring matches but an invalid keyring was found earlier return with error message about first invalid keyring.
This would be consistent with current error behaviour but improve the chances of finding valid key in list.
Will add the same for SSH signatures. in the next few days
| @@ -0,0 +1,5 @@ | |||
| sign-user@example.com namespaces="git" ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAACAQCpreiO+8XsB4xXGNmwuO48a7WPghb5ihCJNPyQZpnaPfq6vhNVWSgq8AIjBmJOJYo4HZyiHqpS4OBc86glk6qMv8YHRt4VRVBP+DjPLDIsOR7+2HBlOPHMm8lTDi+iMPHBDxqFy7mSDB4+v7n700+49vYhWjZJpesnnE6JoitxSVhmqp75jeNRNU6PD00z+gMUcviv8UOs/Apg1Cw5f+4T9yOnjlOHaFH/ButvZ0t2VF0cs28tfCuLAoumjine5Gm6tCRQlZOoapNJzvnYT+86f/PEU/4kDYf3wT7S+NnUDfCsIpDVlOXPvjnQ/DudhqEnnXvfch+eBCI7rtJBHIGPKFdmC4cUROa0UDGR6o/JxLtx4ZTbkGpq6MVwdrb7qJ+Oib1U8xVimWFfarkm7deVXWD3wB5Wa8Ko/a/WuYfE3gYRhb8iXPYd71FsEy4F41JCMZDcIqMiQRe3e2gvY+z2sf02kHOFeWJmrAY9FFjPL85VD0Dg++jrExkGFjcBTw9gUG5OPGpwqQ9WHO8E8DPza+i5J/wu4DODyLrLxuXHPeSYUjcvh5ln8P70qL+Irwn1mgn2PkIZW0XCPBt6Iylg55t5sfyy03P0Kmb4U3TrppMeig7Lr9LDU4Doh7Fj6oLYDGFUV+F52SSuPs5SfrWd6Apiz+VPjsAh5btPPJNlzQ== test-rsa@example.com | |||
There was a problem hiding this comment.
I do not believe any of the verified_signers_* files are actually utilized in tests?
| ### Script structure | ||
| The script uses separate functions for different key types: | ||
| - `generate_rsa_dsa_key()` - For RSA keys with key length validation | ||
| - `generate_ecc_key()` - For ECC/ECDSA/EdDSA keys with curve validation | ||
| - `create_signed_object()` - For creating signed commits and tags | ||
| - `create_unsigned_commit()` - For creating unsigned test commits |
| ./generate_ssh_fixtures.sh | ||
| ``` | ||
|
|
||
| This script will automatically create all SSH keys, authorized_keys files, verified signers files, signed commits, and signed tags. |
There was a problem hiding this comment.
This seems to not be accurate anymore. (There are probably more things in this doc.)
| // which is not supported by github.com/ProtonMail/go-crypto (only version 4 is supported). | ||
| // The error occurs when trying to read the armored key ring: | ||
| // "unable to read armored key ring: openpgp: invalid data: first packet was not a public/private key" | ||
| {"ed448 valid signature", "commit_ed448_signed.txt", "tag_ed448_signed.txt", "key_ed448.pub", true}, |
There was a problem hiding this comment.
Should we also test tag_unsigned.txt?
| sig: "-----BEGIN SSH SIGNATURE-----\n-----END SSH SIGNATURE-----", | ||
| keyRings: []string{armoredKeyRingFixture}, | ||
| wantErr: "unable to verify openPGP signature, detected signature format: ssh", | ||
| }, |
There was a problem hiding this comment.
Testing a malformed signature (e.g. malformed body and/or missing -----END... would probably be good to test as well.
| sig: "-----BEGIN SSH SIGNATURE-----\n-----END SSH SIGNATURE-----", | ||
| keyRings: []string{armoredKeyRingFixture}, | ||
| wantErr: "unable to verify openPGP signature, detected signature format: ssh", | ||
| }, |
There was a problem hiding this comment.
Testing multi-keyring case would have surfaced the issue I commented on gpg_signature.go itself.
| create_signed_object() { | ||
| local object_type=$1 | ||
| local key_name=$2 | ||
| local key_type=$3 |
| echo "Temporary directory: $TEMP_DIR" | ||
| echo "Output directory: $SCRIPT_DIR" | ||
| echo "" | ||
|
|
There was a problem hiding this comment.
Would probably be good to do a quick command -v safety check to fail early.
Signed-off-by: Ricardo Bartels <ricardo.bartels@telekom.de>
Signed-off-by: Ricardo Bartels <ricardo.bartels@telekom.de>
Signed-off-by: Ricardo Bartels <ricardo.bartels@telekom.de>
This PR adds support of SSH signature validation.
resolves: fluxcd/flux2#4145