search

dotnet / sdk-container-builds

Libraries and build tooling to create container images from .NET projects using MSBuild
https://learn.microsoft.com/en-us/dotnet/core/docker/publish-as-container
MIT License
179 stars 38 forks source link

Authentication Handler makes too many authentication roundtrips. #391

Open Danielku15 opened 1 year ago

Danielku15 commented 1 year ago

The AuthHeaderCache used in AuthHandshakeMessageHandler currently in place, seems very sensitive in regards of the URLs which leads to unnecessary API call roundtrips for authentication.

The root cause: AuthHeaderCache uses the Host + AbsolutePath components of the URI to choose whether we already have an authentication token. This means that for almost every API call we make an authentication roundtrip because the path parts of the URI differ.

The better approach: Looking into AuthHandshakeMessageHandler.SendAsync and AuthHandshakeMessageHandler.GetAuthenticationAsync we can see that ultimately only the host part of the URL is relevant for determining the authentication information. It is passed into the CredsProvider.GetCredentialsAsync(registry) for loading the credentials and then sending them to the server. Hence we could also just consider the domain part in the AuthHeaderCache and avoid roundtrips for every call.

Maybe this was a security concern to allow registries to be operated on different path segements and not send the credentials potentially to a wrong application? If this is a major concern, we should pass the registry base path into the AuthHandshakeMessageHandler (created per registry already) and handle the authentication specifically to the base URL.

rainersigwald commented 1 year ago

IIRC it was per-domain initially but that resulted in overcaching for at least one case, so I backed it down to the current approach, favoring undercaching + working. But if we can keep the "working" property I would love to reduce round trips.

baronfel commented 1 year ago

I can't find the spec now, but the image name (not just the registry name) are valid parts of the auth JSON file, and so I think we HAVE to provide the path parts of the URI here.

baronfel commented 1 year ago

Ah - here's the spec I was thinking of: https://www.mankier.com/5/containers-auth.json

Danielku15 commented 1 year ago

Interesting detail which might be in conflict how other tools are doing it. I will have to dig a bit into some registries to see how they are all handling the authentication.

Github seem to have the image name as part of the token in the scope and then this received token is reused:

GET /token?account=danielku15&scope=repository%3Abaronfel%2Fsdk-container-demo%3Apush%2Cpull&service=ghcr.io HTTP/1.1

I am thinking of a two stage authentication system which might work out for most flows and has the strict auth in place:

  1. First try to use the domain level credentials and see if the call succeeds.
  2. If it fails with an authentication/authorization related error (401/403), reauthenticate with host+path and retry with this token.
  3. If this fails, treat things as unauthenticated and fail.

This two stage part already exist for 401 here. If we get 403 we can do something similar.

  1. Stage 1 takes the token from the host
  2. Stage 2 takes the token with the path.

What do you think of such an approach?