SAML: Make alias for signing key optional (elastic/x-pack-elasticsearch#4248)

We specify an alias for signing key, but when we just have
a single key in key store this is an additional setting which
is annoying. This PR addresses this issue by making it optional.

- Changes in SamlRealmSettings to make signing/encryption
  key alias optional
- Checks if none of the keys are useful for given operation
  signing or encryption throws an error.
- Checks for no of aliases in key-store, if more than one and alias
  is not specified throws error.
- If an alias is not specified and there is just one alias in
  keystore then use it as the credential.
- Unit Tests

Note: A side effect of this change the above-mentioned behavior is
it's also applicable for encryption keys currently, but it is going
to change when fixing elastic/x-pack-elasticsearch#3980 for supporting multiple encryption keys.

relates elastic/x-pack-elasticsearch#3981

Original commit: elastic/x-pack-elasticsearch@2b5af1d8a8

docs/en/security/authentication/saml-realm.asciidoc

@@ -87,7 +87,7 @@ for SAML realms.
                                            _principal_ property.
                                            The attribute value must match the pattern, and the value of the
                                            first _capturing group_ is used as the principal.
-                                           e.g. `^([^@]+)@example.com$` matches email addresses from the 
+                                           e.g. `^([^@]+)@example\\.com$` matches email addresses from the
                                            "example.com" domain and uses the local-part as the principal.
 | `attribute_patterns.groups`    | no    | As per `attribute_patterns.principal`, but for the _group_ property.
 | `attribute_patterns.name`      | no    | As per `attribute_patterns.principal`, but for the _name_ property.
@@ -148,7 +148,8 @@ If a signing key is configured (i.e. is one of `signing.key` or `signing.keystor
                                                  Defaults to "PKCS12" if the keystore path ends in ".p12", ".pfx" or
                                                  "pkcs12", otherwise uses "jks"
 | `signing.keystore.alias`          | no       | Specifies the alias of the key within the keystore that should be
-                                                 used for SAML message signing. Defaults to `key`.
+                                                 used for SAML message signing. Must be specified if the keystore
+                                                 contains more than one private key.
 | `signing.keystore.secure_password` | no      | ({ref}/secure-settings.html[Secure]) The password to the keystore.
 | `signing.keystore.secure_key_password` | no  | ({ref}/secure-settings.html[Secure])
                                                  The password for the key in the keystore.
@@ -186,7 +187,8 @@ Encryption can be configured using the following settings.
                                                    Defaults to "PKCS12" if the keystore path ends in ".p12", ".pfx" or
                                                    "pkcs12", otherwise uses "jks"
 | `encryption.keystore.alias`       | no         | Specifies the alias of the key within the keystore that should be
-                                                   used for SAML message encryption. Defaults to `key`.
+                                                   used for SAML message decryption. Must be specified if the keystore
+                                                   contains more than one private key.
 | `encryption.keystore.secure_password` | no     | ({ref}/secure-settings.html[Secure]) The password to the keystore.
 | `encryption.keystore.secure_key_password` | no | ({ref}/secure-settings.html[Secure])
                                                    The password for the key in the keystore.

docs/en/settings/security-settings.asciidoc

@@ -760,7 +760,8 @@ ends in ".p12", ".pfx" or "pkcs12", otherwise uses "jks".
 
 `signing.keystore.alias`::
 Specifies the alias of the key within the keystore that should be
-used for SAML message signing. Defaults to `key`.
+used for SAML message signing. Must be specified if the keystore
+contains more than one private key.
 
 `signing.keystore.secure_password` (<<secure-settings,Secure>>)::
 The password to the keystore (`signing.keystore.path`).
@@ -794,7 +795,8 @@ ends in ".p12", ".pfx" or "pkcs12", otherwise uses "jks".
 
 `encryption.keystore.alias`::
 Specifies the alias of the key within the keystore (`encryption.keystore.path`)
-that should be used for SAML message decryption. Defaults to `key`.
+that should be used for SAML message decryption. Must be specified if the
+keystore contains more than one private key.
 
 `encryption.keystore.secure_password` (<<secure-settings,Secure>>)::
 The password to the keystore (`encryption.keystore.path`).

plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/saml/SamlRealmSettings.java

@@ -54,12 +54,12 @@ public class SamlRealmSettings {
     public static final AttributeSetting MAIL_ATTRIBUTE = new AttributeSetting("mail");
 
     public static final X509KeyPairSettings ENCRYPTION_SETTINGS = new X509KeyPairSettings("encryption.", false);
-    public static final Setting<String> ENCRYPTION_KEY_ALIAS = new Setting<>("encryption.keystore.alias", "key", Function.identity(),
+    public static final Setting<String> ENCRYPTION_KEY_ALIAS =
-            Setting.Property.NodeScope);
+            Setting.simpleString("encryption.keystore.alias", Setting.Property.NodeScope);
 
     public static final X509KeyPairSettings SIGNING_SETTINGS = new X509KeyPairSettings("signing.", false);
-    public static final Setting<String> SIGNING_KEY_ALIAS = new Setting<>("signing.keystore.alias", "key", Function.identity(),
+    public static final Setting<String> SIGNING_KEY_ALIAS =
-            Setting.Property.NodeScope);
+            Setting.simpleString("signing.keystore.alias", Setting.Property.NodeScope);
     public static final Setting<List<String>> SIGNING_MESSAGE_TYPES = Setting.listSetting("signing.saml_messages",
             Collections.singletonList("*"), Function.identity(), Setting.Property.NodeScope);
 

plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlRealm.java

@@ -84,11 +84,14 @@ import java.security.PrivilegedActionException;
 import java.security.PrivilegedExceptionAction;
 import java.time.Clock;
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.Collections;
 import java.util.HashMap;
+import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
+import java.util.Set;
 import java.util.function.Function;
 import java.util.function.Supplier;
 import java.util.regex.Matcher;
@@ -295,19 +298,54 @@ public final class SamlRealm extends Realm implements Releasable {
         }
     }
 
-    private static X509Credential buildCredential(RealmConfig config, X509KeyPairSettings keyPairSettings, Setting<String> aliasSetting) {
+    private static X509Credential buildCredential(RealmConfig config, X509KeyPairSettings keyPairSettings,
+            Setting<String> aliasSetting) {
         final X509KeyManager keyManager = CertUtils.getKeyManager(keyPairSettings, config.settings(), null, config.env());
         if (keyManager == null) {
             return null;
         }
-        final String alias = aliasSetting.get(config.settings());
+
-        if (keyManager.getPrivateKey(alias) == null) {
+        String alias = aliasSetting.get(config.settings());
-            throw new IllegalArgumentException("The configured encryption store for "
+        if (Strings.isNullOrEmpty(alias)) {
-                    + RealmSettings.getFullSettingKey(config, keyPairSettings.getPrefix())
+
-                    + " does not have a key with alias [" + alias + "] (from setting " +
+            final Set<String> aliases = new HashSet<>();
-                    RealmSettings.getFullSettingKey(config, aliasSetting)
+            final String[] serverAliases = keyManager.getServerAliases("RSA", null);
-                    + ")");
+            if (serverAliases != null) {
+                aliases.addAll(Arrays.asList(serverAliases));
+            }
+
+            if (aliases.isEmpty()) {
+                throw new IllegalArgumentException(
+                        "The configured key store for " + RealmSettings.getFullSettingKey(config, keyPairSettings.getPrefix())
+                                + " does not contain any RSA key pairs");
+            } else if (aliases.size() > 1) {
+                /*
+                 * TODO bizybot : We need to fix this, for encryption we want to support
+                 * multiple keys Refer: #3980
+                 */
+                throw new IllegalArgumentException(
+                        "The configured key store for " + RealmSettings.getFullSettingKey(config, keyPairSettings.getPrefix())
+                                + " has multiple keys but no alias has been specified (from setting "
+                                + RealmSettings.getFullSettingKey(config, aliasSetting) + ")");
+            } else {
+                alias = aliases.iterator().next();
+            }
         }
+
+        if (keyManager.getPrivateKey(alias) == null) {
+            throw new IllegalArgumentException(
+                    "The configured key store for " + RealmSettings.getFullSettingKey(config, keyPairSettings.getPrefix())
+                            + " does not have a certificate key pair associated with alias [" + alias + "] " + "(from setting "
+                            + RealmSettings.getFullSettingKey(config, aliasSetting) + ")");
+        }
+
+        final String keyType = keyManager.getPrivateKey(alias).getAlgorithm();
+        if (keyType.equals("RSA") == false) {
+            throw new IllegalArgumentException("The key associated with alias [" + alias + "] " + "(from setting "
+                    + RealmSettings.getFullSettingKey(config, aliasSetting) + ") uses unsupported key algorithm type [" + keyType
+                    + "], only RSA is supported");
+        }
+
         return new X509KeyManagerX509CredentialAdapter(keyManager, alias);
     }
 

plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlRealmTests.java

@@ -20,6 +20,7 @@ import org.elasticsearch.test.http.MockWebServer;
 import org.elasticsearch.watcher.ResourceWatcherService;
 import org.elasticsearch.xpack.core.security.authc.AuthenticationResult;
 import org.elasticsearch.xpack.core.security.authc.RealmConfig;
+import org.elasticsearch.xpack.core.security.authc.RealmSettings;
 import org.elasticsearch.xpack.core.security.authc.saml.SamlRealmSettings;
 import org.elasticsearch.xpack.core.ssl.CertUtils;
 import org.elasticsearch.xpack.core.ssl.SSLService;
@@ -49,12 +50,14 @@ import java.security.KeyPair;
 import java.security.KeyPairGenerator;
 import java.security.KeyStore;
 import java.security.NoSuchAlgorithmException;
+import java.security.PrivateKey;
 import java.security.PrivilegedActionException;
 import java.security.cert.Certificate;
 import java.security.cert.X509Certificate;
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.List;
+import java.util.Locale;
 import java.util.Set;
 import java.util.concurrent.atomic.AtomicReference;
 import java.util.function.Supplier;
@@ -316,7 +319,7 @@ public class SamlRealmTests extends SamlTestCase {
         assertThat(credential.getPublicKey(), equalTo(pair.getPublic()));
     }
 
-    public void testCreateCredentialFromKeyStore() throws Exception {
+    public void testCreateEncryptionCredentialFromKeyStore() throws Exception {
         final Path dir = createTempDir();
         final Settings.Builder builder = Settings.builder()
                 .put(REALM_SETTINGS_PREFIX + ".type", "saml")
@@ -348,6 +351,140 @@ public class SamlRealmTests extends SamlTestCase {
         assertThat(credential.getPublicKey(), equalTo(pair.getPublic()));
     }
 
+    public void testCreateSigningCredentialFromKeyStoreSuccessScenarios() throws Exception {
+        final Path dir = createTempDir();
+        final Settings.Builder builder = Settings.builder().put(REALM_SETTINGS_PREFIX + ".type", "saml").put("path.home", dir);
+        final Path ksFile = dir.resolve("cred.p12");
+        final Tuple<X509Certificate, PrivateKey> certKeyPair1 = createKeyPair("RSA");
+        final Tuple<X509Certificate, PrivateKey> certKeyPair2 = createKeyPair("EC");
+
+        final KeyStore ks = KeyStore.getInstance("PKCS12");
+        ks.load(null);
+        ks.setKeyEntry(getAliasName(certKeyPair1), certKeyPair1.v2(), "key-password".toCharArray(),
+                new Certificate[] { certKeyPair1.v1() });
+        ks.setKeyEntry(getAliasName(certKeyPair2), certKeyPair2.v2(), "key-password".toCharArray(),
+                new Certificate[] { certKeyPair2.v1() });
+        try (OutputStream out = Files.newOutputStream(ksFile)) {
+            ks.store(out, "ks-password".toCharArray());
+        }
+        builder.put(REALM_SETTINGS_PREFIX + ".signing.keystore.path", ksFile.toString());
+        builder.put(REALM_SETTINGS_PREFIX + ".signing.keystore.type", "PKCS12");
+        final boolean isSigningKeyStoreAliasSet = randomBoolean();
+        if (isSigningKeyStoreAliasSet) {
+            builder.put(REALM_SETTINGS_PREFIX + ".signing.keystore.alias", getAliasName(certKeyPair1));
+        }
+
+        MockSecureSettings secureSettings = new MockSecureSettings();
+        secureSettings.setString(REALM_SETTINGS_PREFIX + ".signing.keystore.secure_password", "ks-password");
+        secureSettings.setString(REALM_SETTINGS_PREFIX + ".signing.keystore.secure_key_password", "key-password");
+        builder.setSecureSettings(secureSettings);
+
+        final Settings settings = builder.build();
+        final RealmConfig realmConfig = realmConfigFromGlobalSettings(settings);
+
+        // Should build signing credential and use the key from KS.
+        final SigningConfiguration signingConfig = SamlRealm.buildSigningConfiguration(realmConfig);
+        final Credential credential = signingConfig.getCredential();
+        assertThat(credential, notNullValue());
+        assertThat(credential.getPrivateKey(), equalTo(certKeyPair1.v2()));
+        assertThat(credential.getPublicKey(), equalTo(certKeyPair1.v1().getPublicKey()));
+    }
+
+    public void testCreateSigningCredentialFromKeyStoreFailureScenarios() throws Exception {
+        final Path dir = createTempDir();
+        final Settings.Builder builder = Settings.builder().put(REALM_SETTINGS_PREFIX + ".type", "saml").put("path.home", dir);
+        final Path ksFile = dir.resolve("cred.p12");
+        final Tuple<X509Certificate, PrivateKey> certKeyPair1 = createKeyPair("RSA");
+        final Tuple<X509Certificate, PrivateKey> certKeyPair2 = createKeyPair("RSA");
+        final Tuple<X509Certificate, PrivateKey> certKeyPair3 = createKeyPair("EC");
+
+        final KeyStore ks = KeyStore.getInstance("PKCS12");
+        ks.load(null);
+        final boolean noRSAKeysInKS = randomBoolean();
+        if (noRSAKeysInKS == false) {
+            ks.setKeyEntry(getAliasName(certKeyPair1), certKeyPair1.v2(), "key-password".toCharArray(),
+                    new Certificate[] { certKeyPair1.v1() });
+            ks.setKeyEntry(getAliasName(certKeyPair2), certKeyPair2.v2(), "key-password".toCharArray(),
+                    new Certificate[] { certKeyPair2.v1() });
+        }
+        ks.setKeyEntry(getAliasName(certKeyPair3), certKeyPair3.v2(), "key-password".toCharArray(),
+                new Certificate[] { certKeyPair3.v1() });
+        try (OutputStream out = Files.newOutputStream(ksFile)) {
+            ks.store(out, "ks-password".toCharArray());
+        }
+
+        builder.put(REALM_SETTINGS_PREFIX + ".signing.keystore.path", ksFile.toString());
+        builder.put(REALM_SETTINGS_PREFIX + ".signing.keystore.type", "PKCS12");
+        final boolean isSigningKeyStoreAliasSet = randomBoolean();
+        final Tuple<X509Certificate, PrivateKey> chosenAliasCertKeyPair;
+        final String unknownAlias = randomAlphaOfLength(5);
+        if (isSigningKeyStoreAliasSet) {
+            chosenAliasCertKeyPair = randomFrom(Arrays.asList(certKeyPair3, null));
+            if (chosenAliasCertKeyPair == null) {
+                // Unknown alias
+                builder.put(REALM_SETTINGS_PREFIX + ".signing.keystore.alias", unknownAlias);
+            } else {
+                builder.put(REALM_SETTINGS_PREFIX + ".signing.keystore.alias", getAliasName(chosenAliasCertKeyPair));
+            }
+        } else {
+            chosenAliasCertKeyPair = null;
+        }
+
+        MockSecureSettings secureSettings = new MockSecureSettings();
+        secureSettings.setString(REALM_SETTINGS_PREFIX + ".signing.keystore.secure_password", "ks-password");
+        secureSettings.setString(REALM_SETTINGS_PREFIX + ".signing.keystore.secure_key_password", "key-password");
+        builder.setSecureSettings(secureSettings);
+
+        final Settings settings = builder.build();
+        final RealmConfig realmConfig = realmConfigFromGlobalSettings(settings);
+
+        if (isSigningKeyStoreAliasSet) {
+            if (chosenAliasCertKeyPair == null) {
+                // Unknown alias, this must throw exception
+                final IllegalArgumentException illegalArgumentException =
+                        expectThrows(IllegalArgumentException.class, () -> SamlRealm.buildSigningConfiguration(realmConfig));
+                final String expectedErrorMessage = "The configured key store for "
+                        + RealmSettings.getFullSettingKey(realmConfig, SamlRealmSettings.SIGNING_SETTINGS.getPrefix())
+                        + " does not have a certificate key pair associated with alias [" + unknownAlias + "] " + "(from setting "
+                        + RealmSettings.getFullSettingKey(realmConfig, SamlRealmSettings.SIGNING_KEY_ALIAS) + ")";
+                assertEquals(expectedErrorMessage, illegalArgumentException.getLocalizedMessage());
+            } else {
+                final String chosenAliasName = getAliasName(chosenAliasCertKeyPair);
+                // Since this is unsupported key type, this must throw exception
+                final IllegalArgumentException illegalArgumentException =
+                        expectThrows(IllegalArgumentException.class, () -> SamlRealm.buildSigningConfiguration(realmConfig));
+                final String expectedErrorMessage = "The key associated with alias [" + chosenAliasName + "] " + "(from setting "
+                        + RealmSettings.getFullSettingKey(realmConfig, SamlRealmSettings.SIGNING_KEY_ALIAS)
+                        + ") uses unsupported key algorithm type [" + chosenAliasCertKeyPair.v2().getAlgorithm()
+                        + "], only RSA is supported";
+                assertEquals(expectedErrorMessage, illegalArgumentException.getLocalizedMessage());
+            }
+        } else {
+            if (noRSAKeysInKS) {
+                // Should throw exception as no RSA keys in the keystore
+                final IllegalArgumentException illegalArgumentException =
+                        expectThrows(IllegalArgumentException.class, () -> SamlRealm.buildSigningConfiguration(realmConfig));
+                final String expectedErrorMessage = "The configured key store for "
+                        + RealmSettings.getFullSettingKey(realmConfig, SamlRealmSettings.SIGNING_SETTINGS.getPrefix())
+                        + " does not contain any RSA key pairs";
+                assertEquals(expectedErrorMessage, illegalArgumentException.getLocalizedMessage());
+            } else {
+                // Should throw exception when multiple signing keys found and alias not set
+                final IllegalArgumentException illegalArgumentException =
+                        expectThrows(IllegalArgumentException.class, () -> SamlRealm.buildSigningConfiguration(realmConfig));
+                final String expectedErrorMessage = "The configured key store for "
+                        + RealmSettings.getFullSettingKey(realmConfig, SamlRealmSettings.SIGNING_SETTINGS.getPrefix())
+                        + " has multiple keys but no alias has been specified (from setting "
+                        + RealmSettings.getFullSettingKey(realmConfig, SamlRealmSettings.SIGNING_KEY_ALIAS) + ")";
+                assertEquals(expectedErrorMessage, illegalArgumentException.getLocalizedMessage());
+            }
+        }
+    }
+
+    private String getAliasName(final Tuple<X509Certificate, PrivateKey> certKeyPair) {
+        return certKeyPair.v1().getSubjectX500Principal().getName().toLowerCase(Locale.US) + "-alias";
+    }
+
     public void testBuildLogoutRequest() throws Exception {
         final Boolean useSingleLogout = randomFrom(true, false, null);
         final UserRoleMapper roleMapper = mock(UserRoleMapper.class);