From 4cd63342d5454bb7ed15b3b49ed65c1453364264 Mon Sep 17 00:00:00 2001 From: Ashitha Santhosh Date: Wed, 21 Dec 2022 15:50:03 -0800 Subject: [PATCH 1/3] File Watcher Authorization Server Interceptor --- .../authz/AuthorizationServerInterceptor.java | 3 +- ...WatcherAuthorizationServerInterceptor.java | 130 +++++ .../grpc/authz/AuthorizationEnd2EndTest.java | 478 +++++++++++++++++- ...herAuthorizationServerInterceptorTest.java | 116 +++++ 4 files changed, 710 insertions(+), 17 deletions(-) create mode 100644 authz/src/main/java/io/grpc/authz/FileWatcherAuthorizationServerInterceptor.java create mode 100644 authz/src/test/java/io/grpc/authz/FileWatcherAuthorizationServerInterceptorTest.java diff --git a/authz/src/main/java/io/grpc/authz/AuthorizationServerInterceptor.java b/authz/src/main/java/io/grpc/authz/AuthorizationServerInterceptor.java index 2e398666093..2c01e356f59 100644 --- a/authz/src/main/java/io/grpc/authz/AuthorizationServerInterceptor.java +++ b/authz/src/main/java/io/grpc/authz/AuthorizationServerInterceptor.java @@ -35,7 +35,8 @@ * * gRPC Authorization policy as a JSON string during initialization. * This policy will be translated to Envoy RBAC policies to make - * authorization decisions. The policy cannot be changed once created. + * authorization decisions. The policy cannot be changed once created. To + * change the policy after creation, see FileWatcherAuthorizationServerInterceptor. */ @ExperimentalApi("https://github.com/grpc/grpc-java/issues/9746") public final class AuthorizationServerInterceptor implements ServerInterceptor { diff --git a/authz/src/main/java/io/grpc/authz/FileWatcherAuthorizationServerInterceptor.java b/authz/src/main/java/io/grpc/authz/FileWatcherAuthorizationServerInterceptor.java new file mode 100644 index 00000000000..6cc8bf9b033 --- /dev/null +++ b/authz/src/main/java/io/grpc/authz/FileWatcherAuthorizationServerInterceptor.java @@ -0,0 +1,130 @@ +/* + * Copyright 2022 The gRPC 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 + * + * http://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.grpc.authz; + +import static com.google.common.base.Preconditions.checkNotNull; +import static java.nio.charset.StandardCharsets.UTF_8; + +import com.google.common.annotations.VisibleForTesting; +import com.google.common.util.concurrent.ThreadFactoryBuilder; +import io.grpc.Metadata; +import io.grpc.ServerCall; +import io.grpc.ServerCallHandler; +import io.grpc.ServerInterceptor; +import java.io.Closeable; +import java.io.File; +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.attribute.FileTime; +import java.util.concurrent.Executors; +import java.util.concurrent.ScheduledExecutorService; +import java.util.concurrent.ScheduledFuture; +import java.util.concurrent.TimeUnit; +import java.util.logging.Level; +import java.util.logging.Logger; + +/** + * Authorization server interceptor for policy from file with refresh capability. + * The class will get + * gRPC Authorization policy from a JSON file during initialization. + */ +public final class FileWatcherAuthorizationServerInterceptor implements ServerInterceptor { + private static final Logger logger = + Logger.getLogger(FileWatcherAuthorizationServerInterceptor.class.getName()); + + private volatile AuthorizationServerInterceptor internalAuthzServerInterceptor; + private final ScheduledExecutorService scheduledExecutorService = + Executors.newSingleThreadScheduledExecutor( + new ThreadFactoryBuilder() + .setNameFormat("filewatcher" + "-%d") + .setDaemon(true) + .build()); + + private final File policyFile; + private FileTime lastModifiedTime; + + private Thread testCallback; + + private FileWatcherAuthorizationServerInterceptor(File policyFile) + throws IllegalArgumentException, IOException { + this.policyFile = policyFile; + updateInternalInterceptor(); + } + + @Override + public ServerCall.Listener interceptCall( + ServerCall call, Metadata headers, + ServerCallHandler next) { + return internalAuthzServerInterceptor.interceptCall(call, headers, next); + } + + void updateInternalInterceptor() throws IOException { + FileTime currentTime = Files.getLastModifiedTime(policyFile.toPath()); + if (currentTime.equals(lastModifiedTime)) { + return; + } + String policyContents = new String(Files.readAllBytes(policyFile.toPath()), UTF_8); + lastModifiedTime = currentTime; + internalAuthzServerInterceptor = AuthorizationServerInterceptor.create(policyContents); + if (testCallback != null) { + testCallback.start(); + } + } + + /** + * Policy is reloaded periodically as per the provided refresh interval. Unlike the + * constructor, exception thrown during reload will be caught and logged and the + * previous AuthorizationServerInterceptor will be used to make authorization + * decisions. + * + * @param period the period between successive file load executions. + * @param unit the time unit for period parameter + * @return an object that caller should close when the file refreshes are not needed + */ + public Closeable scheduleRefreshes(long period, TimeUnit unit) + throws IOException { + if (period <= 0) { + throw new IllegalArgumentException("Refresh interval must be greater than 0"); + } + final ScheduledFuture future = + scheduledExecutorService.scheduleWithFixedDelay(new Runnable() { + @Override public void run() { + try { + updateInternalInterceptor(); + } catch (Exception e) { + logger.log(Level.WARNING, "Authorization Policy file reload failed: " + e); + } + } + }, period, period, unit); + return new Closeable() { + @Override public void close() { + future.cancel(false); + } + }; + } + + @VisibleForTesting + public void setCallbackForTesting(Thread callback) { + testCallback = callback; + } + + public static FileWatcherAuthorizationServerInterceptor create(File policyFile) + throws IllegalArgumentException, IOException { + checkNotNull(policyFile, "policyFile"); + return new FileWatcherAuthorizationServerInterceptor(policyFile); + } +} diff --git a/authz/src/test/java/io/grpc/authz/AuthorizationEnd2EndTest.java b/authz/src/test/java/io/grpc/authz/AuthorizationEnd2EndTest.java index 28c17718d11..cb76aa8fe01 100644 --- a/authz/src/test/java/io/grpc/authz/AuthorizationEnd2EndTest.java +++ b/authz/src/test/java/io/grpc/authz/AuthorizationEnd2EndTest.java @@ -17,6 +17,8 @@ package io.grpc.authz; import static com.google.common.truth.Truth.assertThat; +import static java.nio.charset.StandardCharsets.UTF_8; +import static org.junit.Assert.assertNotNull; import static org.junit.Assert.fail; import io.grpc.ChannelCredentials; @@ -26,6 +28,7 @@ import io.grpc.ManagedChannel; import io.grpc.Server; import io.grpc.ServerCredentials; +import io.grpc.ServerInterceptor; import io.grpc.StatusRuntimeException; import io.grpc.TlsChannelCredentials; import io.grpc.TlsServerCredentials; @@ -35,7 +38,10 @@ import io.grpc.testing.protobuf.SimpleRequest; import io.grpc.testing.protobuf.SimpleResponse; import io.grpc.testing.protobuf.SimpleServiceGrpc; +import java.io.Closeable; import java.io.File; +import java.io.FileOutputStream; +import java.util.concurrent.TimeUnit; import org.junit.After; import org.junit.Test; import org.junit.runner.RunWith; @@ -52,10 +58,24 @@ public class AuthorizationEnd2EndTest { private Server server; private ManagedChannel channel; - private void initServerWithStaticAuthz( - String authorizationPolicy, ServerCredentials serverCredentials) throws Exception { - AuthorizationServerInterceptor authzInterceptor = + private AuthorizationServerInterceptor createStaticAuthorizationInterceptor( + String authorizationPolicy) throws Exception { + AuthorizationServerInterceptor interceptor = AuthorizationServerInterceptor.create(authorizationPolicy); + assertNotNull(interceptor); + return interceptor; + } + + private FileWatcherAuthorizationServerInterceptor + createFileWatcherAuthorizationInterceptor(File policyFile) throws Exception { + FileWatcherAuthorizationServerInterceptor interceptor = + FileWatcherAuthorizationServerInterceptor.create(policyFile); + assertNotNull(interceptor); + return interceptor; + } + + private void initServerWithAuthzInterceptor( + ServerInterceptor authzInterceptor, ServerCredentials serverCredentials) throws Exception { server = Grpc.newServerBuilderForPort(0, serverCredentials) .addService(new SimpleServiceImpl()) .intercept(authzInterceptor) @@ -63,11 +83,29 @@ private void initServerWithStaticAuthz( .start(); } + private File createTempAuthorizationPolicy(String authorizationPolicy) throws Exception { + File policyFile = File.createTempFile("temp", "json"); + try (FileOutputStream outputStream = new FileOutputStream(policyFile, false)) { + outputStream.write(authorizationPolicy.getBytes(UTF_8)); + outputStream.close(); + } + return policyFile; + } + + private void rewriteAuthorizationPolicy( + File policyFile, String newPolicy) throws Exception { + try (FileOutputStream outputStream = new FileOutputStream(policyFile, false)) { + outputStream.write(newPolicy.getBytes(UTF_8)); + outputStream.close(); + } + } + private SimpleServiceGrpc.SimpleServiceBlockingStub getStub() { - channel = - Grpc.newChannelBuilderForAddress( - "localhost", server.getPort(), InsecureChannelCredentials.create()) - .build(); + if (channel == null) { + channel = Grpc.newChannelBuilderForAddress( + "localhost", server.getPort(), InsecureChannelCredentials.create()) + .build(); + } return SimpleServiceGrpc.newBlockingStub(channel); } @@ -116,7 +154,8 @@ public void staticAuthzAllowsRpcNoMatchInDenyMatchInAllowTest() throws Exception + " }" + " ]" + "}"; - initServerWithStaticAuthz(policy, InsecureServerCredentials.create()); + AuthorizationServerInterceptor interceptor = createStaticAuthorizationInterceptor(policy); + initServerWithAuthzInterceptor(interceptor, InsecureServerCredentials.create()); getStub().unaryRpc(SimpleRequest.getDefaultInstance()); } @@ -145,7 +184,8 @@ public void staticAuthzDeniesRpcNoMatchInDenyAndAllowTest() throws Exception { + " }" + " ]" + "}"; - initServerWithStaticAuthz(policy, InsecureServerCredentials.create()); + AuthorizationServerInterceptor interceptor = createStaticAuthorizationInterceptor(policy); + initServerWithAuthzInterceptor(interceptor, InsecureServerCredentials.create()); try { getStub().unaryRpc(SimpleRequest.getDefaultInstance()); fail("exception expected"); @@ -182,7 +222,8 @@ public void staticAuthzDeniesRpcMatchInDenyAndAllowTest() throws Exception { + " }" + " ]" + "}"; - initServerWithStaticAuthz(policy, InsecureServerCredentials.create()); + AuthorizationServerInterceptor interceptor = createStaticAuthorizationInterceptor(policy); + initServerWithAuthzInterceptor(interceptor, InsecureServerCredentials.create()); try { getStub().unaryRpc(SimpleRequest.getDefaultInstance()); fail("exception expected"); @@ -219,7 +260,8 @@ public void staticAuthzDeniesRpcMatchInDenyNoMatchInAllowTest() throws Exception + " }" + " ]" + "}"; - initServerWithStaticAuthz(policy, InsecureServerCredentials.create()); + AuthorizationServerInterceptor interceptor = createStaticAuthorizationInterceptor(policy); + initServerWithAuthzInterceptor(interceptor, InsecureServerCredentials.create()); try { getStub().unaryRpc(SimpleRequest.getDefaultInstance()); fail("exception expected"); @@ -246,7 +288,8 @@ public void staticAuthzAllowsRpcEmptyDenyMatchInAllowTest() throws Exception { + " }" + " ]" + "}"; - initServerWithStaticAuthz(policy, InsecureServerCredentials.create()); + AuthorizationServerInterceptor interceptor = createStaticAuthorizationInterceptor(policy); + initServerWithAuthzInterceptor(interceptor, InsecureServerCredentials.create()); getStub().unaryRpc(SimpleRequest.getDefaultInstance()); } @@ -265,7 +308,8 @@ public void staticAuthzDeniesRpcEmptyDenyNoMatchInAllowTest() throws Exception { + " }" + " ]" + "}"; - initServerWithStaticAuthz(policy, InsecureServerCredentials.create()); + AuthorizationServerInterceptor interceptor = createStaticAuthorizationInterceptor(policy); + initServerWithAuthzInterceptor(interceptor, InsecureServerCredentials.create()); try { getStub().unaryRpc(SimpleRequest.getDefaultInstance()); fail("exception expected"); @@ -291,7 +335,8 @@ public void staticAuthzDeniesRpcWithPrincipalsFieldOnUnauthenticatedConnectionTe + " }" + " ]" + "}"; - initServerWithStaticAuthz(policy, InsecureServerCredentials.create()); + AuthorizationServerInterceptor interceptor = createStaticAuthorizationInterceptor(policy); + initServerWithAuthzInterceptor(interceptor, InsecureServerCredentials.create()); try { getStub().unaryRpc(SimpleRequest.getDefaultInstance()); fail("exception expected"); @@ -322,12 +367,13 @@ public void staticAuthzAllowsRpcWithPrincipalsFieldOnMtlsAuthenticatedConnection + " }" + " ]" + "}"; + AuthorizationServerInterceptor interceptor = createStaticAuthorizationInterceptor(policy); ServerCredentials serverCredentials = TlsServerCredentials.newBuilder() .keyManager(serverCert0File, serverKey0File) .trustManager(caCertFile) .clientAuth(ClientAuth.REQUIRE) .build(); - initServerWithStaticAuthz(policy, serverCredentials); + initServerWithAuthzInterceptor(interceptor, serverCredentials); ChannelCredentials channelCredentials = TlsChannelCredentials.newBuilder() .keyManager(clientCert0File, clientKey0File) .trustManager(caCertFile) @@ -352,18 +398,418 @@ public void staticAuthzAllowsRpcWithPrincipalsFieldOnTlsAuthenticatedConnectionT + " }" + " ]" + "}"; + AuthorizationServerInterceptor interceptor = createStaticAuthorizationInterceptor(policy); ServerCredentials serverCredentials = TlsServerCredentials.newBuilder() .keyManager(serverCert0File, serverKey0File) .trustManager(caCertFile) .clientAuth(ClientAuth.OPTIONAL) .build(); - initServerWithStaticAuthz(policy, serverCredentials); + initServerWithAuthzInterceptor(interceptor, serverCredentials); ChannelCredentials channelCredentials = TlsChannelCredentials.newBuilder() .trustManager(caCertFile) .build(); getStub(channelCredentials).unaryRpc(SimpleRequest.getDefaultInstance()); } + @Test + public void fileWatcherAuthzAllowsRpcNoMatchInDenyMatchInAllowTest() throws Exception { + String policy = "{" + + " \"name\" : \"authz\" ," + + " \"deny_rules\": [" + + " {" + + " \"name\": \"deny_UnaryRpc\"," + + " \"request\": {" + + " \"paths\": [" + + " \"*/UnaryRpc\"" + + " ]," + + " \"headers\": [" + + " {" + + " \"key\": \"dev-path\"," + + " \"values\": [\"/dev/path/*\"]" + + " }" + + " ]" + + " }" + + " }" + + " ]," + + " \"allow_rules\": [" + + " {" + + " \"name\": \"allow_all\"" + + " }" + + " ]" + + "}"; + File policyFile = createTempAuthorizationPolicy(policy); + FileWatcherAuthorizationServerInterceptor interceptor = + createFileWatcherAuthorizationInterceptor(policyFile); + Closeable closeable = interceptor.scheduleRefreshes(100, TimeUnit.MILLISECONDS); + initServerWithAuthzInterceptor(interceptor, InsecureServerCredentials.create()); + closeable.close(); + policyFile.deleteOnExit(); + getStub().unaryRpc(SimpleRequest.getDefaultInstance()); + } + + @Test + public void fileWatcherAuthzDeniesRpcNoMatchInDenyAndAllowTest() throws Exception { + String policy = "{" + + " \"name\" : \"authz\" ," + + " \"deny_rules\": [" + + " {" + + " \"name\": \"deny_foo\"," + + " \"source\": {" + + " \"principals\": [" + + " \"foo\"" + + " ]" + + " }" + + " }" + + " ]," + + " \"allow_rules\": [" + + " {" + + " \"name\": \"allow_ClientStreamingRpc\"," + + " \"request\": {" + + " \"paths\": [" + + " \"*/ClientStreamingRpc\"" + + " ]" + + " }" + + " }" + + " ]" + + "}"; + File policyFile = createTempAuthorizationPolicy(policy); + FileWatcherAuthorizationServerInterceptor interceptor = + createFileWatcherAuthorizationInterceptor(policyFile); + Closeable closeable = interceptor.scheduleRefreshes(100, TimeUnit.MILLISECONDS); + initServerWithAuthzInterceptor(interceptor, InsecureServerCredentials.create()); + closeable.close(); + policyFile.deleteOnExit(); + try { + getStub().unaryRpc(SimpleRequest.getDefaultInstance()); + fail("exception expected"); + } catch (StatusRuntimeException sre) { + assertThat(sre).hasMessageThat().isEqualTo( + "PERMISSION_DENIED: Access Denied"); + } catch (Exception e) { + throw new AssertionError("the test failed ", e); + } + } + + @Test + public void fileWatcherAuthzDeniesRpcMatchInDenyAndAllowTest() throws Exception { + String policy = "{" + + " \"name\" : \"authz\" ," + + " \"deny_rules\": [" + + " {" + + " \"name\": \"deny_UnaryRpc\"," + + " \"request\": {" + + " \"paths\": [" + + " \"*/UnaryRpc\"" + + " ]" + + " }" + + " }" + + " ]," + + " \"allow_rules\": [" + + " {" + + " \"name\": \"allow_UnaryRpc\"," + + " \"request\": {" + + " \"paths\": [" + + " \"*/UnaryRpc\"" + + " ]" + + " }" + + " }" + + " ]" + + "}"; + File policyFile = createTempAuthorizationPolicy(policy); + FileWatcherAuthorizationServerInterceptor interceptor = + createFileWatcherAuthorizationInterceptor(policyFile); + Closeable closeable = interceptor.scheduleRefreshes(100, TimeUnit.MILLISECONDS); + initServerWithAuthzInterceptor(interceptor, InsecureServerCredentials.create()); + closeable.close(); + policyFile.deleteOnExit(); + try { + getStub().unaryRpc(SimpleRequest.getDefaultInstance()); + fail("exception expected"); + } catch (StatusRuntimeException sre) { + assertThat(sre).hasMessageThat().isEqualTo( + "PERMISSION_DENIED: Access Denied"); + } catch (Exception e) { + throw new AssertionError("the test failed ", e); + } + } + + @Test + public void fileWatcherAuthzDeniesRpcMatchInDenyNoMatchInAllowTest() throws Exception { + String policy = "{" + + " \"name\" : \"authz\" ," + + " \"deny_rules\": [" + + " {" + + " \"name\": \"deny_UnaryRpc\"," + + " \"request\": {" + + " \"paths\": [" + + " \"*/UnaryRpc\"" + + " ]" + + " }" + + " }" + + " ]," + + " \"allow_rules\": [" + + " {" + + " \"name\": \"allow_ClientStreamingRpc\"," + + " \"request\": {" + + " \"paths\": [" + + " \"*/ClientStreamingRpc\"" + + " ]" + + " }" + + " }" + + " ]" + + "}"; + File policyFile = createTempAuthorizationPolicy(policy); + FileWatcherAuthorizationServerInterceptor interceptor = + createFileWatcherAuthorizationInterceptor(policyFile); + Closeable closeable = interceptor.scheduleRefreshes(100, TimeUnit.MILLISECONDS); + initServerWithAuthzInterceptor(interceptor, InsecureServerCredentials.create()); + closeable.close(); + policyFile.deleteOnExit(); + try { + getStub().unaryRpc(SimpleRequest.getDefaultInstance()); + fail("exception expected"); + } catch (StatusRuntimeException sre) { + assertThat(sre).hasMessageThat().isEqualTo( + "PERMISSION_DENIED: Access Denied"); + } catch (Exception e) { + throw new AssertionError("the test failed ", e); + } + } + + @Test + public void fileWatcherAuthzAllowsRpcEmptyDenyMatchInAllowTest() throws Exception { + String policy = "{" + + " \"name\" : \"authz\" ," + + " \"allow_rules\": [" + + " {" + + " \"name\": \"allow_UnaryRpc\"," + + " \"request\": {" + + " \"paths\": [" + + " \"*/UnaryRpc\"" + + " ]" + + " }" + + " }" + + " ]" + + "}"; + File policyFile = createTempAuthorizationPolicy(policy); + FileWatcherAuthorizationServerInterceptor interceptor = + createFileWatcherAuthorizationInterceptor(policyFile); + Closeable closeable = interceptor.scheduleRefreshes(100, TimeUnit.MILLISECONDS); + initServerWithAuthzInterceptor(interceptor, InsecureServerCredentials.create()); + closeable.close(); + policyFile.deleteOnExit(); + getStub().unaryRpc(SimpleRequest.getDefaultInstance()); + } + + @Test + public void fileWatcherAuthzDeniesRpcEmptyDenyNoMatchInAllowTest() throws Exception { + String policy = "{" + + " \"name\" : \"authz\" ," + + " \"allow_rules\": [" + + " {" + + " \"name\": \"allow_ClientStreamingRpc\"," + + " \"request\": {" + + " \"paths\": [" + + " \"*/ClientStreamingRpc\"" + + " ]" + + " }" + + " }" + + " ]" + + "}"; + File policyFile = createTempAuthorizationPolicy(policy); + FileWatcherAuthorizationServerInterceptor interceptor = + createFileWatcherAuthorizationInterceptor(policyFile); + Closeable closeable = interceptor.scheduleRefreshes(100, TimeUnit.MILLISECONDS); + initServerWithAuthzInterceptor(interceptor, InsecureServerCredentials.create()); + closeable.close(); + policyFile.deleteOnExit(); + try { + getStub().unaryRpc(SimpleRequest.getDefaultInstance()); + fail("exception expected"); + } catch (StatusRuntimeException sre) { + assertThat(sre).hasMessageThat().isEqualTo( + "PERMISSION_DENIED: Access Denied"); + } catch (Exception e) { + throw new AssertionError("the test failed ", e); + } + } + + @Test + public void fileWatcherAuthzValidPolicyRefreshTest() throws Exception { + String policy = "{" + + " \"name\" : \"authz\" ," + + " \"deny_rules\": [" + + " {" + + " \"name\": \"deny_UnaryRpc\"," + + " \"request\": {" + + " \"paths\": [" + + " \"*/UnaryRpc\"" + + " ]" + + " }" + + " }" + + " ]," + + " \"allow_rules\": [" + + " {" + + " \"name\": \"allow_all\"" + + " }" + + " ]" + + "}"; + File policyFile = createTempAuthorizationPolicy(policy); + FileWatcherAuthorizationServerInterceptor interceptor = + createFileWatcherAuthorizationInterceptor(policyFile); + Closeable closeable = interceptor.scheduleRefreshes(100, TimeUnit.MILLISECONDS); + initServerWithAuthzInterceptor(interceptor, InsecureServerCredentials.create()); + closeable.close(); + policyFile.deleteOnExit(); + try { + getStub().unaryRpc(SimpleRequest.getDefaultInstance()); + fail("exception expected"); + } catch (StatusRuntimeException sre) { + assertThat(sre).hasMessageThat().isEqualTo( + "PERMISSION_DENIED: Access Denied"); + } catch (Exception e) { + throw new AssertionError("the test failed ", e); + } + policy = "{" + + " \"name\" : \"authz\" ," + + " \"allow_rules\": [" + + " {" + + " \"name\": \"allow_UnaryRpc\"," + + " \"request\": {" + + " \"paths\": [" + + " \"*/UnaryRpc\"" + + " ]" + + " }" + + " }" + + " ]" + + "}"; + rewriteAuthorizationPolicy(policyFile, policy); + Runnable callback = () -> { + getStub().unaryRpc(SimpleRequest.getDefaultInstance()); + }; + Thread onReloadDone = new Thread(callback); + interceptor.setCallbackForTesting(onReloadDone); + onReloadDone.join(); + } + + @Test + public void fileWatcherAuthzInvalidPolicySkipRefreshTest() throws Exception { + String policy = "{" + + " \"name\" : \"authz\" ," + + " \"allow_rules\": [" + + " {" + + " \"name\": \"allow_ClientStreamingRpc\"," + + " \"request\": {" + + " \"paths\": [" + + " \"*/ClientStreamingRpc\"" + + " ]" + + " }" + + " }" + + " ]" + + "}"; + File policyFile = createTempAuthorizationPolicy(policy); + FileWatcherAuthorizationServerInterceptor interceptor = + createFileWatcherAuthorizationInterceptor(policyFile); + Closeable closeable = interceptor.scheduleRefreshes(100, TimeUnit.MILLISECONDS); + initServerWithAuthzInterceptor(interceptor, InsecureServerCredentials.create()); + closeable.close(); + policyFile.deleteOnExit(); + try { + getStub().unaryRpc(SimpleRequest.getDefaultInstance()); + fail("exception expected"); + } catch (StatusRuntimeException sre) { + assertThat(sre).hasMessageThat().isEqualTo( + "PERMISSION_DENIED: Access Denied"); + } catch (Exception e) { + throw new AssertionError("the test failed ", e); + } + policy = "{}"; + rewriteAuthorizationPolicy(policyFile, policy); + Runnable callback = () -> { + try { + getStub().unaryRpc(SimpleRequest.getDefaultInstance()); + fail("exception expected"); + } catch (StatusRuntimeException sre) { + assertThat(sre).hasMessageThat().isEqualTo( + "PERMISSION_DENIED: Access Denied"); + } catch (Exception e) { + throw new AssertionError("the test failed ", e); + } + }; + Thread onReloadDone = new Thread(callback); + interceptor.setCallbackForTesting(onReloadDone); + onReloadDone.join(); + } + + @Test + public void fileWatcherAuthzRecoversFromReloadTest() throws Exception { + String policy = "{" + + " \"name\" : \"authz\" ," + + " \"allow_rules\": [" + + " {" + + " \"name\": \"allow_ClientStreamingRpc\"," + + " \"request\": {" + + " \"paths\": [" + + " \"*/ClientStreamingRpc\"" + + " ]" + + " }" + + " }" + + " ]" + + "}"; + File policyFile = createTempAuthorizationPolicy(policy); + FileWatcherAuthorizationServerInterceptor interceptor = + createFileWatcherAuthorizationInterceptor(policyFile); + Closeable closeable = interceptor.scheduleRefreshes(100, TimeUnit.MILLISECONDS); + initServerWithAuthzInterceptor(interceptor, InsecureServerCredentials.create()); + closeable.close(); + policyFile.deleteOnExit(); + try { + getStub().unaryRpc(SimpleRequest.getDefaultInstance()); + fail("exception expected"); + } catch (StatusRuntimeException sre) { + assertThat(sre).hasMessageThat().isEqualTo( + "PERMISSION_DENIED: Access Denied"); + } catch (Exception e) { + throw new AssertionError("the test failed ", e); + } + policy = "{}"; + rewriteAuthorizationPolicy(policyFile, policy); + Runnable callback = () -> { + try { + getStub().unaryRpc(SimpleRequest.getDefaultInstance()); + fail("exception expected"); + } catch (StatusRuntimeException sre) { + assertThat(sre).hasMessageThat().isEqualTo( + "PERMISSION_DENIED: Access Denied"); + } catch (Exception e) { + throw new AssertionError("the test failed ", e); + } + }; + Thread onFirstReloadDone = new Thread(callback); + interceptor.setCallbackForTesting(onFirstReloadDone); + onFirstReloadDone.join(); + policy = "{" + + " \"name\" : \"authz\" ," + + " \"allow_rules\": [" + + " {" + + " \"name\": \"allow_UnaryRpc\"," + + " \"request\": {" + + " \"paths\": [" + + " \"*/UnaryRpc\"" + + " ]" + + " }" + + " }" + + " ]" + + "}"; + rewriteAuthorizationPolicy(policyFile, policy); + callback = () -> { + getStub().unaryRpc(SimpleRequest.getDefaultInstance()); + }; + Thread onSecondReloadDone = new Thread(callback); + interceptor.setCallbackForTesting(onSecondReloadDone); + onSecondReloadDone.join(); + } + private static class SimpleServiceImpl extends SimpleServiceGrpc.SimpleServiceImplBase { @Override public void unaryRpc(SimpleRequest req, StreamObserver respOb) { diff --git a/authz/src/test/java/io/grpc/authz/FileWatcherAuthorizationServerInterceptorTest.java b/authz/src/test/java/io/grpc/authz/FileWatcherAuthorizationServerInterceptorTest.java new file mode 100644 index 00000000000..f2e3efb9397 --- /dev/null +++ b/authz/src/test/java/io/grpc/authz/FileWatcherAuthorizationServerInterceptorTest.java @@ -0,0 +1,116 @@ +/* + * Copyright 2022 The gRPC 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 + * + * http://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.grpc.authz; + +import static com.google.common.truth.Truth.assertThat; +import static java.nio.charset.StandardCharsets.UTF_8; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.fail; + +import java.io.File; +import java.io.FileOutputStream; +import java.io.IOException; +import java.util.concurrent.TimeUnit; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +@RunWith(JUnit4.class) +public class FileWatcherAuthorizationServerInterceptorTest { + @Test + public void invalidPolicyFailsAuthzInterceptorCreation() throws Exception { + File policyFile = File.createTempFile("temp", "json"); + policyFile.deleteOnExit(); + try (FileOutputStream outputStream = new FileOutputStream(policyFile)) { + String policy = "{ \"name\": \"abc\",, }"; + outputStream.write(policy.getBytes(UTF_8)); + outputStream.close(); + } + try { + FileWatcherAuthorizationServerInterceptor.create(policyFile); + fail("exception expected"); + } catch (IOException ioe) { + assertThat(ioe).hasMessageThat().isEqualTo( + "Use JsonReader.setLenient(true) to accept malformed JSON" + + " at line 1 column 18 path $.name"); + } catch (Exception e) { + throw new AssertionError("the test failed ", e); + } + } + + @Test + public void validPolicyCreatesFileWatcherAuthzInterceptor() throws Exception { + File policyFile = File.createTempFile("temp", "json"); + policyFile.deleteOnExit(); + try (FileOutputStream outputStream = new FileOutputStream(policyFile)) { + String policy = "{" + + " \"name\" : \"authz\"," + + " \"deny_rules\": [" + + " {" + + " \"name\": \"deny_foo\"," + + " \"source\": {" + + " \"principals\": [" + + " \"spiffe://foo.com\"" + + " ]" + + " }" + + " }" + + " ]," + + " \"allow_rules\": [" + + " {" + + " \"name\": \"allow_all\"" + + " }" + + " ]" + + "}"; + outputStream.write(policy.getBytes(UTF_8)); + outputStream.close(); + } + FileWatcherAuthorizationServerInterceptor interceptor = + FileWatcherAuthorizationServerInterceptor.create(policyFile); + assertNotNull(interceptor); + interceptor.scheduleRefreshes(1, TimeUnit.SECONDS); + } + + @Test + public void invalidRefreshIntervalFailsScheduleRefreshes() throws Exception { + File policyFile = File.createTempFile("temp", "json"); + policyFile.deleteOnExit(); + try (FileOutputStream outputStream = new FileOutputStream(policyFile)) { + String policy = "{" + + " \"name\" : \"authz\"," + + " \"allow_rules\": [" + + " {" + + " \"name\": \"allow_all\"" + + " }" + + " ]" + + "}"; + outputStream.write(policy.getBytes(UTF_8)); + outputStream.close(); + } + FileWatcherAuthorizationServerInterceptor interceptor = + FileWatcherAuthorizationServerInterceptor.create(policyFile); + assertNotNull(interceptor); + try { + interceptor.scheduleRefreshes(0, TimeUnit.SECONDS); + fail("exception expected"); + } catch (IllegalArgumentException iae) { + assertThat(iae).hasMessageThat().isEqualTo( + "Refresh interval must be greater than 0"); + } catch (Exception e) { + throw new AssertionError("the test failed ", e); + } + } +} From 3f4d2c6203a9de260538ca9615c540f1cc4fbd26 Mon Sep 17 00:00:00 2001 From: Ashitha Santhosh Date: Mon, 27 Feb 2023 00:40:11 -0800 Subject: [PATCH 2/3] replace callback with fakeclock --- authz/build.gradle | 3 +- ...WatcherAuthorizationServerInterceptor.java | 51 ++-- .../grpc/authz/AuthorizationEnd2EndTest.java | 223 ++++++++---------- .../AuthorizationPolicyTranslatorTest.java | 24 -- .../AuthorizationServerInterceptorTest.java | 2 - ...herAuthorizationServerInterceptorTest.java | 85 +++---- 6 files changed, 155 insertions(+), 233 deletions(-) diff --git a/authz/build.gradle b/authz/build.gradle index 50084752d0e..f2fd149be3c 100644 --- a/authz/build.gradle +++ b/authz/build.gradle @@ -17,7 +17,8 @@ dependencies { compileOnly libraries.javax.annotation testImplementation project(':grpc-testing'), - project(':grpc-testing-proto') + project(':grpc-testing-proto'), + project(':grpc-core').sourceSets.test.output // for FakeClock testImplementation (libraries.guava.testlib) { exclude group: 'junit', module: 'junit' } diff --git a/authz/src/main/java/io/grpc/authz/FileWatcherAuthorizationServerInterceptor.java b/authz/src/main/java/io/grpc/authz/FileWatcherAuthorizationServerInterceptor.java index 6cc8bf9b033..50e868b062a 100644 --- a/authz/src/main/java/io/grpc/authz/FileWatcherAuthorizationServerInterceptor.java +++ b/authz/src/main/java/io/grpc/authz/FileWatcherAuthorizationServerInterceptor.java @@ -19,8 +19,6 @@ import static com.google.common.base.Preconditions.checkNotNull; import static java.nio.charset.StandardCharsets.UTF_8; -import com.google.common.annotations.VisibleForTesting; -import com.google.common.util.concurrent.ThreadFactoryBuilder; import io.grpc.Metadata; import io.grpc.ServerCall; import io.grpc.ServerCallHandler; @@ -29,8 +27,6 @@ import java.io.File; import java.io.IOException; import java.nio.file.Files; -import java.nio.file.attribute.FileTime; -import java.util.concurrent.Executors; import java.util.concurrent.ScheduledExecutorService; import java.util.concurrent.ScheduledFuture; import java.util.concurrent.TimeUnit; @@ -47,20 +43,11 @@ public final class FileWatcherAuthorizationServerInterceptor implements ServerIn Logger.getLogger(FileWatcherAuthorizationServerInterceptor.class.getName()); private volatile AuthorizationServerInterceptor internalAuthzServerInterceptor; - private final ScheduledExecutorService scheduledExecutorService = - Executors.newSingleThreadScheduledExecutor( - new ThreadFactoryBuilder() - .setNameFormat("filewatcher" + "-%d") - .setDaemon(true) - .build()); private final File policyFile; - private FileTime lastModifiedTime; + private String policyContents; - private Thread testCallback; - - private FileWatcherAuthorizationServerInterceptor(File policyFile) - throws IllegalArgumentException, IOException { + private FileWatcherAuthorizationServerInterceptor(File policyFile) throws IOException { this.policyFile = policyFile; updateInternalInterceptor(); } @@ -73,16 +60,12 @@ public ServerCall.Listener interceptCall( } void updateInternalInterceptor() throws IOException { - FileTime currentTime = Files.getLastModifiedTime(policyFile.toPath()); - if (currentTime.equals(lastModifiedTime)) { + String currentPolicyContents = new String(Files.readAllBytes(policyFile.toPath()), UTF_8); + if (currentPolicyContents.equals(policyContents)) { return; } - String policyContents = new String(Files.readAllBytes(policyFile.toPath()), UTF_8); - lastModifiedTime = currentTime; + policyContents = currentPolicyContents; internalAuthzServerInterceptor = AuthorizationServerInterceptor.create(policyContents); - if (testCallback != null) { - testCallback.start(); - } } /** @@ -93,22 +76,25 @@ void updateInternalInterceptor() throws IOException { * * @param period the period between successive file load executions. * @param unit the time unit for period parameter + * @param executor the execute service we use to read and update authorization policy * @return an object that caller should close when the file refreshes are not needed */ - public Closeable scheduleRefreshes(long period, TimeUnit unit) - throws IOException { + public Closeable scheduleRefreshes( + long period, TimeUnit unit, ScheduledExecutorService executor) throws IOException { + checkNotNull(executor, "scheduledExecutorService"); if (period <= 0) { throw new IllegalArgumentException("Refresh interval must be greater than 0"); } final ScheduledFuture future = - scheduledExecutorService.scheduleWithFixedDelay(new Runnable() { - @Override public void run() { + executor.scheduleWithFixedDelay(new Runnable() { + @Override + public void run() { try { - updateInternalInterceptor(); + updateInternalInterceptor(); } catch (Exception e) { - logger.log(Level.WARNING, "Authorization Policy file reload failed: " + e); - } + logger.log(Level.WARNING, "Authorization Policy file reload failed: ", e); } + } }, period, period, unit); return new Closeable() { @Override public void close() { @@ -117,13 +103,8 @@ public Closeable scheduleRefreshes(long period, TimeUnit unit) }; } - @VisibleForTesting - public void setCallbackForTesting(Thread callback) { - testCallback = callback; - } - public static FileWatcherAuthorizationServerInterceptor create(File policyFile) - throws IllegalArgumentException, IOException { + throws IOException { checkNotNull(policyFile, "policyFile"); return new FileWatcherAuthorizationServerInterceptor(policyFile); } diff --git a/authz/src/test/java/io/grpc/authz/AuthorizationEnd2EndTest.java b/authz/src/test/java/io/grpc/authz/AuthorizationEnd2EndTest.java index cb76aa8fe01..949c41c56c3 100644 --- a/authz/src/test/java/io/grpc/authz/AuthorizationEnd2EndTest.java +++ b/authz/src/test/java/io/grpc/authz/AuthorizationEnd2EndTest.java @@ -18,6 +18,7 @@ import static com.google.common.truth.Truth.assertThat; import static java.nio.charset.StandardCharsets.UTF_8; +import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.fail; @@ -33,6 +34,7 @@ import io.grpc.TlsChannelCredentials; import io.grpc.TlsServerCredentials; import io.grpc.TlsServerCredentials.ClientAuth; +import io.grpc.internal.FakeClock; import io.grpc.internal.testing.TestUtils; import io.grpc.stub.StreamObserver; import io.grpc.testing.protobuf.SimpleRequest; @@ -40,7 +42,8 @@ import io.grpc.testing.protobuf.SimpleServiceGrpc; import java.io.Closeable; import java.io.File; -import java.io.FileOutputStream; +import java.nio.file.Files; +import java.nio.file.Paths; import java.util.concurrent.TimeUnit; import org.junit.After; import org.junit.Test; @@ -58,6 +61,9 @@ public class AuthorizationEnd2EndTest { private Server server; private ManagedChannel channel; + private FakeClock fakeClock = new FakeClock(); + private File policyFile; + private AuthorizationServerInterceptor createStaticAuthorizationInterceptor( String authorizationPolicy) throws Exception { AuthorizationServerInterceptor interceptor = @@ -83,21 +89,14 @@ private void initServerWithAuthzInterceptor( .start(); } - private File createTempAuthorizationPolicy(String authorizationPolicy) throws Exception { - File policyFile = File.createTempFile("temp", "json"); - try (FileOutputStream outputStream = new FileOutputStream(policyFile, false)) { - outputStream.write(authorizationPolicy.getBytes(UTF_8)); - outputStream.close(); - } - return policyFile; + private void createTempAuthorizationPolicy(String authorizationPolicy) throws Exception { + policyFile = File.createTempFile("temp", "json"); + Files.write(Paths.get(policyFile.getAbsolutePath()), authorizationPolicy.getBytes(UTF_8)); } - private void rewriteAuthorizationPolicy( - File policyFile, String newPolicy) throws Exception { - try (FileOutputStream outputStream = new FileOutputStream(policyFile, false)) { - outputStream.write(newPolicy.getBytes(UTF_8)); - outputStream.close(); - } + private void rewriteAuthorizationPolicy(String newPolicy) throws Exception { + assertNotNull(policyFile); + Files.write(Paths.get(policyFile.getAbsolutePath()), newPolicy.getBytes(UTF_8)); } private SimpleServiceGrpc.SimpleServiceBlockingStub getStub() { @@ -120,6 +119,9 @@ private SimpleServiceGrpc.SimpleServiceBlockingStub getStub( @After public void tearDown() { + if (policyFile != null) { + policyFile.delete(); + } if (server != null) { server.shutdown(); } @@ -192,8 +194,6 @@ public void staticAuthzDeniesRpcNoMatchInDenyAndAllowTest() throws Exception { } catch (StatusRuntimeException sre) { assertThat(sre).hasMessageThat().isEqualTo( "PERMISSION_DENIED: Access Denied"); - } catch (Exception e) { - throw new AssertionError("the test failed ", e); } } @@ -230,8 +230,6 @@ public void staticAuthzDeniesRpcMatchInDenyAndAllowTest() throws Exception { } catch (StatusRuntimeException sre) { assertThat(sre).hasMessageThat().isEqualTo( "PERMISSION_DENIED: Access Denied"); - } catch (Exception e) { - throw new AssertionError("the test failed ", e); } } @@ -268,8 +266,6 @@ public void staticAuthzDeniesRpcMatchInDenyNoMatchInAllowTest() throws Exception } catch (StatusRuntimeException sre) { assertThat(sre).hasMessageThat().isEqualTo( "PERMISSION_DENIED: Access Denied"); - } catch (Exception e) { - throw new AssertionError("the test failed ", e); } } @@ -316,8 +312,6 @@ public void staticAuthzDeniesRpcEmptyDenyNoMatchInAllowTest() throws Exception { } catch (StatusRuntimeException sre) { assertThat(sre).hasMessageThat().isEqualTo( "PERMISSION_DENIED: Access Denied"); - } catch (Exception e) { - throw new AssertionError("the test failed ", e); } } @@ -343,8 +337,6 @@ public void staticAuthzDeniesRpcWithPrincipalsFieldOnUnauthenticatedConnectionTe } catch (StatusRuntimeException sre) { assertThat(sre).hasMessageThat().isEqualTo( "PERMISSION_DENIED: Access Denied"); - } catch (Exception e) { - throw new AssertionError("the test failed ", e); } } @@ -437,13 +429,13 @@ public void fileWatcherAuthzAllowsRpcNoMatchInDenyMatchInAllowTest() throws Exce + " }" + " ]" + "}"; - File policyFile = createTempAuthorizationPolicy(policy); + createTempAuthorizationPolicy(policy); FileWatcherAuthorizationServerInterceptor interceptor = createFileWatcherAuthorizationInterceptor(policyFile); - Closeable closeable = interceptor.scheduleRefreshes(100, TimeUnit.MILLISECONDS); + Closeable closeable = interceptor.scheduleRefreshes( + 100, TimeUnit.MILLISECONDS, fakeClock.getScheduledExecutorService()); initServerWithAuthzInterceptor(interceptor, InsecureServerCredentials.create()); closeable.close(); - policyFile.deleteOnExit(); getStub().unaryRpc(SimpleRequest.getDefaultInstance()); } @@ -472,21 +464,19 @@ public void fileWatcherAuthzDeniesRpcNoMatchInDenyAndAllowTest() throws Exceptio + " }" + " ]" + "}"; - File policyFile = createTempAuthorizationPolicy(policy); + createTempAuthorizationPolicy(policy); FileWatcherAuthorizationServerInterceptor interceptor = createFileWatcherAuthorizationInterceptor(policyFile); - Closeable closeable = interceptor.scheduleRefreshes(100, TimeUnit.MILLISECONDS); + Closeable closeable = interceptor.scheduleRefreshes( + 100, TimeUnit.MILLISECONDS, fakeClock.getScheduledExecutorService()); initServerWithAuthzInterceptor(interceptor, InsecureServerCredentials.create()); closeable.close(); - policyFile.deleteOnExit(); try { getStub().unaryRpc(SimpleRequest.getDefaultInstance()); fail("exception expected"); } catch (StatusRuntimeException sre) { assertThat(sre).hasMessageThat().isEqualTo( "PERMISSION_DENIED: Access Denied"); - } catch (Exception e) { - throw new AssertionError("the test failed ", e); } } @@ -515,21 +505,19 @@ public void fileWatcherAuthzDeniesRpcMatchInDenyAndAllowTest() throws Exception + " }" + " ]" + "}"; - File policyFile = createTempAuthorizationPolicy(policy); + createTempAuthorizationPolicy(policy); FileWatcherAuthorizationServerInterceptor interceptor = createFileWatcherAuthorizationInterceptor(policyFile); - Closeable closeable = interceptor.scheduleRefreshes(100, TimeUnit.MILLISECONDS); + Closeable closeable = interceptor.scheduleRefreshes( + 100, TimeUnit.MILLISECONDS, fakeClock.getScheduledExecutorService()); initServerWithAuthzInterceptor(interceptor, InsecureServerCredentials.create()); closeable.close(); - policyFile.deleteOnExit(); try { getStub().unaryRpc(SimpleRequest.getDefaultInstance()); fail("exception expected"); } catch (StatusRuntimeException sre) { assertThat(sre).hasMessageThat().isEqualTo( "PERMISSION_DENIED: Access Denied"); - } catch (Exception e) { - throw new AssertionError("the test failed ", e); } } @@ -558,21 +546,19 @@ public void fileWatcherAuthzDeniesRpcMatchInDenyNoMatchInAllowTest() throws Exce + " }" + " ]" + "}"; - File policyFile = createTempAuthorizationPolicy(policy); + createTempAuthorizationPolicy(policy); FileWatcherAuthorizationServerInterceptor interceptor = createFileWatcherAuthorizationInterceptor(policyFile); - Closeable closeable = interceptor.scheduleRefreshes(100, TimeUnit.MILLISECONDS); + Closeable closeable = interceptor.scheduleRefreshes( + 100, TimeUnit.MILLISECONDS, fakeClock.getScheduledExecutorService()); initServerWithAuthzInterceptor(interceptor, InsecureServerCredentials.create()); closeable.close(); - policyFile.deleteOnExit(); try { getStub().unaryRpc(SimpleRequest.getDefaultInstance()); fail("exception expected"); } catch (StatusRuntimeException sre) { assertThat(sre).hasMessageThat().isEqualTo( "PERMISSION_DENIED: Access Denied"); - } catch (Exception e) { - throw new AssertionError("the test failed ", e); } } @@ -591,13 +577,13 @@ public void fileWatcherAuthzAllowsRpcEmptyDenyMatchInAllowTest() throws Exceptio + " }" + " ]" + "}"; - File policyFile = createTempAuthorizationPolicy(policy); + createTempAuthorizationPolicy(policy); FileWatcherAuthorizationServerInterceptor interceptor = createFileWatcherAuthorizationInterceptor(policyFile); - Closeable closeable = interceptor.scheduleRefreshes(100, TimeUnit.MILLISECONDS); + Closeable closeable = interceptor.scheduleRefreshes( + 100, TimeUnit.MILLISECONDS, fakeClock.getScheduledExecutorService()); initServerWithAuthzInterceptor(interceptor, InsecureServerCredentials.create()); closeable.close(); - policyFile.deleteOnExit(); getStub().unaryRpc(SimpleRequest.getDefaultInstance()); } @@ -616,27 +602,25 @@ public void fileWatcherAuthzDeniesRpcEmptyDenyNoMatchInAllowTest() throws Except + " }" + " ]" + "}"; - File policyFile = createTempAuthorizationPolicy(policy); + createTempAuthorizationPolicy(policy); FileWatcherAuthorizationServerInterceptor interceptor = createFileWatcherAuthorizationInterceptor(policyFile); - Closeable closeable = interceptor.scheduleRefreshes(100, TimeUnit.MILLISECONDS); + Closeable closeable = interceptor.scheduleRefreshes( + 100, TimeUnit.MILLISECONDS, fakeClock.getScheduledExecutorService()); initServerWithAuthzInterceptor(interceptor, InsecureServerCredentials.create()); closeable.close(); - policyFile.deleteOnExit(); try { getStub().unaryRpc(SimpleRequest.getDefaultInstance()); fail("exception expected"); } catch (StatusRuntimeException sre) { assertThat(sre).hasMessageThat().isEqualTo( "PERMISSION_DENIED: Access Denied"); - } catch (Exception e) { - throw new AssertionError("the test failed ", e); } } @Test public void fileWatcherAuthzValidPolicyRefreshTest() throws Exception { - String policy = "{" + String policy1 = "{" + " \"name\" : \"authz\" ," + " \"deny_rules\": [" + " {" @@ -654,23 +638,13 @@ public void fileWatcherAuthzValidPolicyRefreshTest() throws Exception { + " }" + " ]" + "}"; - File policyFile = createTempAuthorizationPolicy(policy); + createTempAuthorizationPolicy(policy1); FileWatcherAuthorizationServerInterceptor interceptor = createFileWatcherAuthorizationInterceptor(policyFile); - Closeable closeable = interceptor.scheduleRefreshes(100, TimeUnit.MILLISECONDS); + Closeable closeable = interceptor.scheduleRefreshes( + 100, TimeUnit.NANOSECONDS, fakeClock.getScheduledExecutorService()); initServerWithAuthzInterceptor(interceptor, InsecureServerCredentials.create()); - closeable.close(); - policyFile.deleteOnExit(); - try { - getStub().unaryRpc(SimpleRequest.getDefaultInstance()); - fail("exception expected"); - } catch (StatusRuntimeException sre) { - assertThat(sre).hasMessageThat().isEqualTo( - "PERMISSION_DENIED: Access Denied"); - } catch (Exception e) { - throw new AssertionError("the test failed ", e); - } - policy = "{" + String policy2 = "{" + " \"name\" : \"authz\" ," + " \"allow_rules\": [" + " {" @@ -683,18 +657,25 @@ public void fileWatcherAuthzValidPolicyRefreshTest() throws Exception { + " }" + " ]" + "}"; - rewriteAuthorizationPolicy(policyFile, policy); - Runnable callback = () -> { + rewriteAuthorizationPolicy(policy2); + // Reload is yet to take place at 100ns. policy1 will be active here. + assertEquals(0, fakeClock.forwardNanos(99)); + try { getStub().unaryRpc(SimpleRequest.getDefaultInstance()); - }; - Thread onReloadDone = new Thread(callback); - interceptor.setCallbackForTesting(onReloadDone); - onReloadDone.join(); + fail("exception expected"); + } catch (StatusRuntimeException sre) { + assertThat(sre).hasMessageThat().isEqualTo( + "PERMISSION_DENIED: Access Denied"); + } + // Reload will take place making policy2 the active policy. + assertEquals(1, fakeClock.forwardNanos(2)); + closeable.close(); + getStub().unaryRpc(SimpleRequest.getDefaultInstance()); } @Test public void fileWatcherAuthzInvalidPolicySkipRefreshTest() throws Exception { - String policy = "{" + String validPolicy = "{" + " \"name\" : \"authz\" ," + " \"allow_rules\": [" + " {" @@ -707,43 +688,39 @@ public void fileWatcherAuthzInvalidPolicySkipRefreshTest() throws Exception { + " }" + " ]" + "}"; - File policyFile = createTempAuthorizationPolicy(policy); + createTempAuthorizationPolicy(validPolicy); FileWatcherAuthorizationServerInterceptor interceptor = createFileWatcherAuthorizationInterceptor(policyFile); - Closeable closeable = interceptor.scheduleRefreshes(100, TimeUnit.MILLISECONDS); + Closeable closeable = interceptor.scheduleRefreshes( + 100, TimeUnit.NANOSECONDS, fakeClock.getScheduledExecutorService()); initServerWithAuthzInterceptor(interceptor, InsecureServerCredentials.create()); + String invalidPolicy = "{}"; + rewriteAuthorizationPolicy(invalidPolicy); + // Reload is yet to take place at 100ns. validPolicy will be active here. + assertEquals(0, fakeClock.forwardNanos(99)); + try { + getStub().unaryRpc(SimpleRequest.getDefaultInstance()); + fail("exception expected"); + } catch (StatusRuntimeException sre) { + assertThat(sre).hasMessageThat().isEqualTo( + "PERMISSION_DENIED: Access Denied"); + } + // Reload will take place which skips the invalidPolicy. validPolicy remains + // the active policy. + assertEquals(1, fakeClock.forwardNanos(2)); closeable.close(); - policyFile.deleteOnExit(); try { getStub().unaryRpc(SimpleRequest.getDefaultInstance()); fail("exception expected"); } catch (StatusRuntimeException sre) { assertThat(sre).hasMessageThat().isEqualTo( "PERMISSION_DENIED: Access Denied"); - } catch (Exception e) { - throw new AssertionError("the test failed ", e); } - policy = "{}"; - rewriteAuthorizationPolicy(policyFile, policy); - Runnable callback = () -> { - try { - getStub().unaryRpc(SimpleRequest.getDefaultInstance()); - fail("exception expected"); - } catch (StatusRuntimeException sre) { - assertThat(sre).hasMessageThat().isEqualTo( - "PERMISSION_DENIED: Access Denied"); - } catch (Exception e) { - throw new AssertionError("the test failed ", e); - } - }; - Thread onReloadDone = new Thread(callback); - interceptor.setCallbackForTesting(onReloadDone); - onReloadDone.join(); } @Test public void fileWatcherAuthzRecoversFromReloadTest() throws Exception { - String policy = "{" + String validPolicy1 = "{" + " \"name\" : \"authz\" ," + " \"allow_rules\": [" + " {" @@ -756,39 +733,34 @@ public void fileWatcherAuthzRecoversFromReloadTest() throws Exception { + " }" + " ]" + "}"; - File policyFile = createTempAuthorizationPolicy(policy); + createTempAuthorizationPolicy(validPolicy1); FileWatcherAuthorizationServerInterceptor interceptor = createFileWatcherAuthorizationInterceptor(policyFile); - Closeable closeable = interceptor.scheduleRefreshes(100, TimeUnit.MILLISECONDS); + Closeable closeable = interceptor.scheduleRefreshes( + 100, TimeUnit.NANOSECONDS, fakeClock.getScheduledExecutorService()); initServerWithAuthzInterceptor(interceptor, InsecureServerCredentials.create()); - closeable.close(); - policyFile.deleteOnExit(); + String invalidPolicy = "{}"; + rewriteAuthorizationPolicy(invalidPolicy); + // Reload is yet to take place at 100ns. validPolicy1 will be active here. + assertEquals(0, fakeClock.forwardNanos(99)); try { getStub().unaryRpc(SimpleRequest.getDefaultInstance()); fail("exception expected"); } catch (StatusRuntimeException sre) { assertThat(sre).hasMessageThat().isEqualTo( "PERMISSION_DENIED: Access Denied"); - } catch (Exception e) { - throw new AssertionError("the test failed ", e); } - policy = "{}"; - rewriteAuthorizationPolicy(policyFile, policy); - Runnable callback = () -> { - try { - getStub().unaryRpc(SimpleRequest.getDefaultInstance()); - fail("exception expected"); - } catch (StatusRuntimeException sre) { - assertThat(sre).hasMessageThat().isEqualTo( + // Reload will take place which skips the invalidPolicy. validPolicy1 remains + // the active policy. + assertEquals(1, fakeClock.forwardNanos(2)); + try { + getStub().unaryRpc(SimpleRequest.getDefaultInstance()); + fail("exception expected"); + } catch (StatusRuntimeException sre) { + assertThat(sre).hasMessageThat().isEqualTo( "PERMISSION_DENIED: Access Denied"); - } catch (Exception e) { - throw new AssertionError("the test failed ", e); - } - }; - Thread onFirstReloadDone = new Thread(callback); - interceptor.setCallbackForTesting(onFirstReloadDone); - onFirstReloadDone.join(); - policy = "{" + } + String validPolicy2 = "{" + " \"name\" : \"authz\" ," + " \"allow_rules\": [" + " {" @@ -801,13 +773,20 @@ public void fileWatcherAuthzRecoversFromReloadTest() throws Exception { + " }" + " ]" + "}"; - rewriteAuthorizationPolicy(policyFile, policy); - callback = () -> { + rewriteAuthorizationPolicy(validPolicy2); + // Next reload is yet to take place. validPolicy1 remains the active policy. + assertEquals(0, fakeClock.forwardNanos(98)); + try { getStub().unaryRpc(SimpleRequest.getDefaultInstance()); - }; - Thread onSecondReloadDone = new Thread(callback); - interceptor.setCallbackForTesting(onSecondReloadDone); - onSecondReloadDone.join(); + fail("exception expected"); + } catch (StatusRuntimeException sre) { + assertThat(sre).hasMessageThat().isEqualTo( + "PERMISSION_DENIED: Access Denied"); + } + // Reload occurs making validPolicy2 the active policy. + assertEquals(1, fakeClock.forwardNanos(2)); + closeable.close(); + getStub().unaryRpc(SimpleRequest.getDefaultInstance()); } private static class SimpleServiceImpl extends SimpleServiceGrpc.SimpleServiceImplBase { diff --git a/authz/src/test/java/io/grpc/authz/AuthorizationPolicyTranslatorTest.java b/authz/src/test/java/io/grpc/authz/AuthorizationPolicyTranslatorTest.java index b957bf283e7..557458e97d7 100644 --- a/authz/src/test/java/io/grpc/authz/AuthorizationPolicyTranslatorTest.java +++ b/authz/src/test/java/io/grpc/authz/AuthorizationPolicyTranslatorTest.java @@ -48,8 +48,6 @@ public void invalidPolicy() throws Exception { assertThat(ioe).hasMessageThat().isEqualTo( "Use JsonReader.setLenient(true) to accept malformed JSON" + " at line 1 column 18 path $.name"); - } catch (Exception e) { - throw new AssertionError("the test failed ", e); } } @@ -61,8 +59,6 @@ public void missingAuthorizationPolicyName() throws Exception { fail("exception expected"); } catch (IllegalArgumentException iae) { assertThat(iae).hasMessageThat().isEqualTo("\"name\" is absent or empty"); - } catch (Exception e) { - throw new AssertionError("the test failed ", e); } } @@ -75,8 +71,6 @@ public void incorrectAuthorizationPolicyName() throws Exception { } catch (ClassCastException cce) { assertThat(cce).hasMessageThat().isEqualTo( "value '[abc]' for key 'name' in '{name=[abc]}' is not String"); - } catch (Exception e) { - throw new AssertionError("the test failed ", e); } } @@ -88,8 +82,6 @@ public void missingAllowRules() throws Exception { fail("exception expected"); } catch (IllegalArgumentException iae) { assertThat(iae).hasMessageThat().isEqualTo("\"allow_rules\" is absent"); - } catch (Exception e) { - throw new AssertionError("the test failed ", e); } } @@ -106,8 +98,6 @@ public void missingRuleName() throws Exception { fail("exception expected"); } catch (IllegalArgumentException iae) { assertThat(iae).hasMessageThat().isEqualTo("rule \"name\" is absent or empty"); - } catch (Exception e) { - throw new AssertionError("the test failed ", e); } } @@ -173,8 +163,6 @@ public void incorrectRulesType() throws Exception { } catch (ClassCastException cce) { assertThat(cce).hasMessageThat().isEqualTo( "value '{}' for key 'allow_rules' in '{name=abc, allow_rules={}}' is not List"); - } catch (Exception e) { - throw new AssertionError("the test failed ", e); } } @@ -281,8 +269,6 @@ public void unsupportedPseudoHeaders() throws Exception { fail("exception expected"); } catch (IllegalArgumentException iae) { assertThat(iae).hasMessageThat().isEqualTo("Unsupported \"key\" :method"); - } catch (Exception e) { - throw new AssertionError("the test failed ", e); } } @@ -311,8 +297,6 @@ public void unsupportedGrpcPrefixHeaders() throws Exception { fail("exception expected"); } catch (IllegalArgumentException iae) { assertThat(iae).hasMessageThat().isEqualTo("Unsupported \"key\" grpc-xxx"); - } catch (Exception e) { - throw new AssertionError("the test failed ", e); } } @@ -341,8 +325,6 @@ public void unsupportedHostHeaders() throws Exception { fail("exception expected"); } catch (IllegalArgumentException iae) { assertThat(iae).hasMessageThat().isEqualTo("Unsupported \"key\" Host"); - } catch (Exception e) { - throw new AssertionError("the test failed ", e); } } @@ -366,8 +348,6 @@ public void missingHeaderKey() throws Exception { fail("exception expected"); } catch (IllegalArgumentException iae) { assertThat(iae).hasMessageThat().isEqualTo("\"key\" is absent or empty"); - } catch (Exception e) { - throw new AssertionError("the test failed ", e); } } @@ -393,8 +373,6 @@ public void missingHeaderValues() throws Exception { fail("exception expected"); } catch (IllegalArgumentException iae) { assertThat(iae).hasMessageThat().isEqualTo("\"values\" is absent or empty"); - } catch (Exception e) { - throw new AssertionError("the test failed ", e); } } @@ -421,8 +399,6 @@ public void emptyHeaderValues() throws Exception { fail("exception expected"); } catch (IllegalArgumentException iae) { assertThat(iae).hasMessageThat().isEqualTo("\"values\" is absent or empty"); - } catch (Exception e) { - throw new AssertionError("the test failed ", e); } } diff --git a/authz/src/test/java/io/grpc/authz/AuthorizationServerInterceptorTest.java b/authz/src/test/java/io/grpc/authz/AuthorizationServerInterceptorTest.java index 990228e2c96..b07a71bfb9f 100644 --- a/authz/src/test/java/io/grpc/authz/AuthorizationServerInterceptorTest.java +++ b/authz/src/test/java/io/grpc/authz/AuthorizationServerInterceptorTest.java @@ -38,8 +38,6 @@ public void invalidPolicyFailsStaticAuthzInterceptorCreation() throws Exception assertThat(ioe).hasMessageThat().isEqualTo( "Use JsonReader.setLenient(true) to accept malformed JSON" + " at line 1 column 18 path $.name"); - } catch (Exception e) { - throw new AssertionError("the test failed ", e); } } diff --git a/authz/src/test/java/io/grpc/authz/FileWatcherAuthorizationServerInterceptorTest.java b/authz/src/test/java/io/grpc/authz/FileWatcherAuthorizationServerInterceptorTest.java index f2e3efb9397..0ed63541fa6 100644 --- a/authz/src/test/java/io/grpc/authz/FileWatcherAuthorizationServerInterceptorTest.java +++ b/authz/src/test/java/io/grpc/authz/FileWatcherAuthorizationServerInterceptorTest.java @@ -22,8 +22,10 @@ import static org.junit.Assert.fail; import java.io.File; -import java.io.FileOutputStream; import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Paths; +import java.util.concurrent.Executors; import java.util.concurrent.TimeUnit; import org.junit.Test; import org.junit.runner.RunWith; @@ -35,20 +37,13 @@ public class FileWatcherAuthorizationServerInterceptorTest { public void invalidPolicyFailsAuthzInterceptorCreation() throws Exception { File policyFile = File.createTempFile("temp", "json"); policyFile.deleteOnExit(); - try (FileOutputStream outputStream = new FileOutputStream(policyFile)) { - String policy = "{ \"name\": \"abc\",, }"; - outputStream.write(policy.getBytes(UTF_8)); - outputStream.close(); - } + String policy = "{ \"name\": \"abc\",, }"; + Files.write(Paths.get(policyFile.getAbsolutePath()), policy.getBytes(UTF_8)); try { FileWatcherAuthorizationServerInterceptor.create(policyFile); fail("exception expected"); } catch (IOException ioe) { - assertThat(ioe).hasMessageThat().isEqualTo( - "Use JsonReader.setLenient(true) to accept malformed JSON" - + " at line 1 column 18 path $.name"); - } catch (Exception e) { - throw new AssertionError("the test failed ", e); + assertThat(ioe).hasMessageThat().contains("malformed"); } } @@ -56,61 +51,53 @@ public void invalidPolicyFailsAuthzInterceptorCreation() throws Exception { public void validPolicyCreatesFileWatcherAuthzInterceptor() throws Exception { File policyFile = File.createTempFile("temp", "json"); policyFile.deleteOnExit(); - try (FileOutputStream outputStream = new FileOutputStream(policyFile)) { - String policy = "{" - + " \"name\" : \"authz\"," - + " \"deny_rules\": [" - + " {" - + " \"name\": \"deny_foo\"," - + " \"source\": {" - + " \"principals\": [" - + " \"spiffe://foo.com\"" - + " ]" - + " }" - + " }" - + " ]," - + " \"allow_rules\": [" - + " {" - + " \"name\": \"allow_all\"" - + " }" - + " ]" - + "}"; - outputStream.write(policy.getBytes(UTF_8)); - outputStream.close(); - } + String policy = "{" + + " \"name\" : \"authz\"," + + " \"deny_rules\": [" + + " {" + + " \"name\": \"deny_foo\"," + + " \"source\": {" + + " \"principals\": [" + + " \"spiffe://foo.com\"" + + " ]" + + " }" + + " }" + + " ]," + + " \"allow_rules\": [" + + " {" + + " \"name\": \"allow_all\"" + + " }" + + " ]" + + "}"; + Files.write(Paths.get(policyFile.getAbsolutePath()), policy.getBytes(UTF_8)); FileWatcherAuthorizationServerInterceptor interceptor = FileWatcherAuthorizationServerInterceptor.create(policyFile); assertNotNull(interceptor); - interceptor.scheduleRefreshes(1, TimeUnit.SECONDS); } @Test public void invalidRefreshIntervalFailsScheduleRefreshes() throws Exception { File policyFile = File.createTempFile("temp", "json"); policyFile.deleteOnExit(); - try (FileOutputStream outputStream = new FileOutputStream(policyFile)) { - String policy = "{" - + " \"name\" : \"authz\"," - + " \"allow_rules\": [" - + " {" - + " \"name\": \"allow_all\"" - + " }" - + " ]" - + "}"; - outputStream.write(policy.getBytes(UTF_8)); - outputStream.close(); - } + String policy = "{" + + " \"name\" : \"authz\"," + + " \"allow_rules\": [" + + " {" + + " \"name\": \"allow_all\"" + + " }" + + " ]" + + "}"; + Files.write(Paths.get(policyFile.getAbsolutePath()), policy.getBytes(UTF_8)); FileWatcherAuthorizationServerInterceptor interceptor = FileWatcherAuthorizationServerInterceptor.create(policyFile); assertNotNull(interceptor); try { - interceptor.scheduleRefreshes(0, TimeUnit.SECONDS); + interceptor.scheduleRefreshes(0, TimeUnit.SECONDS, + Executors.newSingleThreadScheduledExecutor()); fail("exception expected"); } catch (IllegalArgumentException iae) { assertThat(iae).hasMessageThat().isEqualTo( "Refresh interval must be greater than 0"); - } catch (Exception e) { - throw new AssertionError("the test failed ", e); } } } From 956589a9363a8094b08fb9c7b4ebc68fdf28e0cb Mon Sep 17 00:00:00 2001 From: Eric Anderson Date: Wed, 19 Apr 2023 08:51:29 -0700 Subject: [PATCH 3/3] Remove unnecessary : from log line --- .../grpc/authz/FileWatcherAuthorizationServerInterceptor.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/authz/src/main/java/io/grpc/authz/FileWatcherAuthorizationServerInterceptor.java b/authz/src/main/java/io/grpc/authz/FileWatcherAuthorizationServerInterceptor.java index 50e868b062a..c474ead5aa5 100644 --- a/authz/src/main/java/io/grpc/authz/FileWatcherAuthorizationServerInterceptor.java +++ b/authz/src/main/java/io/grpc/authz/FileWatcherAuthorizationServerInterceptor.java @@ -92,7 +92,7 @@ public void run() { try { updateInternalInterceptor(); } catch (Exception e) { - logger.log(Level.WARNING, "Authorization Policy file reload failed: ", e); + logger.log(Level.WARNING, "Authorization Policy file reload failed", e); } } }, period, period, unit);