From cc4299126f7dd6cd96db32d2d44cfc7855013281 Mon Sep 17 00:00:00 2001 From: German Muzquiz Date: Mon, 6 Jul 2020 13:06:05 -0500 Subject: [PATCH 1/2] feat(authn/saml): New optional SAML parameter for signing messages --- docs/commands.md | 1 + .../security/authn/saml/EditSamlCommand.java | 7 +++++++ .../config/model/v1/security/Saml.java | 2 ++ .../validate/v1/security/SamlValidator.java | 21 +++++++++++++++++++ .../spinnaker/v1/profile/SamlConfig.java | 4 ++++ 5 files changed, 35 insertions(+) diff --git a/docs/commands.md b/docs/commands.md index 71f3df760f..c4cae968e5 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -11177,6 +11177,7 @@ hal config security authn saml edit [parameters] * `--metadata`: The address to your identity provider's metadata XML file. This can be a URL or the path of a local file. * `--no-validate`: (*Default*: `false`) Skip validation. * `--service-address-url`: The address of the Gate server that will be accesible by the SAML identity provider. This should be the full URL, including port, e.g. [https://gate.org.com:8084/](https://gate.org.com:8084/). If deployed behind a load balancer, this would be the laod balancer's address. + * `--signature-digest`: Digest algorithm to sign SAML messages (optional). Valid values include "SHA1", "SHA256", "SHA384", "SHA512", "RIPEMD160" and "MD5". * `--user-attribute-mapping-email`: The email field returned from your SAML provider. * `--user-attribute-mapping-first-name`: The first name field returned from your SAML provider. * `--user-attribute-mapping-last-name`: The last name field returned from your SAML provider. diff --git a/halyard-cli/src/main/java/com/netflix/spinnaker/halyard/cli/command/v1/config/security/authn/saml/EditSamlCommand.java b/halyard-cli/src/main/java/com/netflix/spinnaker/halyard/cli/command/v1/config/security/authn/saml/EditSamlCommand.java index e7837bc154..4b96b21d81 100644 --- a/halyard-cli/src/main/java/com/netflix/spinnaker/halyard/cli/command/v1/config/security/authn/saml/EditSamlCommand.java +++ b/halyard-cli/src/main/java/com/netflix/spinnaker/halyard/cli/command/v1/config/security/authn/saml/EditSamlCommand.java @@ -81,6 +81,12 @@ public class EditSamlCommand extends AbstractEditAuthnMethodCommand { + ". If deployed behind a load balancer, this would be the laod balancer's address.") private URL serviceAddress; + @Parameter( + names = "--signature-digest", + description = + "Digest algorithm to sign SAML messages (optional). Valid values include \"SHA1\", \"SHA256\", \"SHA384\", \"SHA512\", \"RIPEMD160\" and \"MD5\".") + private String signatureDigest; + @Parameter( names = "--user-attribute-mapping-first-name", description = "The first name field returned from your SAML provider.") @@ -118,6 +124,7 @@ protected AuthnMethod editAuthnMethod(Saml s) { s.setKeyStorePassword(isSet(keystorePassword) ? keystorePassword : s.getKeyStorePassword()); s.setKeyStoreAliasName(isSet(keystoreAliasName) ? keystoreAliasName : s.getKeyStoreAliasName()); s.setServiceAddress(isSet(serviceAddress) ? serviceAddress : s.getServiceAddress()); + s.setSignatureDigest(isSet(signatureDigest) ? signatureDigest : s.getSignatureDigest()); if (isSet(metadata)) { if (metadata.startsWith("http")) { diff --git a/halyard-config/src/main/java/com/netflix/spinnaker/halyard/config/model/v1/security/Saml.java b/halyard-config/src/main/java/com/netflix/spinnaker/halyard/config/model/v1/security/Saml.java index 0007165852..fc5eac4c26 100644 --- a/halyard-config/src/main/java/com/netflix/spinnaker/halyard/config/model/v1/security/Saml.java +++ b/halyard-config/src/main/java/com/netflix/spinnaker/halyard/config/model/v1/security/Saml.java @@ -40,6 +40,8 @@ public class Saml extends AuthnMethod { private URL serviceAddress; + private String signatureDigest; + private UserAttributeMapping userAttributeMapping = new UserAttributeMapping(); @Data diff --git a/halyard-config/src/main/java/com/netflix/spinnaker/halyard/config/validate/v1/security/SamlValidator.java b/halyard-config/src/main/java/com/netflix/spinnaker/halyard/config/validate/v1/security/SamlValidator.java index 0364f961fd..5757af68a8 100644 --- a/halyard-config/src/main/java/com/netflix/spinnaker/halyard/config/validate/v1/security/SamlValidator.java +++ b/halyard-config/src/main/java/com/netflix/spinnaker/halyard/config/validate/v1/security/SamlValidator.java @@ -25,6 +25,8 @@ import java.io.IOException; import java.net.URI; import java.security.KeyStore; +import java.util.Arrays; +import java.util.Collection; import java.util.Collections; import lombok.val; import org.apache.commons.lang3.StringUtils; @@ -34,6 +36,13 @@ @Component public class SamlValidator extends Validator { + + // Known algorithms supported by Gate (SamlSsoConfig.SignatureAlgorithms). + // This is a copy of the known names so far, so we don't create a hard binary dependency between + // Halyard and Gate. + public static Collection KNOWN_DIGEST_ALGORITHMS = + Arrays.asList("SHA1", "SHA256", "SHA384", "SHA512", "RIPEMD160", "MD5"); + @Override public void validate(ConfigProblemSetBuilder p, Saml saml) { if (!saml.isEnabled()) { @@ -105,5 +114,17 @@ public void validate(ConfigProblemSetBuilder p, Saml saml) { } else if (!saml.getServiceAddress().getProtocol().equalsIgnoreCase("https")) { p.addProblem(Problem.Severity.WARNING, "Gate should operate on HTTPS"); } + + // Printing a warning instead of an error because Halyard doesn't depend on Gate, + // and we don't want to prevent installing Spinnaker if new algorithms are added to Gate but not + // to this validator + String digest = saml.getSignatureDigest(); + if (digest != null && !KNOWN_DIGEST_ALGORITHMS.contains(digest.toUpperCase())) { + p.addProblem( + Problem.Severity.WARNING, + String.format( + "Unrecognized SAML signatureDigest '%s'. Known algorithms are %s", + digest, KNOWN_DIGEST_ALGORITHMS)); + } } } diff --git a/halyard-deploy/src/main/java/com/netflix/spinnaker/halyard/deploy/spinnaker/v1/profile/SamlConfig.java b/halyard-deploy/src/main/java/com/netflix/spinnaker/halyard/deploy/spinnaker/v1/profile/SamlConfig.java index a2e4dbbcbc..0d83487a72 100644 --- a/halyard-deploy/src/main/java/com/netflix/spinnaker/halyard/deploy/spinnaker/v1/profile/SamlConfig.java +++ b/halyard-deploy/src/main/java/com/netflix/spinnaker/halyard/deploy/spinnaker/v1/profile/SamlConfig.java @@ -48,6 +48,8 @@ public class SamlConfig { String redirectHostname; String redirectBasePath; + String signatureDigest; + Saml.UserAttributeMapping userAttributeMapping; public SamlConfig(Security security) { @@ -77,6 +79,8 @@ public SamlConfig(Security security) { this.redirectBasePath = u.getPath(); } + this.signatureDigest = saml.getSignatureDigest(); + this.userAttributeMapping = saml.getUserAttributeMapping(); } } From 4d4e45c47811c2b3091457a8fc6dd49951f57c9d Mon Sep 17 00:00:00 2001 From: German Muzquiz Date: Tue, 7 Jul 2020 09:08:18 -0500 Subject: [PATCH 2/2] feat(authn/saml): SignatureDigest should be upper case supplied --- .../halyard/config/validate/v1/security/SamlValidator.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/halyard-config/src/main/java/com/netflix/spinnaker/halyard/config/validate/v1/security/SamlValidator.java b/halyard-config/src/main/java/com/netflix/spinnaker/halyard/config/validate/v1/security/SamlValidator.java index 5757af68a8..995c2f3ad4 100644 --- a/halyard-config/src/main/java/com/netflix/spinnaker/halyard/config/validate/v1/security/SamlValidator.java +++ b/halyard-config/src/main/java/com/netflix/spinnaker/halyard/config/validate/v1/security/SamlValidator.java @@ -119,7 +119,7 @@ public void validate(ConfigProblemSetBuilder p, Saml saml) { // and we don't want to prevent installing Spinnaker if new algorithms are added to Gate but not // to this validator String digest = saml.getSignatureDigest(); - if (digest != null && !KNOWN_DIGEST_ALGORITHMS.contains(digest.toUpperCase())) { + if (digest != null && !KNOWN_DIGEST_ALGORITHMS.contains(digest)) { p.addProblem( Problem.Severity.WARNING, String.format(