From: Todd C. Miller <millert@openbsd.org>
Subject: Re: [PATCH] ssh-add: Support @ in the user part of destination constraints
To: Damien Miller <djm@mindrot.org>
Cc: Max Zettlmeißl <max@zettlmeissl.de>, tech@openbsd.org
Date: Thu, 05 Sep 2024 20:26:39 -0600

On Fri, 06 Sep 2024 12:16:25 +1000, Damien Miller wrote:

> Well, it's still a mess elsewhere in OpenSSH. Maybe we should whack it
> all in one go?
>
> The only problem is that someone, somewhere, somehow will have used '@'
> in a hostname and this will break them.

Comments inline.

 - todd

> diff --git a/match.c b/match.c
> index 5888865..2698042 100644
> --- a/match.c
> +++ b/match.c
> @@ -234,7 +234,7 @@ match_user(const char *user, const char *host, const char
>  *ipaddr,
>  
>  	/* test mode */
>  	if (user == NULL && host == NULL && ipaddr == NULL) {
> -		if ((p = strchr(pattern, '@')) != NULL &&
> +		if ((p = strrchr(pattern, '@')) != NULL &&
>  		    match_host_and_ip(NULL, NULL, p + 1) < 0)
>  			return -1;
>  		return 0;

OK

> @@ -243,11 +243,11 @@ match_user(const char *user, const char *host, const ch
> ar *ipaddr,
>  	if (user == NULL)
>  		return 0; /* shouldn't happen */
>  
> -	if ((p = strchr(pattern, '@')) == NULL)
> +	if ((p = strrchr(pattern, '@')) == NULL)
>  		return match_pattern(user, pattern);

It doesn't look like 'p' is used here at all.  I think this is only
supposed to check whether pattern has a '@'.  Maybe leave this as:

	if (strchr(pattern, '@') == NULL)
		return match_pattern(user, pattern);

>  
>  	pat = xstrdup(pattern);
> -	p = strchr(pat, '@');
> +	p = strrchr(pat, '@');
>  	*p++ = '\0';
>  
>  	if ((ret = match_pattern(user, pat)) == 1)
> diff --git a/misc.c b/misc.c
> index 33327ad..6451b85 100644
> --- a/misc.c
> +++ b/misc.c
> @@ -982,7 +982,7 @@ parse_uri(const char *scheme, const char *uri, char **use
> rp, char **hostp,
>  	uridup = tmp = xstrdup(uri);
>  
>  	/* Extract optional ssh-info (username + connection params) */
> -	if ((cp = strchr(tmp, '@')) != NULL) {
> +	if ((cp = strrchr(tmp, '@')) != NULL) {
>  		char *delim;
>  
>  		*cp = '\0';

I'm not sure about this one.  Currently, an '@' in a user name just
needs to be url-encoded.  Isn't that considered "normal" or URIs?

> diff --git a/ssh-add.c b/ssh-add.c
> index c98442e..d1a402c 100644
> --- a/ssh-add.c
> +++ b/ssh-add.c
> @@ -691,7 +691,7 @@ parse_dest_constraint_hop(const char *s, struct dest_cons
> traint_hop *dch,
>  
>  	memset(dch, '\0', sizeof(*dch));
>  	os = xstrdup(s);
> -	if ((host = strchr(os, '@')) == NULL)
> +	if ((host = strrchr(os, '@')) == NULL)
>  		host = os;
>  	else {
>  		*host++ = '\0';

OK