Re: [PROPOSAL]: Authentication extension to SourceMirror proposal

[Jürg Billeter <j...@bitron.ch>]

Hi all,

On Tue, 2024-02-27 at 19:00 +0900, Tristan van Berkom wrote:
> On Fri, 2024-02-23 at 18:33 +0900, Tristan van Berkom wrote:
> 
> I now have the following PRs up:
> 
>   Implement SourceMirror plugins:
>   https://github.com/apache/buildstream/pull/1890
> 
>   DownloadableFileSource to support "auth-header-format"
>   https://github.com/apache/buildstream/pull/1895
> 

I've added a few comments. They look reasonable to me overall.

> In Jürg's previous reply on the last proposal, he asks:
> 
>     Could custom source mirror plugins also support URLs that are not
>     aliased?
> 
>     https://lists.apache.org/thread/y2hoobkxdh38wytcfhpgcy4c93r2hd6w
> 
> Unfortunately not with the current implementation.
> 
> [...]
> 
> This would allow for projects to use custom authorization in the
> default case, fetching and tracking from the default URLs.
> 
> I feel like this could be extended in a subsequent addition, and does
> not necessarily block these patches, but I defer to reviewers if they
> have anything to add here.

I agree that actual support for this could be done in the future. I've
mentioned in a review comment a potential tweak to make this a bit
easier.

> Second... I'm not sure I'm completely satisfied with the currently
> implemented API in #1895 (nor am I completely satisfied with the mock
> http server I used to create the test case here).
> 
> In upstream python, we can observe how "Basic Authentication" is done
> with the AbstractBasicAuthHandler (and HTTPBasicAuthHandler which is
> the concrete class):
> 
> [...]
> 
> There may be a purpose to all this extra code - if we were to be more
> stringent here, we would probably want to implement our own
> "HTTPBearerAuthHandler" in the DownloadableFileSource code, and have
> it
> run more stringent checks, rather than simply adding a header that
> says:
> 
>   "Authentication: Bearer <token>"

As I understand it, bearer token authorization is actually simpler and
doesn't require the 401 retry logic that basic auth needs. So unless
anyone speaks up with specific concerns, I think the current approach
is sufficient and Bazel doing the same is also a good indication.

If necessary, also parsing the supplied auth header does not seem
completely unreasonable.

Cheers,
Jürg