-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Handle Uri-less Realms during basic auth, and fix auth lookups for hosts with ports specified #32371
Handle Uri-less Realms during basic auth, and fix auth lookups for hosts with ports specified #32371
Conversation
Basic auth can have a realm that doesn't have any of the other parameters, and isn't itself a Uri, so for non-Uri realms we should defer to that of the underlying registry being accessed.
I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label. |
… dependencies to output directory
@vlada-shubina the test I made relies on docker-compose (for easier local testing), but compose isn't available on the ubuntu runner. Do you know what we can do to request that? or should I just do the long |
906e68a
to
8e093e3
Compare
if (authValues.TryGetValue("realm", out string? realm) && authValues.TryGetValue("service", out string? service)) | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (authValues.TryGetValue("realm", out string? realm) && authValues.TryGetValue("service", out string? service)) | |
{ | |
if (authValues.TryGetValue("realm", out string? realm)) | |
{ | |
string? service = null; | |
authValues.TryGetValue("service", out service); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fixes an issue with the OpenShift internal registry which doesn't include a service
.
if (authValues.TryGetValue("realm", out string? realm)) | ||
{ | ||
if (string.IsNullOrWhiteSpace(realm) || !Uri.IsWellFormedUriString(realm, UriKind.Absolute)) | ||
{ | ||
realm = $"{requestUri.Scheme}://{registry}"; | ||
} | ||
string? scope = null; | ||
authInfo = new AuthInfo(new Uri(realm), string.Empty, scope); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This still treats the realm
as a URI if specified. RFC 7617 section 2:
The realm value is a free-form string that can only be compared for equality with other realms on that server.
A protection space is defined by the canonical root URI (the scheme and authority components of the effective request URI; see Section 5.5 of [RFC7230]) of the server being accessed, in combination with the realm value if present.
so, I'd expect a tuple (uri.GetLeftPart(UriPartial.Authority), realm) as a key. Docker Registry v2 Token Authentication Specification does not specify any different treatment of realm
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This still treats the realm as a URI if specified
Right, it should be treated as an opaque string.
so, I'd expect a tuple (uri.GetLeftPart(UriPartial.Authority), realm) as a key.
You mean as a key for the AuthHeaderCache
?
What do you think about getting rid of the static AuthHeaderCache
and instead storing the AuthenticationHeaderValue
directly in the AuthHandshakeMessageHandler
as a field?
#33753 supersedes this PR, and targets the main branch. |
Closing in favor of #33753 |
Fixes dotnet/sdk-container-builds#314