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

GCP auth failure logging and testing #942

Merged
merged 6 commits into from
Oct 12, 2023
Merged
Show file tree
Hide file tree
Changes from 5 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
@@ -0,0 +1,3 @@
plugins {
id 'java-test-fixtures'
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure it is worth creating a new file to apply a single plugin, we could just apply the plugin in gcp-common/build.gradle

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm ambivalent about that. This is a result of following the way it is done in micronaut-test-resources as I was initially having some odd compilation issues when adding the plugin and following what was done there got me past that. That turned out to be more an issue with how dependencies are managed for test-fixtures so I'm fine with getting rid of this file and moving the plugin configuration.

}
6 changes: 6 additions & 0 deletions gcp-common/build.gradle
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
plugins {
id("io.micronaut.build.internal.gcp-module")
id 'io.micronaut.build.internal.test-fixtures'
}

dependencies {
Expand All @@ -10,9 +11,14 @@ dependencies {
implementation(mn.micronaut.json.core)
implementation(mn.jackson.annotations)

testFixturesApi(platform(mn.micronaut.core.bom))

testAnnotationProcessor(mn.micronaut.inject.java)
testImplementation(mn.micronaut.discovery.core)
testImplementation(mn.micronaut.http.server.netty)

testImplementation(mnSerde.micronaut.serde.jackson)

testImplementation(libs.system.stubs.core)

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
/*
* Copyright 2017-2023 original authors
*
* 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
*
* https://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 io.micronaut.gcp.credentials;

import com.google.auth.RequestMetadataCallback;
import io.micronaut.aop.MethodInterceptor;
import io.micronaut.aop.MethodInvocationContext;
import io.micronaut.core.annotation.Nullable;
import io.micronaut.core.type.MutableArgumentValue;
import jakarta.inject.Singleton;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import java.io.IOException;
import java.util.List;
import java.util.Map;

/**
* An interceptor for managed instances of {@link com.google.auth.oauth2.GoogleCredentials} that logs certain types of
* authentication errors that the GCP libraries handle silently as infinitely retryable events.
*
* @author Jeremy Grelle
* @since 5.2.0
*/
@Singleton
public class AuthenticationLoggingInterceptor implements MethodInterceptor<Object, Object> {

private static final Logger LOG = LoggerFactory.getLogger(AuthenticationLoggingInterceptor.class);
private static final String LOGGED_AUTHENTICATION_METHOD = "getRequestMetadata";

/**
* Intercepts the "getRequestMetadata" call and logs any retryable errors before allowing the GCP library to continue
* its normal retry algorithm.
*
* @param context The method invocation context
* @return the result of the method invocation
*/
@Override
public @Nullable Object intercept(MethodInvocationContext<Object, Object> context) {
if (!context.getExecutableMethod().getMethodName().equals(LOGGED_AUTHENTICATION_METHOD)) {
return context.proceed();
}
timyates marked this conversation as resolved.
Show resolved Hide resolved
Map<String, MutableArgumentValue<?>> params = context.getParameters();
params.entrySet().stream().filter(entry -> entry.getValue().getType().equals(RequestMetadataCallback.class))
.findFirst()
.ifPresent(entry -> {
@SuppressWarnings("unchecked") MutableArgumentValue<RequestMetadataCallback> argValue = (MutableArgumentValue<RequestMetadataCallback>) entry.getValue();
RequestMetadataCallback callback = argValue.getValue();
argValue.setValue(new LoggingRequestMetadataCallback(callback));
});
return context.proceed();
}

/**
* A wrapper {@link RequestMetadataCallback} implementation that logs failures with a warning before proceeding with
* the original callback.
*/
private static final class LoggingRequestMetadataCallback implements RequestMetadataCallback {

private final RequestMetadataCallback callback;

private LoggingRequestMetadataCallback(RequestMetadataCallback callback) {
this.callback = callback;
}

@Override
public void onSuccess(Map<String, List<String>> metadata) {
this.callback.onSuccess(metadata);
}

@Override
public void onFailure(Throwable ex) {
if (ex instanceof IOException) {
LOG.warn("A failure occurred while attempting to build credential metadata for a GCP API request. The GCP libraries treat this as " +
"a retryable error, but misconfigured credentials can keep it from ever succeeding.", ex);
timyates marked this conversation as resolved.
Show resolved Hide resolved
}
this.callback.onFailure(ex);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,13 @@
import io.micronaut.context.annotation.Primary;
import io.micronaut.context.annotation.Requires;
import io.micronaut.context.exceptions.ConfigurationException;
import io.micronaut.core.annotation.NonNull;
import io.micronaut.core.util.ArgumentUtils;
import io.micronaut.core.util.StringUtils;
import jakarta.inject.Singleton;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import io.micronaut.core.annotation.NonNull;
import jakarta.inject.Singleton;
import java.io.ByteArrayInputStream;
import java.io.FileInputStream;
import java.io.IOException;
Expand Down Expand Up @@ -83,11 +83,11 @@ public GoogleCredentialsFactory(@NonNull GoogleCredentialsConfiguration configur
* @return The {@link GoogleCredentials}
* @throws IOException An exception if an error occurs
*/
@Requires(missingBeans = GoogleCredentials.class)
Copy link
Contributor

Choose a reason for hiding this comment

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

why removing this requirement?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's somewhat redundant and odd that it was ever added here in the first place, and it was preventing the AOP interceptor from working (which may be a separate issue worth investigating). The documented and intended way to replace the framework-supplied GoogleCredentials being created by this factory is to use gcp.credentials.enabled=false.

Note that I can't think of any good reason for a user to ever supply their own version of GoogleCredentials.

timyates marked this conversation as resolved.
Show resolved Hide resolved
@Requires(classes = com.google.auth.oauth2.GoogleCredentials.class)
@Requires(classes = GoogleCredentials.class)
@Requires(property = GoogleCredentialsConfiguration.PREFIX + ".enabled", value = StringUtils.TRUE, defaultValue = StringUtils.TRUE)
@Primary
@Singleton
@LogAuthenticationFailures
protected GoogleCredentials defaultGoogleCredentials() throws IOException {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you need to expose GoogleCredentials? Can at be some interface?

Copy link
Contributor Author

@jeremyg484 jeremyg484 Oct 4, 2023

Choose a reason for hiding this comment

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

No, unfortunately it needs to be GoogleCredentials (and what actually gets instantiated and returned here is a subclass of GoogleCredentials, depending on which method of authentication is being used - developer credentials, impersonated service account credentials, and proper service account credentials being the variations). This ultimately gets passed to the various higher-level GCP library's service abstractions and is used to facilitate their GRPC-based communication.

The error that we are interested in detecting and reporting to the users of our module happens deep within the GRPC service flow and is never propagated beyond the getRequestMetadata method that is being intercepted here.

final List<String> scopes = configuration.getScopes().stream()
.map(URI::toString).collect(Collectors.toList());
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
/*
* Copyright 2017-2023 original authors
*
* 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
*
* https://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 io.micronaut.gcp.credentials;

import com.google.auth.oauth2.GoogleCredentials;
import io.micronaut.aop.Around;
import io.micronaut.context.annotation.Type;

import java.lang.annotation.Documented;
import java.lang.annotation.ElementType;
import java.lang.annotation.Retention;
import java.lang.annotation.Target;

import static java.lang.annotation.RetentionPolicy.RUNTIME;

/**
* Annotation for applying authentication failure logging AOP advice to a managed instance of
* {@link GoogleCredentials}.
*
* @author Jeremy Grelle
* @since 5.2.0
*/
@Documented
@Retention(RUNTIME)
@Target({ElementType.METHOD, ElementType.TYPE})
@Around(proxyTargetMode = Around.ProxyTargetConstructorMode.ALLOW)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we use?

@Around(proxyTargetMode = Around.ProxyTargetConstructorMode.ALLOW, proxyTarget=true)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just verified via test that it seems to already be behaving as though proxyTarget=true in that the returned instance implements InterceptedProxy. Explicitly setting proxyTarget=true gives the same result.

@Type(AuthenticationLoggingInterceptor.class)
public @interface LogAuthenticationFailures {
}
Loading