Skip to content

Commit

Permalink
fix(openshift): validate user tokens even without required resource a…
Browse files Browse the repository at this point in the history 
…ctions
  • Loading branch information
@andrewazores
andrewazores committed Aug 6, 2021
1 parent c34d94d commit 95321a6
Show file tree
Hide file tree
Showing 2 changed files with 81 additions and 11 deletions.
31 changes: 30 additions & 1 deletion src/main/java/io/cryostat/net/OpenShiftAuthManager.java
View file
Expand Up @@ -61,6 +61,8 @@
import io.cryostat.net.security.ResourceVerb;

import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
import io.fabric8.kubernetes.api.model.authentication.TokenReview;
import io.fabric8.kubernetes.api.model.authentication.TokenReviewBuilder;
import io.fabric8.kubernetes.api.model.authorization.v1.SelfSubjectAccessReview;
import io.fabric8.kubernetes.api.model.authorization.v1.SelfSubjectAccessReviewBuilder;
import io.fabric8.kubernetes.client.Config;
Expand Down Expand Up @@ -99,7 +101,7 @@ public Future<Boolean> validateToken(
return CompletableFuture.completedFuture(false);
}
if (resourceActions.isEmpty()) {
return CompletableFuture.completedFuture(true);
return reviewToken(token);
}

try (OpenShiftClient client = clientProvider.apply(token)) {
Expand All @@ -126,6 +128,22 @@ public Future<Boolean> validateToken(
}
}

Future<Boolean> reviewToken(String token) {
try (OpenShiftClient client = clientProvider.apply(getServiceAccountToken())) {
TokenReview review =
new TokenReviewBuilder().withNewSpec().withToken(token).endSpec().build();
review = client.tokenReviews().create(review);
Boolean authenticated = review.getStatus().getAuthenticated();
return CompletableFuture.completedFuture(authenticated != null && authenticated);
} catch (KubernetesClientException e) {
logger.info(e);
return CompletableFuture.failedFuture(e);
} catch (Exception e) {
logger.error(e);
return CompletableFuture.failedFuture(e);
}
}

