openssh unix dev - Jan 2022 - [PATCH] allow wildcard matches for principals with CA certs and return all matches when calling find-principals

---
ssh-keygen -Y find-principals will fail to return any matches if a 
certificate signature is used and the allowed principals file contains a 
wildcard principal (e.g.: *@example.com). Since we need to specify the full 
princpal for -Y verify, find-principals should return the matching ones.
Instead of adjusting the heavily used function sshkey_cert_check_authority()
we instead duplicate a little bit of the cert princpal matching code into 
cert_filter_principals() and no longer have the former verify the principal.
This makes for a much simpler diff and a more constrained change.
The explicit deny on wildcards is dropped as well.

 regress/sshsig.sh | 17 +++++++++++++++++
 sshsig.c          | 24 +++++++++++++-----------
 2 files changed, 30 insertions(+), 11 deletions(-)

diff --git a/regress/sshsig.sh b/regress/sshsig.sh
index f8d85c2f..dcfd2e78 100644
--- a/regress/sshsig.sh
+++ b/regress/sshsig.sh
@@ -342,6 +342,23 @@ for t in $SIGNKEYS; do
 		-f $OBJ/allowed_signers >/dev/null 2>&1 || \
 		fail "failed find-principals for $t with ca key"
 
+	# CA with wildcard principal
+	(printf "*@example.com cert-authority " ;
+	 cat $CA_PUB) > $OBJ/allowed_signers
+	# find-principals CA with wildcard principal
+	${SSHKEYGEN} -vvv -Y find-principals -s $sigfile \
+		-Overify-time=19850101 \
+		-f $OBJ/allowed_signers 2>/dev/null | \
+		fgrep "$sig_principal" >/dev/null || \
+		fail "failed find-principals for $t with ca key using wildcard
principal"
+
+	# verify CA with wildcard principal
+	${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 using wildcard principal"
+
 	# 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 f8c63f7b..1a61bbc6 100644
--- a/sshsig.c
+++ b/sshsig.c
@@ -820,6 +820,7 @@ cert_filter_principals(const char *path, u_long linenum,
 	const char *reason;
 	struct sshbuf *nprincipals;
 	int r = SSH_ERR_INTERNAL_ERROR, success = 0;
+	u_int i;
 
 	oprincipals = principals = *principalsp;
 	*principalsp = NULL;
@@ -830,22 +831,23 @@ cert_filter_principals(const char *path, u_long linenum,
 	}
 
 	while ((cp = strsep(&principals, ",")) != NULL && *cp !=
'\0') {
-		if (strcspn(cp, "!?*") != strlen(cp)) {
-			debug("%s:%lu: principal \"%s\" not authorized: "
-			    "contains wildcards", path, linenum, cp);
-			continue;
-		}
-		/* Check against principals list in certificate */
+		/* Check certificate validity */
 		if ((r = sshkey_cert_check_authority(cert, 0, 1, 0,
-		    verify_time, cp, &reason)) != 0) {
+		    verify_time, NULL, &reason)) != 0) {
 			debug("%s:%lu: principal \"%s\" not authorized: %s",
 			    path, linenum, cp, reason);
 			continue;
 		}
-		if ((r = sshbuf_putf(nprincipals, "%s%s",
-		    sshbuf_len(nprincipals) != 0 ? "," : "", cp)) != 0) {
-			error_f("buffer error");
-			goto out;
+		/* Return all matching principal names from the cert */
+		for (i = 0; i < cert->cert->nprincipals; i++) {
+			if (match_pattern(cert->cert->principals[i], cp)) {
+				if ((r = sshbuf_putf(nprincipals, "%s%s",
+					sshbuf_len(nprincipals) != 0 ? "," : "",
+						cert->cert->principals[i])) != 0) {
+					error_f("buffer error");
+					goto out;
+				}
+			}
 		}
 	}
 	if (sshbuf_len(nprincipals) == 0) {
-- 
2.34.1

On 25/01/2022 08:54, Fabian Stelzer wrote:
> ssh-keygen -Y find-principals will fail to return any matches if a
> certificate signature is used and the allowed principals file contains a
> wildcard principal (e.g.: *@example.com).
Do you mean the "allowed signers" file, rather than the "allowed 
principals" file?

I'm not aware of any wildcard matching in AuthorizedPrincipalsFile, so 
that confused me a bit: in other words, I thought "*@example.com"
would
only match literally the principal "*@example.com".? If that's not
true,
I'd like to know more.

applied - thanks!

On Tue, 25 Jan 2022, Fabian Stelzer wrote:
> ---
> ssh-keygen -Y find-principals will fail to return any matches if a 
> certificate signature is used and the allowed principals file contains a 
> wildcard principal (e.g.: *@example.com). Since we need to specify the full
> princpal for -Y verify, find-principals should return the matching ones.
> Instead of adjusting the heavily used function
sshkey_cert_check_authority()
> we instead duplicate a little bit of the cert princpal matching code into 
> cert_filter_principals() and no longer have the former verify the
principal.
> This makes for a much simpler diff and a more constrained change.
> The explicit deny on wildcards is dropped as well.
> 
>  regress/sshsig.sh | 17 +++++++++++++++++
>  sshsig.c          | 24 +++++++++++++-----------
>  2 files changed, 30 insertions(+), 11 deletions(-)
> 
> diff --git a/regress/sshsig.sh b/regress/sshsig.sh
> index f8d85c2f..dcfd2e78 100644
> --- a/regress/sshsig.sh
> +++ b/regress/sshsig.sh
> @@ -342,6 +342,23 @@ for t in $SIGNKEYS; do
>  		-f $OBJ/allowed_signers >/dev/null 2>&1 || \
>  		fail "failed find-principals for $t with ca key"
>  
> +	# CA with wildcard principal
> +	(printf "*@example.com cert-authority " ;
> +	 cat $CA_PUB) > $OBJ/allowed_signers
> +	# find-principals CA with wildcard principal
> +	${SSHKEYGEN} -vvv -Y find-principals -s $sigfile \
> +		-Overify-time=19850101 \
> +		-f $OBJ/allowed_signers 2>/dev/null | \
> +		fgrep "$sig_principal" >/dev/null || \
> +		fail "failed find-principals for $t with ca key using wildcard
principal"
> +
> +	# verify CA with wildcard principal
> +	${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 using wildcard principal"
> +
>  	# 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 f8c63f7b..1a61bbc6 100644
> --- a/sshsig.c
> +++ b/sshsig.c
> @@ -820,6 +820,7 @@ cert_filter_principals(const char *path, u_long
linenum,
>  	const char *reason;
>  	struct sshbuf *nprincipals;
>  	int r = SSH_ERR_INTERNAL_ERROR, success = 0;
> +	u_int i;
>  
>  	oprincipals = principals = *principalsp;
>  	*principalsp = NULL;
> @@ -830,22 +831,23 @@ cert_filter_principals(const char *path, u_long
linenum,
>  	}
>  
>  	while ((cp = strsep(&principals, ",")) != NULL &&
*cp != '\0') {
> -		if (strcspn(cp, "!?*") != strlen(cp)) {
> -			debug("%s:%lu: principal \"%s\" not authorized: "
> -			    "contains wildcards", path, linenum, cp);
> -			continue;
> -		}
> -		/* Check against principals list in certificate */
> +		/* Check certificate validity */
>  		if ((r = sshkey_cert_check_authority(cert, 0, 1, 0,
> -		    verify_time, cp, &reason)) != 0) {
> +		    verify_time, NULL, &reason)) != 0) {
>  			debug("%s:%lu: principal \"%s\" not authorized:
%s",
>  			    path, linenum, cp, reason);
>  			continue;
>  		}
> -		if ((r = sshbuf_putf(nprincipals, "%s%s",
> -		    sshbuf_len(nprincipals) != 0 ? "," : "", cp)) !=
0) {
> -			error_f("buffer error");
> -			goto out;
> +		/* Return all matching principal names from the cert */
> +		for (i = 0; i < cert->cert->nprincipals; i++) {
> +			if (match_pattern(cert->cert->principals[i], cp)) {
> +				if ((r = sshbuf_putf(nprincipals, "%s%s",
> +					sshbuf_len(nprincipals) != 0 ? "," : "",
> +						cert->cert->principals[i])) != 0) {
> +					error_f("buffer error");
> +					goto out;
> +				}
> +			}
>  		}
>  	}
>  	if (sshbuf_len(nprincipals) == 0) {
> -- 
> 2.34.1
> 
> _______________________________________________
> openssh-unix-dev mailing list
> openssh-unix-dev at mindrot.org
> https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev
>