User-Agent: Cyrus-JMAP/3.5.0-alpha0-4514-g2bdc19e04f-fm-20211209.002-g2bdc19e0
Mime-Version: 1.0
Message-Id: <e4a2e0eb-762f-41e7-92ce-0c58023f280d@www.fastmail.com>
In-Reply-To: <20211214213430.029b34c5@vostro>
References: <20211213212929.925-1-git@jfarr.cc>
 <20211214213430.029b34c5@vostro>
Date: Wed, 15 Dec 2021 00:21:34 +0100
From: "Jan Hendrik Farr" <git@jfarr.cc>
To: "Timo Teras" <timo.teras@iki.fi>
Cc: ~alpine/apk-tools@lists.alpinelinux.org
Subject: Re: [PATCH 1/1] libfetch: Allow special characters in http basic auth
Content-Type: text/plain

Hi,

sorry, forgot to wrap my lines. Wrapped version:

> Thanks! There is a related bug
> https://gitlab.alpinelinux.org/alpine/apk-tools/-/issues/10775
>
> If this get's fixed, add to commit message: ref #10775
>

Looks like this patch should fix that issue.



> Is the upstream file somewhere?

Yes. As I understand, libfetch.c was originally taken from NetBSD (with
certain bits taken from FreeBSD):
https://github.com/NetBSD/src/blob/trunk/external/bsd/fetch/dist/libfetch/fetch.c

That file has no fix for this and has the same issue. I took this fix
from the FreeBSD version:
https://cgit.freebsd.org/src/tree/lib/libfetch/fetch.c

I took the functions fetch_hexval and fetch_pctdecode (both exact
copies) from that file and integrated them into fetchParseURL.



> minor readability issue; i'd prefer something like:
>   if (!dlen)
>     return NULL;
>   dlen--;
>   *dst++ = c;
>
> To make the error path and normal path more obvious.

Since fetch_pctdecode is an exact copy from FreeBSD I would prefer to
keep it unchanged.



> This will terminate also with ':' in the string now. And will handle
> the rest of the URL incorrectly, and break password with ':'.

Usernames used for http basic authentication are not allowed to contain
":" according RFC7617 section 2.1
(https://datatracker.ietf.org/doc/html/rfc7617#section-2.1). They are
allowed to contain other special characters though and the password is
allowed to contain special characters including ":". The characters ":",
"@", or "/" present in the username or password must be encoded using
percent encoding per RFC1738 section 3.1
(https://datatracker.ietf.org/doc/html/rfc1738#section-3.1). So the only
":" present in this part of the string will be the separator between
username and password.

Example:

username: foo@123/bar
password: pass1234:hello:

A correct proxy setting (or other URL) using these credentials should be
encoded by the user as:

http://foo%40123%2Fbar:pass1234%3Ahello%3A@example.com

libfetch should then decode the percent encoding to obtain the username
and password in plain text, append them using a ":", and then encode the
result using base64 to be used during http basic authentication:

result: Zm9vQDEyMy9iYXI6cGFzczEyMzQ6aGVsbG86

The decoding of the percent encoding is added in this patch and the
appending of username and password and base64 encoding is already
correctly handled in http.c.



Would you prefer for me to open a merge request on your Gitlab instead?
If yes: Which branch should I open the request for?


With kind regards, Jan