| Message ID | 20211130141112.78193-8-fs@gigacodes.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ssh signing: verify key lifetime | expand |
Fabian Stelzer <fs@gigacodes.de> writes: > Do a full ssh signing, find-principals and verify operation in the test > prereq's to make sure ssh-keygen works as expected. Only generating the > keys and verifying its presence is not sufficient in some situations. > One example was ssh-keygen creating unusable ssh keys in cygwin because > of unsafe default permissions for the key files. The other a broken > openssh 8.7 that segfaulted on any find-principals operation. This > extended prereq check avoids future test breakages in case ssh-keygen or > any environment behaviour changes. > > Signed-off-by: Fabian Stelzer <fs@gigacodes.de> > --- The way keys are set-up has become much easier to follow. This unfortunately interacts with the old way of adding a test key done in <20211119150707.3924636-2-fs@gigacodes.de> 350a2518 (ssh signing: support non ssh-* keytypes, 2021-11-19) Here is my attempt (which is in 'seen') to resolve the inevitable merge conflicts between the topics. Thanks. commit fa6c2973744b419c95b5eaf6a697c795ab7823fa Merge: 2a8505f6a0 3b4b5a793a Author: Junio C Hamano <gitster@pobox.com> Date: Wed Dec 1 16:01:54 2021 -0800 Merge branch 'fs/ssh-signing-other-keytypes' into jch * fs/ssh-signing-other-keytypes: ssh signing: make sign/amend test more resilient ssh signing: support non ssh-* keytypes diff --git a/t/lib-gpg.sh b/t/lib-gpg.sh index ff944f0548..3e7ee1386a 100644 --- a/t/lib-gpg.sh +++ b/t/lib-gpg.sh @@ -117,13 +117,14 @@ test_lazy_prereq GPGSSH ' ssh-keygen -t ed25519 -N "" -C "git ed25519 key" -f "${GPGSSH_KEY_PRIMARY}" >/dev/null && ssh-keygen -t rsa -b 2048 -N "" -C "git rsa2048 key" -f "${GPGSSH_KEY_SECONDARY}" >/dev/null && ssh-keygen -t ed25519 -N "${GPGSSH_KEY_PASSPHRASE}" -C "git ed25519 encrypted key" -f "${GPGSSH_KEY_WITH_PASSPHRASE}" >/dev/null && -<<<<<<< 2a8505f6a0 (Merge branch 'fs/ssh-signing-key-lifetime' into jch) + ssh-keygen -t ecdsa -N "" -f "${GPGSSH_KEY_ECDSA}" >/dev/null && ssh-keygen -t ed25519 -N "" -C "git ed25519 key" -f "${GPGSSH_KEY_UNTRUSTED}" >/dev/null && cat >"${GPGSSH_ALLOWED_SIGNERS}" <<-EOF && "principal with number 1" $(cat "${GPGSSH_KEY_PRIMARY}.pub")" "principal with number 2" $(cat "${GPGSSH_KEY_SECONDARY}.pub")" "principal with number 3" $(cat "${GPGSSH_KEY_WITH_PASSPHRASE}.pub")" + "principal with number 4" $(cat "${GPGSSH_KEY_ECDSA}.pub")" EOF # Verify if at least one key and ssh-keygen works as expected @@ -166,15 +167,6 @@ test_lazy_prereq GPGSSH_VERIFYTIME ' echo "testpayload" | ssh-keygen -Y sign -n "git" -f "${GPGSSH_KEY_EXPIRED}" >gpgssh_verifytime_prereq.sig && ! (ssh-keygen -Y verify -n "git" -f "${GPGSSH_ALLOWED_SIGNERS}" -I "principal with expired key" -s gpgssh_verifytime_prereq.sig) -||||||| cd3e606211 - echo "\"principal with number 3\" $(cat "${GPGSSH_KEY_WITH_PASSPHRASE}.pub")" >> "${GPGSSH_ALLOWED_SIGNERS}" && - ssh-keygen -t ed25519 -N "" -f "${GPGSSH_KEY_UNTRUSTED}" >/dev/null -======= - echo "\"principal with number 3\" $(cat "${GPGSSH_KEY_WITH_PASSPHRASE}.pub")" >> "${GPGSSH_ALLOWED_SIGNERS}" && - ssh-keygen -t ecdsa -N "" -f "${GPGSSH_KEY_ECDSA}" >/dev/null - echo "\"principal with number 4\" $(cat "${GPGSSH_KEY_ECDSA}.pub")" >> "${GPGSSH_ALLOWED_SIGNERS}" && - ssh-keygen -t ed25519 -N "" -f "${GPGSSH_KEY_UNTRUSTED}" >/dev/null ->>>>>>> 3b4b5a793a (ssh signing: make sign/amend test more resilient) ' sanitize_pgp() {
On 01.12.2021 16:18, Junio C Hamano wrote: >Fabian Stelzer <fs@gigacodes.de> writes: > >> Do a full ssh signing, find-principals and verify operation in the test >> prereq's to make sure ssh-keygen works as expected. Only generating the >> keys and verifying its presence is not sufficient in some situations. >> One example was ssh-keygen creating unusable ssh keys in cygwin because >> of unsafe default permissions for the key files. The other a broken >> openssh 8.7 that segfaulted on any find-principals operation. This >> extended prereq check avoids future test breakages in case ssh-keygen or >> any environment behaviour changes. >> >> Signed-off-by: Fabian Stelzer <fs@gigacodes.de> >> --- > >The way keys are set-up has become much easier to follow. > >This unfortunately interacts with the old way of adding a test key >done in <20211119150707.3924636-2-fs@gigacodes.de> 350a2518 (ssh >signing: support non ssh-* keytypes, 2021-11-19) > >Here is my attempt (which is in 'seen') to resolve the inevitable >merge conflicts between the topics. Yes, that looks good. In this case the conflict is rather trivial, but how could i prevent this / make it easier for you to merge these? Especially since in this case the conflict only arose after a reroll when both topics were already in seen. For a new topic i can of course make them as "on top of XXX". Should I in the future rebase the "support non ssh-* keytypes" topic on top of this series and mark it as such? Or whats a good way to deal with things like this? (besides avoiding merge conflicts altogether :D) Thanks > >Thanks. > >commit fa6c2973744b419c95b5eaf6a697c795ab7823fa >Merge: 2a8505f6a0 3b4b5a793a >Author: Junio C Hamano <gitster@pobox.com> >Date: Wed Dec 1 16:01:54 2021 -0800 > > Merge branch 'fs/ssh-signing-other-keytypes' into jch > > * fs/ssh-signing-other-keytypes: > ssh signing: make sign/amend test more resilient > ssh signing: support non ssh-* keytypes > >diff --git a/t/lib-gpg.sh b/t/lib-gpg.sh >index ff944f0548..3e7ee1386a 100644 >--- a/t/lib-gpg.sh >+++ b/t/lib-gpg.sh >@@ -117,13 +117,14 @@ test_lazy_prereq GPGSSH ' > ssh-keygen -t ed25519 -N "" -C "git ed25519 key" -f "${GPGSSH_KEY_PRIMARY}" >/dev/null && > ssh-keygen -t rsa -b 2048 -N "" -C "git rsa2048 key" -f "${GPGSSH_KEY_SECONDARY}" >/dev/null && > ssh-keygen -t ed25519 -N "${GPGSSH_KEY_PASSPHRASE}" -C "git ed25519 encrypted key" -f "${GPGSSH_KEY_WITH_PASSPHRASE}" >/dev/null && >-<<<<<<< 2a8505f6a0 (Merge branch 'fs/ssh-signing-key-lifetime' into jch) >+ ssh-keygen -t ecdsa -N "" -f "${GPGSSH_KEY_ECDSA}" >/dev/null && > ssh-keygen -t ed25519 -N "" -C "git ed25519 key" -f "${GPGSSH_KEY_UNTRUSTED}" >/dev/null && > > cat >"${GPGSSH_ALLOWED_SIGNERS}" <<-EOF && > "principal with number 1" $(cat "${GPGSSH_KEY_PRIMARY}.pub")" > "principal with number 2" $(cat "${GPGSSH_KEY_SECONDARY}.pub")" > "principal with number 3" $(cat "${GPGSSH_KEY_WITH_PASSPHRASE}.pub")" >+ "principal with number 4" $(cat "${GPGSSH_KEY_ECDSA}.pub")" > EOF > > # Verify if at least one key and ssh-keygen works as expected >@@ -166,15 +167,6 @@ test_lazy_prereq GPGSSH_VERIFYTIME ' > echo "testpayload" | > ssh-keygen -Y sign -n "git" -f "${GPGSSH_KEY_EXPIRED}" >gpgssh_verifytime_prereq.sig && > ! (ssh-keygen -Y verify -n "git" -f "${GPGSSH_ALLOWED_SIGNERS}" -I "principal with expired key" -s gpgssh_verifytime_prereq.sig) >-||||||| cd3e606211 >- echo "\"principal with number 3\" $(cat "${GPGSSH_KEY_WITH_PASSPHRASE}.pub")" >> "${GPGSSH_ALLOWED_SIGNERS}" && >- ssh-keygen -t ed25519 -N "" -f "${GPGSSH_KEY_UNTRUSTED}" >/dev/null >-======= >- echo "\"principal with number 3\" $(cat "${GPGSSH_KEY_WITH_PASSPHRASE}.pub")" >> "${GPGSSH_ALLOWED_SIGNERS}" && >- ssh-keygen -t ecdsa -N "" -f "${GPGSSH_KEY_ECDSA}" >/dev/null >- echo "\"principal with number 4\" $(cat "${GPGSSH_KEY_ECDSA}.pub")" >> "${GPGSSH_ALLOWED_SIGNERS}" && >- ssh-keygen -t ed25519 -N "" -f "${GPGSSH_KEY_UNTRUSTED}" >/dev/null >->>>>>>> 3b4b5a793a (ssh signing: make sign/amend test more resilient) > ' > > sanitize_pgp() {
Fabian Stelzer <fs@gigacodes.de> writes: > Yes, that looks good. In this case the conflict is rather trivial, but > how could i prevent this / make it easier for you to merge these? > Especially since in this case the conflict only arose after a reroll > when both topics were already in seen. For a new topic i can of course > make them as "on top of XXX". Should I in the future rebase the > "support non ssh-* keytypes" topic on top of this series and mark it > as such? Or whats a good way to deal with things like this? (besides > avoiding merge conflicts altogether :D) For this particular one, my rerere database already knows how the conflict and its resolution should look like, so there is nothing that is urgently needed. If the other topic were to be rerolled, since either has hit 'next', basing it on top of the other, essentially making the result into a single series, may be the easiest (and that is basically avoiding conflicts altogether ;-). Thanks.
On Thu, Dec 02 2021, Junio C Hamano wrote: > Fabian Stelzer <fs@gigacodes.de> writes: > >> Yes, that looks good. In this case the conflict is rather trivial, but >> how could i prevent this / make it easier for you to merge these? >> Especially since in this case the conflict only arose after a reroll >> when both topics were already in seen. For a new topic i can of course >> make them as "on top of XXX". Should I in the future rebase the >> "support non ssh-* keytypes" topic on top of this series and mark it >> as such? Or whats a good way to deal with things like this? (besides >> avoiding merge conflicts altogether :D) > > For this particular one, my rerere database already knows how the > conflict and its resolution should look like, so there is nothing > that is urgently needed. > > If the other topic were to be rerolled, since either has hit 'next', > basing it on top of the other, essentially making the result into a > single series, may be the easiest (and that is basically avoiding > conflicts altogether ;-). ...but to answer a bit of Fabian's question: Just as someone giving these two topics a brief look it's not clear to me why the existing GPGSSH prerequisite needs an adjustment at the same time as adding a test that uses it (in addition to existing tests). I.e. was it that it was always wrong, in that case I'd expect a patch that fixes the prereq and doesn't make any other test changes in the same commit as [1] does. Or does it need to be more strict to cater to one new test being added in the same commit, but that strictness doesn't apply to existing tests? Then maybe it should be a new GPGSSH_THAT_NEW_REQUIRED_FEATURE, which can in turn depend on the GPGSSH prerequisite. Which, incidentally would help with any textual conflict, but more importantly makes for clearer end-state, and maps prerequisites to those existing tests that need those specific things, and not a more stricter & recent requirement. I don't know/think that any of this needs re-rolling, just my 0.02. 1. https://lore.kernel.org/git/20211119150707.3924636-2-fs@gigacodes.de/
On 03.12.2021 12:07, Ævar Arnfjörð Bjarmason wrote: > >On Thu, Dec 02 2021, Junio C Hamano wrote: > >> Fabian Stelzer <fs@gigacodes.de> writes: >> >>> Yes, that looks good. In this case the conflict is rather trivial, but >>> how could i prevent this / make it easier for you to merge these? >>> Especially since in this case the conflict only arose after a reroll >>> when both topics were already in seen. For a new topic i can of course >>> make them as "on top of XXX". Should I in the future rebase the >>> "support non ssh-* keytypes" topic on top of this series and mark it >>> as such? Or whats a good way to deal with things like this? (besides >>> avoiding merge conflicts altogether :D) >> >> For this particular one, my rerere database already knows how the >> conflict and its resolution should look like, so there is nothing >> that is urgently needed. >> >> If the other topic were to be rerolled, since either has hit 'next', >> basing it on top of the other, essentially making the result into a >> single series, may be the easiest (and that is basically avoiding >> conflicts altogether ;-). > >...but to answer a bit of Fabian's question: Just as someone giving >these two topics a brief look it's not clear to me why the existing >GPGSSH prerequisite needs an adjustment at the same time as adding a >test that uses it (in addition to existing tests). > >I.e. was it that it was always wrong, in that case I'd expect a patch >that fixes the prereq and doesn't make any other test changes in the >same commit as [1] does. > >Or does it need to be more strict to cater to one new test being added >in the same commit, but that strictness doesn't apply to existing tests? > >Then maybe it should be a new GPGSSH_THAT_NEW_REQUIRED_FEATURE, which >can in turn depend on the GPGSSH prerequisite. > What [1] needed was another keytype set up for some tests to run. I could have done this in the test itself or in a separate setup function. But since all the other keys are set up in the prereq and we might want to reuse this new ecdsa key in other tests as well this seemed like the better place for it. A new prereq depending on GPGSSH feels kinda wrong to me when it would just create the key and always succeed otherwise. I guess this is a side effect of using the prereq to do setup as well (something that we discussed in https://lore.kernel.org/git/YYXAwxmhrLLMBqa+@coredump.intra.peff.net/ ). >Which, incidentally would help with any textual conflict, but more >importantly makes for clearer end-state, and maps prerequisites to those >existing tests that need those specific things, and not a more stricter >& recent requirement. > The problem here was just that I also refactored the existing GPGSSH prereq to be more readable while doing the same for the new one. Only refactoring the new one while leaving the older one just above it untouched would have avoided the conflict but would have needed a follow up series. In the future I would probably do that or leave the code as is and refactor both in a follow up. >I don't know/think that any of this needs re-rolling, just my 0.02. > >1. https://lore.kernel.org/git/20211119150707.3924636-2-fs@gigacodes.de/ Always glad for your input. Thanks
Junio C Hamano <gitster@pobox.com> writes: > Fabian Stelzer <fs@gigacodes.de> writes: > >> Yes, that looks good. In this case the conflict is rather trivial, but >> how could i prevent this / make it easier for you to merge these? >> Especially since in this case the conflict only arose after a reroll >> when both topics were already in seen. For a new topic i can of course >> make them as "on top of XXX". Should I in the future rebase the >> "support non ssh-* keytypes" topic on top of this series and mark it >> as such? Or whats a good way to deal with things like this? (besides >> avoiding merge conflicts altogether :D) > > For this particular one, my rerere database already knows how the > conflict and its resolution should look like, so there is nothing > that is urgently needed. > > If the other topic were to be rerolled, since either has hit 'next', Sorry, "since neither has hit 'next' yet," was what I meant to type. Not being in 'next' allows us to replace the series without having to do "oops, that earlier one was wrong, and here is a band-aid". Thanks. > basing it on top of the other, essentially making the result into a > single series, may be the easiest (and that is basically avoiding > conflicts altogether ;-). > > Thanks.
diff --git a/t/lib-gpg.sh b/t/lib-gpg.sh index fc03c8f89b..3fadfcb306 100644 --- a/t/lib-gpg.sh +++ b/t/lib-gpg.sh @@ -109,34 +109,61 @@ test_lazy_prereq GPGSSH ' echo $ssh_version | grep -q "find-principals:missing signature file" test $? = 0 || exit 1; - # some broken versions of ssh-keygen segfault on find-principals; - # avoid testing with them. - ssh-keygen -Y find-principals -f /dev/null -s /dev/null - test $? = 139 && exit 1 - + # Setup some keys and an allowed signers file mkdir -p "${GNUPGHOME}" && chmod 0700 "${GNUPGHOME}" && (setfacl -k "${GNUPGHOME}" 2>/dev/null || true) && ssh-keygen -t ed25519 -N "" -C "git ed25519 key" -f "${GPGSSH_KEY_PRIMARY}" >/dev/null && - echo "\"principal with number 1\" $(cat "${GPGSSH_KEY_PRIMARY}.pub")" >> "${GPGSSH_ALLOWED_SIGNERS}" && ssh-keygen -t rsa -b 2048 -N "" -C "git rsa2048 key" -f "${GPGSSH_KEY_SECONDARY}" >/dev/null && - echo "\"principal with number 2\" $(cat "${GPGSSH_KEY_SECONDARY}.pub")" >> "${GPGSSH_ALLOWED_SIGNERS}" && ssh-keygen -t ed25519 -N "${GPGSSH_KEY_PASSPHRASE}" -C "git ed25519 encrypted key" -f "${GPGSSH_KEY_WITH_PASSPHRASE}" >/dev/null && - echo "\"principal with number 3\" $(cat "${GPGSSH_KEY_WITH_PASSPHRASE}.pub")" >> "${GPGSSH_ALLOWED_SIGNERS}" && - ssh-keygen -t ed25519 -N "" -C "git ed25519 key" -f "${GPGSSH_KEY_UNTRUSTED}" >/dev/null + ssh-keygen -t ed25519 -N "" -C "git ed25519 key" -f "${GPGSSH_KEY_UNTRUSTED}" >/dev/null && + + cat >"${GPGSSH_ALLOWED_SIGNERS}" <<-EOF && + "principal with number 1" $(cat "${GPGSSH_KEY_PRIMARY}.pub")" + "principal with number 2" $(cat "${GPGSSH_KEY_SECONDARY}.pub")" + "principal with number 3" $(cat "${GPGSSH_KEY_WITH_PASSPHRASE}.pub")" + EOF + + # Verify if at least one key and ssh-keygen works as expected + echo "testpayload" | + ssh-keygen -Y sign -n "git" -f "${GPGSSH_KEY_PRIMARY}" >gpgssh_prereq.sig && + ssh-keygen -Y find-principals -f "${GPGSSH_ALLOWED_SIGNERS}" -s gpgssh_prereq.sig && + echo "testpayload" | + ssh-keygen -Y verify -n "git" -f "${GPGSSH_ALLOWED_SIGNERS}" -I "principal with number 1" -s gpgssh_prereq.sig ' test_lazy_prereq GPGSSH_VERIFYTIME ' # Check if ssh-keygen has a verify-time option by passing an invalid date to it ssh-keygen -Overify-time=INVALID -Y check-novalidate -s doesnotmatter 2>&1 | grep -q -F "Invalid \"verify-time\"" && + + # Set up keys with key lifetimes ssh-keygen -t ed25519 -N "" -C "timeboxed valid key" -f "${GPGSSH_KEY_TIMEBOXEDVALID}" >/dev/null && - echo "\"timeboxed valid key\" valid-after=\"20050407000000\",valid-before=\"200504100000\" $(cat "${GPGSSH_KEY_TIMEBOXEDVALID}.pub")" >> "${GPGSSH_ALLOWED_SIGNERS}" && + key_valid=$(cat "${GPGSSH_KEY_TIMEBOXEDVALID}.pub") && ssh-keygen -t ed25519 -N "" -C "timeboxed invalid key" -f "${GPGSSH_KEY_TIMEBOXEDINVALID}" >/dev/null && - echo "\"timeboxed invalid key\" valid-after=\"20050401000000\",valid-before=\"20050402000000\" $(cat "${GPGSSH_KEY_TIMEBOXEDINVALID}.pub")" >> "${GPGSSH_ALLOWED_SIGNERS}" && + key_invalid=$(cat "${GPGSSH_KEY_TIMEBOXEDINVALID}.pub") && ssh-keygen -t ed25519 -N "" -C "expired key" -f "${GPGSSH_KEY_EXPIRED}" >/dev/null && - echo "\"principal with expired key\" valid-before=\"20000101000000\" $(cat "${GPGSSH_KEY_EXPIRED}.pub")" >> "${GPGSSH_ALLOWED_SIGNERS}" && + key_expired=$(cat "${GPGSSH_KEY_EXPIRED}.pub") && ssh-keygen -t ed25519 -N "" -C "not yet valid key" -f "${GPGSSH_KEY_NOTYETVALID}" >/dev/null && - echo "\"principal with not yet valid key\" valid-after=\"29990101000000\" $(cat "${GPGSSH_KEY_NOTYETVALID}.pub")" >> "${GPGSSH_ALLOWED_SIGNERS}" + key_notyetvalid=$(cat "${GPGSSH_KEY_NOTYETVALID}.pub") && + + # Timestamps outside of test_tick span + ts2005a=20050401000000 ts2005b=200504020000 && + # Timestamps within test_tick span + ts2005c=20050407000000 ts2005d=200504100000 && + # Definitely not yet valid / expired timestamps + ts2000=20000101000000 ts2999=29990101000000 && + + cat >>"${GPGSSH_ALLOWED_SIGNERS}" <<-EOF && + "timeboxed valid key" valid-after="$ts2005c",valid-before="$ts2005d" $key_valid" + "timeboxed invalid key" valid-after="$ts2005a",valid-before="$ts2005b" $key_invalid" + "principal with expired key" valid-before="$ts2000" $key_expired" + "principal with not yet valid key" valid-after="$ts2999" $key_notyetvalid" + EOF + + # and verify ssh-keygen verifies the key lifetime + echo "testpayload" | + ssh-keygen -Y sign -n "git" -f "${GPGSSH_KEY_EXPIRED}" >gpgssh_verifytime_prereq.sig && + ! (ssh-keygen -Y verify -n "git" -f "${GPGSSH_ALLOWED_SIGNERS}" -I "principal with expired key" -s gpgssh_verifytime_prereq.sig) ' sanitize_pgp() {
Do a full ssh signing, find-principals and verify operation in the test prereq's to make sure ssh-keygen works as expected. Only generating the keys and verifying its presence is not sufficient in some situations. One example was ssh-keygen creating unusable ssh keys in cygwin because of unsafe default permissions for the key files. The other a broken openssh 8.7 that segfaulted on any find-principals operation. This extended prereq check avoids future test breakages in case ssh-keygen or any environment behaviour changes. Signed-off-by: Fabian Stelzer <fs@gigacodes.de> --- t/lib-gpg.sh | 53 +++++++++++++++++++++++++++++++++++++++------------- 1 file changed, 40 insertions(+), 13 deletions(-)