From 58523704c5f6a6aaf1a5b186f8aa10eaf5f5c8a6 Mon Sep 17 00:00:00 2001 From: Andrew Azores Date: Wed, 16 Jun 2021 22:12:03 -0400 Subject: [PATCH] [autorules] Credentials deletion enhancements (#516) * 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 --- rulestest.sh | 8 +- .../configuration/CredentialsManager.java | 10 +- .../v2/TargetCredentialsDeleteHandler.java | 4 +- .../io/cryostat/rules/PeriodicArchiver.java | 11 +- .../rules/PeriodicArchiverFactory.java | 11 +- .../java/io/cryostat/rules/RuleProcessor.java | 2 +- .../TargetCredentialsDeleteHandlerTest.java | 163 ++++++++++++++++++ .../cryostat/rules/PeriodicArchiverTest.java | 8 +- src/test/java/itest/AutoRulesIT.java | 14 +- 9 files changed, 207 insertions(+), 24 deletions(-) create mode 100644 src/test/java/io/cryostat/net/web/http/api/v2/TargetCredentialsDeleteHandlerTest.java diff --git a/rulestest.sh b/rulestest.sh index 343503849a..f1561bf6d6 100755 --- a/rulestest.sh +++ b/rulestest.sh @@ -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" @@ -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" diff --git a/src/main/java/io/cryostat/configuration/CredentialsManager.java b/src/main/java/io/cryostat/configuration/CredentialsManager.java index 1579e058cd..5ef799fb62 100644 --- a/src/main/java/io/cryostat/configuration/CredentialsManager.java +++ b/src/main/java/io/cryostat/configuration/CredentialsManager.java @@ -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; @@ -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())); } diff --git a/src/main/java/io/cryostat/net/web/http/api/v2/TargetCredentialsDeleteHandler.java b/src/main/java/io/cryostat/net/web/http/api/v2/TargetCredentialsDeleteHandler.java index 57fb52e120..aaa455c2f7 100644 --- a/src/main/java/io/cryostat/net/web/http/api/v2/TargetCredentialsDeleteHandler.java +++ b/src/main/java/io/cryostat/net/web/http/api/v2/TargetCredentialsDeleteHandler.java @@ -101,11 +101,11 @@ public boolean isOrdered() { public IntermediateResponse 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().statusCode(status ? 200 : 404); } catch (IOException e) { throw new ApiException( 500, "IOException occurred while clearing persisted credentials", e); } - return new IntermediateResponse().body(null); } } diff --git a/src/main/java/io/cryostat/rules/PeriodicArchiver.java b/src/main/java/io/cryostat/rules/PeriodicArchiver.java index 8c189e9b44..27ad9527dd 100644 --- a/src/main/java/io/cryostat/rules/PeriodicArchiver.java +++ b/src/main/java/io/cryostat/rules/PeriodicArchiver.java @@ -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; @@ -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 headersFactory; @@ -72,7 +73,7 @@ class PeriodicArchiver implements Runnable { PeriodicArchiver( ServiceRef serviceRef, - Credentials credentials, + CredentialsManager credentialsManager, Rule rule, WebClient webClient, Function headersFactory, @@ -80,7 +81,7 @@ class PeriodicArchiver implements Runnable { Logger logger) { this.webClient = webClient; this.serviceRef = serviceRef; - this.credentials = credentials; + this.credentialsManager = credentialsManager; this.rule = rule; this.headersFactory = headersFactory; this.failureNotifier = failureNotifier; @@ -129,7 +130,7 @@ void performArchival() throws InterruptedException, ExecutionException { CompletableFuture future = new CompletableFuture<>(); this.webClient .patch(path) - .putHeaders(headersFactory.apply(credentials)) + .putHeaders(headersFactory.apply(credentialsManager.getCredentials(serviceRef))) .sendBuffer( Buffer.buffer("save"), ar -> { @@ -163,7 +164,7 @@ Future pruneArchive(String recordingName) { CompletableFuture future = new CompletableFuture<>(); this.webClient .delete(path) - .putHeaders(headersFactory.apply(credentials)) + .putHeaders(headersFactory.apply(credentialsManager.getCredentials(serviceRef))) .send( ar -> { if (ar.failed()) { diff --git a/src/main/java/io/cryostat/rules/PeriodicArchiverFactory.java b/src/main/java/io/cryostat/rules/PeriodicArchiverFactory.java index 201e08f8e1..2da6554be2 100644 --- a/src/main/java/io/cryostat/rules/PeriodicArchiverFactory.java +++ b/src/main/java/io/cryostat/rules/PeriodicArchiverFactory.java @@ -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; @@ -62,10 +63,16 @@ class PeriodicArchiverFactory { PeriodicArchiver create( ServiceRef serviceRef, - Credentials credentials, + CredentialsManager credentialsManager, Rule rule, Function, Void> failureNotifier) { return new PeriodicArchiver( - serviceRef, credentials, rule, webClient, headersFactory, failureNotifier, logger); + serviceRef, + credentialsManager, + rule, + webClient, + headersFactory, + failureNotifier, + logger); } } diff --git a/src/main/java/io/cryostat/rules/RuleProcessor.java b/src/main/java/io/cryostat/rules/RuleProcessor.java index af3b1b5ae9..4082bc7e22 100644 --- a/src/main/java/io/cryostat/rules/RuleProcessor.java +++ b/src/main/java/io/cryostat/rules/RuleProcessor.java @@ -200,7 +200,7 @@ public synchronized void accept(TargetDiscoveryEvent tde) { scheduler.scheduleAtFixedRate( periodicArchiverFactory.create( tde.getServiceRef(), - credentials, + credentialsManager, rule, this::archivalFailureHandler), rule.getArchivalPeriodSeconds(), diff --git a/src/test/java/io/cryostat/net/web/http/api/v2/TargetCredentialsDeleteHandlerTest.java b/src/test/java/io/cryostat/net/web/http/api/v2/TargetCredentialsDeleteHandlerTest.java new file mode 100644 index 0000000000..d95c4583ef --- /dev/null +++ b/src/test/java/io/cryostat/net/web/http/api/v2/TargetCredentialsDeleteHandlerTest.java @@ -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 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 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 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); + } + } +} diff --git a/src/test/java/io/cryostat/rules/PeriodicArchiverTest.java b/src/test/java/io/cryostat/rules/PeriodicArchiverTest.java index 2ef04ac6cf..9da06afe6e 100644 --- a/src/test/java/io/cryostat/rules/PeriodicArchiverTest.java +++ b/src/test/java/io/cryostat/rules/PeriodicArchiverTest.java @@ -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; @@ -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") @@ -95,7 +95,7 @@ void setup() throws Exception { this.archiver = new PeriodicArchiver( serviceRef, - credentials, + credentialsManager, rule, webClient, c -> headers, @@ -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"); diff --git a/src/test/java/itest/AutoRulesIT.java b/src/test/java/itest/AutoRulesIT.java index 78211ba79c..d206bf2934 100644 --- a/src/test/java/itest/AutoRulesIT.java +++ b/src/test/java/itest/AutoRulesIT.java @@ -65,6 +65,10 @@ class AutoRulesIT extends TestBase { static final List CONTAINERS = new ArrayList<>(); static final Map 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); } @@ -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 -> { @@ -293,7 +291,7 @@ void testRuleCanBeDeleted() throws Exception { void testCredentialsCanBeDeleted() throws Exception { CompletableFuture 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)) {