From: Damien Miller <djm@mindrot.org>
Subject: Re: [PATCH] ssh-add: Support @ in the user part of destination constraints
To: "Todd C. Miller" <millert@openbsd.org>
Cc: Max Zettlmeißl <max@zettlmeissl.de>, tech@openbsd.org
Date: Fri, 6 Sep 2024 12:16:25 +1000

On Thu, 5 Sep 2024, Todd C. Miller wrote:

> On Tue, 20 Aug 2024 04:13:23 +0200, Max =?utf-8?Q?Zettlmei=C3=9Fl?= wrote:
> 
> > Properly adding a (complete) host constraint for one of my Git SSH
> > identities was impossible because the string got split into username
> > and host at the first @ sign, yet the username itself contains an @
> > sign.
> >
> > This patch changes the behaviour to split on the last @ sign.
> >
> > In addition to running the patched version against all my constraints,
> > I also tested it with the additional line `debug3_f("User: \"%s\"
> > Host: \"%s\"", dch->user, dch->hostname);` to make sure that I have no
> > off-by-one error which would lead to wrongly parsed components. I
> > decided against including that in the patch.
> 
> This looks correct to me and is consistent with how we parse user@host
> in other parts of the code.  For example, parse_user_host_path()
> and parse_user_host_port() in misc.c all use strrchr().  So does
> ssh.c when parsing something other than a uri.
> 
> It may be worth adding a parse_user_host() function to misc.c so
> we parse this consistently everywhere.  But for now, this simple
> one-liner seems fine.

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.

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;
@@ -243,11 +243,11 @@ match_user(const char *user, const char *host, const char *ipaddr,
 	if (user == NULL)
 		return 0; /* shouldn't happen */
 
-	if ((p = strchr(pattern, '@')) == NULL)
+	if ((p = strrchr(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 **userp, 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';
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_constraint_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';