From ed6a6af64c71300bf788cfb9c384e246960d75e9 Mon Sep 17 00:00:00 2001 From: Yogesh Gaikwad <902768+bizybot@users.noreply.github.com> Date: Fri, 6 Apr 2018 10:43:35 +1000 Subject: [PATCH] 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@2b5af1d8a8e9f43125cf6d1c9315e2bd60809f93 --- .../authentication/saml-realm.asciidoc | 8 +- docs/en/settings/security-settings.asciidoc | 6 +- .../authc/saml/SamlRealmSettings.java | 8 +- .../xpack/security/authc/saml/SamlRealm.java | 54 ++++++- .../security/authc/saml/SamlRealmTests.java | 139 +++++++++++++++++- 5 files changed, 197 insertions(+), 18 deletions(-) diff --git a/docs/en/security/authentication/saml-realm.asciidoc b/docs/en/security/authentication/saml-realm.asciidoc index 19c5bf18ab7..b49f8b24772 100644 --- a/docs/en/security/authentication/saml-realm.asciidoc +++ b/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. diff --git a/docs/en/settings/security-settings.asciidoc b/docs/en/settings/security-settings.asciidoc index 04a5bc9857b..48b78140c63 100644 --- a/docs/en/settings/security-settings.asciidoc +++ b/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` (<>):: 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` (<>):: The password to the keystore (`encryption.keystore.path`). diff --git a/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/saml/SamlRealmSettings.java b/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/saml/SamlRealmSettings.java index 3001f8dc509..996b886c0db 100644 --- a/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/saml/SamlRealmSettings.java +++ b/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 ENCRYPTION_KEY_ALIAS = new Setting<>("encryption.keystore.alias", "key", Function.identity(), - Setting.Property.NodeScope); + public static final Setting ENCRYPTION_KEY_ALIAS = + Setting.simpleString("encryption.keystore.alias", Setting.Property.NodeScope); public static final X509KeyPairSettings SIGNING_SETTINGS = new X509KeyPairSettings("signing.", false); - public static final Setting SIGNING_KEY_ALIAS = new Setting<>("signing.keystore.alias", "key", Function.identity(), - Setting.Property.NodeScope); + public static final Setting SIGNING_KEY_ALIAS = + Setting.simpleString("signing.keystore.alias", Setting.Property.NodeScope); public static final Setting> SIGNING_MESSAGE_TYPES = Setting.listSetting("signing.saml_messages", Collections.singletonList("*"), Function.identity(), Setting.Property.NodeScope); diff --git a/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlRealm.java b/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlRealm.java index bfb1a1cf541..40a79380ee5 100644 --- a/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlRealm.java +++ b/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 aliasSetting) { + private static X509Credential buildCredential(RealmConfig config, X509KeyPairSettings keyPairSettings, + Setting 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) { - throw new IllegalArgumentException("The configured encryption store for " - + RealmSettings.getFullSettingKey(config, keyPairSettings.getPrefix()) - + " does not have a key with alias [" + alias + "] (from setting " + - RealmSettings.getFullSettingKey(config, aliasSetting) - + ")"); + + String alias = aliasSetting.get(config.settings()); + if (Strings.isNullOrEmpty(alias)) { + + final Set aliases = new HashSet<>(); + 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); } diff --git a/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlRealmTests.java b/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlRealmTests.java index e6774b2c9d3..a7dcdc9bb5c 100644 --- a/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlRealmTests.java +++ b/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 certKeyPair1 = createKeyPair("RSA"); + final Tuple 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 certKeyPair1 = createKeyPair("RSA"); + final Tuple certKeyPair2 = createKeyPair("RSA"); + final Tuple 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 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 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);