From 554221b89cf53100ea0fc12161409efccc489fb2 Mon Sep 17 00:00:00 2001 From: Justin Cummins Date: Wed, 10 Jun 2015 16:38:49 -0700 Subject: [PATCH] Block bad requests sooner with preconditions Adds precondition checks to KeywhizClient, to block bad requests sooner. Move base64'ing content into KeywhizClient instead of CLI. --- api/pom.xml | 5 ++ .../keywhiz/api/CreateClientRequestTest.java | 31 +++++++++ .../java/keywhiz/cli/commands/AddAction.java | 16 +++-- .../keywhiz/cli/commands/AddActionTest.java | 68 +++++++++---------- .../java/keywhiz/client/KeywhizClient.java | 27 ++++++-- .../ClientsResourceIntegrationTest.java | 6 -- .../SecretsResourceIntegrationTest.java | 12 ++-- 7 files changed, 105 insertions(+), 60 deletions(-) diff --git a/api/pom.xml b/api/pom.xml index d350b8136..bbc45dc75 100644 --- a/api/pom.xml +++ b/api/pom.xml @@ -53,6 +53,11 @@ dropwizard-testing test + + javax.inject + javax.inject + test + com.squareup.keywhiz keywhiz-testing diff --git a/api/src/test/java/keywhiz/api/CreateClientRequestTest.java b/api/src/test/java/keywhiz/api/CreateClientRequestTest.java index ae106f3b9..a34a6d322 100644 --- a/api/src/test/java/keywhiz/api/CreateClientRequestTest.java +++ b/api/src/test/java/keywhiz/api/CreateClientRequestTest.java @@ -16,17 +16,48 @@ package keywhiz.api; +import io.dropwizard.testing.junit.ResourceTestRule; +import javax.validation.ConstraintViolationException; +import javax.validation.Valid; +import javax.ws.rs.Consumes; +import javax.ws.rs.POST; +import javax.ws.rs.Path; +import javax.ws.rs.ProcessingException; +import org.junit.ClassRule; import org.junit.Test; +import static javax.ws.rs.client.Entity.entity; import static keywhiz.testing.JsonHelpers.fromJson; import static keywhiz.testing.JsonHelpers.jsonFixture; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.catchThrowable; public class CreateClientRequestTest { + @ClassRule public static final ResourceTestRule resources = ResourceTestRule.builder() + .addResource(new Resource()) + .build(); + @Test public void deserializesCorrectly() throws Exception { CreateClientRequest createClientRequest = new CreateClientRequest("client-name"); assertThat(fromJson( jsonFixture("fixtures/createClientRequest.json"), CreateClientRequest.class)) .isEqualTo(createClientRequest); } + + @Test public void emptyNameFailsValidation() throws Exception { + CreateClientRequest createClientRequest = new CreateClientRequest(""); + Throwable exception = catchThrowable(() -> + resources.client().target("/").request() + .post(entity(createClientRequest, "application/json"))); + + assertThat(exception) + .isInstanceOf(ProcessingException.class) + .hasCauseInstanceOf(ConstraintViolationException.class); + } + + @Path("/") private static class Resource { + @POST @Consumes("application/json") public String method(@Valid CreateClientRequest request) { + throw new UnsupportedOperationException(); + } + } } diff --git a/cli/src/main/java/keywhiz/cli/commands/AddAction.java b/cli/src/main/java/keywhiz/cli/commands/AddAction.java index b2137e2a4..cc775f251 100644 --- a/cli/src/main/java/keywhiz/cli/commands/AddAction.java +++ b/cli/src/main/java/keywhiz/cli/commands/AddAction.java @@ -18,14 +18,12 @@ import com.fasterxml.jackson.core.type.TypeReference; import com.fasterxml.jackson.databind.ObjectMapper; -import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Throwables; import com.google.common.collect.ImmutableMap; import com.google.common.io.ByteStreams; import java.io.IOException; import java.io.InputStream; import java.text.ParseException; -import java.util.Base64; import java.util.List; import keywhiz.api.SecretDetailResponse; import keywhiz.api.model.Group; @@ -104,7 +102,7 @@ public AddAction(AddActionConfig addActionConfig, KeywhizClient client, ObjectMa throw Throwables.propagate(e); } String secretName = parts[0]; - String content = readSecretContent(); + byte[] content = readSecretContent(); ImmutableMap metadata = getMetadata(); String version = getVersion(parts); @@ -135,7 +133,7 @@ public AddAction(AddActionConfig addActionConfig, KeywhizClient client, ObjectMa } } - private void createAndAssignSecret(String secretName, String content, boolean useVersion, + private void createAndAssignSecret(String secretName, byte[] content, boolean useVersion, String version, ImmutableMap metadata) { try { SecretDetailResponse secretResponse = keywhizClient.createSecret(secretName, "", content, @@ -178,11 +176,15 @@ private String getVersion(String[] parts) { return version; } - @VisibleForTesting String readSecretContent() { + private byte[] readSecretContent() { try { - return Base64.getEncoder().encodeToString(ByteStreams.toByteArray(stream)); + byte[] content = ByteStreams.toByteArray(stream); + if (content.length == 0) { + throw new RuntimeException("Secret content empty!"); + } + return content; } catch (IOException e) { - throw new AssertionError(e); + throw Throwables.propagate(e); } } diff --git a/cli/src/test/java/keywhiz/cli/commands/AddActionTest.java b/cli/src/test/java/keywhiz/cli/commands/AddActionTest.java index ff4e25b6a..c6752ba54 100644 --- a/cli/src/test/java/keywhiz/cli/commands/AddActionTest.java +++ b/cli/src/test/java/keywhiz/cli/commands/AddActionTest.java @@ -72,12 +72,10 @@ public void addCallsAddForGroup() throws Exception { addActionConfig.addType = Arrays.asList("group"); addActionConfig.name = group.getName(); - when(keywhizClient.getGroupByName(group.getName())).thenThrow( - new NotFoundException()); + when(keywhizClient.getGroupByName(group.getName())).thenThrow(new NotFoundException()); addAction.run(); - verify(keywhizClient) - .createGroup(addActionConfig.name, null); + verify(keywhizClient).createGroup(addActionConfig.name, null); } @Test @@ -85,17 +83,17 @@ public void addCallsAddForSecret() throws Exception { addActionConfig.addType = Arrays.asList("secret"); addActionConfig.name = secret.getDisplayName(); - addAction.stream = new ByteArrayInputStream(base64Decoder.decode(secret.getSecret())); + byte[] content = base64Decoder.decode(secret.getSecret()); + addAction.stream = new ByteArrayInputStream(content); when(keywhizClient.getSanitizedSecretByNameAndVersion(secret.getName(), secret.getVersion())) .thenThrow(new NotFoundException()); // Call checks for existence. - when(keywhizClient.createSecret(secret.getName(), "", secret.getSecret(), - true, secret.getMetadata())) + when(keywhizClient.createSecret(secret.getName(), "", content, true, secret.getMetadata())) .thenReturn(secretDetailResponse); addAction.run(); - verify(keywhizClient, times(1)).createSecret(secret.getName(), "", secret.getSecret(), - true, secret.getMetadata()); + verify(keywhizClient, times(1)) + .createSecret(secret.getName(), "", content, true, secret.getMetadata()); } @Test @@ -103,12 +101,10 @@ public void addCallsAddForClient() throws Exception { addActionConfig.addType = Arrays.asList("client"); addActionConfig.name = client.getName(); - when(keywhizClient.getClientByName(client.getName())).thenThrow( - new NotFoundException()); + when(keywhizClient.getClientByName(client.getName())).thenThrow(new NotFoundException()); addAction.run(); - verify(keywhizClient) - .createClient(addActionConfig.name); + verify(keywhizClient).createClient(addActionConfig.name); } @Test @@ -117,14 +113,15 @@ public void addSecretCanAssignGroup() throws Exception { addActionConfig.name = secret.getDisplayName(); addActionConfig.group = group.getName(); - addAction.stream = new ByteArrayInputStream(base64Decoder.decode(secret.getSecret())); + byte[] content = base64Decoder.decode(secret.getSecret()); + addAction.stream = new ByteArrayInputStream(content); when(keywhizClient.getGroupByName(group.getName())).thenReturn(group); when(keywhizClient.getSanitizedSecretByNameAndVersion(secret.getName(), secret.getVersion())) .thenThrow(new NotFoundException()); // Call checks for existence. - when(keywhizClient.createSecret(secret.getName(), "", secret.getSecret(), - true, secret.getMetadata())).thenReturn(secretDetailResponse); + when(keywhizClient.createSecret(secret.getName(), "", content, true, secret.getMetadata())) + .thenReturn(secretDetailResponse); addAction.run(); verify(keywhizClient).grantSecretToGroupByIds((int) secret.getId(), (int) group.getId()); @@ -135,19 +132,18 @@ public void addCreatesWithoutVersionByDefault() throws Exception { addActionConfig.addType = Arrays.asList("secret"); addActionConfig.name = secret.getName(); // Name without version - addAction.stream = new ByteArrayInputStream(base64Decoder.decode(secret.getSecret())); + byte[] content = base64Decoder.decode(secret.getSecret()); + addAction.stream = new ByteArrayInputStream(content); when(keywhizClient.getSanitizedSecretByNameAndVersion(secret.getName(), "")) .thenThrow(new NotFoundException()); // Call checks for existence. - when(keywhizClient.createSecret(secret.getName(), "", secret.getSecret(), - false, secret.getMetadata())) + when(keywhizClient.createSecret(secret.getName(), "", content, false, secret.getMetadata())) .thenReturn(secretDetailResponse); addAction.run(); - verify(keywhizClient, never()).createSecret(secret.getName(), "", secret.getSecret(), - true, secret.getMetadata()); + verify(keywhizClient, never()).createSecret(secret.getName(), "", content, true, secret.getMetadata()); } @Test @@ -155,18 +151,18 @@ public void addCreatesVersionedSecretWhenVersionInName() throws Exception { addActionConfig.addType = Arrays.asList("secret"); addActionConfig.name = secret.getDisplayName(); // Name includes version, e.g. newSecret..df97a - addAction.stream = new ByteArrayInputStream(base64Decoder.decode(secret.getSecret())); + byte[] content = base64Decoder.decode(secret.getSecret()); + addAction.stream = new ByteArrayInputStream(content); when(keywhizClient.getSanitizedSecretByNameAndVersion(secret.getName(), secret.getVersion())) .thenThrow(new NotFoundException()); // Call checks for existence. - when(keywhizClient.createSecret(secret.getName(), "", secret.getSecret(), - true, secret.getMetadata())) + when(keywhizClient.createSecret(secret.getName(), "", content, true, secret.getMetadata())) .thenReturn(secretDetailResponse); addAction.run(); - verify(keywhizClient, times(1)).createSecret(secret.getName(), "", secret.getSecret(), - true, secret.getMetadata()); + verify(keywhizClient, times(1)) + .createSecret(secret.getName(), "", content, true, secret.getMetadata()); } @Test @@ -175,18 +171,19 @@ public void addCanCreateWithVersion() throws Exception { addActionConfig.name = secret.getDisplayName(); addActionConfig.withVersion = true; - addAction.stream = new ByteArrayInputStream(base64Decoder.decode(secret.getSecret())); + byte[] content = base64Decoder.decode(secret.getSecret()); + addAction.stream = new ByteArrayInputStream(content); when(keywhizClient.getSanitizedSecretByNameAndVersion(secret.getName(), secret.getVersion())) .thenThrow(new NotFoundException()); // Call checks for existence. - when(keywhizClient.createSecret(secret.getName(), "", secret.getSecret(), - addActionConfig.withVersion, secret.getMetadata())) + when(keywhizClient.createSecret(secret.getName(), "", content, addActionConfig.withVersion, + secret.getMetadata())) .thenReturn(secretDetailResponse); addAction.run(); - verify(keywhizClient, times(1)).createSecret(secret.getName(), "", secret.getSecret(), - true, secret.getMetadata()); + verify(keywhizClient, times(1)) + .createSecret(secret.getName(), "", content, true, secret.getMetadata()); } @Test @@ -195,20 +192,19 @@ public void addWithMetadata() throws Exception { addActionConfig.name = secret.getDisplayName(); addActionConfig.json = "{\"owner\":\"example-name\", \"group\":\"example-group\"}"; - addAction.stream = new ByteArrayInputStream(base64Decoder.decode(secret.getSecret())); + byte[] content = base64Decoder.decode(secret.getSecret()); + addAction.stream = new ByteArrayInputStream(content); when(keywhizClient.getSanitizedSecretByNameAndVersion(secret.getName(), secret.getVersion())) .thenThrow(new NotFoundException()); // Call checks for existence. ImmutableMap expected = ImmutableMap.of("owner", "example-name", "group", "example-group"); - when(keywhizClient.createSecret(secret.getName(), "", secret.getSecret(), - true, expected)) + when(keywhizClient.createSecret(secret.getName(), "", content, true, expected)) .thenReturn(secretDetailResponse); addAction.run(); - verify(keywhizClient, times(1)).createSecret(secret.getName(), "", secret.getSecret(), - true, expected); + verify(keywhizClient, times(1)).createSecret(secret.getName(), "", content, true, expected); } @Test(expected = IllegalArgumentException.class) diff --git a/client/src/main/java/keywhiz/client/KeywhizClient.java b/client/src/main/java/keywhiz/client/KeywhizClient.java index bf5e97e94..22730b8b0 100644 --- a/client/src/main/java/keywhiz/client/KeywhizClient.java +++ b/client/src/main/java/keywhiz/client/KeywhizClient.java @@ -27,6 +27,7 @@ import com.squareup.okhttp.RequestBody; import com.squareup.okhttp.Response; import java.io.IOException; +import java.util.Base64; import java.util.List; import javax.ws.rs.core.HttpHeaders; import keywhiz.api.ClientDetailResponse; @@ -41,6 +42,7 @@ import keywhiz.api.model.SanitizedSecret; import org.apache.http.HttpStatus; +import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Preconditions.checkNotNull; import static java.lang.String.format; @@ -123,6 +125,7 @@ public List allGroups() throws IOException { } public GroupDetailResponse createGroup(String name, String description) throws IOException { + checkArgument(!name.isEmpty()); String response = httpPost(baseUrl.resolve("/admin/groups"), new CreateGroupRequest(name, description)); return mapper.readValue(response, GroupDetailResponse.class); } @@ -141,9 +144,13 @@ public List allSecrets() throws IOException { return mapper.readValue(response, new TypeReference>() {}); } - public SecretDetailResponse createSecret(String name, String description, String content, boolean withVersion, + public SecretDetailResponse createSecret(String name, String description, byte[] content, boolean withVersion, ImmutableMap metadata) throws IOException { - CreateSecretRequest request = new CreateSecretRequest(name, description, content, + checkArgument(!name.isEmpty()); + checkArgument(content.length > 0, "Content must not be empty"); + + String b64Content = Base64.getEncoder().encodeToString(content); + CreateSecretRequest request = new CreateSecretRequest(name, description, b64Content, withVersion, metadata); String response = httpPost(baseUrl.resolve("/admin/secrets"), request); return mapper.readValue(response, SecretDetailResponse.class); @@ -159,13 +166,17 @@ public void deleteSecretWithId(int secretId) throws IOException { } public List generateSecrets(String generatorName, T params) throws IOException { - String response = httpPost(baseUrl.resolve(format("/admin/secrets/generators/%s", generatorName)), + checkArgument(!generatorName.isEmpty()); + String response = httpPost(baseUrl.resolve( + format("/admin/secrets/generators/%s", generatorName)), params); return mapper.readValue(response, new TypeReference>() {}); } public List batchGenerateSecrets(String generatorName, List params) throws IOException { - String response = httpPost(baseUrl.resolve(format("/admin/secrets/generators/%s/batch", generatorName)), + checkArgument(!generatorName.isEmpty()); + String response = httpPost(baseUrl.resolve( + format("/admin/secrets/generators/%s/batch", generatorName)), params); return mapper.readValue(response, new TypeReference>() {}); } @@ -176,6 +187,7 @@ public List allClients() throws IOException { } public ClientDetailResponse createClient(String name) throws IOException { + checkArgument(!name.isEmpty()); String response = httpPost(baseUrl.resolve("/admin/clients"), new CreateClientRequest(name)); return mapper.readValue(response, ClientDetailResponse.class); } @@ -206,21 +218,26 @@ public void revokeSecretFromGroupByIds(int secretId, int groupId) throws IOExcep } public Client getClientByName(String name) throws IOException { + checkArgument(!name.isEmpty()); String response = httpGet(baseUrl.resolve(format("/admin/clients?name=%s", name))); return mapper.readValue(response, Client.class); } public Group getGroupByName(String name) throws IOException { + checkArgument(!name.isEmpty()); String response = httpGet(baseUrl.resolve(format("/admin/groups?name=%s", name))); return mapper.readValue(response, Group.class); } public SanitizedSecret getSanitizedSecretByNameAndVersion(String name, String version) throws IOException { - String response = httpGet(baseUrl.resolve(format("/admin/secrets?name=%s&version=%s", name, version))); + checkArgument(!name.isEmpty()); + String response = httpGet(baseUrl.resolve( + format("/admin/secrets?name=%s&version=%s", name, version))); return mapper.readValue(response, SanitizedSecret.class); } public List getVersionsForSecretName(String name) throws IOException { + checkNotNull(name); String response = httpGet(baseUrl.resolve(format("/admin/secrets/versions?name=%s", name))); return mapper.readValue(response, new TypeReference>() {}); } diff --git a/server/src/test/java/keywhiz/service/resources/ClientsResourceIntegrationTest.java b/server/src/test/java/keywhiz/service/resources/ClientsResourceIntegrationTest.java index 2dc81eeb0..8d75c29f7 100644 --- a/server/src/test/java/keywhiz/service/resources/ClientsResourceIntegrationTest.java +++ b/server/src/test/java/keywhiz/service/resources/ClientsResourceIntegrationTest.java @@ -99,12 +99,6 @@ public void createDuplicateClients() throws IOException { keywhizClient.createClient("varys"); } - @Test(expected = KeywhizClient.ValidationException.class) - public void creatingClientWithEmptyNameFailsOnValidation() throws IOException { - keywhizClient.login(DbSeedCommand.defaultUser, DbSeedCommand.defaultPassword.toCharArray()); - keywhizClient.createClient(""); - } - @Test(expected = KeywhizClient.NotFoundException.class) public void notFoundOnMissingId() throws IOException { keywhizClient.login(DbSeedCommand.defaultUser, DbSeedCommand.defaultPassword.toCharArray()); diff --git a/server/src/test/java/keywhiz/service/resources/SecretsResourceIntegrationTest.java b/server/src/test/java/keywhiz/service/resources/SecretsResourceIntegrationTest.java index 2d7c63322..f8d840d08 100644 --- a/server/src/test/java/keywhiz/service/resources/SecretsResourceIntegrationTest.java +++ b/server/src/test/java/keywhiz/service/resources/SecretsResourceIntegrationTest.java @@ -74,8 +74,8 @@ public void adminRejectsWithoutCookie() throws IOException { @Test public void createsSecret() throws IOException { keywhizClient.login(DbSeedCommand.defaultUser, DbSeedCommand.defaultPassword.toCharArray()); - SecretDetailResponse secretDetails = keywhizClient.createSecret("newSecret", "", "content", - false, ImmutableMap.of()); + SecretDetailResponse secretDetails = keywhizClient.createSecret("newSecret", "", + "content".getBytes(), false, ImmutableMap.of()); assertThat(secretDetails.name).isEqualTo("newSecret"); assertThat(keywhizClient.allSecrets().stream().map(SanitizedSecret::name).toArray()) @@ -85,7 +85,7 @@ public void adminRejectsWithoutCookie() throws IOException { @Test public void createsDuplicateSecretWithVersion() throws IOException { keywhizClient.login(DbSeedCommand.defaultUser, DbSeedCommand.defaultPassword.toCharArray()); - keywhizClient.createSecret("trapdoor", "v1", "content", true, ImmutableMap.of()); + keywhizClient.createSecret("trapdoor", "v1", "content".getBytes(), true, ImmutableMap.of()); List sanitizedSecrets1 = keywhizClient.allSecrets(); @@ -99,7 +99,7 @@ public void adminRejectsWithoutCookie() throws IOException { } assertThat(found1).isTrue(); - keywhizClient.createSecret("trapdoor", "v2", "content", true, ImmutableMap.of()); + keywhizClient.createSecret("trapdoor", "v2", "content".getBytes(), true, ImmutableMap.of()); List sanitizedSecrets2 = keywhizClient.allSecrets(); @@ -118,8 +118,8 @@ public void adminRejectsWithoutCookie() throws IOException { public void rejectsCreatingDuplicateSecretWithoutVersion() throws IOException { keywhizClient.login(DbSeedCommand.defaultUser, DbSeedCommand.defaultPassword.toCharArray()); - keywhizClient.createSecret("passage", "v1", "content", false, ImmutableMap.of()); - keywhizClient.createSecret("passage", "v2", "content", false, ImmutableMap.of()); + keywhizClient.createSecret("passage", "v1", "content".getBytes(), false, ImmutableMap.of()); + keywhizClient.createSecret("passage", "v2", "content".getBytes(), false, ImmutableMap.of()); } @Test public void deletesSecret() throws IOException {