From f775e5c1a74c48d67365d1a92ecfc4d6f3e1f861 Mon Sep 17 00:00:00 2001 From: Lawrence Qiu Date: Wed, 8 Oct 2025 11:36:51 -0400 Subject: [PATCH 1/3] fix: Add setCredentials method as alternative for setCredentialsFile in LoggingAppender --- .../logging/logback/LoggingAppender.java | 43 +++++++++++++++++-- .../reflect-config.json | 1 + .../logging/logback/LoggingAppenderTest.java | 15 +++++++ 3 files changed, 55 insertions(+), 4 deletions(-) diff --git a/src/main/java/com/google/cloud/logging/logback/LoggingAppender.java b/src/main/java/com/google/cloud/logging/logback/LoggingAppender.java index ca3c41838..16b471800 100644 --- a/src/main/java/com/google/cloud/logging/logback/LoggingAppender.java +++ b/src/main/java/com/google/cloud/logging/logback/LoggingAppender.java @@ -23,6 +23,7 @@ import ch.qos.logback.core.UnsynchronizedAppenderBase; import ch.qos.logback.core.util.Loader; import com.google.api.core.InternalApi; +import com.google.api.core.ObsoleteApi; import com.google.auth.oauth2.GoogleCredentials; import com.google.cloud.MonitoredResource; import com.google.cloud.logging.Instrumentation; @@ -35,10 +36,12 @@ import com.google.cloud.logging.Payload; import com.google.cloud.logging.Severity; import com.google.cloud.logging.Synchronicity; +import com.google.common.base.Preconditions; import com.google.common.base.Strings; import com.google.common.collect.ImmutableList; -import java.io.FileInputStream; import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Paths; import java.time.Instant; import java.util.ArrayList; import java.util.HashMap; @@ -138,6 +141,7 @@ public class LoggingAppender extends UnsynchronizedAppenderBase { private String log; private String resourceType; private String credentialsFile; + private GoogleCredentials credentials; private String logDestinationProjectId; private boolean autoPopulateMetadata = true; private boolean redirectToStdout = false; @@ -185,17 +189,46 @@ public void setResourceType(String resourceType) { } /** - * Sets the path to the If you know that you will be loading credential configurations of a specific type, it is + * recommended to use a credential-type-specific `fromStream()` method. This will ensure that an + * unexpected credential type with potential for malicious intent is not loaded unintentionally. + * You might still have to do validation for certain credential types. Please follow the + * recommendation for that method. + * + *

If you are loading your credential configuration from an untrusted source and have not + * mitigated the risks (e.g. by validating the configuration yourself), make these changes as soon + * as possible to prevent security risks to your environment. + * + *

Regardless of the method used, it is always your responsibility to validate configurations + * received from external sources. + * + *

Sets the path to the credential * file. If not set the appender will use {@link GoogleCredentials#getApplicationDefault()} to * authenticate. * * @param credentialsFile the path to the credentials file. */ + @ObsoleteApi( + "This method is obsolete because of a potential security risk. Use the setCredentials() instead") public void setCredentialsFile(String credentialsFile) { this.credentialsFile = credentialsFile; } + /** + * Sets the credential to use. If not set the appender will use {@link + * GoogleCredentials#getApplicationDefault()} to authenticate. + * + * @param credentials the GoogleCredentials to set + */ + public void setCredentials(GoogleCredentials credentials) { + Preconditions.checkNotNull(credentials, "Credentials cannot be null"); + this.credentials = credentials; + } + /** * Sets project ID to be used to customize log destination name for written log entries. * @@ -445,10 +478,12 @@ protected LoggingOptions getLoggingOptions() { if (loggingOptions == null) { LoggingOptions.Builder builder = LoggingOptions.newBuilder(); builder.setProjectId(logDestinationProjectId); - if (!Strings.isNullOrEmpty(credentialsFile)) { + if (credentials != null) { + builder.setCredentials(credentials); + } else if (!Strings.isNullOrEmpty(credentialsFile)) { try { builder.setCredentials( - GoogleCredentials.fromStream(new FileInputStream(credentialsFile))); + GoogleCredentials.fromStream(Files.newInputStream(Paths.get(credentialsFile)))); } catch (IOException e) { throw new RuntimeException( String.format( diff --git a/src/main/resources/META-INF/native-image/com.google.cloud/google-cloud-logging-logback/reflect-config.json b/src/main/resources/META-INF/native-image/com.google.cloud/google-cloud-logging-logback/reflect-config.json index 68b566d6e..9d249db03 100644 --- a/src/main/resources/META-INF/native-image/com.google.cloud/google-cloud-logging-logback/reflect-config.json +++ b/src/main/resources/META-INF/native-image/com.google.cloud/google-cloud-logging-logback/reflect-config.json @@ -35,6 +35,7 @@ {"name":"","parameterTypes":[] }, {"name":"setAutoPopulateMetadata","parameterTypes":["boolean"] }, {"name":"setCredentialsFile","parameterTypes":["java.lang.String"] }, + {"name":"setCredentials","parameterTypes":["com.google.auth.oauth2.GoogleCredentials"] }, {"name":"setFlushLevel","parameterTypes":["ch.qos.logback.classic.Level"] }, {"name":"setLog","parameterTypes":["java.lang.String"] }, {"name":"setLogDestinationProjectId","parameterTypes":["java.lang.String"] }, diff --git a/src/test/java/com/google/cloud/logging/logback/LoggingAppenderTest.java b/src/test/java/com/google/cloud/logging/logback/LoggingAppenderTest.java index 9632cf330..038ad4d4b 100644 --- a/src/test/java/com/google/cloud/logging/logback/LoggingAppenderTest.java +++ b/src/test/java/com/google/cloud/logging/logback/LoggingAppenderTest.java @@ -23,11 +23,13 @@ import static org.easymock.EasyMock.replay; import static org.easymock.EasyMock.verify; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertThrows; import ch.qos.logback.classic.Level; import ch.qos.logback.classic.filter.ThresholdFilter; import ch.qos.logback.classic.spi.ILoggingEvent; import ch.qos.logback.classic.spi.LoggingEvent; +import com.google.auth.oauth2.GoogleCredentials; import com.google.cloud.MonitoredResource; import com.google.cloud.Timestamp; import com.google.cloud.logging.Instrumentation; @@ -289,6 +291,19 @@ public void testMdcValuesAreConvertedToLabels() { assertThat(capturedArgument.getValue().iterator().next()).isEqualTo(INFO_ENTRY); } + @Test + public void testCreateLoggingOptionsWithValidCredentials() { + LoggingAppender appender = new LoggingAppender(); + appender.setCredentials(GoogleCredentials.newBuilder().build()); + appender.getLoggingOptions(); + } + + @Test + public void testCreateLoggingOptionsWithNullCredentials() { + LoggingAppender appender = new LoggingAppender(); + assertThrows(NullPointerException.class, () -> appender.setCredentials(null)); + } + @Test(expected = RuntimeException.class) public void testCreateLoggingOptionsWithInvalidCredentials() { final String nonExistentFile = "/path/to/non/existent/file"; From 36c5e0c3528193d2b6288c2d1da68ba1eda6730d Mon Sep 17 00:00:00 2001 From: Lawrence Qiu Date: Wed, 8 Oct 2025 11:47:28 -0400 Subject: [PATCH 2/3] chore: Fix test requiring ProjectId --- .../cloud/logging/logback/LoggingAppenderTest.java | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/test/java/com/google/cloud/logging/logback/LoggingAppenderTest.java b/src/test/java/com/google/cloud/logging/logback/LoggingAppenderTest.java index 038ad4d4b..027230a83 100644 --- a/src/test/java/com/google/cloud/logging/logback/LoggingAppenderTest.java +++ b/src/test/java/com/google/cloud/logging/logback/LoggingAppenderTest.java @@ -62,7 +62,7 @@ public class LoggingAppenderTest { private static final String PROJECT_ID = "test-project"; private static final String CRED_FILE_PROJECT_ID = "project-12345"; - private static final String OVERRIDED_PROJECT_ID = "some-project-id"; + private static final String OVERRIDDEN_PROJECT_ID = "some-project-id"; private static final String DUMMY_CRED_FILE_PATH = "src/test/java/com/google/cloud/logging/logback/dummy-credentials.json"; private static final Payload.JsonPayload JSON_PAYLOAD = @@ -295,6 +295,9 @@ public void testMdcValuesAreConvertedToLabels() { public void testCreateLoggingOptionsWithValidCredentials() { LoggingAppender appender = new LoggingAppender(); appender.setCredentials(GoogleCredentials.newBuilder().build()); + // ServiceOptions requires a projectId to be set. Normally this is determined by the + // GoogleCredentials (Credential set above is a dummy value with no ProjectId). + appender.setLogDestinationProjectId(PROJECT_ID); appender.getLoggingOptions(); } @@ -325,8 +328,8 @@ public void testCreateLoggingOptionsWithDestination() { // Try to build LoggingOptions with file based credentials. LoggingAppender appender = new LoggingAppender(); appender.setCredentialsFile(DUMMY_CRED_FILE_PATH); - appender.setLogDestinationProjectId(OVERRIDED_PROJECT_ID); - assertThat(appender.getLoggingOptions().getProjectId()).isEqualTo(OVERRIDED_PROJECT_ID); + appender.setLogDestinationProjectId(OVERRIDDEN_PROJECT_ID); + assertThat(appender.getLoggingOptions().getProjectId()).isEqualTo(OVERRIDDEN_PROJECT_ID); } private LoggingEvent createLoggingEvent(Level level, long timestamp) { From f133f11248a6524e60f66014acfa24f3dc8372bb Mon Sep 17 00:00:00 2001 From: Lawrence Qiu Date: Wed, 8 Oct 2025 16:48:02 -0400 Subject: [PATCH 3/3] Apply suggestion from @gkevinzheng Co-authored-by: Kevin Zheng <147537668+gkevinzheng@users.noreply.github.com> --- .../java/com/google/cloud/logging/logback/LoggingAppender.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/com/google/cloud/logging/logback/LoggingAppender.java b/src/main/java/com/google/cloud/logging/logback/LoggingAppender.java index 16b471800..b247af0c5 100644 --- a/src/main/java/com/google/cloud/logging/logback/LoggingAppender.java +++ b/src/main/java/com/google/cloud/logging/logback/LoggingAppender.java @@ -213,7 +213,7 @@ public void setResourceType(String resourceType) { * @param credentialsFile the path to the credentials file. */ @ObsoleteApi( - "This method is obsolete because of a potential security risk. Use the setCredentials() instead") + "This method is obsolete because of a potential security risk. Use the setCredentials() method instead") public void setCredentialsFile(String credentialsFile) { this.credentialsFile = credentialsFile; }