From 48ff83dc68e29dcae07fdea963cbbe5525f86a89 Mon Sep 17 00:00:00 2001 From: Leo <39062083+lsirac@users.noreply.github.com> Date: Fri, 5 Aug 2022 10:47:25 -0700 Subject: [PATCH] fix: updates executable response spec for executable-sourced credentials (#955) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: expiration_time is only required for successful responses when an output file is specified in the credential configuration * fix: updates PluggableAuthCredentials java docs and missing spot in README * fix: doc fix * 🦉 Updates from OwlBot post-processor See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md Co-authored-by: Owl Bot --- README.md | 15 +- .../auth/oauth2/ExecutableResponse.java | 15 +- .../auth/oauth2/PluggableAuthCredentials.java | 9 +- .../auth/oauth2/PluggableAuthHandler.java | 12 + .../auth/oauth2/ExecutableResponseTest.java | 56 +++-- .../auth/oauth2/PluggableAuthHandlerTest.java | 212 ++++++++++++++++++ 6 files changed, 281 insertions(+), 38 deletions(-) diff --git a/README.md b/README.md index de8018ac4..8882f7a74 100644 --- a/README.md +++ b/README.md @@ -421,11 +421,15 @@ A sample executable error response: These are all required fields for an error response. The code and message fields will be used by the library as part of the thrown exception. +For successful responses, the `expiration_time` field is only required +when an output file is specified in the credential configuration. + Response format fields summary: * `version`: The version of the JSON output. Currently only version 1 is supported. - * `success`: The status of the response. When true, the response must contain the 3rd party token, - token type, and expiration. The executable must also exit with exit code 0. - When false, the response must contain the error code and message fields and exit with a non-zero value. + * `success`: When true, the response must contain the 3rd party token and token type. The response must also contain + the expiration_time field if an output file was specified in the credential configuration. The executable must also + exit with exit code 0. When false, the response must contain the error code and message fields and exit with a + non-zero value. * `token_type`: The 3rd party subject token type. Must be *urn:ietf:params:oauth:token-type:jwt*, *urn:ietf:params:oauth:token-type:id_token*, or *urn:ietf:params:oauth:token-type:saml2*. * `id_token`: The 3rd party OIDC token. @@ -435,8 +439,9 @@ Response format fields summary: * `message`: The error message. All response types must include both the `version` and `success` fields. - * Successful responses must include the `token_type`, `expiration_time`, and one of - `id_token` or `saml_response`. + * Successful responses must include the `token_type` and one of + `id_token` or `saml_response`. The `expiration_time` field must also be present if an output file was specified in + the credential configuration. * Error responses must include both the `code` and `message` fields. The library will populate the following environment variables when the executable is run: diff --git a/oauth2_http/java/com/google/auth/oauth2/ExecutableResponse.java b/oauth2_http/java/com/google/auth/oauth2/ExecutableResponse.java index 5559b5442..278b71047 100644 --- a/oauth2_http/java/com/google/auth/oauth2/ExecutableResponse.java +++ b/oauth2_http/java/com/google/auth/oauth2/ExecutableResponse.java @@ -75,14 +75,11 @@ class ExecutableResponse { "The executable response is missing the `token_type` field."); } - if (!json.containsKey("expiration_time")) { - throw new PluggableAuthException( - "INVALID_EXECUTABLE_RESPONSE", - "The executable response is missing the `expiration_time` field."); - } - this.tokenType = (String) json.get("token_type"); - this.expirationTime = parseLongField(json.get("expiration_time")); + + if (json.containsKey("expiration_time")) { + this.expirationTime = parseLongField(json.get("expiration_time")); + } if (SAML_SUBJECT_TOKEN_TYPE.equals(tokenType)) { this.subjectToken = (String) json.get("saml_response"); @@ -132,9 +129,9 @@ boolean isSuccessful() { return this.success; } - /** Returns true if the subject token is expired or not present, false otherwise. */ + /** Returns true if the subject token is expired, false otherwise. */ boolean isExpired() { - return this.expirationTime == null || this.expirationTime <= Instant.now().getEpochSecond(); + return this.expirationTime != null && this.expirationTime <= Instant.now().getEpochSecond(); } /** Returns whether the execution was successful and returned an unexpired token. */ diff --git a/oauth2_http/java/com/google/auth/oauth2/PluggableAuthCredentials.java b/oauth2_http/java/com/google/auth/oauth2/PluggableAuthCredentials.java index e3506c080..7fba136a4 100644 --- a/oauth2_http/java/com/google/auth/oauth2/PluggableAuthCredentials.java +++ b/oauth2_http/java/com/google/auth/oauth2/PluggableAuthCredentials.java @@ -54,9 +54,9 @@ *

Both OIDC and SAML are supported. The executable must adhere to a specific response format * defined below. * - *

The executable should print out the 3rd party token to STDOUT in JSON format. This is not - * required when an output_file is specified in the credential source, with the expectation being - * that the output file will contain the JSON response instead. + *

The executable must print out the 3rd party token to STDOUT in JSON format. When an + * output_file is specified in the credential configuration, the executable must also handle writing + * the JSON response to this file. * * 

  * OIDC response sample:
@@ -85,6 +85,9 @@
  *   "message": "Error message."
  * }
  *
