Re: [PATCH 2/2] t/t7510-signed-commit.sh: add signing subkey to Eris Discordia key
On Mon, 2018-11-05 at 10:08 +0900, Junio C Hamano wrote: > Michał Górny writes: > > > > It's my understanding that GnuPG will use the most recent subkey > > > suitable for a particular purpose, and I think the test relies on that > > > behavior. However, I'm not sure that's documented. Do we want to rely > > > on that behavior or be more explicit? (This is a question, not an > > > opinion.) > > > > To be honest, I don't recall which suitable subkey is used. However, it > > definitely will prefer a subkey with signing capabilities over > > the primary key if one is present, and this is well-known and expected > > behavior. > > > > In fact, if you have a key with two signing subkeys A and B and it > > considers A better, then even if you explicitly pass keyid of B, it will > > use A. To force another subkey you have to append '!' to keyid. > > > > Therefore, I think this is a behavior we can rely on. > > I didn't check how the signing key configuration is done in the test > sript (which is outside the patch context), but do you mean that we > create these signed objects by specifying which key to use with a > keyid with "!" appended? If so I agree that would make sense, > because we would then know which subkey should be used for signing > and checking with %GF/%GP would be a good way to do so. > No, we don't have duplicate subkeys to be required to use that. Some of the tests use explicit '-S' to force using the other key; other seem to use a default key (I can't find a place where the default would be set, so I suppose it's GnuPG default). -- Best regards, Michał Górny signature.asc Description: This is a digitally signed message part
Re: [PATCH 2/2] t/t7510-signed-commit.sh: add signing subkey to Eris Discordia key
On Sun, 2018-11-04 at 15:10 +, brian m. carlson wrote: > On Sun, Nov 04, 2018 at 10:47:10AM +0100, Michał Górny wrote: > > diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh > > index e8377286d..86d3f93fa 100755 > > --- a/t/t7510-signed-commit.sh > > +++ b/t/t7510-signed-commit.sh > > @@ -197,9 +197,9 @@ test_expect_success GPG 'show bad signature with custom > > format' ' > > test_expect_success GPG 'show untrusted signature with custom format' ' > > cat >expect <<-\EOF && > > U > > - 61092E85B7227189 > > + 65A0EEA02E30CAD7 > > Eris Discordia > > - D4BE22311AD3131E5EDA29A461092E85B7227189 > > + F8364A59E07FFE9F4D63005A65A0EEA02E30CAD7 > > D4BE22311AD3131E5EDA29A461092E85B7227189 > > EOF > > git log -1 --format="%G?%n%GK%n%GS%n%GF%n%GP" eighth-signed-alt >actual > > && > > @@ -209,7 +209,7 @@ test_expect_success GPG 'show untrusted signature with > > custom format' ' > > test_expect_success GPG 'show unknown signature with custom format' ' > > cat >expect <<-\EOF && > > E > > - 61092E85B7227189 > > + 65A0EEA02E30CAD7 > > It's my understanding that GnuPG will use the most recent subkey > suitable for a particular purpose, and I think the test relies on that > behavior. However, I'm not sure that's documented. Do we want to rely > on that behavior or be more explicit? (This is a question, not an > opinion.) To be honest, I don't recall which suitable subkey is used. However, it definitely will prefer a subkey with signing capabilities over the primary key if one is present, and this is well-known and expected behavior. In fact, if you have a key with two signing subkeys A and B and it considers A better, then even if you explicitly pass keyid of B, it will use A. To force another subkey you have to append '!' to keyid. Therefore, I think this is a behavior we can rely on. -- Best regards, Michał Górny signature.asc Description: This is a digitally signed message part
[PATCH 2/2] t/t7510-signed-commit.sh: add signing subkey to Eris Discordia key
Add a dedicated signing subkey to the key identified as 'Eris Discordia', and update tests appropriately. GnuPG will now sign commits using the dedicated signing subkey, changing the value of %GK and %GF, and effectively creating a test case for %GF!=%GP. Signed-off-by: Michał Górny --- t/lib-gpg/keyring.gpg| 62 t/t7510-signed-commit.sh | 6 ++-- 2 files changed, 59 insertions(+), 9 deletions(-) diff --git a/t/lib-gpg/keyring.gpg b/t/lib-gpg/keyring.gpg index d4754a1f1..918dfce33 100644 --- a/t/lib-gpg/keyring.gpg +++ b/t/lib-gpg/keyring.gpg @@ -30,7 +30,6 @@ Cezx4Q2khACcCs+/LtE8Lb9hC+2cvr3uH5p82AI= =aEiU -END PGP PRIVATE KEY BLOCK- -BEGIN PGP PRIVATE KEY BLOCK- -Version: GnuPG v1 lQOYBFFMlkcBCADJi/xnAF8yI34PHilSCbM7VtOFO17oFMkpu4cgN2QpPuM5MVjy cvrzKSguZFvPCDLzeAFJW1uPxL4SHaHSkisCrFhijH7OJWcOPNPSFCwu+inAoAsv @@ -83,11 +82,43 @@ fn1sY/IG5atoKK+ypmV/TlBlMZqFQzuPIJQT8VLbmxtLlDhJG04LbI6c8axIZxOO ZKLy5nTTSy16ztqEeS7eifHLPZg1UFFyEEIQ1XW0CNDAeuWKh90ERjyl4Cg7PnWS Z9Ei+zj6JD5Pcdi3BJhQo9WOLOVEJ0NHmewTYqk9QVXH/0v1Hdl4LMJtgcbdbDWk 4UTkXbg9pn3umCgkNJ3Vs8fWnIWO9Izdr2/wrFY2JvUT7Yvl+wsNIWatvOEzGy7n -BOW78WUxzhu0YJTLKy+iKCjg5HS5dx6OC+e4aEEgfhNPCMkbvDsJjtQ= -=hieJ +BOW78WUxzhu0YJTLKy+iKCjg5HS5dx6OC+e4aEEgfhNPCMkbvDsJjtSdA5gEW967 +3AEIAKjseT0sTQjyN39fOn0fzxWp89REMUUKgLigb01MKuuNI3cedBZsz3hpFOKV +cii5rldw8uf3yS3Okht2DfHPSD4NrGzLGEzSTpQ10S8N2q0DUYwyLU6C0U8HnMZm +/n+lCGBbUoxvnruohAvKAjpHO3rmJ8D4De9hlWg/fwdAxQQ0Sve0kN8Vwk2p1GuO +OWQKV1SU9c+kBiou7dewQmbilPRanKmP5ZSU4emhpTOMlJFXF+kmYSODQk1cMvWW +Ob3ttll2llX0Gul7Sjf+haq/FcRyRk7Tw5MHwZjr5aWiCny0/0+byvfF6SBIfzyE +qlyWURQ2gHZUqSiG3QPMZiYr04cAEQEAAQAH/Am4rv/oQF6wodgz5y4zc6JJiTDA +4+nKdIuR7OKqUxk1oo7eZjJML/xvMumygNyUvJ9nodl1SlMKilOhdAswfkKj9gJY +BdDJLm1OufhW3pJwy6ahbjeqEgwJFVENtSPF0zkuyED9kElrpbD2ZTGfzwdM0e9D +10ZDFWtODCw8rzOFcijujgI8oilLtxSNrkkTKW+25WJFRNPSHgIkMIm8UlPAG+rj +3Yj9UqodeXTSvXwG2zceOxjFJadV77sOFJDgwWslN6J8El4+GcgwFVepJxoZEj7e +cKkmVr0Dc9/Q04D5dWATc1FYcIhZbTu3oImCAh45ep4u9WYLUV5PGyeMviEEAMwo +mJbYBxWuPjpNa722HQcbvMUiZWWDwHfLCib/SaP0AgfDahid8/PcZwxOPHPByBrm +GDi0z7ibn/pgJr07kpp1Cic9ntfc2FvkI0QMzG0EuiekzQyPEnzjoDHF+V4nJIj2 +GWVjLYYqlZWEmhsfKt1CnlPXBunKoDJ30ABPcHJ/BADT0WxAIVKF4lO2HlrDVP44 +bufBEG9Ct7dl/G08Qve4Ag3VEZpT82vEFp0LzX0mTCDIUKJUYAYLxAIPhP7IvIfc +EZXrwyDUxU7YSgKTHMKo9nFC6fIc1GeGPRalIF1gmTY32qlYJC6y5BTDhZNV5ydG +u8QL2P/orP7XuRrJyeyK+QP/XTekr/DS6Jkct826MPA52ciIkWVgYLatH5fO4HCq +ssDU8vz7FbbvGs0G1Xn7GA4m9dNYVOZtKwX++3nf2IEOpgPiZVTn/nP2u3HutpJb +/HMLlcfZGiGdxS6n/vdz6wsEobJoi6STkHkA+VFNOSZmdsw6eKl3X911tpCTYfOG +2U47/IkCbAQYAQgAIBYhBNS+IjEa0xMeXtoppGEJLoW3InGJBQJb3rvcAhsCAUAJ +EGEJLoW3InGJwHQgBBkBCAAdFiEE+DZKWeB//p9NYwBaZaDuoC4wytcFAlveu9wA +CgkQZaDuoC4wytcD9gf/WigtHl7lFyl8RaE/uqROFEelZyM00v1h55fd/IGRG88E +tN0Lr4FaqBqPkMZjU/LN9UMBaTd+748vHlHaweZqljXJu99CO9Id7Y4w7WzF3C3Y +yQsGZ92EGxthsPK0+rhHV0MbaINupI1oO9gATFglSxq17o83FJatGRjaXCZau8jr +57/By1MGtjk+Iq1NkzGkrX778LdRQGLKDw2Qa7lsdHY8d3lUPAH8mbb97ELmIc9t +PG2aM7ATJL7nBmFuTHo6hmEcIw32Ei9KK1zxM0ZylEYkjBjHAlklWmKb9MiayMC5 +uHW7Iyhjl+NbgbIEr2JTamW/9tL6UrIIxiDEdqaHNfCaB/9D+V31Upcohc9azwB4 +AF8diQwt5nfiVpnVeF/W8+eS1By2W6QrwLNthNRabYFnuSf9USHAY6atDWe+egId +MLIv4ce0i3ykoczSu0oMoUCMxdl9kQrsNHZCqWX/OiDDLSb05u/P/3he900y6tSB +15MbIPA6i5Bw/693nHguqxS1ASbBB/LiIu3vCXdFEs9RMvIJ+qkP3xQA96oImQiK +R3U6OGv593eONKijUINNqHRq6+UxIyJ+OCAi+L2QTidAhJLRCp6EZD96u02cthYq +8KA8j1+rx9BcbeacVVHepeG1JsgxsXX8BTJ7ZuS5VVndZOjag8URW/9nJMf01w/h +el64 +=Iv7W -END PGP PRIVATE KEY BLOCK- -BEGIN PGP PUBLIC KEY BLOCK- -Version: GnuPG v1 mQGiBEZnyykRBACzCPjIpTYNL7Y2tQqlEGTTDlvZcWNLjF5f7ZzuyOqNOidLUgFD 36qch1LZLSZkShdR3Gae+bsolyjxrlFuFP0eXRPMtqK20aLw7WZvPFpEV1ThMne+ @@ -137,6 +168,25 @@ bGPyBuWraCivsqZlf05QZTGahUM7jyCUE/FS25sbS5Q4SRtOC2yOnPGsSGcTjmSi 8uZ000stes7ahHku3onxyz2YNVBRchBCENV1tAjQwHrliofdBEY8peAoOz51kmfR Ivs4+iQ+T3HYtwSYUKPVjizlRCdDR5nsE2KpPUFVx/9L9R3ZeCzCbYHG3Ww1pOFE 5F24PaZ97pgoJDSd1bPH1pyFjvSM3a9v8KxWNib1E+2L5fsLDSFmrbzhMxsu5wTl -u/FlMc4btGCUyysvoigo4OR0uXcejgvnuGhBIH4TTwjJG7w7CY7U -=iYv/ +u/FlMc4btGCUyysvoigo4OR0uXcejgvnuGhBIH4TTwjJG7w7CY7UuQENBFveu9wB +CACo7Hk9LE0I8jd/Xzp9H88VqfPURDFFCoC4oG9NTCrrjSN3HnQWbM94aRTilXIo +ua5XcPLn98ktzpIbdg3xz0g+DaxsyxhM0k6UNdEvDdqtA1GMMi1OgtFPB5zGZv5/ +pQhgW1KMb567qIQLygI6Rzt65ifA+A3vYZVoP38HQMUENEr3tJDfFcJNqdRrjjlk +CldUlPXPpAYqLu3XsEJm4pT0Wpypj+WUlOHpoaUzjJSRVxfpJmEjg0JNXDL1ljm9 +7bZZdpZV9Brpe0o3/oWqvxXEckZO08OTB8GY6+Wlogp8tP9Pm8r3xekgSH88hKpc +llEUNoB2VKkoht0DzGYmK9OHABEBAAGJAmwEGAEIACAWIQTUviIxGtMTHl7aKaRh +CS6FtyJxiQUCW9673AIbAgFACRBhCS6FtyJxicB0IAQZAQgAHRYhBPg2Slngf/6f +TWMAWmWg7qAuMMrXBQJb3rvcAAoJEGWg7qAuMMrXA/YH/1ooLR5e5RcpfEWhP7qk +ThRHpWcjNNL9YeeX3fyBkRvPBLTdC6+BWqgaj5DGY1PyzfVDAWk3fu+PLx5R2sHm +apY1ybvfQjvSHe2OMO1sxdwt2MkLBmfdhBsbYbDytPq4R1dDG2iDbqSNaDvYAExY +JUsate6PNxSWrRkY2lwmWrvI6+e/wctTBrY5PiKtTZMxpK1++/C3UUBiyg8NkGu5 +bHR2PHd5VDwB/Jm2/exC5iHPbTxtmjOwEyS+5wZhbkx6OoZhHCMN9hIvSitc8TNG +cpRGJIwYxwJZJVpim/TImsjAubh1uyMoY5fjW4GyBK9iU2plv/bS+lKyCMYgxHam +hzXwmgf/Q/ld9VKXKIXPWs8AeABfHYkMLeZ34laZ1Xhf1vPnktQctlukK8CzbYTU
[PATCH 1/2] t/t7510-signed-commit.sh: Add %GP to custom format checks
Test %GP in addition to %GF in custom format checks. With current keyring, both have the same value. Signed-off-by: Michał Górny --- t/t7510-signed-commit.sh | 18 -- 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh index 19ccae286..e8377286d 100755 --- a/t/t7510-signed-commit.sh +++ b/t/t7510-signed-commit.sh @@ -176,8 +176,9 @@ test_expect_success GPG 'show good signature with custom format' ' 13B6F51ECDDE430D C O Mitter 73D758744BE721698EC54E8713B6F51ECDDE430D + 73D758744BE721698EC54E8713B6F51ECDDE430D EOF - git log -1 --format="%G?%n%GK%n%GS%n%GF" sixth-signed >actual && + git log -1 --format="%G?%n%GK%n%GS%n%GF%n%GP" sixth-signed >actual && test_cmp expect actual ' @@ -187,8 +188,9 @@ test_expect_success GPG 'show bad signature with custom format' ' 13B6F51ECDDE430D C O Mitter + EOF - git log -1 --format="%G?%n%GK%n%GS%n%GF" $(cat forged1.commit) >actual && + git log -1 --format="%G?%n%GK%n%GS%n%GF%n%GP" $(cat forged1.commit) >actual && test_cmp expect actual ' @@ -198,8 +200,9 @@ test_expect_success GPG 'show untrusted signature with custom format' ' 61092E85B7227189 Eris Discordia D4BE22311AD3131E5EDA29A461092E85B7227189 + D4BE22311AD3131E5EDA29A461092E85B7227189 EOF - git log -1 --format="%G?%n%GK%n%GS%n%GF" eighth-signed-alt >actual && + git log -1 --format="%G?%n%GK%n%GS%n%GF%n%GP" eighth-signed-alt >actual && test_cmp expect actual ' @@ -209,8 +212,9 @@ test_expect_success GPG 'show unknown signature with custom format' ' 61092E85B7227189 + EOF - GNUPGHOME="$GNUPGHOME_NOT_USED" git log -1 --format="%G?%n%GK%n%GS%n%GF" eighth-signed-alt >actual && + GNUPGHOME="$GNUPGHOME_NOT_USED" git log -1 --format="%G?%n%GK%n%GS%n%GF%n%GP" eighth-signed-alt >actual && test_cmp expect actual ' @@ -220,8 +224,9 @@ test_expect_success GPG 'show lack of signature with custom format' ' + EOF - git log -1 --format="%G?%n%GK%n%GS%n%GF" seventh-unsigned >actual && + git log -1 --format="%G?%n%GK%n%GS%n%GF%n%GP" seventh-unsigned >actual && test_cmp expect actual ' @@ -261,8 +266,9 @@ test_expect_success GPG 'show double signature with custom format' ' + EOF - git log -1 --format="%G?%n%GK%n%GS%n%GF" $(cat double-commit.commit) >actual && + git log -1 --format="%G?%n%GK%n%GS%n%GF%n%GP" $(cat double-commit.commit) >actual && test_cmp expect actual ' -- 2.19.1
Re: Git Test Coverage Report (Friday, Nov 2)
On Sat, 2018-11-03 at 19:03 +0900, Junio C Hamano wrote: > Michał Górny writes: > > > As for how involved... we'd just have to use a key that has split > > signing subkey. Would it be fine to add the subkey to the existing key? > > It would imply updating keyids/fingerprints everywhere. > > Yes, that "everywhere" is exactly what I meant by "how involved", > and your suggestion answers "very much involved". > > If we can easily add _another_ key with a subkey that is not the > primary one we use for other tests, without touching the existing > key and the existing tests that use it (including the one I touched > below--- we'd want to see a sig with a key that is not split is > shown with the same %GF and %GP), while adding a handful of new > tests that create signed objects under the new & split key and > view them with %GF and %GP, then the end result would be that we > managed to add a new test case where %GF/%GP are different without > making very much involved changes. I guess that was what I was > getting at. > I've just did a little research and came to the following results: 1. modifying the 'C. O. Mitter' key would require changes to 4 tests, 2. modifying the 'Eris Discordia' key would require changes to 2 tests (both in 7510). Do you think 2. would be an acceptable option? I think changing 2 tests would be preferable to proliferating a third key for one test case. Also, given that both failing tests are specifically format string tests, one of them would serve additional purpose of testing %GP!=%GF. -- Best regards, Michał Górny signature.asc Description: This is a digitally signed message part
Re: [PATCH v4] gpg-interface.c: detect and reject multiple signatures on commits
On Sat, 2018-11-03 at 16:36 +0100, Duy Nguyen wrote: > On Sat, Nov 3, 2018 at 4:32 PM Michał Górny wrote: > > > Perhaps my gpg is too old? > > > > > > $ gpg --version > > > gpg (GnuPG) 2.1.15 > > > libgcrypt 1.7.3 > > > Copyright (C) 2016 Free Software Foundation, Inc. > > > License GPLv3+: GNU GPL version 3 or later > > > <https://gnu.org/licenses/gpl.html> > > > This is free software: you are free to change and redistribute it. > > > There is NO WARRANTY, to the extent permitted by law. > > > > > > Home: /home/pclouds/.gnupg > > > Supported algorithms: > > > Pubkey: RSA, ELG, DSA, ECDH, ECDSA, EDDSA > > > Cipher: IDEA, 3DES, CAST5, BLOWFISH, AES, AES192, AES256, TWOFISH, > > > CAMELLIA128, CAMELLIA192, CAMELLIA256 > > > Hash: SHA1, RIPEMD160, SHA256, SHA384, SHA512, SHA224 > > > Compression: Uncompressed, ZIP, ZLIB, BZIP2 > > > > Perhaps this is indeed specific to this version of GnuPG. The tests > > pass for me with both 1.4.21 and 2.2.10. We don't have 2.1* in Gentoo > > anymore. > > Yeah I have not really used gpg and neglected updating it. Will try it > now. The question remains though whether we need to support 2.1* (I > don't know at all about gnupg status, maybe 2.1* is indeed too > old/buggy that nobody should use it and so we don't need to support > it). GnuPG upstream considers 2.2 as continuation/mature version of 2.1 branch. They currently support running either newest version of 1.4 (legacy) or newest version of 2.2 [1]. In other words, this might have been a bug that was fixed in newer release (possibly 2.2.x). [1]:https://gnupg.org/download/index.html#text-end-of-life -- Best regards, Michał Górny signature.asc Description: This is a digitally signed message part
Re: [PATCH v4] gpg-interface.c: detect and reject multiple signatures on commits
On Sat, 2018-11-03 at 16:17 +0100, Duy Nguyen wrote: > On Sat, Oct 20, 2018 at 9:31 PM Michał Górny wrote: > > +test_expect_success GPG 'detect fudged commit with double signature' ' > > + sed -e "/gpgsig/,/END PGP/d" forged1 >double-base && > > + sed -n -e "/gpgsig/,/END PGP/p" forged1 | \ > > + sed -e "s/^gpgsig//;s/^ //" | gpg --dearmor > > >double-sig1.sig && > > + gpg -o double-sig2.sig -u 29472784 --detach-sign double-base && > > + cat double-sig1.sig double-sig2.sig | gpg --enarmor > > >double-combined.asc && > > + sed -e "s/^\(-.*\)ARMORED FILE/\1SIGNATURE/;1s/^/gpgsig /;2,\$s/^/ > > /" \ > > + double-combined.asc > double-gpgsig && > > + sed -e "/committer/r double-gpgsig" double-base >double-commit && > > + git hash-object -w -t commit double-commit >double-commit.commit && > > + test_must_fail git verify-commit $(cat double-commit.commit) && > > + git show --pretty=short --show-signature $(cat > > double-commit.commit) >double-actual && > > + grep "BAD signature from" double-actual && > > + grep "Good signature from" double-actual > > +' > > This test fails on 'master' today for me > > gpg: WARNING: multiple signatures detected. Only the first will be checked. > gpg: Signature made Sat Nov 3 15:13:28 2018 UTC > gpg:using DSA key 13B6F51ECDDE430D > gpg:issuer "commit...@example.com" > gpg: BAD signature from "C O Mitter " [ultimate] > gpg: BAD signature from "C O Mitter " [ultimate] > not ok 16 - detect fudged commit with double signature > > Perhaps my gpg is too old? > > $ gpg --version > gpg (GnuPG) 2.1.15 > libgcrypt 1.7.3 > Copyright (C) 2016 Free Software Foundation, Inc. > License GPLv3+: GNU GPL version 3 or later <https://gnu.org/licenses/gpl.html> > This is free software: you are free to change and redistribute it. > There is NO WARRANTY, to the extent permitted by law. > > Home: /home/pclouds/.gnupg > Supported algorithms: > Pubkey: RSA, ELG, DSA, ECDH, ECDSA, EDDSA > Cipher: IDEA, 3DES, CAST5, BLOWFISH, AES, AES192, AES256, TWOFISH, > CAMELLIA128, CAMELLIA192, CAMELLIA256 > Hash: SHA1, RIPEMD160, SHA256, SHA384, SHA512, SHA224 > Compression: Uncompressed, ZIP, ZLIB, BZIP2 Perhaps this is indeed specific to this version of GnuPG. The tests pass for me with both 1.4.21 and 2.2.10. We don't have 2.1* in Gentoo anymore. -- Best regards, Michał Górny signature.asc Description: This is a digitally signed message part
Re: Git Test Coverage Report (Friday, Nov 2)
On Sat, 2018-11-03 at 12:38 +0900, Junio C Hamano wrote:
> Derrick Stolee writes:
>
> > Uncovered code in 'next' not in 'master'
> >
> >
> > pretty.c
> > 4de9394dcb 1264) if (c->signature_check.primary_key_fingerprint)
> > 4de9394dcb 1265) strbuf_addstr(sb,
> > c->signature_check.primary_key_fingerprint);
> > 4de9394dcb 1266) break;
>
> Perhaps a patch along this line can be appended to the
> mg/gpg-fingerprint topic that ends at 4de9394d ("gpg-interface.c:
> obtain primary key fingerprint as well", 2018-10-22) to cover this
> entry in the report.
>
> I do not know how involved it would be to set up a new test case
> that demonstrates a case where %GF and %GP are different, but if it
> is very involved perhaps it is not worth adding such a case.
Well, I didn't add a test for %GP primarily because we didn't have a key
with different primary and subkey fingerprints.
As for how involved... we'd just have to use a key that has split
signing subkey. Would it be fine to add the subkey to the existing key?
It would imply updating keyids/fingerprints everywhere.
>
> t/t7510-signed-commit.sh | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh
> index 19ccae2869..9ecafedcc4 100755
> --- a/t/t7510-signed-commit.sh
> +++ b/t/t7510-signed-commit.sh
> @@ -176,8 +176,9 @@ test_expect_success GPG 'show good signature with custom
> format' '
> 13B6F51ECDDE430D
> C O Mitter
> 73D758744BE721698EC54E8713B6F51ECDDE430D
> + 73D758744BE721698EC54E8713B6F51ECDDE430D
> EOF
> - git log -1 --format="%G?%n%GK%n%GS%n%GF" sixth-signed >actual &&
> + git log -1 --format="%G?%n%GK%n%GS%n%GF%n%GP" sixth-signed >actual &&
> test_cmp expect actual
> '
>
--
Best regards,
Michał Górny
signature.asc
Description: This is a digitally signed message part
[PATCH 2/3] gpg-interface.c: Support getting key fingerprint via %GF format
Support processing VALIDSIG status that provides additional information
for valid signatures. Use this information to propagate signing key
fingerprint and expose it via %GF pretty format. This format can be
used to build safer key verification systems that verify the key via
complete fingerprint rather than short/long identifier provided by %GK.
Signed-off-by: Michał Górny
---
Documentation/pretty-formats.txt | 1 +
gpg-interface.c | 14 +-
gpg-interface.h | 1 +
pretty.c | 4
t/t7510-signed-commit.sh | 18 --
5 files changed, 31 insertions(+), 7 deletions(-)
diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 6109ef09a..8ab7d6dd1 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -153,6 +153,7 @@ endif::git-rev-list[]
and "N" for no signature
- '%GS': show the name of the signer for a signed commit
- '%GK': show the key used to sign a signed commit
+- '%GF': show the fingerprint of the key used to sign a signed commit
- '%gD': reflog selector, e.g., `refs/stash@{1}` or
`refs/stash@{2 minutes ago`}; the format follows the rules described
for the `-g` option. The portion before the `@` is the refname as
diff --git a/gpg-interface.c b/gpg-interface.c
index c7cd24ec0..a406484e4 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -73,6 +73,7 @@ void signature_check_clear(struct signature_check *sigc)
FREE_AND_NULL(sigc->gpg_status);
FREE_AND_NULL(sigc->signer);
FREE_AND_NULL(sigc->key);
+ FREE_AND_NULL(sigc->fingerprint);
}
/* An exclusive status -- only one of them can appear in output */
@@ -81,6 +82,8 @@ void signature_check_clear(struct signature_check *sigc)
#define GPG_STATUS_KEYID (1<<1)
/* The status includes user identifier */
#define GPG_STATUS_UID (1<<2)
+/* The status includes key fingerprints */
+#define GPG_STATUS_FINGERPRINT (1<<3)
/* Short-hand for standard exclusive *SIG status with keyid & UID */
#define GPG_STATUS_STDSIG
(GPG_STATUS_EXCLUSIVE|GPG_STATUS_KEYID|GPG_STATUS_UID)
@@ -98,6 +101,7 @@ static struct {
{ 'X', "EXPSIG ", GPG_STATUS_STDSIG },
{ 'Y', "EXPKEYSIG ", GPG_STATUS_STDSIG },
{ 'R', "REVKEYSIG ", GPG_STATUS_STDSIG },
+ { 0, "VALIDSIG ", GPG_STATUS_FINGERPRINT },
};
static void parse_gpg_output(struct signature_check *sigc)
@@ -123,7 +127,8 @@ static void parse_gpg_output(struct signature_check *sigc)
goto found_duplicate_status;
}
- sigc->result = sigcheck_gpg_status[i].result;
+ if (sigcheck_gpg_status[i].result)
+ sigc->result =
sigcheck_gpg_status[i].result;
/* Do we have key information? */
if (sigcheck_gpg_status[i].flags &
GPG_STATUS_KEYID) {
next = strchrnul(line, ' ');
@@ -137,6 +142,12 @@ static void parse_gpg_output(struct signature_check *sigc)
sigc->signer = xmemdupz(line,
next - line);
}
}
+ /* Do we have fingerprint? */
+ if (sigcheck_gpg_status[i].flags &
GPG_STATUS_FINGERPRINT) {
+ next = strchrnul(line, ' ');
+ free(sigc->fingerprint);
+ sigc->fingerprint = xmemdupz(line, next
- line);
+ }
break;
}
@@ -154,6 +165,7 @@ static void parse_gpg_output(struct signature_check *sigc)
*/
sigc->result = 'E';
/* Clear partial data to avoid confusion */
+ FREE_AND_NULL(sigc->fingerprint);
FREE_AND_NULL(sigc->signer);
FREE_AND_NULL(sigc->key);
}
diff --git a/gpg-interface.h b/gpg-interface.h
index acf50c461..8ce614fc9 100644
--- a/gpg-interface.h
+++ b/gpg-interface.h
@@ -23,6 +23,7 @@ struct signature_check {
char result;
char *signer;
char *key;
+ char *fingerprint;
};
void signature_check_clear(struct signature_check *sigc);
diff --git a/pretty.c b/pretty.c
index 8ca29e928..4567b5321 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1256,6 +1256,10 @@ static size_t format_commit_one(struct strbuf *sb, /* in
UTF-8 */
if (c->signature_check.key)
strbuf_addstr(sb, c->signature_check.key);
break;
+ case 'F':
+
[PATCH 1/3] gpg-interface.c: use flags to determine key/signer info presence
Replace the logic used to determine whether key and signer information
is present to use explicit flags in sigcheck_gpg_status[] array. This
is more future-proof, since it makes it possible to add additional
statuses without having to explicitly update the conditions.
Signed-off-by: Michał Górny
---
gpg-interface.c | 27 +--
1 file changed, 17 insertions(+), 10 deletions(-)
diff --git a/gpg-interface.c b/gpg-interface.c
index d72a43b77..c7cd24ec0 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -77,20 +77,27 @@ void signature_check_clear(struct signature_check *sigc)
/* An exclusive status -- only one of them can appear in output */
#define GPG_STATUS_EXCLUSIVE (1<<0)
+/* The status includes key identifier */
+#define GPG_STATUS_KEYID (1<<1)
+/* The status includes user identifier */
+#define GPG_STATUS_UID (1<<2)
+
+/* Short-hand for standard exclusive *SIG status with keyid & UID */
+#define GPG_STATUS_STDSIG
(GPG_STATUS_EXCLUSIVE|GPG_STATUS_KEYID|GPG_STATUS_UID)
static struct {
char result;
const char *check;
unsigned int flags;
} sigcheck_gpg_status[] = {
- { 'G', "GOODSIG ", GPG_STATUS_EXCLUSIVE },
- { 'B', "BADSIG ", GPG_STATUS_EXCLUSIVE },
+ { 'G', "GOODSIG ", GPG_STATUS_STDSIG },
+ { 'B', "BADSIG ", GPG_STATUS_STDSIG },
{ 'U', "TRUST_NEVER", 0 },
{ 'U', "TRUST_UNDEFINED", 0 },
- { 'E', "ERRSIG ", GPG_STATUS_EXCLUSIVE },
- { 'X', "EXPSIG ", GPG_STATUS_EXCLUSIVE },
- { 'Y', "EXPKEYSIG ", GPG_STATUS_EXCLUSIVE },
- { 'R', "REVKEYSIG ", GPG_STATUS_EXCLUSIVE },
+ { 'E', "ERRSIG ", GPG_STATUS_EXCLUSIVE|GPG_STATUS_KEYID },
+ { 'X', "EXPSIG ", GPG_STATUS_STDSIG },
+ { 'Y', "EXPKEYSIG ", GPG_STATUS_STDSIG },
+ { 'R', "REVKEYSIG ", GPG_STATUS_STDSIG },
};
static void parse_gpg_output(struct signature_check *sigc)
@@ -117,13 +124,13 @@ static void parse_gpg_output(struct signature_check *sigc)
}
sigc->result = sigcheck_gpg_status[i].result;
- /* The trust messages are not followed by
key/signer information */
- if (sigc->result != 'U') {
+ /* Do we have key information? */
+ if (sigcheck_gpg_status[i].flags &
GPG_STATUS_KEYID) {
next = strchrnul(line, ' ');
free(sigc->key);
sigc->key = xmemdupz(line, next - line);
- /* The ERRSIG message is not followed
by signer information */
- if (*next && sigc->result != 'E') {
+ /* Do we have signer information? */
+ if (*next &&
(sigcheck_gpg_status[i].flags & GPG_STATUS_UID)) {
line = next + 1;
next = strchrnul(line, '\n');
free(sigc->signer);
--
2.19.1
[PATCH 3/3] gpg-interface.c: Obtain primary key fingerprint as well
Obtain the primary key fingerprint off VALIDSIG status message,
and expose it via %GP format.
Signed-off-by: Michał Górny
---
Documentation/pretty-formats.txt | 2 ++
gpg-interface.c | 16 +++-
gpg-interface.h | 1 +
pretty.c | 4
4 files changed, 22 insertions(+), 1 deletion(-)
diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 8ab7d6dd1..417b638cd 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -154,6 +154,8 @@ endif::git-rev-list[]
- '%GS': show the name of the signer for a signed commit
- '%GK': show the key used to sign a signed commit
- '%GF': show the fingerprint of the key used to sign a signed commit
+- '%GP': show the fingerprint of the primary key whose subkey was used
+ to sign a signed commit
- '%gD': reflog selector, e.g., `refs/stash@{1}` or
`refs/stash@{2 minutes ago`}; the format follows the rules described
for the `-g` option. The portion before the `@` is the refname as
diff --git a/gpg-interface.c b/gpg-interface.c
index a406484e4..8ed274533 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -74,6 +74,7 @@ void signature_check_clear(struct signature_check *sigc)
FREE_AND_NULL(sigc->signer);
FREE_AND_NULL(sigc->key);
FREE_AND_NULL(sigc->fingerprint);
+ FREE_AND_NULL(sigc->primary_key_fingerprint);
}
/* An exclusive status -- only one of them can appear in output */
@@ -108,7 +109,7 @@ static void parse_gpg_output(struct signature_check *sigc)
{
const char *buf = sigc->gpg_status;
const char *line, *next;
- int i;
+ int i, j;
int seen_exclusive_status = 0;
/* Iterate over all lines */
@@ -147,6 +148,18 @@ static void parse_gpg_output(struct signature_check *sigc)
next = strchrnul(line, ' ');
free(sigc->fingerprint);
sigc->fingerprint = xmemdupz(line, next
- line);
+
+ /* Skip interim fields */
+ for (j = 9; j > 0; j--) {
+ if (!*next)
+ break;
+ line = next + 1;
+ next = strchrnul(line, ' ');
+ }
+
+ next = strchrnul(line, '\n');
+ free(sigc->primary_key_fingerprint);
+ sigc->primary_key_fingerprint =
xmemdupz(line, next - line);
}
break;
@@ -165,6 +178,7 @@ static void parse_gpg_output(struct signature_check *sigc)
*/
sigc->result = 'E';
/* Clear partial data to avoid confusion */
+ FREE_AND_NULL(sigc->primary_key_fingerprint);
FREE_AND_NULL(sigc->fingerprint);
FREE_AND_NULL(sigc->signer);
FREE_AND_NULL(sigc->key);
diff --git a/gpg-interface.h b/gpg-interface.h
index 8ce614fc9..3e624ec28 100644
--- a/gpg-interface.h
+++ b/gpg-interface.h
@@ -24,6 +24,7 @@ struct signature_check {
char *signer;
char *key;
char *fingerprint;
+ char *primary_key_fingerprint;
};
void signature_check_clear(struct signature_check *sigc);
diff --git a/pretty.c b/pretty.c
index 4567b5321..b83a3ecd2 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1260,6 +1260,10 @@ static size_t format_commit_one(struct strbuf *sb, /* in
UTF-8 */
if (c->signature_check.fingerprint)
strbuf_addstr(sb,
c->signature_check.fingerprint);
break;
+ case 'P':
+ if (c->signature_check.primary_key_fingerprint)
+ strbuf_addstr(sb,
c->signature_check.primary_key_fingerprint);
+ break;
default:
return 0;
}
--
2.19.1
Re: [PATCH v4] gpg-interface.c: detect and reject multiple signatures on commits
On Mon, 2018-10-22 at 08:04 +, Michał Górny wrote:
> Dnia October 20, 2018 11:57:36 PM UTC, Junio C Hamano
> napisał(a):
> > Michał Górny writes:
> >
> > > GnuPG supports creating signatures consisting of multiple signature
> > > packets. If such a signature is verified, it outputs all the status
> > > messages for each signature separately. However, git currently does
> >
> > not
> > > account for such scenario and gets terribly confused over getting
> > > multiple *SIG statuses.
> > >
> > > For example, if a malicious party alters a signed commit and appends
> > > a new untrusted signature, git is going to ignore the original bad
> > > signature and report untrusted commit instead. However, %GK and %GS
> > > format strings may still expand to the data corresponding
> > > to the original signature, potentially tricking the scripts into
> > > trusting the malicious commit.
> > >
> > > Given that the use of multiple signatures is quite rare, git does not
> > > support creating them without jumping through a few hoops, and
> >
> > finally
> > > supporting them properly would require extensive API improvement, it
> > > seems reasonable to just reject them at the moment.
> > >
> > > Signed-off-by: Michał Górny
> > > ---
> > > gpg-interface.c | 90
> >
> > +++-
> > > t/t7510-signed-commit.sh | 26
> > > 2 files changed, 87 insertions(+), 29 deletions(-)
> > >
> > > Changes in v4:
> > > * switched to using skip_prefix(),
> > > * renamed the variable to seen_exclusive_status,
> > > * made the loop terminate early on first duplicate status seen.
> >
> > Thanks for sticking to the topic and polishing it further. Looks
> > very good.
> >
> > Will replace.
> >
> > > + int seen_exclusive_status = 0;
> > > +
> > > + /* Iterate over all lines */
> > > + for (line = buf; *line; line = strchrnul(line+1, '\n')) {
> > > + while (*line == '\n')
> > > + line++;
> > > + /* Skip lines that don't start with GNUPG status */
> > > + if (!skip_prefix(line, "[GNUPG:] ", ))
> > > + continue;
> > > +
> > > + /* Iterate over all search strings */
> > > + for (i = 0; i < ARRAY_SIZE(sigcheck_gpg_status); i++) {
> > > + if (skip_prefix(line, sigcheck_gpg_status[i].check,
> > > )) {
> > > + if (sigcheck_gpg_status[i].flags &
> > > GPG_STATUS_EXCLUSIVE) {
> > > + if (++seen_exclusive_status > 1)
> > > + goto found_duplicate_status;
> >
> > Very minor point but by not using pre-increment, i.e.
> >
> > if (seen_exclusive_status++)
> > goto found_duplicate_status;
> >
> > you can use the expression as a "have we already seen?" boolean,
> > whic may probably be more idiomatic.
> >
> > The patch is good in the way written as-is, and this is so minor
> > that it is not worth rerolling to only update this part.
>
> Please don't merge it yet. I gave it some more thought and I think the loop
> refactoring may cause TRUST_* to override BADSIG (i.e. upgrade from 'bad' to
> 'untrusted'). I'm going to verify this when I get home.
>
I was wrong. I'm sorry about the noise. I've reverified the logic,
and it correct. That is:
1) for trusted signature, only GOODSIG is emitted and 'G' is returned
correctly,
2) for untrusted signature, GOODSIG is followed by TRUST_* messages,
so line-wise TRUST_* check replaces the 'G' with 'U',
3) for bad signature, only BADSIG is emitted without TRUST_* messages.
Furthermore, GnuPG documentation confirms that TRUST_* is only emitted
for good signatures [1].
[1]:https://github.com/gpg/gnupg/blob/master/doc/DETAILS#trust_
--
Best regards,
Michał Górny
signature.asc
Description: This is a digitally signed message part
Re: [PATCH v4] gpg-interface.c: detect and reject multiple signatures on commits
Dnia October 20, 2018 11:57:36 PM UTC, Junio C Hamano
napisał(a):
>Michał Górny writes:
>
>> GnuPG supports creating signatures consisting of multiple signature
>> packets. If such a signature is verified, it outputs all the status
>> messages for each signature separately. However, git currently does
>not
>> account for such scenario and gets terribly confused over getting
>> multiple *SIG statuses.
>>
>> For example, if a malicious party alters a signed commit and appends
>> a new untrusted signature, git is going to ignore the original bad
>> signature and report untrusted commit instead. However, %GK and %GS
>> format strings may still expand to the data corresponding
>> to the original signature, potentially tricking the scripts into
>> trusting the malicious commit.
>>
>> Given that the use of multiple signatures is quite rare, git does not
>> support creating them without jumping through a few hoops, and
>finally
>> supporting them properly would require extensive API improvement, it
>> seems reasonable to just reject them at the moment.
>>
>> Signed-off-by: Michał Górny
>> ---
>> gpg-interface.c | 90
>+++-
>> t/t7510-signed-commit.sh | 26
>> 2 files changed, 87 insertions(+), 29 deletions(-)
>>
>> Changes in v4:
>> * switched to using skip_prefix(),
>> * renamed the variable to seen_exclusive_status,
>> * made the loop terminate early on first duplicate status seen.
>
>Thanks for sticking to the topic and polishing it further. Looks
>very good.
>
>Will replace.
>
>> +int seen_exclusive_status = 0;
>> +
>> +/* Iterate over all lines */
>> +for (line = buf; *line; line = strchrnul(line+1, '\n')) {
>> +while (*line == '\n')
>> +line++;
>> +/* Skip lines that don't start with GNUPG status */
>> +if (!skip_prefix(line, "[GNUPG:] ", ))
>> +continue;
>> +
>> +/* Iterate over all search strings */
>> +for (i = 0; i < ARRAY_SIZE(sigcheck_gpg_status); i++) {
>> +if (skip_prefix(line, sigcheck_gpg_status[i].check,
>> )) {
>> +if (sigcheck_gpg_status[i].flags &
>> GPG_STATUS_EXCLUSIVE) {
>> +if (++seen_exclusive_status > 1)
>> +goto found_duplicate_status;
>
>Very minor point but by not using pre-increment, i.e.
>
> if (seen_exclusive_status++)
> goto found_duplicate_status;
>
>you can use the expression as a "have we already seen?" boolean,
>whic may probably be more idiomatic.
>
>The patch is good in the way written as-is, and this is so minor
>that it is not worth rerolling to only update this part.
Please don't merge it yet. I gave it some more thought and I think the loop
refactoring may cause TRUST_* to override BADSIG (i.e. upgrade from 'bad' to
'untrusted'). I'm going to verify this when I get home.
>
>Thanks.
--
Best regards,
Michał Górny
Re: [PATCH v4] gpg-interface.c: detect and reject multiple signatures on commits
On Sun, 2018-10-21 at 08:57 +0900, Junio C Hamano wrote:
> Michał Górny writes:
>
> > GnuPG supports creating signatures consisting of multiple signature
> > packets. If such a signature is verified, it outputs all the status
> > messages for each signature separately. However, git currently does not
> > account for such scenario and gets terribly confused over getting
> > multiple *SIG statuses.
> >
> > For example, if a malicious party alters a signed commit and appends
> > a new untrusted signature, git is going to ignore the original bad
> > signature and report untrusted commit instead. However, %GK and %GS
> > format strings may still expand to the data corresponding
> > to the original signature, potentially tricking the scripts into
> > trusting the malicious commit.
> >
> > Given that the use of multiple signatures is quite rare, git does not
> > support creating them without jumping through a few hoops, and finally
> > supporting them properly would require extensive API improvement, it
> > seems reasonable to just reject them at the moment.
> >
> > Signed-off-by: Michał Górny
> > ---
> > gpg-interface.c | 90 +++-
> > t/t7510-signed-commit.sh | 26
> > 2 files changed, 87 insertions(+), 29 deletions(-)
> >
> > Changes in v4:
> > * switched to using skip_prefix(),
> > * renamed the variable to seen_exclusive_status,
> > * made the loop terminate early on first duplicate status seen.
>
> Thanks for sticking to the topic and polishing it further. Looks
> very good.
>
> Will replace.
>
> > + int seen_exclusive_status = 0;
> > +
> > + /* Iterate over all lines */
> > + for (line = buf; *line; line = strchrnul(line+1, '\n')) {
> > + while (*line == '\n')
> > + line++;
> > + /* Skip lines that don't start with GNUPG status */
> > + if (!skip_prefix(line, "[GNUPG:] ", ))
> > + continue;
> > +
> > + /* Iterate over all search strings */
> > + for (i = 0; i < ARRAY_SIZE(sigcheck_gpg_status); i++) {
> > + if (skip_prefix(line, sigcheck_gpg_status[i].check,
> > )) {
> > + if (sigcheck_gpg_status[i].flags &
> > GPG_STATUS_EXCLUSIVE) {
> > + if (++seen_exclusive_status > 1)
> > + goto found_duplicate_status;
>
> Very minor point but by not using pre-increment, i.e.
>
> if (seen_exclusive_status++)
> goto found_duplicate_status;
>
> you can use the expression as a "have we already seen?" boolean,
> whic may probably be more idiomatic.
>
> The patch is good in the way written as-is, and this is so minor
> that it is not worth rerolling to only update this part.
>
Sure, thanks. For the record, I've been taught to use pre-increment
whenever possible to avoid copying the variable but I suppose it doesn't
really matter here. Just a habit.
I'll start working on my next ideas once this is merged and I rebase.
--
Best regards,
Michał Górny
signature.asc
Description: This is a digitally signed message part
[PATCH v4] gpg-interface.c: detect and reject multiple signatures on commits
GnuPG supports creating signatures consisting of multiple signature
packets. If such a signature is verified, it outputs all the status
messages for each signature separately. However, git currently does not
account for such scenario and gets terribly confused over getting
multiple *SIG statuses.
For example, if a malicious party alters a signed commit and appends
a new untrusted signature, git is going to ignore the original bad
signature and report untrusted commit instead. However, %GK and %GS
format strings may still expand to the data corresponding
to the original signature, potentially tricking the scripts into
trusting the malicious commit.
Given that the use of multiple signatures is quite rare, git does not
support creating them without jumping through a few hoops, and finally
supporting them properly would require extensive API improvement, it
seems reasonable to just reject them at the moment.
Signed-off-by: Michał Górny
---
gpg-interface.c | 90 +++-
t/t7510-signed-commit.sh | 26
2 files changed, 87 insertions(+), 29 deletions(-)
Changes in v4:
* switched to using skip_prefix(),
* renamed the variable to seen_exclusive_status,
* made the loop terminate early on first duplicate status seen.
diff --git a/gpg-interface.c b/gpg-interface.c
index db17d65f8..efe2c0d38 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -75,48 +75,80 @@ void signature_check_clear(struct signature_check *sigc)
FREE_AND_NULL(sigc->key);
}
+/* An exclusive status -- only one of them can appear in output */
+#define GPG_STATUS_EXCLUSIVE (1<<0)
+
static struct {
char result;
const char *check;
+ unsigned int flags;
} sigcheck_gpg_status[] = {
- { 'G', "\n[GNUPG:] GOODSIG " },
- { 'B', "\n[GNUPG:] BADSIG " },
- { 'U', "\n[GNUPG:] TRUST_NEVER" },
- { 'U', "\n[GNUPG:] TRUST_UNDEFINED" },
- { 'E', "\n[GNUPG:] ERRSIG "},
- { 'X', "\n[GNUPG:] EXPSIG "},
- { 'Y', "\n[GNUPG:] EXPKEYSIG "},
- { 'R', "\n[GNUPG:] REVKEYSIG "},
+ { 'G', "GOODSIG ", GPG_STATUS_EXCLUSIVE },
+ { 'B', "BADSIG ", GPG_STATUS_EXCLUSIVE },
+ { 'U', "TRUST_NEVER", 0 },
+ { 'U', "TRUST_UNDEFINED", 0 },
+ { 'E', "ERRSIG ", GPG_STATUS_EXCLUSIVE },
+ { 'X', "EXPSIG ", GPG_STATUS_EXCLUSIVE },
+ { 'Y', "EXPKEYSIG ", GPG_STATUS_EXCLUSIVE },
+ { 'R', "REVKEYSIG ", GPG_STATUS_EXCLUSIVE },
};
static void parse_gpg_output(struct signature_check *sigc)
{
const char *buf = sigc->gpg_status;
+ const char *line, *next;
int i;
-
- /* Iterate over all search strings */
- for (i = 0; i < ARRAY_SIZE(sigcheck_gpg_status); i++) {
- const char *found, *next;
-
- if (!skip_prefix(buf, sigcheck_gpg_status[i].check + 1,
)) {
- found = strstr(buf, sigcheck_gpg_status[i].check);
- if (!found)
- continue;
- found += strlen(sigcheck_gpg_status[i].check);
- }
- sigc->result = sigcheck_gpg_status[i].result;
- /* The trust messages are not followed by key/signer
information */
- if (sigc->result != 'U') {
- next = strchrnul(found, ' ');
- sigc->key = xmemdupz(found, next - found);
- /* The ERRSIG message is not followed by signer
information */
- if (*next && sigc-> result != 'E') {
- found = next + 1;
- next = strchrnul(found, '\n');
- sigc->signer = xmemdupz(found, next - found);
+ int seen_exclusive_status = 0;
+
+ /* Iterate over all lines */
+ for (line = buf; *line; line = strchrnul(line+1, '\n')) {
+ while (*line == '\n')
+ line++;
+ /* Skip lines that don't start with GNUPG status */
+ if (!skip_prefix(line, "[GNUPG:] ", ))
+ continue;
+
+ /* Iterate over all search strings */
+ for (i = 0; i < ARRAY_SIZE(sigcheck_gpg_status); i++) {
+ if (skip_prefix(line, sigcheck_gpg_status[i].check,
)) {
+ if (sigcheck_gpg_status[i].flags &
GPG_STATUS_EXCLUSIVE) {
+ if (++seen_exclusive_status > 1)
+ goto found_duplicate_status;
+ }
+
+ sigc->result = sigcheck_gpg_status[i].result;
+ /* The trust messages are not
Re: [PATCH v3] gpg-interface.c: detect and reject multiple signatures on commits
On Mon, 2018-10-15 at 12:31 +0900, Junio C Hamano wrote:
> Michał Górny writes:
>
> > GnuPG supports creating signatures consisting of multiple signature
> > packets. If such a signature is verified, it outputs all the status
> > messages for each signature separately. However, git currently does not
> > account for such scenario and gets terribly confused over getting
> > multiple *SIG statuses.
> >
> > For example, if a malicious party alters a signed commit and appends
> > a new untrusted signature, git is going to ignore the original bad
> > signature and report untrusted commit instead. However, %GK and %GS
> > format strings may still expand to the data corresponding
> > to the original signature, potentially tricking the scripts into
> > trusting the malicious commit.
> >
> > Given that the use of multiple signatures is quite rare, git does not
> > support creating them without jumping through a few hoops, and finally
> > supporting them properly would require extensive API improvement, it
> > seems reasonable to just reject them at the moment.
> >
> > Signed-off-by: Michał Górny
> > ---
> > gpg-interface.c | 94 +++-
> > t/t7510-signed-commit.sh | 26 +++
> > 2 files changed, 91 insertions(+), 29 deletions(-)
> >
> > Changes in v3: reworked the whole loop to iterate over lines rather
> > than scanning the whole buffer, as requested. Now it also catches
> > duplicate instances of the same status.
> >
> > diff --git a/gpg-interface.c b/gpg-interface.c
> > index db17d65f8..480aab4ee 100644
> > --- a/gpg-interface.c
> > +++ b/gpg-interface.c
> > @@ -75,48 +75,84 @@ void signature_check_clear(struct signature_check *sigc)
> > FREE_AND_NULL(sigc->key);
> > }
> >
> > +/* An exclusive status -- only one of them can appear in output */
> > +#define GPG_STATUS_EXCLUSIVE (1<<0)
> > +
> > static struct {
> > char result;
> > const char *check;
> > + unsigned int flags;
> > } sigcheck_gpg_status[] = {
> > - { 'G', "\n[GNUPG:] GOODSIG " },
> > - { 'B', "\n[GNUPG:] BADSIG " },
> > - { 'U', "\n[GNUPG:] TRUST_NEVER" },
> > - { 'U', "\n[GNUPG:] TRUST_UNDEFINED" },
> > - { 'E', "\n[GNUPG:] ERRSIG "},
> > - { 'X', "\n[GNUPG:] EXPSIG "},
> > - { 'Y', "\n[GNUPG:] EXPKEYSIG "},
> > - { 'R', "\n[GNUPG:] REVKEYSIG "},
> > + { 'G', "GOODSIG ", GPG_STATUS_EXCLUSIVE },
> > + { 'B', "BADSIG ", GPG_STATUS_EXCLUSIVE },
> > + { 'U', "TRUST_NEVER", 0 },
> > + { 'U', "TRUST_UNDEFINED", 0 },
> > + { 'E', "ERRSIG ", GPG_STATUS_EXCLUSIVE },
> > + { 'X', "EXPSIG ", GPG_STATUS_EXCLUSIVE },
> > + { 'Y', "EXPKEYSIG ", GPG_STATUS_EXCLUSIVE },
> > + { 'R', "REVKEYSIG ", GPG_STATUS_EXCLUSIVE },
> > };
> >
> > static void parse_gpg_output(struct signature_check *sigc)
> > {
> > const char *buf = sigc->gpg_status;
> > + const char *line, *next;
> > int i;
> > -
> > - /* Iterate over all search strings */
> > - for (i = 0; i < ARRAY_SIZE(sigcheck_gpg_status); i++) {
> > - const char *found, *next;
> > -
> > - if (!skip_prefix(buf, sigcheck_gpg_status[i].check + 1,
> > )) {
> > - found = strstr(buf, sigcheck_gpg_status[i].check);
> > - if (!found)
> > - continue;
> > - found += strlen(sigcheck_gpg_status[i].check);
> > - }
> > - sigc->result = sigcheck_gpg_status[i].result;
> > - /* The trust messages are not followed by key/signer
> > information */
> > - if (sigc->result != 'U') {
> > - next = strchrnul(found, ' ');
> > - sigc->key = xmemdupz(found, next - found);
> > - /* The ERRSIG message is not followed by signer
> > information */
> > - if (*next && sigc-> result != 'E') {
> > - found = next + 1;
> > - next = strchrnul(found, '\n');
> > - sigc->signer = xmemdupz(found, next - found);
> > + int had_exclusive_status = 0;
> > +
> > + /* Iterate over all lines */
> > + for (line = buf; *line; line = strchrnul(line+1, '\n'
[PATCH v3] gpg-interface.c: detect and reject multiple signatures on commits
GnuPG supports creating signatures consisting of multiple signature
packets. If such a signature is verified, it outputs all the status
messages for each signature separately. However, git currently does not
account for such scenario and gets terribly confused over getting
multiple *SIG statuses.
For example, if a malicious party alters a signed commit and appends
a new untrusted signature, git is going to ignore the original bad
signature and report untrusted commit instead. However, %GK and %GS
format strings may still expand to the data corresponding
to the original signature, potentially tricking the scripts into
trusting the malicious commit.
Given that the use of multiple signatures is quite rare, git does not
support creating them without jumping through a few hoops, and finally
supporting them properly would require extensive API improvement, it
seems reasonable to just reject them at the moment.
Signed-off-by: Michał Górny
---
gpg-interface.c | 94 +++-
t/t7510-signed-commit.sh | 26 +++
2 files changed, 91 insertions(+), 29 deletions(-)
Changes in v3: reworked the whole loop to iterate over lines rather
than scanning the whole buffer, as requested. Now it also catches
duplicate instances of the same status.
diff --git a/gpg-interface.c b/gpg-interface.c
index db17d65f8..480aab4ee 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -75,48 +75,84 @@ void signature_check_clear(struct signature_check *sigc)
FREE_AND_NULL(sigc->key);
}
+/* An exclusive status -- only one of them can appear in output */
+#define GPG_STATUS_EXCLUSIVE (1<<0)
+
static struct {
char result;
const char *check;
+ unsigned int flags;
} sigcheck_gpg_status[] = {
- { 'G', "\n[GNUPG:] GOODSIG " },
- { 'B', "\n[GNUPG:] BADSIG " },
- { 'U', "\n[GNUPG:] TRUST_NEVER" },
- { 'U', "\n[GNUPG:] TRUST_UNDEFINED" },
- { 'E', "\n[GNUPG:] ERRSIG "},
- { 'X', "\n[GNUPG:] EXPSIG "},
- { 'Y', "\n[GNUPG:] EXPKEYSIG "},
- { 'R', "\n[GNUPG:] REVKEYSIG "},
+ { 'G', "GOODSIG ", GPG_STATUS_EXCLUSIVE },
+ { 'B', "BADSIG ", GPG_STATUS_EXCLUSIVE },
+ { 'U', "TRUST_NEVER", 0 },
+ { 'U', "TRUST_UNDEFINED", 0 },
+ { 'E', "ERRSIG ", GPG_STATUS_EXCLUSIVE },
+ { 'X', "EXPSIG ", GPG_STATUS_EXCLUSIVE },
+ { 'Y', "EXPKEYSIG ", GPG_STATUS_EXCLUSIVE },
+ { 'R', "REVKEYSIG ", GPG_STATUS_EXCLUSIVE },
};
static void parse_gpg_output(struct signature_check *sigc)
{
const char *buf = sigc->gpg_status;
+ const char *line, *next;
int i;
-
- /* Iterate over all search strings */
- for (i = 0; i < ARRAY_SIZE(sigcheck_gpg_status); i++) {
- const char *found, *next;
-
- if (!skip_prefix(buf, sigcheck_gpg_status[i].check + 1,
)) {
- found = strstr(buf, sigcheck_gpg_status[i].check);
- if (!found)
- continue;
- found += strlen(sigcheck_gpg_status[i].check);
- }
- sigc->result = sigcheck_gpg_status[i].result;
- /* The trust messages are not followed by key/signer
information */
- if (sigc->result != 'U') {
- next = strchrnul(found, ' ');
- sigc->key = xmemdupz(found, next - found);
- /* The ERRSIG message is not followed by signer
information */
- if (*next && sigc-> result != 'E') {
- found = next + 1;
- next = strchrnul(found, '\n');
- sigc->signer = xmemdupz(found, next - found);
+ int had_exclusive_status = 0;
+
+ /* Iterate over all lines */
+ for (line = buf; *line; line = strchrnul(line+1, '\n')) {
+ while (*line == '\n')
+ line++;
+ /* Skip lines that don't start with GNUPG status */
+ if (strncmp(line, "[GNUPG:] ", 9))
+ continue;
+ line += 9;
+
+ /* Iterate over all search strings */
+ for (i = 0; i < ARRAY_SIZE(sigcheck_gpg_status); i++) {
+ if (!strncmp(line, sigcheck_gpg_status[i].check,
+ strlen(sigcheck_gpg_status[i].check))) {
+ line += strlen(sigcheck_gpg_status[i].check);
+
+ if (sigcheck_gpg_status[i].flags &
GPG_STATUS_EXCLUSIVE)
+ had_exclusive_status++;
+
+ sigc->result = sigcheck_gpg_status[i].result;
+
Re: [PATCH v2] gpg-interface.c: detect and reject multiple signatures on commits
On Fri, 2018-08-17 at 09:34 +0200, Michał Górny wrote: > GnuPG supports creating signatures consisting of multiple signature > packets. If such a signature is verified, it outputs all the status > messages for each signature separately. However, git currently does not > account for such scenario and gets terribly confused over getting > multiple *SIG statuses. > > For example, if a malicious party alters a signed commit and appends > a new untrusted signature, git is going to ignore the original bad > signature and report untrusted commit instead. However, %GK and %GS > format strings may still expand to the data corresponding > to the original signature, potentially tricking the scripts into > trusting the malicious commit. > > Given that the use of multiple signatures is quite rare, git does not > support creating them without jumping through a few hoops, and finally > supporting them properly would require extensive API improvement, it > seems reasonable to just reject them at the moment. > Gentle ping. -- Best regards, Michał Górny signature.asc Description: This is a digitally signed message part
Re: [PATCH] gpg-interface.c: Fix potentially freeing NULL values
On Fri, 2018-08-17 at 05:28 -0400, Eric Sunshine wrote:
> On Fri, Aug 17, 2018 at 5:17 AM Michał Górny wrote:
> > Fix signature_check_clear() to free only values that are non-NULL. This
> > especially applies to 'key' and 'signer' members that can be NULL during
> > normal operations, depending on exact GnuPG output. While at it, also
> > allow other members to be NULL to make the function easier to use,
> > even if there is no real need to account for that right now.
>
> free(NULL) is valid behavior[1] and much of the Git codebase relies upon it.
>
> Did you run into a case where it misbehaved?
Nope. I was actually wondering if it's expected, so I did a quick grep
to check whether git is checking pointers for non-NULL before free()ing,
and found at least one:
blame.c-static void drop_origin_blob(struct blame_origin *o)
blame.c-{
blame.c-if (o->file.ptr) {
blame.c:FREE_AND_NULL(o->file.ptr);
blame.c-}
blame.c-}
So I wrongly presumed it might be desirable. If it's not, that's fine
by me.
>
> [1]: http://pubs.opengroup.org/onlinepubs/9699919799/functions/free.html
>
> > Signed-off-by: Michał Górny
> > ---
> > diff --git a/gpg-interface.c b/gpg-interface.c
> > index 35c25106a..9aedaf464 100644
> > --- a/gpg-interface.c
> > +++ b/gpg-interface.c
> > @@ -15,9 +15,14 @@ static const char *gpg_program = "gpg";
> > void signature_check_clear(struct signature_check *sigc)
> > {
> > - FREE_AND_NULL(sigc->payload);
> > - FREE_AND_NULL(sigc->gpg_output);
> > - FREE_AND_NULL(sigc->gpg_status);
> > - FREE_AND_NULL(sigc->signer);
> > - FREE_AND_NULL(sigc->key);
> > + if (sigc->payload)
> > + FREE_AND_NULL(sigc->payload);
> > + if (sigc->gpg_output)
> > + FREE_AND_NULL(sigc->gpg_output);
> > + if (sigc->gpg_status)
> > + FREE_AND_NULL(sigc->gpg_status);
> > + if (sigc->signer)
> > + FREE_AND_NULL(sigc->signer);
> > + if (sigc->key)
> > + FREE_AND_NULL(sigc->key);
> > }
--
Best regards,
Michał Górny
signature.asc
Description: This is a digitally signed message part
[PATCH] gpg-interface.c: Fix potentially freeing NULL values
Fix signature_check_clear() to free only values that are non-NULL. This
especially applies to 'key' and 'signer' members that can be NULL during
normal operations, depending on exact GnuPG output. While at it, also
allow other members to be NULL to make the function easier to use,
even if there is no real need to account for that right now.
Signed-off-by: Michał Górny
---
gpg-interface.c | 15 ++-
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/gpg-interface.c b/gpg-interface.c
index 35c25106a..9aedaf464 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -15,9 +15,14 @@ static const char *gpg_program = "gpg";
void signature_check_clear(struct signature_check *sigc)
{
- FREE_AND_NULL(sigc->payload);
- FREE_AND_NULL(sigc->gpg_output);
- FREE_AND_NULL(sigc->gpg_status);
- FREE_AND_NULL(sigc->signer);
- FREE_AND_NULL(sigc->key);
+ if (sigc->payload)
+ FREE_AND_NULL(sigc->payload);
+ if (sigc->gpg_output)
+ FREE_AND_NULL(sigc->gpg_output);
+ if (sigc->gpg_status)
+ FREE_AND_NULL(sigc->gpg_status);
+ if (sigc->signer)
+ FREE_AND_NULL(sigc->signer);
+ if (sigc->key)
+ FREE_AND_NULL(sigc->key);
}
--
2.18.0
[PATCH v2] gpg-interface.c: detect and reject multiple signatures on commits
GnuPG supports creating signatures consisting of multiple signature
packets. If such a signature is verified, it outputs all the status
messages for each signature separately. However, git currently does not
account for such scenario and gets terribly confused over getting
multiple *SIG statuses.
For example, if a malicious party alters a signed commit and appends
a new untrusted signature, git is going to ignore the original bad
signature and report untrusted commit instead. However, %GK and %GS
format strings may still expand to the data corresponding
to the original signature, potentially tricking the scripts into
trusting the malicious commit.
Given that the use of multiple signatures is quite rare, git does not
support creating them without jumping through a few hoops, and finally
supporting them properly would require extensive API improvement, it
seems reasonable to just reject them at the moment.
Signed-off-by: Michał Górny
---
gpg-interface.c | 41
t/t7510-signed-commit.sh | 26 +
2 files changed, 59 insertions(+), 8 deletions(-)
Changes in v2:
* used generic 'flags' instead of boolean field,
* added test case for git-verify-commit & git-show.
diff --git a/gpg-interface.c b/gpg-interface.c
index 09ddfbc26..35c25106a 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -21,24 +21,29 @@ void signature_check_clear(struct signature_check *sigc)
FREE_AND_NULL(sigc->key);
}
+/* An exclusive status -- only one of them can appear in output */
+#define GPG_STATUS_EXCLUSIVE (1<<0)
+
static struct {
char result;
const char *check;
+ unsigned int flags;
} sigcheck_gpg_status[] = {
- { 'G', "\n[GNUPG:] GOODSIG " },
- { 'B', "\n[GNUPG:] BADSIG " },
- { 'U', "\n[GNUPG:] TRUST_NEVER" },
- { 'U', "\n[GNUPG:] TRUST_UNDEFINED" },
- { 'E', "\n[GNUPG:] ERRSIG "},
- { 'X', "\n[GNUPG:] EXPSIG "},
- { 'Y', "\n[GNUPG:] EXPKEYSIG "},
- { 'R', "\n[GNUPG:] REVKEYSIG "},
+ { 'G', "\n[GNUPG:] GOODSIG ", GPG_STATUS_EXCLUSIVE },
+ { 'B', "\n[GNUPG:] BADSIG ", GPG_STATUS_EXCLUSIVE },
+ { 'U', "\n[GNUPG:] TRUST_NEVER", 0 },
+ { 'U', "\n[GNUPG:] TRUST_UNDEFINED", 0 },
+ { 'E', "\n[GNUPG:] ERRSIG ", GPG_STATUS_EXCLUSIVE },
+ { 'X', "\n[GNUPG:] EXPSIG ", GPG_STATUS_EXCLUSIVE },
+ { 'Y', "\n[GNUPG:] EXPKEYSIG ", GPG_STATUS_EXCLUSIVE },
+ { 'R', "\n[GNUPG:] REVKEYSIG ", GPG_STATUS_EXCLUSIVE },
};
static void parse_gpg_output(struct signature_check *sigc)
{
const char *buf = sigc->gpg_status;
int i;
+ int had_exclusive_status = 0;
/* Iterate over all search strings */
for (i = 0; i < ARRAY_SIZE(sigcheck_gpg_status); i++) {
@@ -50,6 +55,10 @@ static void parse_gpg_output(struct signature_check *sigc)
continue;
found += strlen(sigcheck_gpg_status[i].check);
}
+
+ if (sigcheck_gpg_status[i].flags & GPG_STATUS_EXCLUSIVE)
+ had_exclusive_status++;
+
sigc->result = sigcheck_gpg_status[i].result;
/* The trust messages are not followed by key/signer
information */
if (sigc->result != 'U') {
@@ -62,6 +71,22 @@ static void parse_gpg_output(struct signature_check *sigc)
}
}
}
+
+ /*
+* GOODSIG, BADSIG etc. can occur only once for each signature.
+* Therefore, if we had more than one then we're dealing with multiple
+* signatures. We don't support them currently, and they're rather
+* hard to create, so something is likely fishy and we should reject
+* them altogether.
+*/
+ if (had_exclusive_status > 1) {
+ sigc->result = 'E';
+ /* Clear partial data to avoid confusion */
+ if (sigc->signer)
+ FREE_AND_NULL(sigc->signer);
+ if (sigc->key)
+ FREE_AND_NULL(sigc->key);
+ }
}
int check_signature(const char *payload, size_t plen, const char *signature,
diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh
index 6e2015ed9..51fb92a72 100755
--- a/t/t7510-signed-commit.sh
+++ b/t/t7510-signed-commit.sh
@@ -227,4 +227,30 @@ test_expect_success GPG 'log.showsignature behaves like
--show-signature' '
grep "gpg: Good signature" actual
'
+test_expect_success GPG 'detect fudged commit with double signature' '
+ sed -e "/gpgsig/,/END PGP/d" forged1 >double-base &&
+ sed -n -e "/gpgsig/,/END PGP/p" forged1 | \
+ sed -e "s/^gpgsig//;s/
Re: [PATCH] gpg-interface.c: detect and reject multiple signatures on commits
On Wed, 2018-08-15 at 14:31 -0700, Jonathan Nieder wrote:
> Michał Górny wrote:
>
> > GnuPG supports creating signatures consisting of multiple signature
> > packets. If such a signature is verified, it outputs all the status
> > messages for each signature separately. However, git currently does not
> > account for such scenario and gets terribly confused over getting
> > multiple *SIG statuses.
> >
> > For example, if a malicious party alters a signed commit and appends
> > a new untrusted signature, git is going to ignore the original bad
> > signature and report untrusted commit instead. However, %GK and %GS
> > format strings may still expand to the data corresponding
> > to the original signature, potentially tricking the scripts into
> > trusting the malicious commit.
> >
> > Given that the use of multiple signatures is quite rare, git does not
> > support creating them without jumping through a few hoops, and finally
> > supporting them properly would require extensive API improvement, it
> > seems reasonable to just reject them at the moment.
> > ---
>
> Thanks for the clear analysis and fix.
>
> May we have your sign-off? See
> https://www.kernel.org/pub/software/scm/git/docs/SubmittingPatches.html#sign-off
> (or the equivalent section of your local copy of
> Documentation/SubmittingPatches) for what this means.
Of course, I'm sorry for missing it in the original submission.
>
> > gpg-interface.c | 38 ++
> > 1 file changed, 30 insertions(+), 8 deletions(-)
> >
> > diff --git a/gpg-interface.c b/gpg-interface.c
> > index 09ddfbc26..4e03aec15 100644
> > --- a/gpg-interface.c
> > +++ b/gpg-interface.c
> > @@ -24,21 +24,23 @@ void signature_check_clear(struct signature_check *sigc)
> > static struct {
> > char result;
> > const char *check;
> > + int is_status;
> > } sigcheck_gpg_status[] = {
> > - { 'G', "\n[GNUPG:] GOODSIG " },
> > - { 'B', "\n[GNUPG:] BADSIG " },
> > - { 'U', "\n[GNUPG:] TRUST_NEVER" },
> > - { 'U', "\n[GNUPG:] TRUST_UNDEFINED" },
> > - { 'E', "\n[GNUPG:] ERRSIG "},
> > - { 'X', "\n[GNUPG:] EXPSIG "},
> > - { 'Y', "\n[GNUPG:] EXPKEYSIG "},
> > - { 'R', "\n[GNUPG:] REVKEYSIG "},
> > + { 'G', "\n[GNUPG:] GOODSIG ", 1 },
> > + { 'B', "\n[GNUPG:] BADSIG ", 1 },
> > + { 'U', "\n[GNUPG:] TRUST_NEVER", 0 },
> > + { 'U', "\n[GNUPG:] TRUST_UNDEFINED", 0 },
> > + { 'E', "\n[GNUPG:] ERRSIG ", 1},
> > + { 'X', "\n[GNUPG:] EXPSIG ", 1},
> > + { 'Y', "\n[GNUPG:] EXPKEYSIG ", 1},
> > + { 'R', "\n[GNUPG:] REVKEYSIG ", 1},
> > };
>
> nit: I wonder if making is_status into a flag field (like 'option' in
> git.c's cmd_struct) and having an explicit SIGNATURE_STATUS value to
> put there would make this easier to read.
I think that makes sense.
>
> It's not clear to me that the name is_status or SIGNATURE_STATUS
> captures what this field represents. Aren't these all sigcheck
> statuses? Can you describe briefly what distinguishes the cases where
> this should be 0 versus 1?
Yes, the name really does suck. Maybe it should be EXCLUSIVE_STATUS
or something like that, to distinguish from things that can occur
simultaneously to them.
>
> >
> > static void parse_gpg_output(struct signature_check *sigc)
> > {
> > const char *buf = sigc->gpg_status;
> > int i;
> > + int had_status = 0;
> >
> > /* Iterate over all search strings */
> > for (i = 0; i < ARRAY_SIZE(sigcheck_gpg_status); i++) {
> > @@ -50,6 +52,10 @@ static void parse_gpg_output(struct signature_check
> > *sigc)
> > continue;
> > found += strlen(sigcheck_gpg_status[i].check);
> > }
> > +
> > + if (sigcheck_gpg_status[i].is_status)
> > + had_status++;
> > +
> > sigc->result = sigcheck_gpg_status[i].result;
> > /* The trust messages are not followed by key/signer
> > information */
> > if (sigc->result != 'U') {
> > @@ -62,6 +68,22 @@ static void parse_gpg_output(struct signature_check
> > *sigc)
> > }
> > }
> > }
> > +
> > + /*
> > +* GOODSIG, BADSIG etc. can occur only once for each signature.
> > +* Therefore, if we had more than one then we're dealing w
Re: Potential vulnerability: 'mixed up' output when commit has multiple signatures
On Tue, 2018-08-14 at 22:35 -0700, Jonathan Nieder wrote: > Hi, > > Michał Górny wrote: > > > I've been testing the git signature verification a bit and I've > > discovered a troubling behavior when the commit object contains > > multiple signatures. > > Thanks for discovering this. Do you mind if I take this conversation > to the public mailing list? (I'd bounce the existing thread there if > that's okay with you.) > I've already asked somewhere else in the thread if you consider this suitable for disclosure, and haven't received a reply yet. In any case, I don't mind it. I can resend my patch there if necessary too. -- Best regards, Michał Górny signature.asc Description: This is a digitally signed message part
Re: [RFC PATCH] checkout: Force matching mtime between files
W dniu sob, 28.04.2018 o godzinie 16∶23 +0200, użytkownik Duy Nguyen napisał: > On Thu, Apr 26, 2018 at 4:46 PM, Michał Górny <mgo...@gentoo.org> wrote: > > For the record, we're using this with ebuilds and respective cache files > > (which are expensive to generate). We are using separate repository > > which combines sources and cache files to keep the development > > repository clean. I have researched different solutions for this but > > git turned out the best option for incremental updates for us. > > > > Tarballs are out of question, unless you expect users to fetch >100 MiB > > every time, and they are also expensive to update. Deltas of tarballs > > are just slow and require storing a lot of extra data. Rsync is not > > very efficient at frequent updates, and has significant overhead > > on every run. With all its disadvantages, git is still something that > > lets our users fetch updates frequently with minimal network overhead. > > I assume you're talking about the metadata directory in gentoo-x86 > repo. This specific case could be solved by renaming metadata to > _metadata or something to put it on the top. "git checkout" always > updates files in strcmp(path) order. This guarantees time(_metadata) > <= time(ebuild) for all ebuilds without any extra touching (either in > git or in a post-checkout hook) We can't really rename it without breaking compatibility with all package managers out there. Preparing to do such a major change for the sake of abusing implementation detail of git doesn't look like a worthwhile idea. > > The behavior has been this way since forever and as far as I can tell > very unlikely to change at least for branch switching (major changes > involved around the index). It's a bit easier to accidentally change > how "git checkout -- path" works though. I don't know if we could just > make this checkout order a promise and guarantee not to break it > though. For it it does not sound like it adds extra maintenance > burden. -- Best regards, Michał Górny
Re: [RFC PATCH] checkout: Force matching mtime between files
W dniu śro, 25.04.2018 o godzinie 11∶18 -0400, użytkownik Marc Branchaud
napisał:
> On 2018-04-25 04:48 AM, Junio C Hamano wrote:
> > "Robin H. Johnson" <robb...@gentoo.org> writes:
> >
> > > In the thread from 6 years ago, you asked about tar's behavior for
> > > mtimes. 'tar xf' restores mtimes from the tar archive, so relative
> > > ordering after restore would be the same, and would only rebuild if the
> > > original source happened to be dirty.
> > >
> > > This behavior is already non-deterministic in Git, and would be improved
> > > by the patch.
> >
> > But Git is not an archiver (tar), but is a source code control
> > system, so I do not think we should spend any extra cycles to
> > "improve" its behaviour wrt the relative ordering, at least for the
> > default case. Only those who rely on having build artifact *and*
> > source should pay the runtime (and preferrably also the
> > maintainance) cost.
>
> Anyone who uses "make" or some other mtime-based tool is affected by
> this. I agree that it's not "Everyone" but it sure is a lot of people.
>
> Are we all that sure that the performance hit is that drastic? After
> all, we've just done write_entry(). Calling utime() at that point
> should just hit the filesystem cache.
>
> > The best approach to do so is to have those people do the "touch"
> > thing in their own post-checkout hook. People who use Git as the
> > source control system won't have to pay runtime cost of doing the
> > touch thing, and we do not have to maintain such a hook script.
> > Only those who use the "feature" would.
>
> The post-checkout hook approach is not exactly straightforward.
>
> Naively, it's simply
>
> for F in `git diff --name-only $1 $2`; do touch "$F"; done
>
> But consider:
>
> * Symlinks can cause the wrong file to be touched. (Granted, Michał's
> proposed patch also doesn't deal with symlinks.) Let's assume that a
> hook can be crafted will all possible sophistication. There are still
> some fundamental problems:
>
> * In a "file checkout" ("git checkout -- path/to/file"), $1 and $2 are
> identical so the above loop does nothing. Offhand I'm not even sure how
> a hook might get the right files in this case.
>
> * The hook has to be set up in every repo and submodule (at least until
> something like Ævar's experiments come to fruition).
>
> * A fresh clone can't run the hook. This is especially important when
> dealing with submodules. (In one case where we were bit by this, make
> though that half of a fresh submodule clone's files were stale, and
> decided to re-autoconf the entire thing.)
>
>
> I just don't think the hook approach can completely solve the problem.
>
There's also the performance aspect. If we deal with checkouts that
include 1000+ files on a busy system (i.e. when mtimes really become
relevant), calling utime() instantly has a good chance of hitting warm
cache. On the other hand, post-checkout hook has a greater risk of
running cold cache, i.e. writing to all inodes twice.
--
Best regards,
Michał Górny
Re: [RFC PATCH] checkout: Force matching mtime between files
W dniu czw, 26.04.2018 o godzinie 10∶25 +0900, użytkownik Junio C Hamano napisał: > Marc Branchaud <marcn...@xiplink.com> writes: > > > > But Git is not an archiver (tar), but is a source code control > > > system, so I do not think we should spend any extra cycles to > > > "improve" its behaviour wrt the relative ordering, at least for the > > > default case. Only those who rely on having build artifact *and* > > > source should pay the runtime (and preferrably also the > > > maintainance) cost. > > > > Anyone who uses "make" or some other mtime-based tool is affected by > > this. I agree that it's not "Everyone" but it sure is a lot of > > people. > > That's an exaggerated misrepresentation. Only those who put build > artifacts as well as source to SCM *AND* depend on mtime are > affected. > > A shipped tarball often contain configure.in as well as generated > configure, so that consumers can just say ./configure without having > the whole autoconf toolchain to regenerate it (I also heard horror > stories that this is done to control the exact version of autoconf > to avoid compatibility issues), but do people arrange configure to > be regenerated from configure.in in their Makefile of such a project > automatically when building the default target? In any case, that is > a tarball usecase, not a SCM one. > > > Are we all that sure that the performance hit is that drastic? After > > all, we've just done write_entry(). Calling utime() at that point > > should just hit the filesystem cache. > > I do not know about others, but I personally am more disburbed by > the conceptual ugliness that comes from having to have such a piece > of code in the codebase. For the record, we're using this with ebuilds and respective cache files (which are expensive to generate). We are using separate repository which combines sources and cache files to keep the development repository clean. I have researched different solutions for this but git turned out the best option for incremental updates for us. Tarballs are out of question, unless you expect users to fetch >100 MiB every time, and they are also expensive to update. Deltas of tarballs are just slow and require storing a lot of extra data. Rsync is not very efficient at frequent updates, and has significant overhead on every run. With all its disadvantages, git is still something that lets our users fetch updates frequently with minimal network overhead. So what did I do to deserve being called insane here? Is it because I wanted to use the tools that work for us? Because I figured out that I can improve our use case without really harming anyone in the process? -- Best regards, Michał Górny
Re: [RFC PATCH] checkout: Force matching mtime between files
W dniu śro, 25.04.2018 o godzinie 06∶58 +, użytkownik Robin H.
Johnson napisał:
> On Fri, Apr 13, 2018 at 07:01:29PM +0200, Michał Górny wrote:
> > --- a/entry.c
> > +++ b/entry.c
> > @@ -411,6 +411,7 @@ int checkout_entry(struct cache_entry *ce,
> > {
> > static struct strbuf path = STRBUF_INIT;
> > struct stat st;
> > + int ret;
> >
> > if (topath)
> > return write_entry(ce, topath, state, 1);
>
> mgorny: Should the topath case trigger utime as well?
I don't think so. AFAIU topath case is only used by 'git checkout-index
--all', and it implies that data is written to temporary files rather
than actual working copy files, so mtimes do not really matter there.
>
> Other questions:
> - Would there be be any value in hoisting the utime change into
> write_entry's finish block rather than having it in checkout_entry?
I've attempted to reduce the scope of my changes to minimum, therefore
checkout_entry() seemed like the 'closest' thing to where it is set
and where it could be implemented. I see no problem in doing that
in write_entry(), though. In fact, it might be useful to do that before
filling the stat cache.
> - Should mtimes on directories be set if the directory is explicitly
> created?
I don't think there's really a purpose in that. I can't think of any
reason why anybody would be able to use directory mtimes reliably, so
maybe keeping their standard behavior is better here.
> - Maybe using futimens on supported platforms?
I'm all for that. However, this is something the git maintainers should
decide as it probably implies some maintenance burden.
--
Best regards,
Michał Górny
[RFC PATCH] checkout: Force matching mtime between files
Currently git does not control mtimes of files being checked out. This
means that the only assumption you could make is that all files created
or modified within a single checkout action will have mtime between
start time and end time of this checkout. The relations between mtimes
of different files depend on the order in which they are checked out,
filesystem speed and timestamp precision.
Git repositories sometimes contain both generated and respective source
files. For example, the Gentoo 'user syncing' repository combines
source ebuild files with generated metadata cache for user convenience.
Ideally, the 'git checkout' would be fast enough that (combined with low
timestamp precision) all files created or modified within a single
checkout would have matching timestamp. However, in reality the cache
files may end up being wrongly 'older' than source file, requiring
unnecessary recheck.
The opposite problem (cache files not being regenerated when they should
have been) might also occur. However, it could not be solved without
preserving timestamps, therefore it is out of scope of this change.
In order to avoid unnecessary cache mismatches, force a matching mtime
between all files created by a single checkout action. This seems to be
the best course of action. Matching mtimes do not trigger cache
updates. They also match the concept of 'checkout' being an atomic
action. Finally, this change does not break backwards compatibility
as the new result is a subset of the possible previous results.
For example, let's say that 'git checkout' creates two files in order,
with respective timestamps T1 and T2. Before this patch, T1 <= T2.
After this patch, T1 == T2 which also satisfies T1 <= T2.
A similar problem was previously being addressed via git-restore-mtime
tool. However, that solution is unnecessarily complex for Gentoo's use
case and does not seem to be suitable for 'seamless' integration.
The patch integrates mtime forcing via adding an additional member of
'struct checkout'. This seemed the simplest way of adding it without
having to modify prototypes and adjust multiple call sites. The member
is set to the current time in check_updates() function and is afterwards
enforced by checkout_entry().
The patch uses utime() rather than futimes() as that seems to be
the function used everywhere else in git and provided some MinGW
compatibility code.
Signed-off-by: Michał Górny <mgo...@gentoo.org>
---
cache.h| 1 +
entry.c| 12 +++-
unpack-trees.c | 1 +
3 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/cache.h b/cache.h
index bbaf5c349..9f0a7c867 100644
--- a/cache.h
+++ b/cache.h
@@ -1526,6 +1526,7 @@ struct checkout {
const char *base_dir;
int base_dir_len;
struct delayed_checkout *delayed_checkout;
+ time_t checkout_mtime;
unsigned force:1,
quiet:1,
not_new:1,
diff --git a/entry.c b/entry.c
index 2101201a1..7ee5a6d28 100644
--- a/entry.c
+++ b/entry.c
@@ -411,6 +411,7 @@ int checkout_entry(struct cache_entry *ce,
{
static struct strbuf path = STRBUF_INIT;
struct stat st;
+ int ret;
if (topath)
return write_entry(ce, topath, state, 1);
@@ -473,5 +474,14 @@ int checkout_entry(struct cache_entry *ce,
return 0;
create_directories(path.buf, path.len, state);
- return write_entry(ce, path.buf, state, 0);
+ ret = write_entry(ce, path.buf, state, 0);
+
+ if (ret == 0 && state->checkout_mtime != 0) {
+ struct utimbuf t;
+ t.modtime = state->checkout_mtime;
+ if (utime(path.buf, ) < 0)
+ warning_errno("failed utime() on %s", path.buf);
+ }
+
+ return ret;
}
diff --git a/unpack-trees.c b/unpack-trees.c
index e73745051..e1efefb68 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -346,6 +346,7 @@ static int check_updates(struct unpack_trees_options *o)
state.quiet = 1;
state.refresh_cache = 1;
state.istate = index;
+ state.checkout_mtime = time(NULL);
progress = get_progress(o);
--
2.17.0
