Fabian Stelzer
2021-Oct-21 13:54 UTC
[PATCH 0/4] sshsig: find-principal fix & match-principals
This patch series adds a few tests to the find-principals & verify operations of ssh-keygen and fixes a bug in find-principals. find-principals was checking key validity times for CA signed certs but not for normal keys or a validity specified on the CA key. The verify operation correctly does both. As find-principals just returns the first match, this could return principals with an expired/notyetvalid key. This patch changes this behaviour and could therefore be considered a breaking change. At the moment the docs are not quite clear about this. find-principals is specified to return a list of principals. It wasn't clear to me that this meant only those found on a single line. I assumed i would get all that match the signatures public key. If my understanding is correct that find-principals should always just return one match (sometimes having multiple principals) then i can update the manpage as well. It also adds a new -Y match-principals that can be used to determine if a principal is present in the allowed signers file (considering wildcard matches). I am implementing "Trust on first use" for git commit signing via ssh keys and needed a safe way to check that i will not add a principal already present (and thereby possibly overriding their key if it expires for example). Generally i only would like to add principals not already matching any existing entry. Fabian Stelzer (4): sshsig: add tests for signing key validity and find-principals sshsig: fix find-principals key lifespan validation ssh-keygen: make verify-time argument parsing optional ssh-keygen: add match-principals call regress/sshsig.sh | 151 ++++++++++++++++++++++++++++++++++++++++++++++ ssh-keygen.1 | 14 +++++ ssh-keygen.c | 43 ++++++++++++- sshsig.c | 95 +++++++++++++++++++++++------ sshsig.h | 4 ++ 5 files changed, 284 insertions(+), 23 deletions(-) -- 2.31.1
Fabian Stelzer
2021-Oct-21 13:54 UTC
[PATCH 1/4] sshsig: add tests for signing key validity and find-principals
- adds generic find-principals tests (this command had none before) - tests certs with a timeboxed validity both with and without a restriced lifetime for the CA - test for a revoked CA cert Signed-off-by: Fabian Stelzer <fs at gigacodes.de> --- regress/sshsig.sh | 76 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 76 insertions(+) diff --git a/regress/sshsig.sh b/regress/sshsig.sh index fc300a8d..fd015378 100644 --- a/regress/sshsig.sh +++ b/regress/sshsig.sh @@ -35,6 +35,7 @@ verbose "$tid: make certificates" for t in $SSH_KEYTYPES ; do ${SSHKEYGEN} -q -s $CA_PRIV -z $$ \ -I "regress signature key for $USER" \ + -V "19840101:19860101" \ -n $sig_principal $OBJ/${t} || \ fatal "couldn't sign ${t}" SIGNKEYS="$SIGNKEYS ${t}-cert.pub" @@ -47,6 +48,8 @@ for t in $SIGNKEYS; do sigfile=${OBJ}/sshsig-${keybase}.sig sigfile_agent=${OBJ}/sshsig-agent-${keybase}.sig pubkey=${OBJ}/${keybase}.pub + cert=${OBJ}/${keybase}-cert.pub + sigfile_cert=${OBJ}/sshsig-${keybase}-cert.sig ${SSHKEYGEN} -vvv -Y sign -f ${OBJ}/$t -n $sig_namespace \ < $DATA > $sigfile 2>/dev/null || fail "sign using $t failed" @@ -175,6 +178,16 @@ for t in $SIGNKEYS; do < $DATA2 >/dev/null 2>&1 && \ fail "succeeded checking signature for $t key with invalid data" + # find-principals with valid public key + (printf "$sig_principal " ; cat $pubkey) > $OBJ/allowed_signers + ${SSHKEYGEN} -vvv -Y find-principals -s $sigfile -f $OBJ/allowed_signers >/dev/null 2>&1 || \ + fail "failed to find valid principals in allowed_signers" + + # find-principals with wrong key not in allowed_signers + (printf "$sig_principal " ; cat $WRONG) > $OBJ/allowed_signers + ${SSHKEYGEN} -vvv -Y find-principals -s $sigfile -f $OBJ/allowed_signers >/dev/null 2>&1 && \ + fail "succeeded finding principal with invalid signers file" + # Check signing keys using ssh-agent. ${SSHADD} -D >/dev/null 2>&1 # Remove all previously-loaded keys. ${SSHADD} ${privkey} > /dev/null 2>&1 || fail "ssh-add failed" @@ -204,6 +217,7 @@ for t in $SIGNKEYS; do cat $CA_PUB) > $OBJ/allowed_signers ${SSHKEYGEN} -vvv -Y verify -s $sigfile -n $sig_namespace \ -I $sig_principal -f $OBJ/allowed_signers \ + -Overify-time=19850101 \ < $DATA >/dev/null 2>&1 || \ fail "failed signature for $t cert" @@ -229,6 +243,68 @@ for t in $SIGNKEYS; do -I $sig_principal -f $OBJ/allowed_signers \ < $DATA >/dev/null 2>&1 && \ fail "accepted signature for $t cert with wrong principal" + + # Cert valid but CA revoked + cat $CA_PUB > $OBJ/revoked_keys + (printf "$sig_principal " ; cat $pubkey) > $OBJ/allowed_signers + ${SSHKEYGEN} -vvv -Y verify -s $sigfile -n $sig_namespace \ + -I $sig_principal -f $OBJ/allowed_signers \ + -r $OBJ/revoked_keys \ + < $DATA >/dev/null 2>&1 && \ + fail "accepted signature for $t key, but CA key in revoked_keys" + + # Set lifespan of CA key and verify signed user certs behave accordingly + ( printf "$sig_principal " ; + printf "cert-authority,valid-after=\"19800101\",valid-before=\"19900101\" " ; + cat $CA_PUB) > $OBJ/allowed_signers + + # CA key lifespan valid + ${SSHKEYGEN} -vvv -Y verify -s $sigfile -n $sig_namespace \ + -I $sig_principal -f $OBJ/allowed_signers \ + -Overify-time=19850101 \ + < $DATA >/dev/null 2>&1 >/dev/null 2>&1 || \ + fail "failed signature for $t key with valid CA expiry interval" + # CA lifespan is valid but user key not yet valid + ${SSHKEYGEN} -vvv -Y verify -s $sigfile -n $sig_namespace \ + -I $sig_principal -f $OBJ/allowed_signers \ + -Overify-time=19810101 \ + < $DATA >/dev/null 2>&1 && \ + fail "accepted signature for $t key with valid CA expiry interval but not yet valid cert" + # CA lifespan is valid but user key expired + ${SSHKEYGEN} -vvv -Y verify -s $sigfile -n $sig_namespace \ + -I $sig_principal -f $OBJ/allowed_signers \ + -Overify-time=19890101 \ + < $DATA >/dev/null 2>&1 && \ + fail "accepted signature for $t key with valid CA expiry interval but expired cert" + # CA key not yet valid + ${SSHKEYGEN} -vvv -Y verify -s $sigfile -n $sig_namespace \ + -I $sig_principal -f $OBJ/allowed_signers \ + -Overify-time=19790101 \ + < $DATA >/dev/null 2>&1 && \ + fail "accepted signature for $t not-yet-valid CA key" + # CA key expired + ${SSHKEYGEN} -vvv -Y verify -s $sigfile -n $sig_namespace \ + -I $sig_principal -f $OBJ/allowed_signers \ + -Overify-time=19910101 \ + < $DATA >/dev/null 2>&1 && \ + fail "accepted signature for $t with expired CA key" + # NB. assumes we're not running this test in the 1980s + ${SSHKEYGEN} -vvv -Y verify -s $sigfile -n $sig_namespace \ + -I $sig_principal -f $OBJ/allowed_signers \ + < $DATA >/dev/null 2>&1 && \ + fail "accepted signature for $t with expired CA key" + + # Set lifespan of CA outside of the cert validity + ( printf "$sig_principal " ; + printf "cert-authority,valid-after=\"19800101\",valid-before=\"19820101\" " ; + cat $CA_PUB) > $OBJ/allowed_signers + # valid cert validity but expired CA + ${SSHKEYGEN} -vvv -Y verify -s $sigfile -n $sig_namespace \ + -I $sig_principal -f $OBJ/allowed_signers \ + -Overify-time=19840101 \ + < $DATA >/dev/null 2>&1 && \ + fail "accepted signature for $t key with expired CA but valid cert" + done trace "kill agent" -- 2.31.1
Fabian Stelzer
2021-Oct-21 13:54 UTC
[PATCH 2/4] sshsig: fix find-principals key lifespan validation
find-principals was verifying key validity when using ca certs but not with simple key lifetimes within the allowed signers file. Since it returns the first keys principal it finds this could result in a principal with an expired key even though a valid one is just below. - moves timespan validity check code from check_allowed_keys_line() into a seperate function - reuses this function then in get_matching_principals_from_line() - adds tests for both CA & normal keys The find-principals tests using normal keys would fail before this patch while the identical ones with CA keys suceeded. Signed-off-by: Fabian Stelzer <fs at gigacodes.de> --- regress/sshsig.sh | 49 ++++++++++++++++++++++++++++++++++++++++++ sshsig.c | 54 +++++++++++++++++++++++++++++------------------ 2 files changed, 83 insertions(+), 20 deletions(-) diff --git a/regress/sshsig.sh b/regress/sshsig.sh index fd015378..9947afc1 100644 --- a/regress/sshsig.sh +++ b/regress/sshsig.sh @@ -149,6 +149,26 @@ for t in $SIGNKEYS; do < $DATA >/dev/null 2>&1 && \ fail "failed signature for $t with expired key" + # key lifespan valid + ${SSHKEYGEN} -vvv -Y find-principals -s $sigfile \ + -Overify-time="19850101" \ + -f $OBJ/allowed_signers >/dev/null 2>&1 || \ + fail "failed find-principals for $t key with valid expiry interval" + # key not yet valid + ${SSHKEYGEN} -vvv -Y find-principals -s $sigfile \ + -Overify-time="19790101" \ + -f $OBJ/allowed_signers >/dev/null 2>&1 && \ + fail "failed find-principals for $t not-yet-valid key" + # key expired + ${SSHKEYGEN} -vvv -Y find-principals -s $sigfile \ + -Overify-time="19990101" \ + -f $OBJ/allowed_signers >/dev/null 2>&1 && \ + fail "failed find-principals for $t with expired key" + # NB. assumes we're not running this test in the 1980s + ${SSHKEYGEN} -vvv -Y find-principals -s $sigfile \ + -f $OBJ/allowed_signers >/dev/null 2>&1 && \ + fail "failed find-principals for $t with expired key" + # public key in revoked keys file cat $pubkey > $OBJ/revoked_keys (printf "$sig_principal namespaces=\"whatever\" " ; @@ -211,6 +231,29 @@ for t in $SIGNKEYS; do *) continue ;; esac + # Check key lifespan on find-principals when using the CA + ( printf "$sig_principal " ; + printf "cert-authority,valid-after=\"19800101\",valid-before=\"19900101\" "; + cat $CA_PUB) > $OBJ/allowed_signers + # key lifespan valid + ${SSHKEYGEN} -vvv -Y find-principals -s $sigfile \ + -Overify-time="19850101" \ + -f $OBJ/allowed_signers >/dev/null 2>&1 || \ + fail "failed find-principals for $t key with valid expiry interval" + # key not yet valid + ${SSHKEYGEN} -vvv -Y find-principals -s $sigfile \ + -Overify-time="19790101" \ + -f $OBJ/allowed_signers >/dev/null 2>&1 && \ + fail "failed find-principals for $t not-yet-valid key" + # key expired + ${SSHKEYGEN} -vvv -Y find-principals -s $sigfile \ + -Overify-time="19990101" \ + -f $OBJ/allowed_signers >/dev/null 2>&1 && \ + fail "failed find-principals for $t with expired key" + # NB. assumes we're not running this test in the 1980s + ${SSHKEYGEN} -vvv -Y find-principals -s $sigfile \ + -f $OBJ/allowed_signers >/dev/null 2>&1 && \ + fail "failed find-principals for $t with expired key" # correct CA key (printf "$sig_principal cert-authority " ; @@ -221,6 +264,12 @@ for t in $SIGNKEYS; do < $DATA >/dev/null 2>&1 || \ fail "failed signature for $t cert" + # find-principals + ${SSHKEYGEN} -vvv -Y find-principals -s $sigfile \ + -Overify-time=19850101 \ + -f $OBJ/allowed_signers >/dev/null 2>&1 || \ + fail "failed find-principals for $t with ca key" + # signing key listed as cert-authority (printf "$sig_principal cert-authority " ; cat $pubkey) > $OBJ/allowed_signers diff --git a/sshsig.c b/sshsig.c index d0d401a3..b50a9779 100644 --- a/sshsig.c +++ b/sshsig.c @@ -812,6 +812,35 @@ parse_principals_key_and_options(const char *path, u_long linenum, char *line, return r; } +static int +check_key_validity(uint64_t verify_time, struct sshsigopt *sigopts, const char *path, u_long linenum) +{ + char tvalid[64], tverify[64]; + /* check key time validity */ + format_absolute_time((uint64_t)verify_time, tverify, sizeof(tverify)); + if (sigopts->valid_after != 0 && + (uint64_t)verify_time < sigopts->valid_after) { + format_absolute_time(sigopts->valid_after, + tvalid, sizeof(tvalid)); + if (path) + error("%s:%lu: key is not yet valid: " + "verify time %s < valid-after %s", path, linenum, + tverify, tvalid); + return 1; + } + if (sigopts->valid_before != 0 && + (uint64_t)verify_time > sigopts->valid_before) { + format_absolute_time(sigopts->valid_before, + tvalid, sizeof(tvalid)); + if (path) + error("%s:%lu: key has expired: " + "verify time %s > valid-before %s", path, linenum, + tverify, tvalid); + return 1; + } + return 0; +} + static int check_allowed_keys_line(const char *path, u_long linenum, char *line, const struct sshkey *sign_key, const char *principal, @@ -821,7 +850,6 @@ check_allowed_keys_line(const char *path, u_long linenum, char *line, int r, success = 0; const char *reason = NULL; struct sshsigopt *sigopts = NULL; - char tvalid[64], tverify[64]; /* Parse the line */ if ((r = parse_principals_key_and_options(path, linenum, line, @@ -856,26 +884,9 @@ check_allowed_keys_line(const char *path, u_long linenum, char *line, goto done; } - /* check key time validity */ - format_absolute_time((uint64_t)verify_time, tverify, sizeof(tverify)); - if (sigopts->valid_after != 0 && - (uint64_t)verify_time < sigopts->valid_after) { - format_absolute_time(sigopts->valid_after, - tvalid, sizeof(tvalid)); - error("%s:%lu: key is not yet valid: " - "verify time %s < valid-after %s", path, linenum, - tverify, tvalid); + if (check_key_validity(verify_time, sigopts, path, linenum)) goto done; - } - if (sigopts->valid_before != 0 && - (uint64_t)verify_time > sigopts->valid_before) { - format_absolute_time(sigopts->valid_before, - tvalid, sizeof(tvalid)); - error("%s:%lu: key has expired: " - "verify time %s > valid-before %s", path, linenum, - tverify, tvalid); - goto done; - } + success = 1; done: @@ -998,6 +1009,9 @@ get_matching_principals_from_line(const char *path, u_long linenum, char *line, goto done; } + if (check_key_validity(verify_time, sigopts, NULL, 0)) + goto done; + if (!sigopts->ca && sshkey_equal(found_key, sign_key)) { /* Exact match of key */ debug("%s:%lu: matched key", path, linenum); -- 2.31.1
Fabian Stelzer
2021-Oct-21 13:54 UTC
[PATCH 3/4] ssh-keygen: make verify-time argument parsing optional
Signed-off-by: Fabian Stelzer <fs at gigacodes.de> --- ssh-keygen.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/ssh-keygen.c b/ssh-keygen.c index 9b912f0a..927a1d62 100644 --- a/ssh-keygen.c +++ b/ssh-keygen.c @@ -2680,11 +2680,13 @@ sig_process_opts(char * const *opts, size_t nopts, uint64_t *verify_timep, size_t i; time_t now; - *verify_timep = 0; + if (verify_timep != NULL) + *verify_timep = 0; if (print_pubkey != NULL) *print_pubkey = 0; for (i = 0; i < nopts; i++) { - if (strncasecmp(opts[i], "verify-time=", 12) == 0) { + if (verify_timep && + strncasecmp(opts[i], "verify-time=", 12) == 0) { if (parse_absolute_time(opts[i] + 12, verify_timep) != 0 || *verify_timep == 0) { error("Invalid \"verify-time\" option"); @@ -2698,7 +2700,7 @@ sig_process_opts(char * const *opts, size_t nopts, uint64_t *verify_timep, return SSH_ERR_INVALID_ARGUMENT; } } - if (*verify_timep == 0) { + if (verify_timep && *verify_timep == 0) { if ((now = time(NULL)) < 0) { error("Time is before epoch"); return SSH_ERR_INVALID_ARGUMENT; -- 2.31.1
Adds "-Y match-principals -s signers_file -I identity" so we are able to look up principals considering wildcard matches. Users (git in this case) implementing "Trust on first use" can use this to make sure they don't add overlapping principals with new keys. Signed-off-by: Fabian Stelzer <fs at gigacodes.de> --- regress/sshsig.sh | 26 ++++++++++++++++++++++++++ ssh-keygen.1 | 14 ++++++++++++++ ssh-keygen.c | 35 +++++++++++++++++++++++++++++++++++ sshsig.c | 41 +++++++++++++++++++++++++++++++++++++++++ sshsig.h | 4 ++++ 5 files changed, 120 insertions(+) diff --git a/regress/sshsig.sh b/regress/sshsig.sh index 9947afc1..420d7e3d 100644 --- a/regress/sshsig.sh +++ b/regress/sshsig.sh @@ -356,6 +356,32 @@ for t in $SIGNKEYS; do done +# Test key independant match-principals +( + printf "principal1 " ; cat $pubkey; + printf "princi* " ; cat $pubkey; + printf "unique " ; cat $pubkey; +) > $OBJ/allowed_signers + +${SSHKEYGEN} -Y match-principals -f $OBJ/allowed_signers \ + -I "unique" \ + | grep -F "unique" || \ + fail "faild to match static principal" + +${SSHKEYGEN} -Y match-principals -f $OBJ/allowed_signers \ + -I "princip" \ + | grep -F "princi*" || \ + fail "faild to match wildcard principal" + +${SSHKEYGEN} -Y match-principals -f $OBJ/allowed_signers \ + -I "principal1" \ + | grep -F -e "principal1" -e "princi*" || \ + fail "faild to match static and wildcard principal" + +${SSHKEYGEN} -Y match-principals -f $OBJ/allowed_signers \ + -I "unknown" >/dev/null 2>&1 && \ + fail "succeeded to match unknown principal" + trace "kill agent" ${SSHAGENT} -k > /dev/null diff --git a/ssh-keygen.1 b/ssh-keygen.1 index f83f515f..08a509cd 100644 --- a/ssh-keygen.1 +++ b/ssh-keygen.1 @@ -151,6 +151,11 @@ .Fl s Ar signature_file .Fl f Ar allowed_signers_file .Nm ssh-keygen +.Fl Y Cm match-principals +.Op Fl O Ar option +.Fl I Ar signer_identity +.Fl f Ar allowed_signers_file +.Nm ssh-keygen .Fl Y Cm check-novalidate .Op Fl O Ar option .Fl n Ar namespace @@ -683,6 +688,15 @@ The format of the allowed signers file is documented in the section below. If one or more matching principals are found, they are returned on standard output. +.It Fl Y Cm match-principals +Find all the principal(s) matching the principal name, +provided using the +.Fl I +flag in an authorized signers file provided using the +.Fl f +flag. +If one or more matching principals are found, they are returned on +standard output. .It Fl Y Cm check-novalidate Checks that a signature generated using .Nm diff --git a/ssh-keygen.c b/ssh-keygen.c index 927a1d62..8fae13f3 100644 --- a/ssh-keygen.c +++ b/ssh-keygen.c @@ -2849,6 +2849,27 @@ done: return ret; } +static int +sig_match_principals(const char *allowed_keys, char *principal, + char * const *opts, size_t nopts) +{ + int ret = -1; + struct sshbuf *matching_principals = sshbuf_new(); + + if (sig_process_opts(opts, nopts, NULL, NULL) != 0) + return ret; /* error already logged */ + + ret = sshsig_match_principals(allowed_keys, principal, &matching_principals); + if (ret == 0 && matching_principals) { + printf("%s", sshbuf_ptr(matching_principals)); + } else { + fprintf(stderr, "No principal matched.\n"); + } + sshbuf_free(matching_principals); + + return ret; +} + static void do_moduli_gen(const char *out_file, char **opts, size_t nopts) { @@ -3162,6 +3183,7 @@ usage(void) " file ...\n" " ssh-keygen -Q [-l] -f krl_file [file ...]\n" " ssh-keygen -Y find-principals -s signature_file -f allowed_signers_file\n" + " ssh-keygen -Y match-principals -I signer_identity -f allowed_signers_file\n" " ssh-keygen -Y check-novalidate -n namespace -s signature_file\n" " ssh-keygen -Y sign -f key_file -n namespace file ...\n" " ssh-keygen -Y verify -f allowed_signers_file -I signer_identity\n" @@ -3443,6 +3465,19 @@ main(int argc, char **argv) } return sig_find_principals(ca_key_path, identity_file, opts, nopts); + } else if (strncmp(sign_op, "match-principals", 16) == 0) { + if (!have_identity) { + error("Too few arguments for match-principals:" + "missing allowed keys file"); + exit(1); + } + if (cert_key_id == NULL) { + error("Too few arguments for verify: " + "missing principal ID"); + exit(1); + } + return sig_match_principals(identity_file, cert_key_id, + opts, nopts); } else if (strncmp(sign_op, "sign", 4) == 0) { if (cert_principals == NULL || *cert_principals == '\0') { diff --git a/sshsig.c b/sshsig.c index b50a9779..ddfb96c4 100644 --- a/sshsig.c +++ b/sshsig.c @@ -841,6 +841,47 @@ check_key_validity(uint64_t verify_time, struct sshsigopt *sigopts, const char * return 0; } +int +sshsig_match_principals(const char *path, + const char *principal, struct sshbuf **matching_principals) +{ + FILE *f = NULL; + char *line = NULL; + size_t linesize = 0; + u_long linenum = 0; + int oerrno; + int found = 0; + + /* Check key and principal against file */ + if ((f = fopen(path, "r")) == NULL) { + oerrno = errno; + error("Unable to open allowed keys file \"%s\": %s", + path, strerror(errno)); + errno = oerrno; + return SSH_ERR_SYSTEM_ERROR; + } + + while (getline(&line, &linesize, f) != -1) { + linenum++; + char *principals = NULL; + + /* Parse the line */ + if (parse_principals_key_and_options(path, linenum, line, + principal, &principals, NULL, NULL) == 0) { + sshbuf_putf(*matching_principals, "%s\n", principals); + found = 1; + } + + free(principals); + free(line); + line = NULL; + linesize = 0; + } + fclose(f); + + return !found; +} + static int check_allowed_keys_line(const char *path, u_long linenum, char *line, const struct sshkey *sign_key, const char *principal, diff --git a/sshsig.h b/sshsig.h index b725c7d7..fee3bc20 100644 --- a/sshsig.h +++ b/sshsig.h @@ -104,4 +104,8 @@ int sshsig_get_pubkey(struct sshbuf *signature, struct sshkey **pubkey); int sshsig_find_principals(const char *path, const struct sshkey *sign_key, uint64_t verify_time, char **principal); +/* Find all principals in allowed_keys file matching *principal */ +int sshsig_match_principals(const char *path, + const char *principal, struct sshbuf **matching_principals); + #endif /* SSHSIG_H */ -- 2.31.1