+ * 
 The `expiration_time` field in the JSON response is only required for successful
+ *  responses when an output file was specified in the credential configuration.
+ *
  * The auth libraries will populate certain environment variables that will be accessible by the
  * executable, such as: GOOGLE_EXTERNAL_ACCOUNT_AUDIENCE, GOOGLE_EXTERNAL_ACCOUNT_TOKEN_TYPE,
  * GOOGLE_EXTERNAL_ACCOUNT_INTERACTIVE, GOOGLE_EXTERNAL_ACCOUNT_IMPERSONATED_EMAIL, and
diff --git a/oauth2_http/java/com/google/auth/oauth2/PluggableAuthHandler.java b/oauth2_http/java/com/google/auth/oauth2/PluggableAuthHandler.java
index 24b0978cd..6d62d6911 100644
--- a/oauth2_http/java/com/google/auth/oauth2/PluggableAuthHandler.java
+++ b/oauth2_http/java/com/google/auth/oauth2/PluggableAuthHandler.java
@@ -112,6 +112,18 @@ public String retrieveTokenFromExecutable(ExecutableOptions options) throws IOEx
       executableResponse = getExecutableResponse(options);
     }
 
+    // If an output file is specified, successful responses must contain the `expiration_time`
+    // field.
+    if (options.getOutputFilePath() != null
+        && !options.getOutputFilePath().isEmpty()
+        && executableResponse.isSuccessful()
+        && executableResponse.getExpirationTime() == null) {
+      throw new PluggableAuthException(
+          "INVALID_EXECUTABLE_RESPONSE",
+          "The executable response must contain the `expiration_time` field for successful responses when an "
+              + "output_file has been specified in the configuration.");
+    }
+
     // The executable response includes a version. Validate that the version is compatible
     // with the library.
     if (executableResponse.getVersion() > EXECUTABLE_SUPPORTED_MAX_VERSION) {
diff --git a/oauth2_http/javatests/com/google/auth/oauth2/ExecutableResponseTest.java b/oauth2_http/javatests/com/google/auth/oauth2/ExecutableResponseTest.java
index b6f85684a..7c8fec60d 100644
--- a/oauth2_http/javatests/com/google/auth/oauth2/ExecutableResponseTest.java
+++ b/oauth2_http/javatests/com/google/auth/oauth2/ExecutableResponseTest.java
@@ -60,12 +60,27 @@ void constructor_successOidcResponse() throws IOException {
 
     assertTrue(response.isSuccessful());
     assertTrue(response.isValid());
-    assertEquals(1, response.getVersion());
+    assertEquals(EXECUTABLE_SUPPORTED_MAX_VERSION, response.getVersion());
     assertEquals(TOKEN_TYPE_OIDC, response.getTokenType());
     assertEquals(ID_TOKEN, response.getSubjectToken());
     assertEquals(
         Instant.now().getEpochSecond() + EXPIRATION_DURATION, response.getExpirationTime());
-    assertEquals(1, response.getVersion());
+  }
+
+  @Test
+  void constructor_successOidcResponseMissingExpirationTimeField_notExpired() throws IOException {
+    GenericJson jsonResponse = buildOidcResponse();
+    jsonResponse.remove("expiration_time");
+
+    ExecutableResponse response = new ExecutableResponse(jsonResponse);
+
+    assertTrue(response.isSuccessful());
+    assertTrue(response.isValid());
+    assertFalse(response.isExpired());
+    assertEquals(EXECUTABLE_SUPPORTED_MAX_VERSION, response.getVersion());
+    assertEquals(TOKEN_TYPE_OIDC, response.getTokenType());
+    assertEquals(ID_TOKEN, response.getSubjectToken());
+    assertNull(response.getExpirationTime());
   }
 
   @Test
@@ -81,17 +96,33 @@ void constructor_successSamlResponse() throws IOException {
         Instant.now().getEpochSecond() + EXPIRATION_DURATION, response.getExpirationTime());
   }
 
