diff --git a/oauth2_http/java/com/google/auth/oauth2/PluggableAuthCredentials.java b/oauth2_http/java/com/google/auth/oauth2/PluggableAuthCredentials.java index 17cff7ef0..7bb465118 100644 --- a/oauth2_http/java/com/google/auth/oauth2/PluggableAuthCredentials.java +++ b/oauth2_http/java/com/google/auth/oauth2/PluggableAuthCredentials.java @@ -33,9 +33,12 @@ import com.google.auth.oauth2.ExecutableHandler.ExecutableOptions; import com.google.common.annotations.VisibleForTesting; -import java.io.*; +import java.io.IOException; import java.math.BigDecimal; -import java.util.*; +import java.util.ArrayList; +import java.util.Collection; +import java.util.HashMap; +import java.util.Map; import javax.annotation.Nullable; /** @@ -227,6 +230,12 @@ public AccessToken refreshAccessToken() throws IOException { return exchangeExternalCredentialForAccessToken(stsTokenExchangeRequest.build()); } + /** + * Returns the 3rd party subject token by calling the executable specified in the credential + * source. + * + * @throws IOException if an error occurs with the executable execution. + */ @Override public String retrieveSubjectToken() throws IOException { String executableCommand = config.getCommand(); diff --git a/oauth2_http/java/com/google/auth/oauth2/PluggableAuthHandler.java b/oauth2_http/java/com/google/auth/oauth2/PluggableAuthHandler.java index d83e541d7..afc0b9840 100644 --- a/oauth2_http/java/com/google/auth/oauth2/PluggableAuthHandler.java +++ b/oauth2_http/java/com/google/auth/oauth2/PluggableAuthHandler.java @@ -35,7 +35,6 @@ import com.google.api.client.json.JsonParser; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Splitter; -import com.google.common.io.CharStreams; import java.io.BufferedReader; import java.io.File; import java.io.FileInputStream; @@ -45,7 +44,12 @@ import java.nio.charset.StandardCharsets; import java.util.List; import java.util.Map; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; import java.util.concurrent.TimeUnit; +import javax.annotation.Nullable; /** * Internal handler for retrieving 3rd party tokens from user defined scripts/executables for @@ -139,31 +143,7 @@ public String retrieveTokenFromExecutable(ExecutableOptions options) throws IOEx // this file. // If specified, we will first check if we have valid unexpired credentials stored in this // location to avoid running the executable until they are expired. - ExecutableResponse executableResponse = null; - if (options.getOutputFilePath() != null && !options.getOutputFilePath().isEmpty()) { - // Try reading cached response from output_file. - try { - File outputFile = new File(options.getOutputFilePath()); - // Check if the output file is valid and not empty. - if (outputFile.isFile() && outputFile.length() > 0) { - InputStream inputStream = new FileInputStream(options.getOutputFilePath()); - BufferedReader reader = - new BufferedReader(new InputStreamReader(inputStream, StandardCharsets.UTF_8)); - JsonParser parser = OAuth2Utils.JSON_FACTORY.createJsonParser(reader); - ExecutableResponse cachedResponse = - new ExecutableResponse(parser.parseAndClose(GenericJson.class)); - // If the cached response is successful and unexpired, we can use it. - // Response version will be validated below. - if (cachedResponse.isValid()) { - executableResponse = cachedResponse; - } - } - } catch (Exception e) { - throw new PluggableAuthException( - "INVALID_OUTPUT_FILE", - "The output_file specified contains an invalid or malformed response." + e); - } - } + ExecutableResponse executableResponse = getCachedExecutableResponse(options); // If the output_file does not contain a valid response, call the executable. if (executableResponse == null) { @@ -194,6 +174,37 @@ public String retrieveTokenFromExecutable(ExecutableOptions options) throws IOEx return executableResponse.getSubjectToken(); } + @Nullable + ExecutableResponse getCachedExecutableResponse(ExecutableOptions options) + throws PluggableAuthException { + ExecutableResponse executableResponse = null; + if (options.getOutputFilePath() != null && !options.getOutputFilePath().isEmpty()) { + // Try reading cached response from output_file. + try { + File outputFile = new File(options.getOutputFilePath()); + // Check if the output file is valid and not empty. + if (outputFile.isFile() && outputFile.length() > 0) { + InputStream inputStream = new FileInputStream(options.getOutputFilePath()); + BufferedReader reader = + new BufferedReader(new InputStreamReader(inputStream, StandardCharsets.UTF_8)); + JsonParser parser = OAuth2Utils.JSON_FACTORY.createJsonParser(reader); + ExecutableResponse cachedResponse = + new ExecutableResponse(parser.parseAndClose(GenericJson.class)); + // If the cached response is successful and unexpired, we can use it. + // Response version will be validated below. + if (cachedResponse.isValid()) { + executableResponse = cachedResponse; + } + } + } catch (Exception e) { + throw new PluggableAuthException( + "INVALID_OUTPUT_FILE", + "The output_file specified contains an invalid or malformed response." + e); + } + } + return executableResponse; + } + ExecutableResponse getExecutableResponse(ExecutableOptions options) throws IOException { List components = Splitter.on(" ").splitToList(options.getExecutableCommand()); @@ -213,6 +224,24 @@ ExecutableResponse getExecutableResponse(ExecutableOptions options) throws IOExc ExecutableResponse execResp; String executableOutput = ""; try { + // Consume the input stream while waiting for the program to finish so that + // the process won't hang if the STDOUT buffer is filled. + ExecutorService executor = Executors.newSingleThreadExecutor(); + Future future = + executor.submit( + () -> { + BufferedReader reader = + new BufferedReader( + new InputStreamReader(process.getInputStream(), StandardCharsets.UTF_8)); + + StringBuilder sb = new StringBuilder(); + String line; + while ((line = reader.readLine()) != null) { + sb.append(line).append(System.lineSeparator()); + } + return sb.toString().trim(); + }); + boolean success = process.waitFor(options.getExecutableTimeoutMs(), TimeUnit.MILLISECONDS); if (!success) { // Process has not terminated within the specified timeout. @@ -224,30 +253,32 @@ ExecutableResponse getExecutableResponse(ExecutableOptions options) throws IOExc throw new PluggableAuthException( "EXIT_CODE", String.format("The executable failed with exit code %s.", exitCode)); } - BufferedReader reader = - new BufferedReader( - new InputStreamReader(process.getInputStream(), StandardCharsets.UTF_8)); - executableOutput = CharStreams.toString(reader); + executableOutput = future.get(); + executor.shutdownNow(); + JsonParser parser = OAuth2Utils.JSON_FACTORY.createJsonParser(executableOutput); execResp = new ExecutableResponse(parser.parseAndClose(GenericJson.class)); - } catch (InterruptedException e) { - // Destroy the process. - process.destroyForcibly(); - throw new PluggableAuthException( - "INTERRUPTED", String.format("The execution was interrupted: %s.", e)); } catch (IOException e) { // Destroy the process. - process.destroyForcibly(); + process.destroy(); + if (e instanceof PluggableAuthException) { throw e; } - // An error may have occurred in the executable and needs to be surfaced. + // An error may have occurred in the executable and should be surfaced. throw new PluggableAuthException( "INVALID_RESPONSE", String.format("The executable returned an invalid response: %s.", executableOutput)); + } catch (InterruptedException | ExecutionException e) { + // Destroy the process. + process.destroy(); + + throw new PluggableAuthException( + "INTERRUPTED", String.format("The execution was interrupted: %s.", e)); } - process.destroyForcibly(); + + process.destroy(); return execResp; } diff --git a/oauth2_http/javatests/com/google/auth/oauth2/PluggableAuthHandlerTest.java b/oauth2_http/javatests/com/google/auth/oauth2/PluggableAuthHandlerTest.java index 5233509cf..4e630d49c 100644 --- a/oauth2_http/javatests/com/google/auth/oauth2/PluggableAuthHandlerTest.java +++ b/oauth2_http/javatests/com/google/auth/oauth2/PluggableAuthHandlerTest.java @@ -130,7 +130,7 @@ void retrieveTokenFromExecutable_oidcResponse() throws IOException, InterruptedE // Call retrieveTokenFromExecutable(). String token = handler.retrieveTokenFromExecutable(DEFAULT_OPTIONS); - verify(mockProcess, times(1)).destroyForcibly(); + verify(mockProcess, times(1)).destroy(); verify(mockProcess, times(1)) .waitFor( eq(Long.valueOf(DEFAULT_OPTIONS.getExecutableTimeoutMs())), eq(TimeUnit.MILLISECONDS)); @@ -175,7 +175,7 @@ void retrieveTokenFromExecutable_samlResponse() throws IOException, InterruptedE // Call retrieveTokenFromExecutable(). String token = handler.retrieveTokenFromExecutable(DEFAULT_OPTIONS); - verify(mockProcess, times(1)).destroyForcibly(); + verify(mockProcess, times(1)).destroy(); verify(mockProcess, times(1)) .waitFor( eq(Long.valueOf(DEFAULT_OPTIONS.getExecutableTimeoutMs())), eq(TimeUnit.MILLISECONDS)); @@ -387,7 +387,7 @@ public String getOutputFilePath() { String token = handler.retrieveTokenFromExecutable(options); // Validate that the executable was called. - verify(mockProcess, times(1)).destroyForcibly(); + verify(mockProcess, times(1)).destroy(); verify(mockProcess, times(1)) .waitFor(eq(Long.valueOf(options.getExecutableTimeoutMs())), eq(TimeUnit.MILLISECONDS)); @@ -517,7 +517,7 @@ void getExecutableResponse_oidcResponse() throws IOException, InterruptedExcepti ExecutableResponse response = handler.getExecutableResponse(DEFAULT_OPTIONS); - verify(mockProcess, times(1)).destroyForcibly(); + verify(mockProcess, times(1)).destroy(); verify(mockProcess, times(1)) .waitFor( eq(Long.valueOf(DEFAULT_OPTIONS.getExecutableTimeoutMs())), eq(TimeUnit.MILLISECONDS)); @@ -564,7 +564,7 @@ void getExecutableResponse_samlResponse() throws IOException, InterruptedExcepti PluggableAuthHandler handler = new PluggableAuthHandler(environmentProvider, processBuilder); ExecutableResponse response = handler.getExecutableResponse(DEFAULT_OPTIONS); - verify(mockProcess, times(1)).destroyForcibly(); + verify(mockProcess, times(1)).destroy(); verify(mockProcess, times(1)) .waitFor( eq(Long.valueOf(DEFAULT_OPTIONS.getExecutableTimeoutMs())), eq(TimeUnit.MILLISECONDS)); @@ -579,7 +579,7 @@ void getExecutableResponse_samlResponse() throws IOException, InterruptedExcepti assertEquals(4, currentEnv.size()); assertEquals(expectedMap, currentEnv); - verify(mockProcess, times(1)).destroyForcibly(); + verify(mockProcess, times(1)).destroy(); } @Test @@ -598,6 +598,7 @@ void getExecutableResponse_errorResponse() throws IOException, InterruptedExcept // 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); @@ -615,7 +616,7 @@ void getExecutableResponse_errorResponse() throws IOException, InterruptedExcept // Call getExecutableResponse(). ExecutableResponse response = handler.getExecutableResponse(DEFAULT_OPTIONS); - verify(mockProcess, times(1)).destroyForcibly(); + verify(mockProcess, times(1)).destroy(); verify(mockProcess, times(1)) .waitFor( eq(Long.valueOf(DEFAULT_OPTIONS.getExecutableTimeoutMs())), eq(TimeUnit.MILLISECONDS)); @@ -654,7 +655,7 @@ void getExecutableResponse_timeoutExceeded_throws() throws InterruptedException verify(mockProcess, times(1)) .waitFor( eq(Long.valueOf(DEFAULT_OPTIONS.getExecutableTimeoutMs())), eq(TimeUnit.MILLISECONDS)); - verify(mockProcess, times(1)).destroyForcibly(); + verify(mockProcess, times(1)).destroy(); } @Test @@ -686,7 +687,7 @@ void getExecutableResponse_nonZeroExitCode_throws() throws InterruptedException verify(mockProcess, times(1)) .waitFor( eq(Long.valueOf(DEFAULT_OPTIONS.getExecutableTimeoutMs())), eq(TimeUnit.MILLISECONDS)); - verify(mockProcess, times(1)).destroyForcibly(); + verify(mockProcess, times(1)).destroy(); } @Test @@ -717,7 +718,7 @@ void getExecutableResponse_processInterrupted_throws() throws InterruptedExcepti verify(mockProcess, times(1)) .waitFor( eq(Long.valueOf(DEFAULT_OPTIONS.getExecutableTimeoutMs())), eq(TimeUnit.MILLISECONDS)); - verify(mockProcess, times(1)).destroyForcibly(); + verify(mockProcess, times(1)).destroy(); } @Test @@ -754,7 +755,7 @@ void getExecutableResponse_invalidResponse_throws() throws InterruptedException verify(mockProcess, times(1)) .waitFor( eq(Long.valueOf(DEFAULT_OPTIONS.getExecutableTimeoutMs())), eq(TimeUnit.MILLISECONDS)); - verify(mockProcess, times(1)).destroyForcibly(); + verify(mockProcess, times(1)).destroy(); } private static GenericJson buildOidcResponse() {