private Stream<CompletableFuture<Void>> validateAction(
OpenShiftClient client, String namespace, ResourceAction resourceAction) {
Set<String> resources = map(resourceAction.getResource());
Expand Down Expand Up @@ -231,6 +249,17 @@ private String getNamespace() throws IOException {
.get();
}

@SuppressFBWarnings(
value = "DMI_HARDCODED_ABSOLUTE_FILENAME",
justification = "Kubernetes serviceaccount file path is well-known and absolute")
private String getServiceAccountToken() throws IOException {
return fs.readFile(Paths.get(Config.KUBERNETES_SERVICE_ACCOUNT_TOKEN_PATH))
.lines()
.filter(StringUtils::isNotBlank)
.findFirst()
.get();
}

private static Set<String> map(ResourceType resource) {
switch (resource) {
case TARGET:
Expand Down
61 changes: 51 additions & 10 deletions src/test/java/io/cryostat/net/OpenShiftAuthManagerTest.java
View file
Expand Up @@ -57,6 +57,8 @@
import io.cryostat.net.security.ResourceVerb;

import com.google.gson.Gson;
import io.fabric8.kubernetes.api.model.authentication.TokenReview;
import io.fabric8.kubernetes.api.model.authentication.TokenReviewBuilder;
import io.fabric8.kubernetes.api.model.authorization.v1.SelfSubjectAccessReview;
import io.fabric8.kubernetes.api.model.authorization.v1.SelfSubjectAccessReviewBuilder;
import io.fabric8.kubernetes.client.Config;
Expand Down Expand Up @@ -87,8 +89,7 @@ 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";
static final String TOKEN_REVIEW_API_PATH = "/apis/authentication.k8s.io/v1/tokenreviews";

OpenShiftAuthManager mgr;
@Mock FileSystem fs;
Expand All @@ -102,6 +103,7 @@ class OpenShiftAuthManagerTest {
static void disableKubeConfig() {
// FIXME Disable reading ~/.kube/config. Remove once updated to 5.5.0 or newer.
System.setProperty(Config.KUBERNETES_AUTH_TRYKUBECONFIG_SYSTEM_PROPERTY, "false");
System.setProperty(Config.KUBERNETES_AUTH_TRYSERVICEACCOUNT_SYSTEM_PROPERTY, "false");
}

@BeforeEach
Expand All @@ -124,9 +126,46 @@ void shouldNotValidateBlankToken(String tok) throws Exception {
}

@Test
void shouldValidateTokenWithNoPermissions() throws Exception {
void shouldValidateTokenWithNoRequiredPermissions() throws Exception {
TokenReview tokenReview =
new TokenReviewBuilder()
.withNewStatus()
.withAuthenticated(true)
.endStatus()
.build();
server.expect()
.post()
.withPath(TOKEN_REVIEW_API_PATH)
.andReturn(HttpURLConnection.HTTP_CREATED, tokenReview)
.once();

Mockito.when(fs.readFile(Paths.get(Config.KUBERNETES_SERVICE_ACCOUNT_TOKEN_PATH)))
.thenReturn(new BufferedReader(new StringReader("serviceAccountToken")));

MatcherAssert.assertThat(
mgr.validateToken(() -> "userToken", ResourceAction.NONE).get(), Matchers.is(true));
}

@Test
void shouldNotValidateTokenWithNoRequiredPermissionsButNoTokenAccess() throws Exception {
TokenReview tokenReview =
new TokenReviewBuilder()
.withNewStatus()
.withAuthenticated(false)
.endStatus()
.build();
server.expect()
.post()
.withPath(TOKEN_REVIEW_API_PATH)
.andReturn(HttpURLConnection.HTTP_CREATED, tokenReview)
.once();

Mockito.when(fs.readFile(Paths.get(Config.KUBERNETES_SERVICE_ACCOUNT_TOKEN_PATH)))
.thenReturn(new BufferedReader(new StringReader("serviceAccountToken")));

MatcherAssert.assertThat(
mgr.validateToken(() -> "token", ResourceAction.NONE).get(), Matchers.is(true));
mgr.validateToken(() -> "userToken", ResourceAction.NONE).get(),
Matchers.is(false));
}

@Test
Expand All @@ -143,7 +182,7 @@ void shouldValidateTokenWithSufficientPermissions() throws Exception {
.andReturn(HttpURLConnection.HTTP_CREATED, accessReview)
.once();

Mockito.when(fs.readFile(Mockito.any()))
Mockito.when(fs.readFile(Paths.get(Config.KUBERNETES_NAMESPACE_PATH)))
.thenReturn(new BufferedReader(new StringReader("mynamespace")));

MatcherAssert.assertThat(
Expand All @@ -153,7 +192,8 @@ void shouldValidateTokenWithSufficientPermissions() throws Exception {
ArgumentCaptor<Path> nsPathCaptor = ArgumentCaptor.forClass(Path.class);
Mockito.verify(fs).readFile(nsPathCaptor.capture());
MatcherAssert.assertThat(
nsPathCaptor.getValue(), Matchers.equalTo(Paths.get(NAMESPACE_FS_PATH)));
nsPathCaptor.getValue(),
Matchers.equalTo(Paths.get(Config.KUBERNETES_NAMESPACE_PATH)));
}

@Test
Expand All @@ -171,7 +211,7 @@ void shouldNotValidateTokenWithInsufficientPermissions() throws Exception {
.once();

String namespace = "mynamespace";
Mockito.when(fs.readFile(Mockito.any()))
Mockito.when(fs.readFile(Paths.get(Config.KUBERNETES_NAMESPACE_PATH)))
.thenReturn(new BufferedReader(new StringReader(namespace)));

ExecutionException ee =
Expand All @@ -191,7 +231,8 @@ void shouldNotValidateTokenWithInsufficientPermissions() throws Exception {
ArgumentCaptor<Path> nsPathCaptor = ArgumentCaptor.forClass(Path.class);
Mockito.verify(fs).readFile(nsPathCaptor.capture());
MatcherAssert.assertThat(
nsPathCaptor.getValue(), Matchers.equalTo(Paths.get(NAMESPACE_FS_PATH)));
nsPathCaptor.getValue(),
Matchers.equalTo(Paths.get(Config.KUBERNETES_NAMESPACE_PATH)));
}

@ParameterizedTest
Expand All @@ -200,7 +241,7 @@ void shouldNotValidateTokenWithInsufficientPermissions() throws Exception {
names = "^([a-zA-Z]+_(RECORDING|TARGET|CERTIFICATE|CREDENTIALS))$")
void shouldValidateExpectedPermissionsPerSecuredResource(ResourceAction resourceAction)
throws Exception {
Mockito.when(fs.readFile(Mockito.any()))
Mockito.when(fs.readFile(Paths.get(Config.KUBERNETES_NAMESPACE_PATH)))
.thenReturn(new BufferedReader(new StringReader("mynamespace")));

String expectedVerb;
Expand Down Expand Up @@ -305,7 +346,7 @@ void shouldValidateExpectedPermissionsPerSecuredResource(ResourceAction resource
})
void shouldValidateExpectedPermissionsForUnsecuredResources(ResourceAction resourceAction)
throws Exception {
Mockito.when(fs.readFile(Mockito.any()))
Mockito.when(fs.readFile(Paths.get(Config.KUBERNETES_NAMESPACE_PATH)))
.thenReturn(new BufferedReader(new StringReader("mynamespace")));
MatcherAssert.assertThat(
mgr.validateToken(() -> "token", Set.of(resourceAction)).get(), Matchers.is(true));
Expand Down

0 comments on commit 95321a6

Please sign in to comment.