From fe11aafdc5f44fc60bb3e073c7654ca9f0b9c20d Mon Sep 17 00:00:00 2001 From: Hanzhen Yi Date: Tue, 30 Jan 2018 11:55:33 -0800 Subject: [PATCH 1/8] Validate project-id returned from metadata server to handle cases in which a descriptive failure html page is returned instead of the projectid itself from server when projects are not running in google cloud machines. --- .../main/java/com/google/cloud/ServiceOptions.java | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/google-cloud-core/src/main/java/com/google/cloud/ServiceOptions.java b/google-cloud-core/src/main/java/com/google/cloud/ServiceOptions.java index 51f5a52f267b..052c89cd1e0e 100644 --- a/google-cloud-core/src/main/java/com/google/cloud/ServiceOptions.java +++ b/google-cloud-core/src/main/java/com/google/cloud/ServiceOptions.java @@ -452,7 +452,8 @@ private static String getAppEngineProjectIdFromMetadataServer() throws IOExcepti .setReadTimeout(500) .setHeaders(new HttpHeaders().set("Metadata-Flavor", "Google")); HttpResponse response = request.execute(); - return response.parseAsString(); + String projectId = response.parseAsString(); + return isValidProjectId(projectId)? projectId : null; } protected static String getServiceAccountProjectId() { @@ -476,6 +477,17 @@ static String getServiceAccountProjectId(String credentialsPath) { return project; } + /* + * projectId must be between 6 and 30 characters + * projectId can have lowercase letters, digits or hyphens + * and must start with a lowercase letter + */ + private boolean isValidProjectId(String projectId) { + Pattern p = Pattern.compile("^[a-z][a-z0-9-]*$"); + Matcher m = p.matcher(projectId); + return projectId.length() >= 6 && projectId.length() <= 30 + && m.matches(); + } /** * Returns a Service object for the current service. For instance, when using Google Cloud From 8348feb98bd6c3138e5cb8eb8f21638edae4c1a3 Mon Sep 17 00:00:00 2001 From: Hanzhen Yi Date: Tue, 30 Jan 2018 13:53:53 -0800 Subject: [PATCH 2/8] update comments --- .../src/main/java/com/google/cloud/ServiceOptions.java | 1 + 1 file changed, 1 insertion(+) diff --git a/google-cloud-core/src/main/java/com/google/cloud/ServiceOptions.java b/google-cloud-core/src/main/java/com/google/cloud/ServiceOptions.java index 052c89cd1e0e..b939afcdca06 100644 --- a/google-cloud-core/src/main/java/com/google/cloud/ServiceOptions.java +++ b/google-cloud-core/src/main/java/com/google/cloud/ServiceOptions.java @@ -478,6 +478,7 @@ static String getServiceAccountProjectId(String credentialsPath) { } /* + * Returns true if the projectId is valid. * projectId must be between 6 and 30 characters * projectId can have lowercase letters, digits or hyphens * and must start with a lowercase letter From 2c86cb293f89811356542a45277742181d578ea7 Mon Sep 17 00:00:00 2001 From: Hanzhen Yi Date: Tue, 30 Jan 2018 11:55:33 -0800 Subject: [PATCH 3/8] Validate project-id returned from metadata server to handle cases in which a descriptive failure html page is returned instead of the projectid itself from server when projects are not running in google cloud machines. --- .../src/main/java/com/google/cloud/ServiceOptions.java | 1 - 1 file changed, 1 deletion(-) diff --git a/google-cloud-core/src/main/java/com/google/cloud/ServiceOptions.java b/google-cloud-core/src/main/java/com/google/cloud/ServiceOptions.java index b939afcdca06..052c89cd1e0e 100644 --- a/google-cloud-core/src/main/java/com/google/cloud/ServiceOptions.java +++ b/google-cloud-core/src/main/java/com/google/cloud/ServiceOptions.java @@ -478,7 +478,6 @@ static String getServiceAccountProjectId(String credentialsPath) { } /* - * Returns true if the projectId is valid. * projectId must be between 6 and 30 characters * projectId can have lowercase letters, digits or hyphens * and must start with a lowercase letter From 7ab63f15a0db183c446bf8867a24a167421bc9a7 Mon Sep 17 00:00:00 2001 From: Hanzhen Yi Date: Tue, 30 Jan 2018 13:53:53 -0800 Subject: [PATCH 4/8] update comments --- .../src/main/java/com/google/cloud/ServiceOptions.java | 1 + 1 file changed, 1 insertion(+) diff --git a/google-cloud-core/src/main/java/com/google/cloud/ServiceOptions.java b/google-cloud-core/src/main/java/com/google/cloud/ServiceOptions.java index 052c89cd1e0e..b939afcdca06 100644 --- a/google-cloud-core/src/main/java/com/google/cloud/ServiceOptions.java +++ b/google-cloud-core/src/main/java/com/google/cloud/ServiceOptions.java @@ -478,6 +478,7 @@ static String getServiceAccountProjectId(String credentialsPath) { } /* + * Returns true if the projectId is valid. * projectId must be between 6 and 30 characters * projectId can have lowercase letters, digits or hyphens * and must start with a lowercase letter From 52d0d98d4653c5d3c2c9302c925e56cee37166b5 Mon Sep 17 00:00:00 2001 From: Hanzhen Yi Date: Wed, 31 Jan 2018 11:19:54 -0800 Subject: [PATCH 5/8] fix code review problems --- .../java/com/google/cloud/ServiceOptions.java | 17 ++++++++--------- .../com/google/cloud/ServiceOptionsTest.java | 13 +++++++++++++ 2 files changed, 21 insertions(+), 9 deletions(-) diff --git a/google-cloud-core/src/main/java/com/google/cloud/ServiceOptions.java b/google-cloud-core/src/main/java/com/google/cloud/ServiceOptions.java index b939afcdca06..7fda059e02d4 100644 --- a/google-cloud-core/src/main/java/com/google/cloud/ServiceOptions.java +++ b/google-cloud-core/src/main/java/com/google/cloud/ServiceOptions.java @@ -86,6 +86,8 @@ public abstract class ServiceOptions, private static final RetrySettings NO_RETRY_SETTINGS = getDefaultRetrySettingsBuilder() .setMaxAttempts(1).build(); + private static final Pattern projectIdPattern = Pattern.compile("^[a-z][a-z0-9-]*[a-z0-9]+$"); + private static final long serialVersionUID = 9198896031667942014L; private final String projectId; @@ -478,16 +480,13 @@ static String getServiceAccountProjectId(String credentialsPath) { } /* - * Returns true if the projectId is valid. - * projectId must be between 6 and 30 characters - * projectId can have lowercase letters, digits or hyphens - * and must start with a lowercase letter + * Returns true if the projectId is valid. This method checks whether the projectId + * contains only lowercase letters, digits and hyphens, starts with a lowercase letter + * and does not end with a hyphen, but does not check the length of projectId. This + * method is primarily used to protect against DNS hijacking. */ - private boolean isValidProjectId(String projectId) { - Pattern p = Pattern.compile("^[a-z][a-z0-9-]*$"); - Matcher m = p.matcher(projectId); - return projectId.length() >= 6 && projectId.length() <= 30 - && m.matches(); + static boolean isValidProjectId(String projectId) { + return projectIdPattern.matcher(projectId).matches(); } /** diff --git a/google-cloud-core/src/test/java/com/google/cloud/ServiceOptionsTest.java b/google-cloud-core/src/test/java/com/google/cloud/ServiceOptionsTest.java index 7afcb7f581b8..ba864657e957 100644 --- a/google-cloud-core/src/test/java/com/google/cloud/ServiceOptionsTest.java +++ b/google-cloud-core/src/test/java/com/google/cloud/ServiceOptionsTest.java @@ -23,6 +23,7 @@ import static org.junit.Assert.assertSame; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; +import static com.google.common.truth.Truth.assertThat; import com.google.api.core.ApiClock; import com.google.api.core.CurrentMillisClock; @@ -313,4 +314,16 @@ public void testGetServiceAccountProjectId_nonExistentFile() throws Exception { assertNull(ServiceOptions.getServiceAccountProjectId(credentialsFile.getPath())); } + + @Test + public void testValidateProjectId() throws Exception { + String validProjectId = "abc-123"; + String invalidProjectId1 = "abc=123"; + String invalidProjectId2 = "abc123-"; + String invalidProjectId3 = "1abc-23"; + assertThat(ServiceOptions.isValidProjectId(validProjectId)).isTrue(); + assertThat(ServiceOptions.isValidProjectId(invalidProjectId1)).isFalse(); + assertThat(ServiceOptions.isValidProjectId(invalidProjectId2)).isFalse(); + assertThat(ServiceOptions.isValidProjectId(invalidProjectId3)).isFalse(); + } } From c88ff33f9f884db1437d32c4a81814b953dd7676 Mon Sep 17 00:00:00 2001 From: Hanzhen Yi Date: Wed, 31 Jan 2018 14:42:45 -0800 Subject: [PATCH 6/8] Fix code review problems --- .../main/java/com/google/cloud/ServiceOptions.java | 2 +- .../java/com/google/cloud/ServiceOptionsTest.java | 13 +++++-------- 2 files changed, 6 insertions(+), 9 deletions(-) diff --git a/google-cloud-core/src/main/java/com/google/cloud/ServiceOptions.java b/google-cloud-core/src/main/java/com/google/cloud/ServiceOptions.java index 7fda059e02d4..a96c14277a88 100644 --- a/google-cloud-core/src/main/java/com/google/cloud/ServiceOptions.java +++ b/google-cloud-core/src/main/java/com/google/cloud/ServiceOptions.java @@ -86,7 +86,7 @@ public abstract class ServiceOptions, private static final RetrySettings NO_RETRY_SETTINGS = getDefaultRetrySettingsBuilder() .setMaxAttempts(1).build(); - private static final Pattern projectIdPattern = Pattern.compile("^[a-z][a-z0-9-]*[a-z0-9]+$"); + private static final Pattern projectIdPattern = Pattern.compile("^[a-z]([a-z0-9]*-*)*[a-z0-9]$"); private static final long serialVersionUID = 9198896031667942014L; diff --git a/google-cloud-core/src/test/java/com/google/cloud/ServiceOptionsTest.java b/google-cloud-core/src/test/java/com/google/cloud/ServiceOptionsTest.java index ba864657e957..ce72245d9e68 100644 --- a/google-cloud-core/src/test/java/com/google/cloud/ServiceOptionsTest.java +++ b/google-cloud-core/src/test/java/com/google/cloud/ServiceOptionsTest.java @@ -317,13 +317,10 @@ public void testGetServiceAccountProjectId_nonExistentFile() throws Exception { @Test public void testValidateProjectId() throws Exception { - String validProjectId = "abc-123"; - String invalidProjectId1 = "abc=123"; - String invalidProjectId2 = "abc123-"; - String invalidProjectId3 = "1abc-23"; - assertThat(ServiceOptions.isValidProjectId(validProjectId)).isTrue(); - assertThat(ServiceOptions.isValidProjectId(invalidProjectId1)).isFalse(); - assertThat(ServiceOptions.isValidProjectId(invalidProjectId2)).isFalse(); - assertThat(ServiceOptions.isValidProjectId(invalidProjectId3)).isFalse(); + assertThat(ServiceOptions.isValidProjectId("abc-123")).isTrue(); + assertThat(ServiceOptions.isValidProjectId("abc-123-ab")).isTrue(); + assertThat(ServiceOptions.isValidProjectId("abc=123")).isFalse(); + assertThat(ServiceOptions.isValidProjectId("abc123-")).isFalse(); + assertThat(ServiceOptions.isValidProjectId("1abc-23")).isFalse(); } } From 04ec228bf6032e716dc0ff2d562f3e949129fe0f Mon Sep 17 00:00:00 2001 From: Hanzhen Yi Date: Wed, 31 Jan 2018 15:54:40 -0800 Subject: [PATCH 7/8] fix code review problems --- .../java/com/google/cloud/ServiceOptions.java | 20 +++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/google-cloud-core/src/main/java/com/google/cloud/ServiceOptions.java b/google-cloud-core/src/main/java/com/google/cloud/ServiceOptions.java index a96c14277a88..f90c92a314c5 100644 --- a/google-cloud-core/src/main/java/com/google/cloud/ServiceOptions.java +++ b/google-cloud-core/src/main/java/com/google/cloud/ServiceOptions.java @@ -86,8 +86,6 @@ public abstract class ServiceOptions, private static final RetrySettings NO_RETRY_SETTINGS = getDefaultRetrySettingsBuilder() .setMaxAttempts(1).build(); - private static final Pattern projectIdPattern = Pattern.compile("^[a-z]([a-z0-9]*-*)*[a-z0-9]$"); - private static final long serialVersionUID = 9198896031667942014L; private final String projectId; @@ -455,7 +453,7 @@ private static String getAppEngineProjectIdFromMetadataServer() throws IOExcepti .setHeaders(new HttpHeaders().set("Metadata-Flavor", "Google")); HttpResponse response = request.execute(); String projectId = response.parseAsString(); - return isValidProjectId(projectId)? projectId : null; + return (projectId != null && isValidProjectId(projectId))? projectId : null; } protected static String getServiceAccountProjectId() { @@ -486,7 +484,21 @@ static String getServiceAccountProjectId(String credentialsPath) { * method is primarily used to protect against DNS hijacking. */ static boolean isValidProjectId(String projectId) { - return projectIdPattern.matcher(projectId).matches(); + for (char c : projectId.toCharArray()) { + if (!isLowerCase(c) && !isDigit(c) && c != '-') { + return false; + } + } + return projectId.length() > 0 && isLowerCase(projectId.charAt(0)) + && !projectId.endsWith("-"); + } + + private static boolean isLowerCase(char c) { + return c >= 'a' && c <= 'z'; + } + + private static boolean isDigit(char c) { + return c >= '0' && c <= '9'; } /** From bf39d933d1b30da5ab99f118a177c64cb75d59cf Mon Sep 17 00:00:00 2001 From: Hanzhen Yi Date: Wed, 31 Jan 2018 16:10:36 -0800 Subject: [PATCH 8/8] fix useless parentheses --- .../src/main/java/com/google/cloud/ServiceOptions.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/google-cloud-core/src/main/java/com/google/cloud/ServiceOptions.java b/google-cloud-core/src/main/java/com/google/cloud/ServiceOptions.java index f90c92a314c5..d57c24bc2a17 100644 --- a/google-cloud-core/src/main/java/com/google/cloud/ServiceOptions.java +++ b/google-cloud-core/src/main/java/com/google/cloud/ServiceOptions.java @@ -453,7 +453,7 @@ private static String getAppEngineProjectIdFromMetadataServer() throws IOExcepti .setHeaders(new HttpHeaders().set("Metadata-Flavor", "Google")); HttpResponse response = request.execute(); String projectId = response.parseAsString(); - return (projectId != null && isValidProjectId(projectId))? projectId : null; + return projectId != null && isValidProjectId(projectId)? projectId : null; } protected static String getServiceAccountProjectId() {