Merge pull request elastic/elasticsearch#2770 from rjernst/simpler_crypto_api

Internal: Simplify CryptoService api

Original commit: elastic/x-pack-elasticsearch@de3f270cbb

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 {

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;

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);
+    boolean isEncrypted(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);
 
     /**
      * 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();
 }

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);
+        if (encryptionKey == null) {
-    }
-
-    @Override
-    public char[] decrypt(char[] chars, SecretKey key) {
-        if (key == 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) {
+    public boolean isEncrypted(char[] chars) {
-        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) {
         return CharArrays.charsBeginsWith(ENCRYPTED_TEXT_PREFIX, chars);
     }
 
     @Override
-    public boolean encrypted(byte[] bytes) {
+    public boolean isEncryptionEnabled() {
-        return bytesBeginsWith(ENCRYPTED_BYTE_PREFIX, bytes);
-    }
-
-    @Override
-    public boolean encryptionEnabled() {
         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)

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));
     }
 }

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<Realm> realmsList= new ArrayList<>();
 

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));

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));
+                assertThat(service.isEncryptionEnabled(), 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));
     }
 
     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.isEncrypted((char[]) null), is(false));
-        assertThat(service.encrypted(new char[0]), is(false));
+        assertThat(service.isEncrypted(new char[0]), is(false));
-        assertThat(service.encrypted(new char[InternalCryptoService.ENCRYPTED_TEXT_PREFIX.length()]), is(false));
+        assertThat(service.isEncrypted(new char[InternalCryptoService.ENCRYPTED_TEXT_PREFIX.length()]), is(false));
-        assertThat(service.encrypted(InternalCryptoService.ENCRYPTED_TEXT_PREFIX.toCharArray()), is(true));
+        assertThat(service.isEncrypted(InternalCryptoService.ENCRYPTED_TEXT_PREFIX.toCharArray()), is(true));
-        assertThat(service.encrypted(randomAsciiOfLengthBetween(0, 100).toCharArray()), is(false));
+        assertThat(service.isEncrypted(randomAsciiOfLengthBetween(0, 100).toCharArray()), is(false));
-        assertThat(service.encrypted(service.encrypt(randomAsciiOfLength(10).toCharArray())), is(true));
+        assertThat(service.isEncrypted(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));
     }
 
     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;
-    }
 }