-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Retries registry call with HTTP if HTTPS fails. #407
Conversation
@@ -161,14 +205,16 @@ private T call(RequestState requestState) throws IOException, RegistryException | |||
} | |||
} | |||
|
|||
} catch (HttpHostConnectException | SSLPeerUnverifiedException ex) { | |||
// Tries to call with HTTP protocol if HTTPS failed to connect. | |||
if (DEFAULT_PROTOCOL.equals(requestState.url.getProtocol())) { |
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.
I feel like we should explicitly compare against "https" instead of DEFAULT_PROTOCOL
for readability (and also to avoid problems on the off chance we change the default), or rename the constant. Since we're assuming DEFAULT_PROTOCOL
is https here.
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.
Good point, will change to that.
* @param requestState the state of the request - determines how to make the request and how to | ||
* process the response | ||
* @return an object representing the response, or {@code null} | ||
* @throws IOException for most I/O exceptions when making the request |
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.
should these be @link?
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.
do you mean like @throws IOException for most {@link IOException}s when making the request
?
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.
nope. Please ignore. I shouldn't make review comments so early in the morning.
@@ -161,14 +205,16 @@ private T call(RequestState requestState) throws IOException, RegistryException | |||
} | |||
} | |||
|
|||
} catch (HttpHostConnectException | SSLPeerUnverifiedException ex) { |
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.
I'm not sure about this, but is it possible users might not want this insecure fallback mechanism? What if they always want to use https or fail?
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.
Hmm, yea it seems that we might want to have the user enable the fallback to HTTP with a configuration option, since Docker only seems to allow insecure registries with a flag.
I agree with @patflynn I'm not sure of the background here, but this one is setting off alarm bells in my head. Are you really sure you want to allow non-https at all? Have you consulted the internal security team? |
Closing in favor of adding a specific configuration parameter to allow insecure |
Should first merge #430.
Fixes #388