From 1e94a689ce4f6d587a200268d71b7179ac3f7f76 Mon Sep 17 00:00:00 2001 From: Chloe Barker Date: Mon, 31 Jul 2023 10:59:50 -0700 Subject: [PATCH] Add tests --- cli/src/main/java/keywhiz/cli/Utilities.java | 2 +- .../keywhiz/cli/commands/AddActionTest.java | 33 ++++ .../java/keywhiz/client/KeywhizClient.java | 6 +- .../src/main/java/keywhiz/KeywhizConfig.java | 19 +- .../java/keywhiz/service/daos/SecretDAO.java | 10 +- .../test/java/keywhiz/KeywhizConfigTest.java | 32 +++- .../keywhiz/service/daos/SecretDAOTest.java | 24 +++ .../configs/with-reserved-prefixes.yaml | 162 ++++++++++++++++++ server/src/test/resources/keywhiz-test.yaml | 5 +- 9 files changed, 271 insertions(+), 22 deletions(-) create mode 100644 server/src/test/resources/configs/with-reserved-prefixes.yaml diff --git a/cli/src/main/java/keywhiz/cli/Utilities.java b/cli/src/main/java/keywhiz/cli/Utilities.java index 2e1e68958..d3b28fc20 100644 --- a/cli/src/main/java/keywhiz/cli/Utilities.java +++ b/cli/src/main/java/keywhiz/cli/Utilities.java @@ -17,7 +17,7 @@ public class Utilities { - public static final String VALID_NAME_PATTERN = "^[a-zA-Z_0-9\\-.:]+$"; + public static final String VALID_NAME_PATTERN = "^[a-zA-Z_0-9\\-.:/]+$"; public static boolean validName(String name) { // "." is allowed at any position but the first. diff --git a/cli/src/test/java/keywhiz/cli/commands/AddActionTest.java b/cli/src/test/java/keywhiz/cli/commands/AddActionTest.java index edb48694d..7f07ce2b2 100644 --- a/cli/src/test/java/keywhiz/cli/commands/AddActionTest.java +++ b/cli/src/test/java/keywhiz/cli/commands/AddActionTest.java @@ -66,6 +66,10 @@ public class AddActionTest { Secret secret = new Secret(15, "newSecret", null, null, () -> "c2VjcmV0MQ==", "checksum", NOW, null, NOW, null, null, null, ImmutableMap.of(), 0, 1L, NOW, null); + + Secret secretWithSpecialName = + new Secret(16, "sp:ns/owner/secret-name", null, null, () -> "c2VjcmV0MQ==", "checksum", NOW, null, NOW, null, + null, null, ImmutableMap.of(), 0, 1L, NOW, null); SanitizedSecret sanitizedSecret = SanitizedSecret.fromSecret(secret); SecretDetailResponse secretDetailResponse = SecretDetailResponse.fromSecret(secret, null, null); @@ -285,4 +289,33 @@ public void addValidatesSecretName() throws Exception { addAction.run(); } + + @Test(expected = IllegalArgumentException.class) + public void addValidatesSecretNameNoDollarSign() throws Exception { + addActionConfig.addType = Arrays.asList("secret"); + addActionConfig.name = "Invalid$Name"; + + addAction.run(); + } + + @Test + public void addValidatesSecretNameSpecialName() throws Exception { + addActionConfig.addType = Arrays.asList("secret"); + addActionConfig.name = secretWithSpecialName.getDisplayName(); + addActionConfig.expiry = "2006-01-02T15:04:05Z"; + + byte[] content = base64Decoder.decode(secretWithSpecialName.getSecret()); + addAction.stream = new ByteArrayInputStream(content); + when(keywhizClient.getSanitizedSecretByName(secretWithSpecialName.getName())) + .thenThrow(new NotFoundException()); // Call checks for existence. + + when( + keywhizClient.createSecret(secretWithSpecialName.getName(), null, "", content, + secretWithSpecialName.getMetadata(), 1136214245)) + .thenReturn(secretDetailResponse); + + addAction.run(); + verify(keywhizClient, times(1)).createSecret(secretWithSpecialName.getName(), null, "", content, + secretWithSpecialName.getMetadata(), 1136214245); + } } diff --git a/client/src/main/java/keywhiz/client/KeywhizClient.java b/client/src/main/java/keywhiz/client/KeywhizClient.java index b365a7293..4db9c15ad 100644 --- a/client/src/main/java/keywhiz/client/KeywhizClient.java +++ b/client/src/main/java/keywhiz/client/KeywhizClient.java @@ -210,7 +210,11 @@ public SecretDetailResponse createSecret( .metadata(metadata) .expiry(expiry) .build(); - String response = httpPost(baseUrl.resolve("/admin/secrets"), request); + HttpUrl url = baseUrl.newBuilder() + .addPathSegment("admin") + .addPathSegment("secrets") + .build(); + String response = httpPost(url, request); return mapper.readValue(response, SecretDetailResponse.class); } diff --git a/server/src/main/java/keywhiz/KeywhizConfig.java b/server/src/main/java/keywhiz/KeywhizConfig.java index 2836d8a4c..111361765 100644 --- a/server/src/main/java/keywhiz/KeywhizConfig.java +++ b/server/src/main/java/keywhiz/KeywhizConfig.java @@ -76,7 +76,7 @@ public class KeywhizConfig extends Configuration { @Nullable @JsonProperty - private Map ownerReservedPrefixes; + private Map reservedPrefixes; @NotNull @JsonProperty @@ -222,23 +222,20 @@ public String getDerivationProviderClass() { return backupExportKeys.get(name); } - @Nullable public String getOwnerRestriction(String secretName) { - if (ownerReservedPrefixes == null) { - return null; + public boolean canCreateSecretWithName(String secretName, String ownerName) { + if (reservedPrefixes == null) { + return true; } - String expectedOwner = null; - for (Map.Entry entry : ownerReservedPrefixes.entrySet()) { + for (Map.Entry entry : reservedPrefixes.entrySet()) { String owner = entry.getKey(); String prefix = entry.getValue(); if (secretName.startsWith(prefix)) { - if (expectedOwner != null) { - throw new RuntimeException("Multiple reserved owners for prefix"); - } else { - expectedOwner = owner; + if (ownerName == null || !ownerName.equals(owner)) { + return false; } } } - return expectedOwner; + return true; } public RowHmacCheck getRowHmacCheck() { diff --git a/server/src/main/java/keywhiz/service/daos/SecretDAO.java b/server/src/main/java/keywhiz/service/daos/SecretDAO.java index cefdcdf17..88bf3a63c 100644 --- a/server/src/main/java/keywhiz/service/daos/SecretDAO.java +++ b/server/src/main/java/keywhiz/service/daos/SecretDAO.java @@ -128,13 +128,9 @@ public long createSecret( + "names must be %d characters or less", name, SECRET_NAME_MAX_LENGTH)); } - String ownerRestriction = config.getOwnerRestriction(name); - if (ownerRestriction != null) { - if (!ownerName.equals(ownerRestriction)) { - throw new BadRequestException(format("secret cannot be created with name `%s` for owner " - + "`%s` - this name format is reserved by owner `%s`", name, ownerName, - ownerRestriction)); - } + if (!config.canCreateSecretWithName(name, ownerName)) { + throw new BadRequestException(format("secret cannot be created with name `%s` for owner " + + "`%s` - this name format is reserved", name, ownerName)); } long now = OffsetDateTime.now().toEpochSecond(); diff --git a/server/src/test/java/keywhiz/KeywhizConfigTest.java b/server/src/test/java/keywhiz/KeywhizConfigTest.java index e06f387f5..c7f4187b6 100644 --- a/server/src/test/java/keywhiz/KeywhizConfigTest.java +++ b/server/src/test/java/keywhiz/KeywhizConfigTest.java @@ -66,12 +66,42 @@ public void parseNewSecretOwnershipStrategyNone() { } @Test - public void parsNewSecretOwnershipStrategyInfer() { + public void parseNewSecretOwnershipStrategyInfer() { KeywhizConfig config = loadConfig("new-secret-ownership-strategy-infer.yaml"); assertThat(config.getNewSecretOwnershipStrategy()).isEqualTo( KeywhizConfig.NewSecretOwnershipStrategy.INFER_FROM_CLIENT); } + @Test + public void parseReservedPrefixes() { + KeywhizConfig config = loadConfig("with-reserved-prefixes.yaml"); + assertThat(config.canCreateSecretWithName("any-secret-name", "any-owner-name")).isTrue(); + + assertThat(config.canCreateSecretWithName("reserved-prefix", "any-owner-name")).isTrue(); + assertThat(config.canCreateSecretWithName("reserved-prefix", "reservedOwner")).isTrue(); + + assertThat(config.canCreateSecretWithName("reserved-prefix:", "any-owner-name")).isFalse(); + assertThat(config.canCreateSecretWithName("reserved-prefix:", "reservedOwner")).isTrue(); + + assertThat(config.canCreateSecretWithName("reserved-prefix:secretName", "any-owner-name")).isFalse(); + assertThat(config.canCreateSecretWithName("reserved-prefix:secretName", "reservedOwner")).isTrue(); + + assertThat(config.canCreateSecretWithName("extra-prefix-reserved-prefix:secretName", "any-owner-name")).isTrue(); + assertThat(config.canCreateSecretWithName("extra-prefix-reserved-prefix:secretName", "reservedOwner")).isTrue(); + + assertThat(config.canCreateSecretWithName("reserved-prefix:extra:secretName", "reservedOwner")).isFalse(); + assertThat(config.canCreateSecretWithName("reserved-prefix:extra:secretName", "anotherOwner")).isFalse(); + + assertThat(config.canCreateSecretWithName("ab", "reservedOwner")).isTrue(); + assertThat(config.canCreateSecretWithName("ab", "noColonInPrefix")).isTrue(); + + assertThat(config.canCreateSecretWithName("abc", "reservedOwner")).isFalse(); + assertThat(config.canCreateSecretWithName("abc", "noColonInPrefix")).isTrue(); + + assertThat(config.canCreateSecretWithName("abcdef", "reservedOwner")).isFalse(); + assertThat(config.canCreateSecretWithName("abcdef", "noColonInPrefix")).isTrue(); + } + private static KeywhizConfig loadConfig(String resource) { Validator validator = Validation.buildDefaultValidatorFactory().getValidator(); ObjectMapper objectMapper = createObjectMapper(); diff --git a/server/src/test/java/keywhiz/service/daos/SecretDAOTest.java b/server/src/test/java/keywhiz/service/daos/SecretDAOTest.java index d2a159800..bf0243a80 100644 --- a/server/src/test/java/keywhiz/service/daos/SecretDAOTest.java +++ b/server/src/test/java/keywhiz/service/daos/SecretDAOTest.java @@ -279,6 +279,30 @@ public void createOrUpdateExistingSecretUpdatesOwner() { assertThat(secretDAO.getSecrets(null, null, null,null, null)).containsOnly(secret1, secret2b, newSecret); } + @Test public void createSecretWithReservedPrefix() { + groupDAO.createGroup("specialOwner", "creator", "description", NO_METADATA); + String name = "sp:namespace/owner/secretName"; + String content = "c2VjcmV0MQ=="; + String hmac = cryptographer.computeHmac(content.getBytes(UTF_8), "hmackey"); + String encryptedContent = cryptographer.encryptionKeyDerivedFrom(name).encrypt(content); + long newId = secretDAO.createSecret(name, "specialOwner", encryptedContent, hmac, "creator", + ImmutableMap.of(), 0, "", null, ImmutableMap.of()); + SecretSeriesAndContent newSecret = secretDAO.getSecretById(newId).get(); + assertThat(newSecret).isNotNull(); + } + + @Test(expected = BadRequestException.class) + public void createSecretFailsIfPrefixReservedByDifferentOwner() { + groupDAO.createGroup("specialOwner", "creator", "description", NO_METADATA); + groupDAO.createGroup("regularOwner", "creator", "description", NO_METADATA); + String name = "sp:namespace/owner/secretName"; + String content = "c2VjcmV0MQ=="; + String hmac = cryptographer.computeHmac(content.getBytes(UTF_8), "hmackey"); + String encryptedContent = cryptographer.encryptionKeyDerivedFrom(name).encrypt(content); + secretDAO.createSecret(name, "regularOwner", encryptedContent, hmac, "creator", + ImmutableMap.of(), 0, "", null, ImmutableMap.of()); + } + @Test(expected = DataAccessException.class) public void createSecretFailsIfSecretExists() { String name = "newSecret"; diff --git a/server/src/test/resources/configs/with-reserved-prefixes.yaml b/server/src/test/resources/configs/with-reserved-prefixes.yaml new file mode 100644 index 000000000..726c72f30 --- /dev/null +++ b/server/src/test/resources/configs/with-reserved-prefixes.yaml @@ -0,0 +1,162 @@ +server: + applicationConnectors: + - type: resources-https + port: 4445 + keyStorePath: dev_and_test_keystore.p12 + keyStorePassword: ponies + keyStoreType: PKCS12 + trustStorePath: dev_and_test_truststore.p12 + trustStorePassword: ponies + trustStoreType: PKCS12 + wantClientAuth: true + validateCerts: true + validatePeers: true + crlPath: dev_and_test.crl + supportedProtocols: [TLSv1.2] + supportedCipherSuites: + - TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256 + - TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256 + - TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256 + - TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 + - TLS_RSA_WITH_AES_128_CBC_SHA256 + - TLS_RSA_WITH_AES_128_GCM_SHA256 + - TLS_ECDH_ECDSA_WITH_AES_128_CBC_SHA256 + - TLS_ECDH_RSA_WITH_AES_128_CBC_SHA256 + - TLS_DHE_RSA_WITH_AES_128_CBC_SHA256 + - TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA + - TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA + - TLS_RSA_WITH_AES_128_CBC_SHA + - TLS_ECDH_ECDSA_WITH_AES_128_CBC_SHA + - TLS_ECDH_RSA_WITH_AES_128_CBC_SHA + - TLS_DHE_RSA_WITH_AES_128_CBC_SHA + adminConnectors: + - type: http + bindHost: localhost + port: 8081 + +logging: + appenders: + - type: console + +environment: testing + +database: + driverClass: com.mysql.cj.jdbc.Driver + url: jdbc:mysql://address=(host=localhost)(%custom_mysql_port%)(useUnicode=true)(characterEncoding=utf8)/keywhizdb_test + user: root + properties: + charSet: UTF-8 + initialSize: 2 + minSize: 2 + maxSize: 2 + # There is explicitly no password. Do not uncomment. + # password: + +readonlyDatabase: + driverClass: com.mysql.cj.jdbc.Driver + url: jdbc:mysql://address=(host=localhost)(%custom_mysql_port%)(useUnicode=true)(characterEncoding=utf8)/keywhizdb_test + user: root + properties: + charSet: UTF-8 + readOnlyByDefault: true + initialSize: 1 + minSize: 1 + maxSize: 1 + # There is explicitly no password. Do not uncomment. + # password: + +migrationsDir: + db/mysql/migration + +statusCacheExpiry: PT3S + +backupExportKeys: + test: | + -----BEGIN PGP PUBLIC KEY BLOCK----- + + xsBNBFltUFoBCAC4aUBq1b6YYK65spHuVx+6FiQ9TiFMoiC4SpiyKH0oKsaa6uRz + EKzpBp0GoCIBhavBpmnzpNzdhuBrkAzK4543bxXEGGmjsbSV69ysgLBhTyrngOuS + diPVgaXIf47FpA/YoIlbyG1uQZFZ6bzJQL8gr8dbO5plFCaIUAFQhx88gNBmGgRk + rW5iU6nzlNzVRlkCAnK18YNv0h08nNRtXKvmLAnM6RSaVWsqDeisA/717dp1o4Hz + CofZGPdUkEoZkx2UekH9E7kzH90D2QmR+PdWtOz+5gtOMXgrpsJoh3fhwXVPo8dz + MT/5iLbReoM8TZVOLPsLyVJdd/oeV/5e6HvzABEBAAHNHlRlc3QgRXhwb3J0IDx0 + ZXN0QGV4YW1wbGUuY29tPsLAbQQTAQoAFwUCWW1QWgIbLwMLCQcDFQoIAh4BAheA + AAoJEEssmAxO8mrteUkH/jnisJDq64UKImwVJa7FF6Wjb82QbY1Sv+sXbw6o0Eog + tH5YBlow+TnaG6Vd1mk4qfWjOAViG8EbXhaEQSuRMZaX/PGh/2DnZqkKHIWkzVmm + R5FdfKBO1KukyTe1HLdC7lJLsuXFw9SwZ/CeP/zQpXJzI5j36Nzj82pKLZA58MSU + 3suJ4dTtUwvHA81yy/tgAIFHWayyJ4lV08QzR4c7Ey58rPqrtxzSZ5jgqTK3bmje + kxWnZ00vPLR3dyDSiYTnMS5f0LEb1nAevXaSyVT9c/0rYOmN41j5rCR6V6SsKZUq + uPJi4ROhQcRApKcC/wxRQitxqAa/RnkwkO0lMHGxdEPOwE0EWW1QWgEIAMRL6Ke+ + sdmWLBnt2/4O7WRQ45aeJjgO/X6kelHEulGlk4bqn+bumgZLnt/NLXJSh5ZSa1XM + mkZVml5zmLLAq1N56583WMlEofXadkBuxUkaP3NWsdbT/JCCzHDhgDK6pePK03ac + KTwahGMia8SZ41VqYMa9aAqPP3ywQKIBxyCvewoUM4YJTGLLY8LMxcZnyM1si9Ip + 3MnyvzXFMmZx9EzQiXePzc7zzdWp7HX63UkT2t567CDGe7NqHIef4GL/ENpSO2XT + PsJlzNZTlVxM6sggIsKDSrOCU5LzpM21Ql12L4hDE6gv0qxquI54cVZyJ8L9HfTf + pnZoSfVUYLqcQ2MAEQEAAcLBhAQYAQoADwUCWW1QWgUJDwmcAAIbLgEpCRBLLJgM + TvJq7cBdIAQZAQoABgUCWW1QWgAKCRCdJ0JDr9ocExyVCACOo/EZAyTEV1IM1gx3 + LuXp6Nmhk9OIJAg4EKVrUa4l5J5mFLeTLiIvHrChuuZUT3HvFa4aHesCiUY+kag7 + Z8SdLot4zRSB2JZQ0dVr/9N82+TEHkVAApq2UXd97O/EbYO3jmGAdfdHNjbmyABa + eEsx9YcIAKeIDrHv9idbM1gkvmQ9iwGznHzmQf/ubHE3f0OFczjYV/8lQmVePEbT + pXSEuJBvRbpVdSWnIjz1XqAxpkIG/4joBbVkoKRSCgX0Mxwrz+BXZM5LuEzeC9H7 + z3L+hfFOYjPZ8BYl8f/Eb5SSsLDoZdKZTN7Gg2YMRTB6xS9+r3Nnu/hFubV0MkGy + 7J9oymsIAJu/dfQlj5ZoyTn4h545J1eMDn0bVlyvGRxfgac2IsGe2ssPoXr0kaE7 + I2tLWZzDixnVs7a/7ES63GX4ONoul5T0pwVTtnebYaum7wbSx0vUEvNftzR9MnrJ + inl8ZKLlWdpg1R5iGCGsKIojo4uEqbM/DBXX5/F+xrX9MCbMmfPLHjKA0f/gB2IW + HpIWh0ibQnZy1y7ApaJnofIUZo5WIgB+wjVgkkE3zvxWNnoK+oNLQEOKyqYobZvo + 7KUyclo9qWVy3EEZTrhO8/XCjVKL3/Tl7Wy3kf+6058tCeQbH6DgpzsaP+XwJ8tR + waxiKBbHE6eyC11mrir2QI4RXT2P7ETOwE0EWW1QWgEIAN0vGNm0aNRD1OrB0DUp + 5ltGRgTzuTA05Rd3RXZ4OnZsxLQjARSzFREQsIgehHgBS6anHMzitL3eLRjZM+8x + 1vCRNT3vhNB2ljf4LdvcsQmAzP9Upe5BzVT+ywrIE6JjKCcg3RVWNJYBYRSq6VwS + MRN8GfrUvUGSCNr97t5W0mt1vQH7Y2BcaER/YYMQ86gqbB3j5M8WQseSHKdYbTh8 + 59QiPQuNR6GGrAvp6HaZWRPH7XZt4IiDYiBet66fHIm6YLjO0Ds+Yad4aYZwLitE + gHHokJUjLm3fqL05FJDSiE3KaBT07tXANH2flqIe8wbGTUCmj7DuwfNuGiXzTnO4 + xZ8AEQEAAcLBhAQYAQoADwUCWW1QWgUJDwmcAAIbLgEpCRBLLJgMTvJq7cBdIAQZ + AQoABgUCWW1QWgAKCRCv6D9VTBZojbe/B/4tp6Fd69/j5wksKfSAxAbIhRNBNj3e + Zyhis1CZc0Xgw4ACqJbq/e+nqEbumV11WJOT5DrqTOZkw6u7ey8FvU7tngCMK1eP + 0vvdiFDR14mhoWo2kdOkYWe5WpDnGfQXRjjfRMg/Pf/dp0sUfAvmutSS1yHsGpE8 + SDLuIRKO9GLTJs4UCopEk53iMHmTebOqC6wokQjSVedDkVhaX5QTvhHIYwU4dTik + OxjIhqn/k1m3rexJIT2SPQeL2eD0fhQxrxND835vIE7NCEvVpni4Mi5sFq0288Rb + +UaUERS9Lssp2NuJSoaLASB0WH+AMhGvneSAQKr5LLnE0jjyv0vZ6Q3FlMkH/Awl + HoP7mUOr3uw/nJOq47l7V/paM1dQgmD2+PvTyxzB5PnclHraJBS91bdUGIUxqula + C9ugQY+/SxCc5EwaleyOZNC2onXqLV4kPYWCYFAS5atMMTQA96TkkXVP4gaAO1hF + NvrndG70YWkoB8ArCKfNEx/CR4msZBBvUKntjKQNiOXI5Y4wzJIZzoBCQ2XDnIid + M+L/BZ5d0PhHRL3WCdSKd/l3/SPRrkaSKp5Ii3G7AH4CVH51rTWi4MFOBYy/LopA + pjVkvUV7XB8oxsBAkjW4DRPFs8EpxvjRNCWFtGXqRYQ5LqGMgi2vnwG8py3uRvsd + EuOSrLrAPaUbk42SHyg= + =bG4m + -----END PGP PUBLIC KEY BLOCK----- + +userAuth: + type: bcrypt + +# Base64 of "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA" +cookieKey: QUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUFBQUE= + +sessionCookie: + name: session + path: /admin + +contentKeyStore: + path: derivation.jceks + type: JCEKS + password: CHANGE + alias: basekey + +rowHmacCheck: enforced + +flywaySchemaTable: schema_version + +clientAuthConfig: + xfcc: + - port: 4446 + allowedClientNames: [] + allowedSpiffeIds: [] + type: + useCommonName: true + useSpiffeId: true + createMissingClients: true + +reservedPrefixes: + reservedOwner: "reserved-prefix:" + anotherOwner: "reserved-prefix:extra:" + noColonInPrefix: "abc" \ No newline at end of file diff --git a/server/src/test/resources/keywhiz-test.yaml b/server/src/test/resources/keywhiz-test.yaml index 743e6bd12..a551ab546 100644 --- a/server/src/test/resources/keywhiz-test.yaml +++ b/server/src/test/resources/keywhiz-test.yaml @@ -160,4 +160,7 @@ clientAuthConfig: type: useCommonName: true useSpiffeId: true - createMissingClients: true \ No newline at end of file + createMissingClients: true + +reservedPrefixes: + specialOwner: "sp:" \ No newline at end of file