+  @Test
+  void constructor_successSamlResponseMissingExpirationTimeField_notExpired() throws IOException {
+    GenericJson jsonResponse = buildSamlResponse();
+    jsonResponse.remove("expiration_time");
+
+    ExecutableResponse response = new ExecutableResponse(jsonResponse);
+
+    assertTrue(response.isSuccessful());
+    assertTrue(response.isValid());
+    assertFalse(response.isExpired());
+    assertEquals(EXECUTABLE_SUPPORTED_MAX_VERSION, response.getVersion());
+    assertEquals(TOKEN_TYPE_SAML, response.getTokenType());
+    assertEquals(SAML_RESPONSE, response.getSubjectToken());
+    assertNull(response.getExpirationTime());
+  }
+
   @Test
   void constructor_validErrorResponse() throws IOException {
     ExecutableResponse response = new ExecutableResponse(buildErrorResponse());
 
     assertFalse(response.isSuccessful());
     assertFalse(response.isValid());
-    assertTrue(response.isExpired());
+    assertFalse(response.isExpired());
     assertNull(response.getSubjectToken());
     assertNull(response.getTokenType());
     assertNull(response.getExpirationTime());
-    assertEquals(1, response.getVersion());
+    assertEquals(EXECUTABLE_SUPPORTED_MAX_VERSION, response.getVersion());
     assertEquals("401", response.getErrorCode());
     assertEquals("Caller not authorized.", response.getErrorMessage());
   }
@@ -189,23 +220,6 @@ void constructor_successResponseMissingTokenTypeField_throws() {
         exception.getMessage());
   }
 
-  @Test
-  void constructor_successResponseMissingExpirationTimeField_throws() {
-    GenericJson jsonResponse = buildOidcResponse();
-    jsonResponse.remove("expiration_time");
-
-    PluggableAuthException exception =
-        assertThrows(
-            PluggableAuthException.class,
-            () -> new ExecutableResponse(jsonResponse),
-            "Exception should be thrown.");
-
-    assertEquals(
-        "Error code INVALID_EXECUTABLE_RESPONSE: The executable response is missing the "
-            + "`expiration_time` field.",
-        exception.getMessage());
-  }
-
   @Test
   void constructor_samlResponseMissingSubjectToken_throws() {
     GenericJson jsonResponse = buildSamlResponse();
diff --git a/oauth2_http/javatests/com/google/auth/oauth2/PluggableAuthHandlerTest.java b/oauth2_http/javatests/com/google/auth/oauth2/PluggableAuthHandlerTest.java
index 4e630d49c..d751c403f 100644
--- a/oauth2_http/javatests/com/google/auth/oauth2/PluggableAuthHandlerTest.java
+++ b/oauth2_http/javatests/com/google/auth/oauth2/PluggableAuthHandlerTest.java
@@ -51,7 +51,9 @@
 import java.io.IOException;
 import java.nio.charset.StandardCharsets;
 import java.time.Instant;
+import java.util.Arrays;
 import java.util.HashMap;
+import java.util.List;
 import java.util.Map;
 import java.util.concurrent.TimeUnit;
 import javax.annotation.Nullable;
@@ -218,6 +220,216 @@ void retrieveTokenFromExecutable_errorResponse_throws() throws InterruptedExcept
     assertEquals("Caller not authorized.", e.getErrorDescription());
   }
 
