diff --git a/pom.xml b/pom.xml index e0839d228c..17e9963d24 100644 --- a/pom.xml +++ b/pom.xml @@ -201,6 +201,18 @@ ${com.github.spotbugs.version} provided + + io.fabric8 + kubernetes-server-mock + ${io.fabric8.client.version} + test + + + io.fabric8 + openshift-server-mock + ${io.fabric8.client.version} + test + diff --git a/src/main/java/io/cryostat/net/NetworkModule.java b/src/main/java/io/cryostat/net/NetworkModule.java index 3f8a3b7abd..48ed6f9db0 100644 --- a/src/main/java/io/cryostat/net/NetworkModule.java +++ b/src/main/java/io/cryostat/net/NetworkModule.java @@ -55,6 +55,8 @@ import dagger.Module; import dagger.Provides; import dagger.multibindings.IntoSet; +import io.fabric8.openshift.client.DefaultOpenShiftClient; +import io.fabric8.openshift.client.OpenShiftConfigBuilder; import io.vertx.core.Vertx; import io.vertx.ext.web.client.WebClient; import io.vertx.ext.web.client.WebClientOptions; @@ -159,7 +161,12 @@ static BasicAuthManager provideBasicAuthManager(Logger logger, FileSystem fs) { @Provides @Singleton static OpenShiftAuthManager provideOpenShiftAuthManager(Logger logger, FileSystem fs) { - return new OpenShiftAuthManager(logger, fs); + return new OpenShiftAuthManager( + logger, + fs, + token -> + new DefaultOpenShiftClient( + new OpenShiftConfigBuilder().withOauthToken(token).build())); } @Binds diff --git a/src/main/java/io/cryostat/net/OpenShiftAuthManager.java b/src/main/java/io/cryostat/net/OpenShiftAuthManager.java index 37735c2327..3ddfa4c5ac 100644 --- a/src/main/java/io/cryostat/net/OpenShiftAuthManager.java +++ b/src/main/java/io/cryostat/net/OpenShiftAuthManager.java @@ -47,40 +47,42 @@ import java.util.concurrent.ExecutionException; import java.util.concurrent.Future; import java.util.concurrent.TimeUnit; +import java.util.function.Function; import java.util.function.Supplier; import java.util.regex.Matcher; import java.util.regex.Pattern; import java.util.stream.Collectors; -import org.apache.commons.lang3.StringUtils; - -import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; import io.cryostat.core.log.Logger; import io.cryostat.core.sys.FileSystem; import io.cryostat.net.security.ResourceAction; import io.cryostat.net.security.ResourceType; import io.cryostat.net.security.ResourceVerb; + +import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; import io.fabric8.kubernetes.api.model.authorization.v1.SelfSubjectAccessReview; import io.fabric8.kubernetes.api.model.authorization.v1.SelfSubjectAccessReviewBuilder; import io.fabric8.kubernetes.client.Config; import io.fabric8.kubernetes.client.KubernetesClientException; -import io.fabric8.openshift.client.DefaultOpenShiftClient; import io.fabric8.openshift.client.OpenShiftClient; -import io.fabric8.openshift.client.OpenShiftConfigBuilder; import jdk.jfr.Category; import jdk.jfr.Event; import jdk.jfr.Label; import jdk.jfr.Name; +import org.apache.commons.lang3.StringUtils; public class OpenShiftAuthManager extends AbstractAuthManager { private static final String PERMISSION_NOT_REQUIRED = "PERMISSION_NOT_REQUIRED"; private final FileSystem fs; + private final Function clientProvider; - public OpenShiftAuthManager(Logger logger, FileSystem fs) { + OpenShiftAuthManager( + Logger logger, FileSystem fs, Function clientProvider) { super(logger); this.fs = fs; + this.clientProvider = clientProvider; } @Override @@ -96,9 +98,7 @@ public Future validateToken( return CompletableFuture.completedFuture(false); } - try (OpenShiftClient authClient = - new DefaultOpenShiftClient( - new OpenShiftConfigBuilder().withOauthToken(token).build())) { + try (OpenShiftClient client = clientProvider.apply(token)) { List> results = resourceActions .parallelStream() @@ -106,7 +106,7 @@ public Future validateToken( resourceAction -> { try { return CompletableFuture.completedFuture( - validateAction(authClient, resourceAction)); + validateAction(client, resourceAction)); } catch (IOException | PermissionDeniedException e) { return CompletableFuture.failedFuture(e); } @@ -142,7 +142,7 @@ public Future validateToken( } } - private boolean validateAction(OpenShiftClient authClient, ResourceAction resourceAction) + private boolean validateAction(OpenShiftClient client, ResourceAction resourceAction) throws IOException, PermissionDeniedException { AuthRequest evt = new AuthRequest(); evt.begin(); @@ -167,7 +167,7 @@ private boolean validateAction(OpenShiftClient authClient, ResourceAction resour .endSpec() .build(); accessReview = - authClient.authorization().v1().selfSubjectAccessReview().create(accessReview); + client.authorization().v1().selfSubjectAccessReview().create(accessReview); boolean allowed = accessReview.getStatus().getAllowed(); evt.setRequestSuccessful(true); if (allowed) { @@ -184,25 +184,6 @@ private boolean validateAction(OpenShiftClient authClient, ResourceAction resour } } - @Name("io.cryostat.net.OpenShiftAuthManager.AuthRequest") - @Label("AuthRequest") - @Category("Cryostat") - @SuppressFBWarnings( - value = "URF_UNREAD_FIELD", - justification = "Event fields are recorded with JFR instead of accessed directly") - public static class AuthRequest extends Event { - - boolean requestSuccessful; - - public AuthRequest() { - this.requestSuccessful = false; - } - - public void setRequestSuccessful(boolean requestSuccessful) { - this.requestSuccessful = requestSuccessful; - } - } - @Override public Future validateHttpHeader( Supplier headerProvider, Set resourceActions) { @@ -254,7 +235,7 @@ private String getNamespace() throws IOException { .get(); } - static String map(ResourceType resource) { + private static String map(ResourceType resource) { switch (resource) { case TARGET: return "flightrecorders"; @@ -270,7 +251,7 @@ static String map(ResourceType resource) { } } - static String map(ResourceVerb verb) { + private static String map(ResourceVerb verb) { switch (verb) { case CREATE: return "create"; @@ -288,12 +269,50 @@ static String map(ResourceVerb verb) { @SuppressWarnings("serial") public static class PermissionDeniedException extends Exception { + private final String namespace; + private final String resource; + private final String verb; + public PermissionDeniedException( String namespace, String group, String resource, String verb, String reason) { super( String.format( "Requesting client in namespace \"%s\" cannot %s %s.%s: %s", namespace, verb, resource, group, reason)); + this.namespace = namespace; + this.resource = resource; + this.verb = verb; + } + + public String getNamespace() { + return namespace; + } + + public String getResourceType() { + return resource; + } + + public String getVerb() { + return verb; + } + } + + @Name("io.cryostat.net.OpenShiftAuthManager.AuthRequest") + @Label("AuthRequest") + @Category("Cryostat") + @SuppressFBWarnings( + value = "URF_UNREAD_FIELD", + justification = "Event fields are recorded with JFR instead of accessed directly") + public static class AuthRequest extends Event { + + boolean requestSuccessful; + + public AuthRequest() { + this.requestSuccessful = false; + } + + public void setRequestSuccessful(boolean requestSuccessful) { + this.requestSuccessful = requestSuccessful; } } } diff --git a/src/test/java/io/cryostat/net/OpenShiftAuthManagerTest.java b/src/test/java/io/cryostat/net/OpenShiftAuthManagerTest.java new file mode 100644 index 0000000000..868abc6dde --- /dev/null +++ b/src/test/java/io/cryostat/net/OpenShiftAuthManagerTest.java @@ -0,0 +1,274 @@ +/* + * Copyright The Cryostat Authors + * + * The Universal Permissive License (UPL), Version 1.0 + * + * Subject to the condition set forth below, permission is hereby granted to any + * person obtaining a copy of this software, associated documentation and/or data + * (collectively the "Software"), free of charge and under any and all copyright + * rights in the Software, and any and all patent rights owned or freely + * licensable by each licensor hereunder covering either (i) the unmodified + * Software as contributed to or provided by such licensor, or (ii) the Larger + * Works (as defined below), to deal in both + * + * (a) the Software, and + * (b) any piece of software and/or hardware listed in the lrgrwrks.txt file if + * one is included with the Software (each a "Larger Work" to which the Software + * is contributed by such licensors), + * + * without restriction, including without limitation the rights to copy, create + * derivative works of, display, perform, and distribute the Software and make, + * use, sell, offer for sale, import, export, have made, and have sold the + * Software and the Larger Work(s), and to sublicense the foregoing rights on + * either these or other terms. + * + * This license is subject to the following condition: + * The above copyright notice and either this complete permission notice or at + * a minimum a reference to the UPL must be included in all copies or + * substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE + * SOFTWARE. + */ +package io.cryostat.net; + +import java.io.BufferedReader; +import java.io.StringReader; +import java.net.HttpURLConnection; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.util.Set; +import java.util.concurrent.ExecutionException; +import java.util.function.Function; + +import io.cryostat.MainModule; +import io.cryostat.core.log.Logger; +import io.cryostat.core.sys.FileSystem; +import io.cryostat.net.OpenShiftAuthManager.PermissionDeniedException; +import io.cryostat.net.security.ResourceAction; +import io.cryostat.net.security.ResourceType; +import io.cryostat.net.security.ResourceVerb; + +import com.google.gson.Gson; +import io.fabric8.kubernetes.api.model.authorization.v1.SelfSubjectAccessReview; +import io.fabric8.kubernetes.api.model.authorization.v1.SelfSubjectAccessReviewBuilder; +import io.fabric8.openshift.client.OpenShiftClient; +import io.fabric8.openshift.client.server.mock.EnableOpenShiftMockClient; +import io.fabric8.openshift.client.server.mock.OpenShiftMockServer; +import io.fabric8.openshift.client.server.mock.OpenShiftMockServerExtension; +import okhttp3.mockwebserver.RecordedRequest; +import org.apache.commons.lang3.exception.ExceptionUtils; +import org.hamcrest.MatcherAssert; +import org.hamcrest.Matchers; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.EnumSource; +import org.junit.jupiter.params.provider.NullAndEmptySource; +import org.mockito.ArgumentCaptor; +import org.mockito.Mock; +import org.mockito.Mockito; +import org.mockito.junit.jupiter.MockitoExtension; + +@ExtendWith({MockitoExtension.class, OpenShiftMockServerExtension.class}) +@EnableOpenShiftMockClient(https = false, crud = false) +class OpenShiftAuthManagerTest { + + static final String SUBJECT_REVIEW_API_PATH = + "/apis/authorization.k8s.io/v1/selfsubjectaccessreviews"; + static final String NAMESPACE_FS_PATH = + "/var/run/secrets/kubernetes.io/serviceaccount/namespace"; + + OpenShiftAuthManager mgr; + @Mock FileSystem fs; + @Mock Logger logger; + OpenShiftClient client; + OpenShiftMockServer server; + TokenProvider tokenProvider; + Gson gson = MainModule.provideGson(logger); + + @BeforeEach + void setup() { + client = Mockito.spy(client); + tokenProvider = new TokenProvider(client); + mgr = new OpenShiftAuthManager(logger, fs, tokenProvider); + } + + @Test + void shouldHandleBearerAuthentication() { + MatcherAssert.assertThat(mgr.getScheme(), Matchers.equalTo(AuthenticationScheme.BEARER)); + } + + @ParameterizedTest + @NullAndEmptySource + void shouldNotValidateBlankToken(String tok) throws Exception { + MatcherAssert.assertThat( + mgr.validateToken(() -> tok, ResourceAction.NONE).get(), Matchers.is(false)); + } + + @Test + void shouldValidateTokenWithNoPermissions() throws Exception { + MatcherAssert.assertThat( + mgr.validateToken(() -> "token", ResourceAction.NONE).get(), Matchers.is(true)); + } + + @Test + void shouldValidateTokenWithSufficientPermissions() throws Exception { + SelfSubjectAccessReview accessReview = + new SelfSubjectAccessReviewBuilder() + .withNewStatus() + .withAllowed(true) + .endStatus() + .build(); + server.expect() + .post() + .withPath(SUBJECT_REVIEW_API_PATH) + .andReturn(HttpURLConnection.HTTP_CREATED, accessReview) + .once(); + + Mockito.when(fs.readFile(Mockito.any())) + .thenReturn(new BufferedReader(new StringReader("mynamespace"))); + + MatcherAssert.assertThat( + mgr.validateToken(() -> "token", Set.of(ResourceAction.READ_TARGET)).get(), + Matchers.is(true)); + + ArgumentCaptor nsPathCaptor = ArgumentCaptor.forClass(Path.class); + Mockito.verify(fs).readFile(nsPathCaptor.capture()); + MatcherAssert.assertThat( + nsPathCaptor.getValue(), Matchers.equalTo(Paths.get(NAMESPACE_FS_PATH))); + } + + @Test + void shouldNotValidateTokenWithSufficientPermissions() throws Exception { + SelfSubjectAccessReview accessReview = + new SelfSubjectAccessReviewBuilder() + .withNewStatus() + .withAllowed(false) + .endStatus() + .build(); + server.expect() + .post() + .withPath(SUBJECT_REVIEW_API_PATH) + .andReturn(HttpURLConnection.HTTP_CREATED, accessReview) + .once(); + + String namespace = "mynamespace"; + Mockito.when(fs.readFile(Mockito.any())) + .thenReturn(new BufferedReader(new StringReader(namespace))); + + ExecutionException ee = + Assertions.assertThrows( + ExecutionException.class, + () -> + mgr.validateToken(() -> "token", Set.of(ResourceAction.READ_TARGET)) + .get()); + MatcherAssert.assertThat( + ExceptionUtils.getRootCause(ee), + Matchers.instanceOf(PermissionDeniedException.class)); + PermissionDeniedException pde = (PermissionDeniedException) ExceptionUtils.getRootCause(ee); + MatcherAssert.assertThat(pde.getNamespace(), Matchers.equalTo(namespace)); + MatcherAssert.assertThat(pde.getResourceType(), Matchers.equalTo("flightrecorders")); + MatcherAssert.assertThat(pde.getVerb(), Matchers.equalTo("get")); + + ArgumentCaptor nsPathCaptor = ArgumentCaptor.forClass(Path.class); + Mockito.verify(fs).readFile(nsPathCaptor.capture()); + MatcherAssert.assertThat( + nsPathCaptor.getValue(), Matchers.equalTo(Paths.get(NAMESPACE_FS_PATH))); + } + + @ParameterizedTest + @EnumSource(mode = EnumSource.Mode.MATCH_ANY, names = "^([a-zA-Z]+_(RECORDING|TARGET))$") + void shouldValidateExpectedPermissionsPerSecuredResource(ResourceAction resourceAction) + throws Exception { + Mockito.when(fs.readFile(Mockito.any())) + .thenReturn(new BufferedReader(new StringReader("mynamespace"))); + + SelfSubjectAccessReview accessReview = + new SelfSubjectAccessReviewBuilder() + .withNewStatus() + .withAllowed(true) + .endStatus() + .build(); + server.expect() + .post() + .withPath(SUBJECT_REVIEW_API_PATH) + .andReturn(HttpURLConnection.HTTP_CREATED, accessReview) + .once(); + + String token = "abcd1234"; + MatcherAssert.assertThat( + mgr.validateToken(() -> token, Set.of(resourceAction)).get(), Matchers.is(true)); + + RecordedRequest req = server.getLastRequest(); + MatcherAssert.assertThat(req.getPath(), Matchers.equalTo(SUBJECT_REVIEW_API_PATH)); + MatcherAssert.assertThat(tokenProvider.token, Matchers.equalTo(token)); + MatcherAssert.assertThat(req.getMethod(), Matchers.equalTo("POST")); + + SelfSubjectAccessReview body = + gson.fromJson(req.getBody().readUtf8(), SelfSubjectAccessReview.class); + + String expectedVerb; + if (resourceAction.getVerb() == ResourceVerb.CREATE) { + expectedVerb = "create"; + } else if (resourceAction.getVerb() == ResourceVerb.READ) { + expectedVerb = "get"; + } else if (resourceAction.getVerb() == ResourceVerb.UPDATE) { + expectedVerb = "patch"; + } else if (resourceAction.getVerb() == ResourceVerb.DELETE) { + expectedVerb = "delete"; + } else { + throw new IllegalArgumentException(resourceAction.getVerb().toString()); + } + MatcherAssert.assertThat( + body.getSpec().getResourceAttributes().getVerb(), Matchers.equalTo(expectedVerb)); + + String expectedResource; + if (resourceAction.getResource() == ResourceType.TARGET) { + expectedResource = "flightrecorders"; + } else if (resourceAction.getResource() == ResourceType.RECORDING) { + expectedResource = "recordings"; + } else { + throw new IllegalArgumentException(resourceAction.getResource().toString()); + } + MatcherAssert.assertThat( + body.getSpec().getResourceAttributes().getResource(), + Matchers.equalTo(expectedResource)); + } + + @ParameterizedTest + @EnumSource( + mode = EnumSource.Mode.MATCH_ALL, + names = {"^[a-zA-Z]+_(?!TARGET).*$", "^[a-zA-Z]+_(?!RECORDING).*$"}) + void shouldValidateExpectedPermissionsForUnsecuredResources(ResourceAction resourceAction) + throws Exception { + MatcherAssert.assertThat( + mgr.validateToken(() -> "token", Set.of(resourceAction)).get(), Matchers.is(true)); + } + + private static class TokenProvider implements Function { + + private final OpenShiftClient osc; + String token; + + TokenProvider(OpenShiftClient osc) { + this.osc = osc; + } + + @Override + public OpenShiftClient apply(String token) { + if (this.token != null) { + throw new IllegalStateException("Token was already set!"); + } + this.token = token; + return osc; + } + } +}