Skip to content

Commit

Permalink
[autorules] Credentials deletion enhancements (#516)
Browse files Browse the repository at this point in the history
* Respond 404 when deleting nonexistent credentials

* Retrieve Credentials from Manager on each PeriodicArchiver run

If Credentials are deleted afer a rule is triggered/a PeriodicArchiver starts, the PeriodicArchiver should fail to archive from that point forward since its Credentials access has been revoked

* Test credentials revocation during archiver run
  • Loading branch information
andrewazores authored Jun 17, 2021
1 parent 8529b9b commit d884cb6
Show file tree
Hide file tree
Showing 9 changed files with 207 additions and 24 deletions.
8 changes: 7 additions & 1 deletion rulestest.sh
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ curl -k \
-F targetAlias="es.andrewazor.demo.Main" \
-F description="This is a test rule" \
-F eventSpecifier="template=Continuous,type=TARGET" \
-F archivalPeriodSeconds="60" \
-F archivalPeriodSeconds="5" \
-F preservedArchives="3" \
"$CRYOSTAT_HOST/api/v2/rules"

Expand All @@ -67,3 +67,9 @@ podman run \
--env JMX_PORT=9094 \
--env USE_AUTH=true \
--rm -d quay.io/andrewazores/vertx-fib-demo:0.7.0

sleep 15
echo "Deleting credentials"
curl -k \
-X DELETE \
"$CRYOSTAT_HOST/api/v2/targets/$demoAppTargetId/credentials"
10 changes: 8 additions & 2 deletions src/main/java/io/cryostat/configuration/CredentialsManager.java
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@
import io.cryostat.core.log.Logger;
import io.cryostat.core.net.Credentials;
import io.cryostat.core.sys.FileSystem;
import io.cryostat.platform.ServiceRef;

import com.google.gson.Gson;

Expand Down Expand Up @@ -114,15 +115,20 @@ boolean addCredentials(String targetId, Credentials credentials, boolean persist
return replaced;
}

public void removeCredentials(String targetId) throws IOException {
this.credentialsMap.remove(targetId);
public boolean removeCredentials(String targetId) throws IOException {
Credentials deleted = this.credentialsMap.remove(targetId);
fs.deleteIfExists(getPersistedPath(targetId));
return deleted != null;
}

public Credentials getCredentials(String targetId) {
return this.credentialsMap.get(targetId);
}

public Credentials getCredentials(ServiceRef serviceRef) {
return getCredentials(serviceRef.getJMXServiceUrl().toString());
}

private Path getPersistedPath(String targetId) {
return credentialsDir.resolve(String.format("%d.json", targetId.hashCode()));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -101,11 +101,11 @@ public boolean isOrdered() {
public IntermediateResponse<Void> handle(RequestParameters params) throws ApiException {
String targetId = params.getPathParams().get("targetId");
try {
this.credentialsManager.removeCredentials(targetId);
boolean status = this.credentialsManager.removeCredentials(targetId);
return new IntermediateResponse<Void>().statusCode(status ? 200 : 404);
} catch (IOException e) {
throw new ApiException(
500, "IOException occurred while clearing persisted credentials", e);
}
return new IntermediateResponse<Void>().body(null);
}
}
11 changes: 6 additions & 5 deletions src/main/java/io/cryostat/rules/PeriodicArchiver.java
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@
import java.util.concurrent.Future;
import java.util.function.Function;

import io.cryostat.configuration.CredentialsManager;
import io.cryostat.core.log.Logger;
import io.cryostat.core.net.Credentials;
import io.cryostat.platform.ServiceRef;
Expand All @@ -61,7 +62,7 @@
class PeriodicArchiver implements Runnable {

private final ServiceRef serviceRef;
private final Credentials credentials;
private final CredentialsManager credentialsManager;
private final Rule rule;
private final WebClient webClient;
private final Function<Credentials, MultiMap> headersFactory;
Expand All @@ -72,15 +73,15 @@ class PeriodicArchiver implements Runnable {

PeriodicArchiver(
ServiceRef serviceRef,
Credentials credentials,
CredentialsManager credentialsManager,
Rule rule,
WebClient webClient,
Function<Credentials, MultiMap> headersFactory,
Function<Pair<ServiceRef, Rule>, Void> failureNotifier,
Logger logger) {
this.webClient = webClient;
this.serviceRef = serviceRef;
this.credentials = credentials;
this.credentialsManager = credentialsManager;
this.rule = rule;
this.headersFactory = headersFactory;
this.failureNotifier = failureNotifier;
Expand Down Expand Up @@ -129,7 +130,7 @@ void performArchival() throws InterruptedException, ExecutionException {
CompletableFuture<String> future = new CompletableFuture<>();
this.webClient
.patch(path)
.putHeaders(headersFactory.apply(credentials))
.putHeaders(headersFactory.apply(credentialsManager.getCredentials(serviceRef)))
.sendBuffer(
Buffer.buffer("save"),
ar -> {
Expand Down Expand Up @@ -163,7 +164,7 @@ Future<Boolean> pruneArchive(String recordingName) {
CompletableFuture<Boolean> future = new CompletableFuture<>();
this.webClient
.delete(path)
.putHeaders(headersFactory.apply(credentials))
.putHeaders(headersFactory.apply(credentialsManager.getCredentials(serviceRef)))
.send(
ar -> {
if (ar.failed()) {
Expand Down
11 changes: 9 additions & 2 deletions src/main/java/io/cryostat/rules/PeriodicArchiverFactory.java
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@

import java.util.function.Function;

import io.cryostat.configuration.CredentialsManager;
import io.cryostat.core.log.Logger;
import io.cryostat.core.net.Credentials;
import io.cryostat.platform.ServiceRef;
Expand All @@ -62,10 +63,16 @@ class PeriodicArchiverFactory {

PeriodicArchiver create(
ServiceRef serviceRef,
Credentials credentials,
CredentialsManager credentialsManager,
Rule rule,
Function<Pair<ServiceRef, Rule>, Void> failureNotifier) {
return new PeriodicArchiver(
serviceRef, credentials, rule, webClient, headersFactory, failureNotifier, logger);
serviceRef,
credentialsManager,
rule,
webClient,
headersFactory,
failureNotifier,
logger);
}
}
2 changes: 1 addition & 1 deletion src/main/java/io/cryostat/rules/RuleProcessor.java
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,7 @@ public synchronized void accept(TargetDiscoveryEvent tde) {
scheduler.scheduleAtFixedRate(
periodicArchiverFactory.create(
tde.getServiceRef(),
credentials,
credentialsManager,
rule,
this::archivalFailureHandler),
rule.getArchivalPeriodSeconds(),
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,163 @@
/*
* 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.web.http.api.v2;

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

import io.cryostat.MainModule;
import io.cryostat.configuration.CredentialsManager;
import io.cryostat.core.log.Logger;
import io.cryostat.net.AuthManager;
import io.cryostat.net.web.http.HttpMimeType;
import io.cryostat.net.web.http.api.ApiVersion;

import com.google.gson.Gson;
import io.vertx.core.http.HttpMethod;
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.Nested;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
import org.mockito.Mock;
import org.mockito.Mockito;
import org.mockito.junit.jupiter.MockitoExtension;

@ExtendWith(MockitoExtension.class)
class TargetCredentialsDeleteHandlerTest {

AbstractV2RequestHandler<Void> handler;
@Mock AuthManager auth;
@Mock CredentialsManager credentialsManager;
@Mock Logger logger;
Gson gson = MainModule.provideGson(logger);

@BeforeEach
void setup() {
this.handler = new TargetCredentialsDeleteHandler(auth, credentialsManager, gson);
}

@Nested
class BasicHandlerDefinition {

@Test
void shouldRequireAuthentication() {
Assertions.assertTrue(handler.requiresAuthentication());
}

@Test
void shouldBeV2API() {
MatcherAssert.assertThat(handler.apiVersion(), Matchers.equalTo(ApiVersion.V2));
}

@Test
void shouldHaveDELETEMethod() {
MatcherAssert.assertThat(handler.httpMethod(), Matchers.equalTo(HttpMethod.DELETE));
}

@Test
void shouldHaveTargetsPath() {
MatcherAssert.assertThat(
handler.path(), Matchers.equalTo("/api/v2/targets/:targetId/credentials"));
}

@Test
void shouldHaveJsonMimeType() {
MatcherAssert.assertThat(handler.mimeType(), Matchers.equalTo(HttpMimeType.PLAINTEXT));
}

@Test
void shouldNotBeAsync() {
Assertions.assertFalse(handler.isAsync());
}

@Test
void shouldBeOrdered() {
Assertions.assertTrue(handler.isOrdered());
}
}

@Nested
class RequestExecutions {
@Mock RequestParameters requestParams;

@Test
void shouldRespond200OnSuccess() throws Exception {
String targetId = "fooTarget";
Mockito.when(requestParams.getPathParams()).thenReturn(Map.of("targetId", targetId));
Mockito.when(credentialsManager.removeCredentials(Mockito.anyString()))
.thenReturn(true);

IntermediateResponse<Void> response = handler.handle(requestParams);

MatcherAssert.assertThat(response.getStatusCode(), Matchers.equalTo(200));
Mockito.verify(credentialsManager).removeCredentials(targetId);
}

@Test
void shouldRespond404OnFailure() throws Exception {
String targetId = "fooTarget";
Mockito.when(requestParams.getPathParams()).thenReturn(Map.of("targetId", targetId));
Mockito.when(credentialsManager.removeCredentials(Mockito.anyString()))
.thenReturn(false);

IntermediateResponse<Void> response = handler.handle(requestParams);

MatcherAssert.assertThat(response.getStatusCode(), Matchers.equalTo(404));
Mockito.verify(credentialsManager).removeCredentials(targetId);
}

@Test
void shouldWrapIOExceptions() throws Exception {
String targetId = "fooTarget";
Mockito.when(requestParams.getPathParams()).thenReturn(Map.of("targetId", targetId));
Mockito.when(credentialsManager.removeCredentials(Mockito.anyString()))
.thenThrow(IOException.class);

ApiException ex =
Assertions.assertThrows(
ApiException.class, () -> handler.handle(requestParams));

MatcherAssert.assertThat(ex.getStatusCode(), Matchers.equalTo(500));
MatcherAssert.assertThat(ex.getCause(), Matchers.instanceOf(IOException.class));
Mockito.verify(credentialsManager).removeCredentials(targetId);
}
}
}
8 changes: 5 additions & 3 deletions src/test/java/io/cryostat/rules/PeriodicArchiverTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,8 @@

import javax.management.remote.JMXServiceURL;

import io.cryostat.configuration.CredentialsManager;
import io.cryostat.core.log.Logger;
import io.cryostat.core.net.Credentials;
import io.cryostat.platform.ServiceRef;

import io.vertx.core.AsyncResult;
Expand Down Expand Up @@ -71,7 +71,7 @@ class PeriodicArchiverTest {
PeriodicArchiver archiver;
String jmxUrl = "service:jmx:rmi://localhost:9091/jndi/rmi://fooHost:9091/jmxrmi";
ServiceRef serviceRef;
Credentials credentials = new Credentials("foouser", "barpassword");
@Mock CredentialsManager credentialsManager;
Rule rule =
new Rule.Builder()
.name("Test Rule")
Expand All @@ -95,7 +95,7 @@ void setup() throws Exception {
this.archiver =
new PeriodicArchiver(
serviceRef,
credentials,
credentialsManager,
rule,
webClient,
c -> headers,
Expand Down Expand Up @@ -141,6 +141,8 @@ public Void answer(InvocationOnMock invocation) throws Throwable {
MultiMap capturedHeaders = headersCaptor.getValue();
MatcherAssert.assertThat(capturedHeaders, Matchers.sameInstance(headers));

Mockito.verify(credentialsManager).getCredentials(serviceRef);

Mockito.verify(webClient)
.patch(
"/api/v1/targets/service:jmx:rmi:%2F%2Flocalhost:9091%2Fjndi%2Frmi:%2F%2FfooHost:9091%2Fjmxrmi/recordings/auto_Test_Rule");
Expand Down
14 changes: 6 additions & 8 deletions src/test/java/itest/AutoRulesIT.java
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,10 @@ class AutoRulesIT extends TestBase {
static final List<String> CONTAINERS = new ArrayList<>();
static final Map<String, String> NULL_RESULT = new HashMap<>();

final String jmxServiceUrl =
String.format("service:jmx:rmi:///jndi/rmi://%s:9093/jmxrmi", Podman.POD_NAME);
final String jmxServiceUrlEncoded = jmxServiceUrl.replaceAll("/", "%2F");

static {
NULL_RESULT.put("result", null);
}
Expand Down Expand Up @@ -193,13 +197,7 @@ void testAddCredentials() throws Exception {
form.add("username", "admin");
form.add("password", "adminpass123");
webClient
.post(
String.format(
"/api/v2/targets/%s/credentials",
String.format(
"service:jmx:rmi:///jndi/rmi://%s:9093/jmxrmi",
Podman.POD_NAME)
.replaceAll("/", "%2F")))
.post(String.format("/api/v2/targets/%s/credentials", jmxServiceUrlEncoded))
.sendForm(
form,
ar -> {
Expand Down Expand Up @@ -293,7 +291,7 @@ void testRuleCanBeDeleted() throws Exception {
void testCredentialsCanBeDeleted() throws Exception {
CompletableFuture<JsonObject> response = new CompletableFuture<>();
webClient
.delete(String.format("/api/v2/targets/%s/credentials", Podman.POD_NAME + ":9093"))
.delete(String.format("/api/v2/targets/%s/credentials", jmxServiceUrlEncoded))
.send(
ar -> {
if (assertRequestStatus(ar, response)) {
Expand Down

0 comments on commit d884cb6

Please sign in to comment.