Skip to content
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

Add error message for credentials not being sent over HTTP #613

Merged
merged 3 commits into from
Jul 13, 2018
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
import com.google.cloud.tools.jib.configuration.CacheConfiguration;
import com.google.cloud.tools.jib.registry.InsecureRegistryException;
import com.google.cloud.tools.jib.registry.RegistryAuthenticationFailedException;
import com.google.cloud.tools.jib.registry.RegistryCredentialsNotSentException;
import com.google.cloud.tools.jib.registry.RegistryUnauthorizedException;
import com.google.common.annotations.VisibleForTesting;
import java.io.IOException;
Expand Down Expand Up @@ -232,6 +233,10 @@ public void build(HelpfulSuggestions helpfulSuggestions) throws BuildStepsExecut
buildConfiguration,
helpfulSuggestions);

} else if (exceptionDuringBuildSteps instanceof RegistryCredentialsNotSentException) {
throw new BuildStepsExecutionException(
helpfulSuggestions.forCredentialsNotSent(), exceptionDuringBuildSteps);

} else if (exceptionDuringBuildSteps instanceof RegistryAuthenticationFailedException
&& exceptionDuringBuildSteps.getCause() instanceof HttpResponseException) {
handleRegistryUnauthorizedException(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,10 @@ public String forCredentialsNotCorrect(String registry) {
return suggest("make sure your credentials for '" + registry + "' are set up correctly");
}

public String forCredentialsNotSent() {
return suggest("use a registry that supports HTTPS so credentials can be sent safely");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think prefixing with an explanation before the suggestion would be good. Something along the lines of:

The registry requires credentials but credentials were not sent because the connection was over HTTP.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm should that prefix go in the new exception instead of HelpfulSuggestions, or is it too specific for that?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, good catch, it should go in the new exception instead.

}

public String forDockerContextInsecureRecursiveDelete(String directory) {
return suggest("clear " + directory + " manually before creating the Docker context");
}
Expand All @@ -108,6 +112,11 @@ public String forDockerNotInstalled() {
return suggest("make sure Docker is installed and you have correct privileges to run it");
}

public String forInsecureRegistry() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like the more useful message, but I don't see a reference to this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one was already here, just moved it because it was all the way at the bottom of the file. Is this actually more useful? I thought the problem happened even if you set allowInsecureRegistries.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's called slightly further down in BuildStepRunner

Copy link
Contributor Author

@TadCordle TadCordle Jul 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think that's the one that happens if allowInsecureRegistries isn't set and it tries to do an HTTP fallback. But the error we're throwing here is when HTTP fallback proceeds, but credentials are required (and not sent).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah right, I should read more before commenting haha.

return suggest(
"use a registry that supports HTTPS or set the configuration parameter 'allowInsecureRegistries'");
}

/**
* @param parameter the parameter name (e.g. 'to.image' or {@literal <to><image>})
* @param buildConfigFilename the name of the build config (build.gradle or pom.xml)
Expand Down Expand Up @@ -145,9 +154,4 @@ private String forNoCredentialHelpersDefined(
+ "' or "
+ authConfiguration);
}

public String forInsecureRegistry() {
return suggest(
"use a registry that supports HTTPS or set the configuration parameter 'allowInsecureRegistries'");
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
/*
* Copyright 2018 Google LLC. All rights reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License"); you may not
* use this file except in compliance with the License. You may obtain a copy of
* the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
* License for the specific language governing permissions and limitations under
* the License.
*/

package com.google.cloud.tools.jib.registry;

/** Thrown when registry request was unauthorized because credentials weren't sent. */
public class RegistryCredentialsNotSentException extends RegistryException {

/**
* Identifies the image registry and repository that denied access.
*
* @param registry the image registry
* @param repository the image repository
*/
RegistryCredentialsNotSentException(String registry, String repository) {
super("Credentials for " + registry + "/" + repository + " were not sent");
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,8 @@ class RegistryEndpointCaller<T> {
private static final String DEFAULT_PROTOCOL = "https";

/** Maintains the state of a request. This is used to retry requests with different parameters. */
private static class RequestState {
@VisibleForTesting
static class RequestState {

@Nullable private final Authorization authorization;
private final URL url;
Expand All @@ -61,7 +62,8 @@ private static class RequestState {
* @param authorization authentication credentials
* @param url the endpoint URL to call
*/
private RequestState(@Nullable Authorization authorization, URL url) {
@VisibleForTesting
RequestState(@Nullable Authorization authorization, URL url) {
this.authorization = authorization;
this.url = url;
}
Expand Down Expand Up @@ -148,8 +150,9 @@ T call() throws IOException, RegistryException {
* @throws IOException for most I/O exceptions when making the request
* @throws RegistryException for known exceptions when interacting with the registry
*/
@VisibleForTesting
@Nullable
private T call(RequestState requestState) throws IOException, RegistryException {
T call(RequestState requestState) throws IOException, RegistryException {
boolean isHttpProtocol = "http".equals(requestState.url.getProtocol());
if (!allowHttp && isHttpProtocol) {
throw new InsecureRegistryException(requestState.url);
Expand Down Expand Up @@ -193,13 +196,28 @@ private T call(RequestState requestState) throws IOException, RegistryException

throw registryErrorExceptionBuilder.build();

} else if (httpResponseException.getStatusCode() == HttpStatusCodes.STATUS_CODE_UNAUTHORIZED
|| httpResponseException.getStatusCode() == HttpStatusCodes.STATUS_CODE_FORBIDDEN) {
} else if (httpResponseException.getStatusCode() == HttpStatusCodes.STATUS_CODE_FORBIDDEN) {
throw new RegistryUnauthorizedException(
registryEndpointRequestProperties.getServerUrl(),
registryEndpointRequestProperties.getImageName(),
httpResponseException);

} else if (httpResponseException.getStatusCode()
== HttpStatusCodes.STATUS_CODE_UNAUTHORIZED) {
if (isHttpProtocol) {
// Using HTTP, so credentials weren't sent.
throw new RegistryCredentialsNotSentException(
registryEndpointRequestProperties.getServerUrl(),
registryEndpointRequestProperties.getImageName());

} else {
// Using HTTPS, so credentials are missing.
throw new RegistryUnauthorizedException(
registryEndpointRequestProperties.getServerUrl(),
registryEndpointRequestProperties.getImageName(),
httpResponseException);
}

} else if (httpResponseException.getStatusCode()
== HttpStatusCodes.STATUS_CODE_TEMPORARY_REDIRECT
|| httpResponseException.getStatusCode()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import com.google.cloud.tools.jib.cache.CacheMetadataCorruptedException;
import com.google.cloud.tools.jib.configuration.CacheConfiguration;
import com.google.cloud.tools.jib.registry.InsecureRegistryException;
import com.google.cloud.tools.jib.registry.RegistryCredentialsNotSentException;
import com.google.cloud.tools.jib.registry.RegistryUnauthorizedException;
import com.google.common.collect.ImmutableList;
import java.io.IOException;
Expand Down Expand Up @@ -64,6 +65,7 @@ public class BuildStepsRunnerTest {
@Mock private SourceFilesConfiguration mockSourceFilesConfiguration;
@Mock private BuildLogger mockBuildLogger;
@Mock private RegistryUnauthorizedException mockRegistryUnauthorizedException;
@Mock private RegistryCredentialsNotSentException mockRegistryCredentialsNotSentException;
@Mock private HttpResponseException mockHttpResponseException;
@Mock private ExecutionException mockExecutionException;
@Mock private BuildConfiguration mockBuildConfiguration;
Expand Down Expand Up @@ -215,6 +217,24 @@ public void testBuildImage_executionException_registryUnauthorizedException_noCr
}
}

@Test
public void testBuildImage_executionException_registryCredentialsNotSentException()
throws InterruptedException, ExecutionException, CacheMetadataCorruptedException, IOException,
CacheDirectoryNotOwnedException, CacheDirectoryCreationException {
Mockito.when(mockExecutionException.getCause())
.thenReturn(mockRegistryCredentialsNotSentException);
Mockito.doThrow(mockExecutionException).when(mockBuildSteps).run();

try {
testBuildImageStepsRunner.build(TEST_HELPFUL_SUGGESTIONS);
Assert.fail("buildImage should have thrown an exception");

} catch (BuildStepsExecutionException ex) {
Assert.assertEquals(TEST_HELPFUL_SUGGESTIONS.forCredentialsNotSent(), ex.getMessage());
Assert.assertEquals(mockRegistryCredentialsNotSentException, ex.getCause());
}
}

@Test
public void testBuildImage_executionException_registryUnauthorizedException_other()
throws InterruptedException, ExecutionException, CacheMetadataCorruptedException, IOException,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,5 +68,8 @@ public void testSuggestions_smoke() {
"messagePrefix, perhaps you should add a parameter configuration parameter to your buildFile or set the parameter via the commandline (e.g. 'command').",
TEST_HELPFUL_SUGGESTIONS.forToNotConfigured("parameter", "buildFile", "command"));
Assert.assertEquals("messagePrefix", TEST_HELPFUL_SUGGESTIONS.none());
Assert.assertEquals(
"messagePrefix, perhaps you should use a registry that supports HTTPS so credentials can be sent safely",
TEST_HELPFUL_SUGGESTIONS.forCredentialsNotSent());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import com.google.cloud.tools.jib.http.Connection;
import com.google.cloud.tools.jib.http.Response;
import com.google.cloud.tools.jib.json.JsonTemplateMapper;
import com.google.cloud.tools.jib.registry.RegistryEndpointCaller.RequestState;
import com.google.cloud.tools.jib.registry.json.ErrorEntryTemplate;
import com.google.cloud.tools.jib.registry.json.ErrorResponseTemplate;
import java.io.IOException;
Expand Down Expand Up @@ -143,6 +144,38 @@ public void testCall_unauthorized() throws IOException, RegistryException {
verifyThrowsRegistryUnauthorizedException(HttpStatusCodes.STATUS_CODE_UNAUTHORIZED);
}

@Test
public void testCall_credentialsNotSent() throws IOException, RegistryException {
// Mocks a response for temporary redirect to a new location.
Mockito.when(mockHttpResponse.getStatusCode())
.thenReturn(HttpStatusCodes.STATUS_CODE_UNAUTHORIZED);
Mockito.when(mockHttpResponse.getHeaders())
.thenReturn(new HttpHeaders().setLocation("http://location"));

HttpResponseException httpResponseException = new HttpResponseException(mockHttpResponse);
Mockito.when(mockConnection.send(Mockito.eq("httpMethod"), Mockito.any()))
.thenThrow(httpResponseException)
.thenReturn(mockResponse);

RegistryEndpointCaller<String> testRegistryEndpointCallerInsecure =
new RegistryEndpointCaller<>(
"userAgent",
"apiRouteBase",
new TestRegistryEndpointProvider(),
Authorizations.withBasicToken("token"),
new RegistryEndpointRequestProperties("serverUrl", "imageName"),
true,
mockConnectionFactory);
try {
testRegistryEndpointCallerInsecure.call(
new RequestState(Authorizations.withBasicToken("token"), new URL("http://location")));
Assert.fail("Call should have failed");

} catch (RegistryCredentialsNotSentException ex) {
Assert.assertEquals("Credentials for serverUrl/imageName were not sent", ex.getMessage());
}
}

@Test
public void testCall_forbidden() throws IOException, RegistryException {
verifyThrowsRegistryUnauthorizedException(HttpStatusCodes.STATUS_CODE_FORBIDDEN);
Expand Down