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.
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.
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.
do you mean like @throws IOException for most {@link IOException}s when making the request?
There was a problem hiding this comment.
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.
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.
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