Optimize authentication for base image #2789
Conversation
| // server and fail. However, to keep some old behavior, try a few things as a last resort. | ||
| // TODO: consider removing this fallback branch. | ||
| if (credential != null && !credential.isOAuth2RefreshToken()) { | ||
| eventHandlers.dispatch( |
There was a problem hiding this comment.
should we actually do LogEvent.warn here with some guidance on reporting this, so that we may identify misbehaving servers by user reports?
There was a problem hiding this comment.
I still think this is a good idea to identify registries that are acting strange. But up to you.
There was a problem hiding this comment.
Sorry, I missed this comment. I thought about that, and I agree logging a warning will be very valuable to us. But I think a warning should be something actionable and fixable on the user side, so that's my hesitation. I've seen some people regard a warning seriously (probably because of a 0-warning policy auto-enforced by some system). Gathering information is really useful to us, but I feel we should be careful for dual-purposing a warning.
| @@ -140,40 +143,43 @@ public ImagesAndRegistryClient call() | |||
| LogEvent.lifecycle( | |||
| "The base image requires auth. Trying again for " + imageReference + "...")); | |||
|
|
|||
| Credential registryCredential = | |||
| Credential credential = | |||
There was a problem hiding this comment.
It seems a little strange that we have so much auth code in PullBaseImage step and not somewhere else like RegistryAuthenticator?
There was a problem hiding this comment.
(First of all, I don't know the initial intention of RegistryAuthenticator, but in reality it has always been "BearerAuthenticator" that's only about bearer auth.)
As discussed offline, we could have introduced an overarching RegistryClient.doRightAuth() that perhaps does
if (!doPull/PushBearerAuth()) {
...
configureBasicAuth();
}I expect this flow should work for both pull and push auth with any registries, and this is exactly the flow we do for target registry auth. However, we were taking a different flow for base image registries (as can be seen in the PR), and what is one of the reasons we don't have doRightAuth().
Given this and that RegistryClient is a low-level API, it also seemed reasonable to have separate nobs for configuring basic auth and completing bearer auth. Another factor to this is the difference between configuring basic auth and completing the whole bearer auth leg: configuring basic auth means that you'll just send the credentials going forward no matter what, while completing bearer auth means interacting with a registry server and an auth server with two round-trips to get another form of credentials to send going forward.
| if (authenticator.isPresent()) { | ||
| doBearerAuth(true, authenticator.get()); | ||
| } else if (credential != null && !credential.isOAuth2RefreshToken()) { | ||
| configureBasicAuth(); |
There was a problem hiding this comment.
I find this flow a little confusing. Should we just have multiple types of credentials instead of checking the state of the Credential everywhere?
There was a problem hiding this comment.
Like defining subclasses of Credentials?
There was a problem hiding this comment.
Yeah I don't know, I've haven't done much in this section of the code base, so it's all a little strange.
Fixes #2784. Fixes #2134.
After we try to pull a base image manifest without sending credentials, we either attempt bearer auth or basic auth based on the
WWW-AuthenticateHTTP header value in a 401 unauthorized response. I have been thinking for a very long time even before filing #2134 this is the most ideal and optimized flow. I believe every sane registry implementation that throws 401 on a manifest request will sendWWW-Authenticate.However, since auth is the most frictional part, I didn't want to make a drastic change. So when the server doesn't return
WWW-Authenticate(I don't expect this to happen), we do some fallback operations. First, we just blindly send credentials via basic auth, and if that fails again with 401, we attempt bearer auth for the last moment. This fallback is basically identical to the current auth flow.