From 1c10efc60f1619785dfa46227e3af372e6294b05 Mon Sep 17 00:00:00 2001 From: Ryan Ernst Date: Sun, 10 Jul 2016 14:47:48 -0700 Subject: [PATCH] Internal: Simplify CryptoService api The CryptoService currently has a lot of variations of methods that are unused. It really only uses sign/unsign, encrypt/decrypt. This change trims the api down to those needed methods. Original commit: elastic/x-pack-elasticsearch@92e83efeb7b896e3845d01b400a25d3694b07fbf --- .../xpack/security/SecurityFeatureSet.java | 2 +- .../action/filter/SecurityActionFilter.java | 2 +- .../xpack/security/crypto/CryptoService.java | 61 +----------- .../crypto/InternalCryptoService.java | 85 ++--------------- .../integration/ScrollIdSigningTests.java | 2 +- .../security/SecurityFeatureSetTests.java | 4 +- .../filter/SecurityActionFilterTests.java | 4 +- .../crypto/InternalCryptoServiceTests.java | 92 +++---------------- 8 files changed, 28 insertions(+), 224 deletions(-) diff --git a/elasticsearch/x-pack/security/src/main/java/org/elasticsearch/xpack/security/SecurityFeatureSet.java b/elasticsearch/x-pack/security/src/main/java/org/elasticsearch/xpack/security/SecurityFeatureSet.java index e5fa6f9f335..8ebc8a6a6d4 100644 --- a/elasticsearch/x-pack/security/src/main/java/org/elasticsearch/xpack/security/SecurityFeatureSet.java +++ b/elasticsearch/x-pack/security/src/main/java/org/elasticsearch/xpack/security/SecurityFeatureSet.java @@ -143,7 +143,7 @@ public class SecurityFeatureSet implements XPackFeatureSet { static boolean systemKeyUsage(CryptoService cryptoService) { // we can piggy back on the encryption enabled method as it is only enabled if there is a system key - return cryptoService != null && cryptoService.encryptionEnabled(); + return cryptoService != null && cryptoService.isEncryptionEnabled(); } static class Usage extends XPackFeatureSet.Usage { diff --git a/elasticsearch/x-pack/security/src/main/java/org/elasticsearch/xpack/security/action/filter/SecurityActionFilter.java b/elasticsearch/x-pack/security/src/main/java/org/elasticsearch/xpack/security/action/filter/SecurityActionFilter.java index 6adde44d6dc..74e8bdda4c1 100644 --- a/elasticsearch/x-pack/security/src/main/java/org/elasticsearch/xpack/security/action/filter/SecurityActionFilter.java +++ b/elasticsearch/x-pack/security/src/main/java/org/elasticsearch/xpack/security/action/filter/SecurityActionFilter.java @@ -191,7 +191,7 @@ public class SecurityActionFilter extends AbstractComponent implements ActionFil if (response instanceof SearchResponse) { SearchResponse searchResponse = (SearchResponse) response; String scrollId = searchResponse.getScrollId(); - if (scrollId != null && !cryptoService.signed(scrollId)) { + if (scrollId != null && !cryptoService.isSigned(scrollId)) { searchResponse.scrollId(cryptoService.sign(scrollId)); } return response; diff --git a/elasticsearch/x-pack/security/src/main/java/org/elasticsearch/xpack/security/crypto/CryptoService.java b/elasticsearch/x-pack/security/src/main/java/org/elasticsearch/xpack/security/crypto/CryptoService.java index 746edd21760..4e978f8cba7 100644 --- a/elasticsearch/x-pack/security/src/main/java/org/elasticsearch/xpack/security/crypto/CryptoService.java +++ b/elasticsearch/x-pack/security/src/main/java/org/elasticsearch/xpack/security/crypto/CryptoService.java @@ -5,7 +5,6 @@ */ package org.elasticsearch.xpack.security.crypto; -import javax.crypto.SecretKey; import java.io.IOException; /** @@ -26,27 +25,10 @@ public interface CryptoService { */ String unsignAndVerify(String text); - /** - * Signs the given text and returns the signed text (original text + signature) - * @param text the string to sign - * @param key the key to sign the text with - * @param systemKey the system key. This is optional and if the key != systemKey then the format of the - * message will change - */ - String sign(String text, SecretKey key, SecretKey systemKey) throws IOException; - - /** - * Unsigns the given signed text, verifies the original text with the attached signature and if valid returns - * the unsigned (original) text. If signature verification fails a {@link IllegalArgumentException} is thrown. - * @param text the string to unsign and verify - * @param key the key to unsign the text with - */ - String unsignAndVerify(String text, SecretKey key); - /** * Checks whether the given text is signed. */ - boolean signed(String text); + boolean isSigned(String text); /** * Encrypts the provided char array and returns the encrypted values in a char array @@ -55,13 +37,6 @@ public interface CryptoService { */ char[] encrypt(char[] chars); - /** - * Encrypts the provided byte array and returns the encrypted value - * @param bytes the data to encrypt - * @return encrypted data - */ - byte[] encrypt(byte[] bytes); - /** * Decrypts the provided char array and returns the plain-text chars * @param chars the data to decrypt @@ -69,46 +44,16 @@ public interface CryptoService { */ char[] decrypt(char[] chars); - /** - * Decrypts the provided char array and returns the plain-text chars - * @param chars the data to decrypt - * @param key the key to decrypt the data with - * @return plaintext chars - */ - char[] decrypt(char[] chars, SecretKey key); - - /** - * Decrypts the provided byte array and returns the unencrypted bytes - * @param bytes the bytes to decrypt - * @return plaintext bytes - */ - byte[] decrypt(byte[] bytes); - - /** - * Decrypts the provided byte array and returns the unencrypted bytes - * @param bytes the bytes to decrypt - * @param key the key to decrypt the data with - * @return plaintext bytes - */ - byte[] decrypt(byte[] bytes, SecretKey key); - /** * Checks whether the given chars are encrypted * @param chars the chars to check if they are encrypted * @return true is data is encrypted */ - boolean encrypted(char[] chars); - - /** - * Checks whether the given bytes are encrypted - * @param bytes the chars to check if they are encrypted - * @return true is data is encrypted - */ - boolean encrypted(byte[] bytes); + boolean isEncrypted(char[] chars); /** * Flag for callers to determine if values will actually be encrypted or returned plaintext * @return true if values will be encrypted */ - boolean encryptionEnabled(); + boolean isEncryptionEnabled(); } diff --git a/elasticsearch/x-pack/security/src/main/java/org/elasticsearch/xpack/security/crypto/InternalCryptoService.java b/elasticsearch/x-pack/security/src/main/java/org/elasticsearch/xpack/security/crypto/InternalCryptoService.java index a081fbb76f2..8ad15024c5e 100644 --- a/elasticsearch/x-pack/security/src/main/java/org/elasticsearch/xpack/security/crypto/InternalCryptoService.java +++ b/elasticsearch/x-pack/security/src/main/java/org/elasticsearch/xpack/security/crypto/InternalCryptoService.java @@ -157,23 +157,12 @@ public class InternalCryptoService extends AbstractComponent implements CryptoSe @Override public String sign(String text) throws IOException { - return sign(text, this.signingKey, this.systemKey); - } - - @Override - public String sign(String text, SecretKey signingKey, @Nullable SecretKey systemKey) throws IOException { - assert signingKey != null; String sigStr = signInternal(text, signingKey); return "$$" + sigStr.length() + "$$" + (systemKey == signingKey ? "" : randomKeyBase64) + "$$" + sigStr + text; } @Override public String unsignAndVerify(String signedText) { - return unsignAndVerify(signedText, this.systemKey); - } - - @Override - public String unsignAndVerify(String signedText, SecretKey systemKey) { if (!signedText.startsWith("$$") || signedText.length() < 2) { throw new IllegalArgumentException("tampered signed text"); } @@ -239,7 +228,7 @@ public class InternalCryptoService extends AbstractComponent implements CryptoSe } @Override - public boolean signed(String text) { + public boolean isSigned(String text) { return SIG_PATTERN.matcher(text).matches(); } @@ -257,33 +246,13 @@ public class InternalCryptoService extends AbstractComponent implements CryptoSe return ENCRYPTED_TEXT_PREFIX.concat(base64).toCharArray(); } - @Override - public byte[] encrypt(byte[] bytes) { - SecretKey key = this.encryptionKey; - if (key == null) { - logger.warn("encrypt called without a key, returning plain text. run syskeygen and copy same key to all nodes to enable " + - "encryption"); - return bytes; - } - byte[] encrypted = encryptInternal(bytes, key); - byte[] prefixed = new byte[ENCRYPTED_BYTE_PREFIX.length + encrypted.length]; - System.arraycopy(ENCRYPTED_BYTE_PREFIX, 0, prefixed, 0, ENCRYPTED_BYTE_PREFIX.length); - System.arraycopy(encrypted, 0, prefixed, ENCRYPTED_BYTE_PREFIX.length, encrypted.length); - return prefixed; - } - @Override public char[] decrypt(char[] chars) { - return decrypt(chars, this.encryptionKey); - } - - @Override - public char[] decrypt(char[] chars, SecretKey key) { - if (key == null) { + if (encryptionKey == null) { return chars; } - if (!encrypted(chars)) { + if (!isEncrypted(chars)) { // Not encrypted return chars; } @@ -296,41 +265,17 @@ public class InternalCryptoService extends AbstractComponent implements CryptoSe throw new ElasticsearchException("unable to decode encrypted data", e); } - byte[] decrypted = decryptInternal(bytes, key); + byte[] decrypted = decryptInternal(bytes, encryptionKey); return CharArrays.utf8BytesToChars(decrypted); } @Override - public byte[] decrypt(byte[] bytes) { - return decrypt(bytes, this.encryptionKey); - } - - @Override - public byte[] decrypt(byte[] bytes, SecretKey key) { - if (key == null) { - return bytes; - } - - if (!encrypted(bytes)) { - return bytes; - } - - byte[] encrypted = Arrays.copyOfRange(bytes, ENCRYPTED_BYTE_PREFIX.length, bytes.length); - return decryptInternal(encrypted, key); - } - - @Override - public boolean encrypted(char[] chars) { + public boolean isEncrypted(char[] chars) { return CharArrays.charsBeginsWith(ENCRYPTED_TEXT_PREFIX, chars); } @Override - public boolean encrypted(byte[] bytes) { - return bytesBeginsWith(ENCRYPTED_BYTE_PREFIX, bytes); - } - - @Override - public boolean encryptionEnabled() { + public boolean isEncryptionEnabled() { return this.encryptionKey != null; } @@ -417,24 +362,6 @@ public class InternalCryptoService extends AbstractComponent implements CryptoSe return new SecretKeySpec(truncatedDigest, algorithm); } - private static boolean bytesBeginsWith(byte[] prefix, byte[] bytes) { - if (bytes == null || prefix == null) { - return false; - } - - if (prefix.length > bytes.length) { - return false; - } - - for (int i = 0; i < prefix.length; i++) { - if (bytes[i] != prefix[i]) { - return false; - } - } - - return true; - } - /** * Provider class for the HmacSHA1 {@link Mac} that provides an optimization by using a thread local instead of calling * Mac#getInstance and obtaining a lock (in the internals) diff --git a/elasticsearch/x-pack/security/src/test/java/org/elasticsearch/integration/ScrollIdSigningTests.java b/elasticsearch/x-pack/security/src/test/java/org/elasticsearch/integration/ScrollIdSigningTests.java index bf76bfe7ac0..4117bac7b8b 100644 --- a/elasticsearch/x-pack/security/src/test/java/org/elasticsearch/integration/ScrollIdSigningTests.java +++ b/elasticsearch/x-pack/security/src/test/java/org/elasticsearch/integration/ScrollIdSigningTests.java @@ -108,6 +108,6 @@ public class ScrollIdSigningTests extends SecurityIntegTestCase { private void assertSigned(String scrollId) { CryptoService cryptoService = internalCluster().getDataNodeInstance(InternalCryptoService.class); String message = String.format(Locale.ROOT, "Expected scrollId [%s] to be signed, but was not", scrollId); - assertThat(message, cryptoService.signed(scrollId), is(true)); + assertThat(message, cryptoService.isSigned(scrollId), is(true)); } } diff --git a/elasticsearch/x-pack/security/src/test/java/org/elasticsearch/xpack/security/SecurityFeatureSetTests.java b/elasticsearch/x-pack/security/src/test/java/org/elasticsearch/xpack/security/SecurityFeatureSetTests.java index bb28a9c1050..ae4689fbefb 100644 --- a/elasticsearch/x-pack/security/src/test/java/org/elasticsearch/xpack/security/SecurityFeatureSetTests.java +++ b/elasticsearch/x-pack/security/src/test/java/org/elasticsearch/xpack/security/SecurityFeatureSetTests.java @@ -96,7 +96,7 @@ public class SecurityFeatureSetTests extends ESTestCase { public void testSystemKeyUsageEnabledByCryptoService() { final boolean enabled = randomBoolean(); - when(cryptoService.encryptionEnabled()).thenReturn(enabled); + when(cryptoService.isEncryptionEnabled()).thenReturn(enabled); assertThat(SecurityFeatureSet.systemKeyUsage(cryptoService), is(enabled)); } @@ -143,7 +143,7 @@ public class SecurityFeatureSetTests extends ESTestCase { when(rolesStore.usageStats()).thenReturn(Collections.emptyMap()); } final boolean useSystemKey = randomBoolean(); - when(cryptoService.encryptionEnabled()).thenReturn(useSystemKey); + when(cryptoService.isEncryptionEnabled()).thenReturn(useSystemKey); List realmsList= new ArrayList<>(); diff --git a/elasticsearch/x-pack/security/src/test/java/org/elasticsearch/xpack/security/action/filter/SecurityActionFilterTests.java b/elasticsearch/x-pack/security/src/test/java/org/elasticsearch/xpack/security/action/filter/SecurityActionFilterTests.java index 730b473a424..8794175d772 100644 --- a/elasticsearch/x-pack/security/src/test/java/org/elasticsearch/xpack/security/action/filter/SecurityActionFilterTests.java +++ b/elasticsearch/x-pack/security/src/test/java/org/elasticsearch/xpack/security/action/filter/SecurityActionFilterTests.java @@ -105,7 +105,7 @@ public class SecurityActionFilterTests extends ESTestCase { Task task = mock(Task.class); Authentication authentication = new Authentication(user, new RealmRef("test", "test", "foo"), null); when(authcService.authenticate("_action", request, SystemUser.INSTANCE)).thenReturn(authentication); - when(cryptoService.signed("signed_scroll_id")).thenReturn(true); + when(cryptoService.isSigned("signed_scroll_id")).thenReturn(true); when(cryptoService.unsignAndVerify("signed_scroll_id")).thenReturn("scroll_id"); filter.apply(task, "_action", request, listener, chain); assertThat(request.scrollId(), equalTo("scroll_id")); @@ -122,7 +122,7 @@ public class SecurityActionFilterTests extends ESTestCase { Task task = mock(Task.class); Authentication authentication = new Authentication(user, new RealmRef("test", "test", "foo"), null); when(authcService.authenticate("_action", request, SystemUser.INSTANCE)).thenReturn(authentication); - when(cryptoService.signed("scroll_id")).thenReturn(true); + when(cryptoService.isSigned("scroll_id")).thenReturn(true); doThrow(sigException).when(cryptoService).unsignAndVerify("scroll_id"); filter.apply(task, "_action", request, listener, chain); verify(listener).onFailure(isA(ElasticsearchSecurityException.class)); diff --git a/elasticsearch/x-pack/security/src/test/java/org/elasticsearch/xpack/security/crypto/InternalCryptoServiceTests.java b/elasticsearch/x-pack/security/src/test/java/org/elasticsearch/xpack/security/crypto/InternalCryptoServiceTests.java index eb0f6196c25..796fcb9b2c7 100644 --- a/elasticsearch/x-pack/security/src/test/java/org/elasticsearch/xpack/security/crypto/InternalCryptoServiceTests.java +++ b/elasticsearch/x-pack/security/src/test/java/org/elasticsearch/xpack/security/crypto/InternalCryptoServiceTests.java @@ -7,7 +7,6 @@ package org.elasticsearch.xpack.security.crypto; import javax.crypto.SecretKey; import javax.crypto.spec.SecretKeySpec; -import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.nio.file.Path; import java.util.Arrays; @@ -46,7 +45,7 @@ public class InternalCryptoServiceTests extends ESTestCase { InternalCryptoService service = new InternalCryptoService(settings, env); String text = randomAsciiOfLength(10); String signed = service.sign(text); - assertThat(service.signed(signed), is(true)); + assertThat(service.isSigned(signed), is(true)); } public void testSignAndUnsign() throws Exception { @@ -133,7 +132,7 @@ public class InternalCryptoServiceTests extends ESTestCase { public void testEncryptionAndDecryptionChars() throws Exception { InternalCryptoService service = new InternalCryptoService(settings, env); - assertThat(service.encryptionEnabled(), is(true)); + assertThat(service.isEncryptionEnabled(), is(true)); final char[] chars = randomAsciiOfLengthBetween(0, 1000).toCharArray(); final char[] encrypted = service.encrypt(chars); assertThat(encrypted, notNullValue()); @@ -143,21 +142,9 @@ public class InternalCryptoServiceTests extends ESTestCase { assertThat(Arrays.equals(chars, decrypted), is(true)); } - public void testEncryptionAndDecryptionBytes() throws Exception { - InternalCryptoService service = new InternalCryptoService(settings, env); - assertThat(service.encryptionEnabled(), is(true)); - final byte[] bytes = randomByteArray(); - final byte[] encrypted = service.encrypt(bytes); - assertThat(encrypted, notNullValue()); - assertThat(Arrays.equals(encrypted, bytes), is(false)); - - final byte[] decrypted = service.decrypt(encrypted); - assertThat(Arrays.equals(bytes, decrypted), is(true)); - } - public void testEncryptionAndDecryptionCharsWithoutKey() throws Exception { InternalCryptoService service = new InternalCryptoService(Settings.EMPTY, env); - assertThat(service.encryptionEnabled(), is(false)); + assertThat(service.isEncryptionEnabled(), is(false)); final char[] chars = randomAsciiOfLengthBetween(0, 1000).toCharArray(); final char[] encryptedChars = service.encrypt(chars); final char[] decryptedChars = service.decrypt(encryptedChars); @@ -165,68 +152,26 @@ public class InternalCryptoServiceTests extends ESTestCase { assertThat(chars, equalTo(decryptedChars)); } - public void testEncryptionAndDecryptionBytesWithoutKey() throws Exception { - InternalCryptoService service = new InternalCryptoService(Settings.EMPTY, env); - assertThat(service.encryptionEnabled(), is(false)); - final byte[] bytes = randomByteArray(); - final byte[] encryptedBytes = service.encrypt(bytes); - final byte[] decryptedBytes = service.decrypt(bytes); - assertThat(bytes, equalTo(encryptedBytes)); - assertThat(decryptedBytes, equalTo(encryptedBytes)); - } - public void testEncryptionEnabledWithKey() throws Exception { InternalCryptoService service = new InternalCryptoService(settings, env); - assertThat(service.encryptionEnabled(), is(true)); + assertThat(service.isEncryptionEnabled(), is(true)); } public void testEncryptionEnabledWithoutKey() throws Exception { InternalCryptoService service = new InternalCryptoService(Settings.EMPTY, env); - assertThat(service.encryptionEnabled(), is(false)); - } - - public void testChangingAByte() throws Exception { - InternalCryptoService service = new InternalCryptoService(settings, env); - assertThat(service.encryptionEnabled(), is(true)); - // We need at least one byte to test changing a byte, otherwise output is always the same - final byte[] bytes = randomByteArray(1); - final byte[] encrypted = service.encrypt(bytes); - assertThat(encrypted, notNullValue()); - assertThat(Arrays.equals(encrypted, bytes), is(false)); - - int tamperedIndex = randomIntBetween(InternalCryptoService.ENCRYPTED_BYTE_PREFIX.length, encrypted.length - 1); - final byte untamperedByte = encrypted[tamperedIndex]; - byte tamperedByte = randomByte(); - while (tamperedByte == untamperedByte) { - tamperedByte = randomByte(); - } - encrypted[tamperedIndex] = tamperedByte; - final byte[] decrypted = service.decrypt(encrypted); - assertThat(Arrays.equals(bytes, decrypted), is(false)); + assertThat(service.isEncryptionEnabled(), is(false)); } public void testEncryptedChar() throws Exception { InternalCryptoService service = new InternalCryptoService(settings, env); - assertThat(service.encryptionEnabled(), is(true)); + assertThat(service.isEncryptionEnabled(), is(true)); - assertThat(service.encrypted((char[]) null), is(false)); - assertThat(service.encrypted(new char[0]), is(false)); - assertThat(service.encrypted(new char[InternalCryptoService.ENCRYPTED_TEXT_PREFIX.length()]), is(false)); - assertThat(service.encrypted(InternalCryptoService.ENCRYPTED_TEXT_PREFIX.toCharArray()), is(true)); - assertThat(service.encrypted(randomAsciiOfLengthBetween(0, 100).toCharArray()), is(false)); - assertThat(service.encrypted(service.encrypt(randomAsciiOfLength(10).toCharArray())), is(true)); - } - - public void testEncryptedByte() throws Exception { - InternalCryptoService service = new InternalCryptoService(settings, env); - assertThat(service.encryptionEnabled(), is(true)); - - assertThat(service.encrypted((byte[]) null), is(false)); - assertThat(service.encrypted(new byte[0]), is(false)); - assertThat(service.encrypted(new byte[InternalCryptoService.ENCRYPTED_BYTE_PREFIX.length]), is(false)); - assertThat(service.encrypted(InternalCryptoService.ENCRYPTED_BYTE_PREFIX), is(true)); - assertThat(service.encrypted(randomAsciiOfLengthBetween(0, 100).getBytes(StandardCharsets.UTF_8)), is(false)); - assertThat(service.encrypted(service.encrypt(randomAsciiOfLength(10).getBytes(StandardCharsets.UTF_8))), is(true)); + assertThat(service.isEncrypted((char[]) null), is(false)); + assertThat(service.isEncrypted(new char[0]), is(false)); + assertThat(service.isEncrypted(new char[InternalCryptoService.ENCRYPTED_TEXT_PREFIX.length()]), is(false)); + assertThat(service.isEncrypted(InternalCryptoService.ENCRYPTED_TEXT_PREFIX.toCharArray()), is(true)); + assertThat(service.isEncrypted(randomAsciiOfLengthBetween(0, 100).toCharArray()), is(false)); + assertThat(service.isEncrypted(service.encrypt(randomAsciiOfLength(10).toCharArray())), is(true)); } public void testSigningKeyCanBeRecomputedConsistently() { @@ -239,17 +184,4 @@ public class InternalCryptoServiceTests extends ESTestCase { assertThat(regenerated, equalTo(signingKey)); } } - - private static byte[] randomByteArray() { - return randomByteArray(0); - } - - private static byte[] randomByteArray(int min) { - int count = randomIntBetween(min, 1000); - byte[] bytes = new byte[count]; - for (int i = 0; i < count; i++) { - bytes[i] = randomByte(); - } - return bytes; - } }