+  @Test
+  void retrieveTokenFromExecutable_successResponseWithoutExpirationTimeField()
+      throws InterruptedException, IOException {
+    TestEnvironmentProvider environmentProvider = new TestEnvironmentProvider();
+    environmentProvider.setEnv("GOOGLE_EXTERNAL_ACCOUNT_ALLOW_EXECUTABLES", "1");
+
+    // Expected environment mappings.
+    HashMap expectedMap = new HashMap<>();
+    expectedMap.putAll(DEFAULT_OPTIONS.getEnvironmentMap());
+
+    Map currentEnv = new HashMap<>();
+
+    // Mock executable handling.
+    Process mockProcess = Mockito.mock(Process.class);
+    when(mockProcess.waitFor(anyLong(), any(TimeUnit.class))).thenReturn(true);
+    when(mockProcess.exitValue()).thenReturn(EXIT_CODE_SUCCESS);
+
+    // Remove expiration_time from the executable responses.
+    GenericJson oidcResponse = buildOidcResponse();
+    oidcResponse.remove("expiration_time");
+
+    GenericJson samlResponse = buildSamlResponse();
+    samlResponse.remove("expiration_time");
+
+    List responses = Arrays.asList(oidcResponse, samlResponse);
+    for (int i = 0; i < responses.size(); i++) {
+      when(mockProcess.getInputStream())
+          .thenReturn(
+              new ByteArrayInputStream(
+                  responses.get(i).toString().getBytes(StandardCharsets.UTF_8)));
+
+      InternalProcessBuilder processBuilder =
+          buildInternalProcessBuilder(
+              currentEnv, mockProcess, DEFAULT_OPTIONS.getExecutableCommand());
+
+      PluggableAuthHandler handler = new PluggableAuthHandler(environmentProvider, processBuilder);
+
+      // Call retrieveTokenFromExecutable().
+      String token = handler.retrieveTokenFromExecutable(DEFAULT_OPTIONS);
+
+      verify(mockProcess, times(i + 1)).destroy();
+      verify(mockProcess, times(i + 1))
+          .waitFor(
+              eq(Long.valueOf(DEFAULT_OPTIONS.getExecutableTimeoutMs())),
+              eq(TimeUnit.MILLISECONDS));
+
+      if (responses.get(i).equals(oidcResponse)) {
+        assertEquals(ID_TOKEN, token);
+      } else {
+        assertEquals(SAML_RESPONSE, token);
+      }
+
+      // Current env map should have the mappings from options.
+      assertEquals(2, currentEnv.size());
+      assertEquals(expectedMap, currentEnv);
+    }
+  }
+
+  @Test
+  void
+      retrieveTokenFromExecutable_successResponseWithoutExpirationTimeFieldWithOutputFileSpecified_throws()
+          throws InterruptedException, IOException {
+    TestEnvironmentProvider environmentProvider = new TestEnvironmentProvider();
+    environmentProvider.setEnv("GOOGLE_EXTERNAL_ACCOUNT_ALLOW_EXECUTABLES", "1");
+
+    // Options with output file specified.
+    ExecutableOptions options =
+        new ExecutableOptions() {
+          @Override
+          public String getExecutableCommand() {
+            return "/path/to/executable";
+          }
+
+          @Override
+          public Map getEnvironmentMap() {
+            return ImmutableMap.of();
+          }
+
+          @Override
+          public int getExecutableTimeoutMs() {
+            return 30000;
+          }
+
+          @Override
+          public String getOutputFilePath() {
+            return "/path/to/output/file";
+          }
+        };
+
+    // Mock executable handling.
+    Process mockProcess = Mockito.mock(Process.class);
+    when(mockProcess.waitFor(anyLong(), any(TimeUnit.class))).thenReturn(true);
+    when(mockProcess.exitValue()).thenReturn(EXIT_CODE_SUCCESS);
+
+    // Remove expiration_time from the executable responses.
+    GenericJson oidcResponse = buildOidcResponse();
+    oidcResponse.remove("expiration_time");
+
+    GenericJson samlResponse = buildSamlResponse();
+    samlResponse.remove("expiration_time");
+
+    List responses = Arrays.asList(oidcResponse, samlResponse);
+    for (int i = 0; i < responses.size(); i++) {
+      when(mockProcess.getInputStream())
+          .thenReturn(
+              new ByteArrayInputStream(
+                  responses.get(i).toString().getBytes(StandardCharsets.UTF_8)));
+
+      InternalProcessBuilder processBuilder =
+          buildInternalProcessBuilder(new HashMap<>(), mockProcess, options.getExecutableCommand());
+
+      PluggableAuthHandler handler = new PluggableAuthHandler(environmentProvider, processBuilder);
+
+      // Call retrieveTokenFromExecutable() should throw an exception as the STDOUT response
+      // is missing
+      // the `expiration_time` field and an output file was specified in the configuration.
+      PluggableAuthException exception =
+          assertThrows(
+              PluggableAuthException.class,
+              () -> handler.retrieveTokenFromExecutable(options),
+              "Exception should be thrown.");
+
+      assertEquals(
+          "Error code INVALID_EXECUTABLE_RESPONSE: The executable response must contain the "
+              + "`expiration_time` field for successful responses when an output_file has been specified in the"
+              + " configuration.",
+          exception.getMessage());
+
+      verify(mockProcess, times(i + 1)).destroy();
+      verify(mockProcess, times(i + 1))
+          .waitFor(eq(Long.valueOf(options.getExecutableTimeoutMs())), eq(TimeUnit.MILLISECONDS));
+    }
+  }
+
+  @Test
+  void retrieveTokenFromExecutable_successResponseInOutputFileMissingExpirationTimeField_throws()
+      throws InterruptedException, IOException {
+    TestEnvironmentProvider environmentProvider = new TestEnvironmentProvider();
+    environmentProvider.setEnv("GOOGLE_EXTERNAL_ACCOUNT_ALLOW_EXECUTABLES", "1");
+
+    // Build output_file.
+    File file = File.createTempFile("output_file", /* suffix= */ null, /* directory= */ null);
+    file.deleteOnExit();
+
+    // Options with output file specified.
+    ExecutableOptions options =
+        new ExecutableOptions() {
+          @Override
+          public String getExecutableCommand() {
+            return "/path/to/executable";
+          }
+
+          @Override
+          public Map getEnvironmentMap() {
+            return ImmutableMap.of();
+          }
+
+          @Override
+          public int getExecutableTimeoutMs() {
+            return 30000;
+          }
+
+          @Override
+          public String getOutputFilePath() {
+            return file.getAbsolutePath();
+          }
+        };
+
+    // Mock executable handling that does nothing since we are using the output file.
+    Process mockProcess = Mockito.mock(Process.class);
+    InternalProcessBuilder processBuilder =
+        buildInternalProcessBuilder(new HashMap<>(), mockProcess, options.getExecutableCommand());
+
+    // Remove expiration_time from the executable responses.
+    GenericJson oidcResponse = buildOidcResponse();
+    oidcResponse.remove("expiration_time");
+
+    GenericJson samlResponse = buildSamlResponse();
+    samlResponse.remove("expiration_time");
+
+    List responses = Arrays.asList(oidcResponse, samlResponse);
+    for (GenericJson json : responses) {
+      OAuth2Utils.writeInputStreamToFile(
+          new ByteArrayInputStream(json.toString().getBytes(StandardCharsets.UTF_8)),
+          file.getAbsolutePath());
+
+      PluggableAuthHandler handler = new PluggableAuthHandler(environmentProvider, processBuilder);
+
+      // Call retrieveTokenFromExecutable() which should throw an exception as the output file
+      // response is missing
+      // the `expiration_time` field.
+      PluggableAuthException exception =
+          assertThrows(
+              PluggableAuthException.class,
+              () -> handler.retrieveTokenFromExecutable(options),
+              "Exception should be thrown.");
+
+      assertEquals(
+          "Error code INVALID_EXECUTABLE_RESPONSE: The executable response must contain the "
+              + "`expiration_time` field for successful responses when an output_file has been specified in the"
+              + " configuration.",
+          exception.getMessage());
+
+      // Validate executable not invoked.
+      verify(mockProcess, times(0)).destroyForcibly();
+      verify(mockProcess, times(0))
+          .waitFor(eq(Long.valueOf(options.getExecutableTimeoutMs())), eq(TimeUnit.MILLISECONDS));
+    }
+  }
+
   @Test
   void retrieveTokenFromExecutable_withOutputFile_usesCachedResponse()
       throws IOException, InterruptedException {