From 24a36e684890799d98f0f6c7229271d44c830b11 Mon Sep 17 00:00:00 2001 From: Hynek Mlnarik Date: Fri, 9 Dec 2016 14:33:40 +0100 Subject: [PATCH] KEYCLOAK-4057 Do not include KeyName for brokered IdPs Active Directory Federation Services require that the subject name matches KeyName element when present. While KeyName is beneficial for Keycloak adapters, it breaks functionality for AD FS as the name included there is a key ID, not certificate subject expected by AD FS. This patch contains functionality that excludes KeyName from SAML messages to identity providers. This behaviour should be made configurable per client/identity provider and is prepared to do so, however actual GUI changes are left for a separate patch. --- .../saml/BaseSAML2BindingBuilder.java | 20 ++++---- .../util/XmlKeyInfoKeyNameTransformer.java | 50 +++++++++++++++++++ .../api/saml/v2/sig/SAML2Signature.java | 10 ++-- .../util/SignatureUtilTransferObject.java | 10 ++-- .../core/util/XMLSignatureUtil.java | 36 ++++++------- .../keycloak/broker/saml/SAMLEndpoint.java | 5 +- .../broker/saml/SAMLIdentityProvider.java | 8 +-- .../saml/SAMLIdentityProviderConfig.java | 19 +++++++ .../keycloak/protocol/saml/SamlClient.java | 20 ++++++++ .../protocol/saml/SamlConfigAttributes.java | 1 + .../keycloak/protocol/saml/SamlProtocol.java | 26 +++++++--- .../keycloak/protocol/saml/SamlService.java | 1 + 12 files changed, 156 insertions(+), 50 deletions(-) create mode 100644 saml-core/src/main/java/org/keycloak/saml/common/util/XmlKeyInfoKeyNameTransformer.java diff --git a/saml-core/src/main/java/org/keycloak/saml/BaseSAML2BindingBuilder.java b/saml-core/src/main/java/org/keycloak/saml/BaseSAML2BindingBuilder.java index f820a5ece4..ca1f82eec9 100755 --- a/saml-core/src/main/java/org/keycloak/saml/BaseSAML2BindingBuilder.java +++ b/saml-core/src/main/java/org/keycloak/saml/BaseSAML2BindingBuilder.java @@ -58,7 +58,7 @@ import static org.keycloak.saml.common.util.StringUtil.isNotNull; public class BaseSAML2BindingBuilder { protected static final Logger logger = Logger.getLogger(BaseSAML2BindingBuilder.class); - protected String signingKeyId; + protected String signingKeyName; protected KeyPair signingKeyPair; protected X509Certificate signingCertificate; protected boolean sign; @@ -86,27 +86,27 @@ public class BaseSAML2BindingBuilder { return (T)this; } - public T signWith(String signingKeyId, KeyPair keyPair) { - this.signingKeyId = signingKeyId; + public T signWith(String signingKeyName, KeyPair keyPair) { + this.signingKeyName = signingKeyName; this.signingKeyPair = keyPair; return (T)this; } - public T signWith(String signingKeyId, PrivateKey privateKey, PublicKey publicKey) { - this.signingKeyId = signingKeyId; + public T signWith(String signingKeyName, PrivateKey privateKey, PublicKey publicKey) { + this.signingKeyName = signingKeyName; this.signingKeyPair = new KeyPair(publicKey, privateKey); return (T)this; } - public T signWith(String signingKeyId, KeyPair keyPair, X509Certificate cert) { - this.signingKeyId = signingKeyId; + public T signWith(String signingKeyName, KeyPair keyPair, X509Certificate cert) { + this.signingKeyName = signingKeyName; this.signingKeyPair = keyPair; this.signingCertificate = cert; return (T)this; } - public T signWith(String signingKeyId, PrivateKey privateKey, PublicKey publicKey, X509Certificate cert) { - this.signingKeyId = signingKeyId; + public T signWith(String signingKeyName, PrivateKey privateKey, PublicKey publicKey, X509Certificate cert) { + this.signingKeyName = signingKeyName; this.signingKeyPair = new KeyPair(publicKey, privateKey); this.signingCertificate = cert; return (T)this; @@ -271,7 +271,7 @@ public class BaseSAML2BindingBuilder { samlSignature.setX509Certificate(signingCertificate); } - samlSignature.signSAMLDocument(samlDocument, signingKeyId, signingKeyPair, canonicalizationMethodType); + samlSignature.signSAMLDocument(samlDocument, signingKeyName, signingKeyPair, canonicalizationMethodType); } public void signAssertion(Document samlDocument) throws ProcessingException { diff --git a/saml-core/src/main/java/org/keycloak/saml/common/util/XmlKeyInfoKeyNameTransformer.java b/saml-core/src/main/java/org/keycloak/saml/common/util/XmlKeyInfoKeyNameTransformer.java new file mode 100644 index 0000000000..cda01d3652 --- /dev/null +++ b/saml-core/src/main/java/org/keycloak/saml/common/util/XmlKeyInfoKeyNameTransformer.java @@ -0,0 +1,50 @@ +/* + * Copyright 2016 Red Hat, Inc. and/or its affiliates + * and other contributors as indicated by the @author tags. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.keycloak.saml.common.util; + +import java.security.cert.X509Certificate; + +/** + * + * @author hmlnarik + */ +public enum XmlKeyInfoKeyNameTransformer { + NONE { @Override public String getKeyName(String keyId, X509Certificate certificate) { return null; } }, + KEY_ID { @Override public String getKeyName(String keyId, X509Certificate certificate) { return keyId; } }, + CERT_SUBJECT { @Override public String getKeyName(String keyId, X509Certificate certificate) { + return certificate == null + ? null + : (certificate.getSubjectDN() == null + ? null + : certificate.getSubjectDN().getName()); + } } + ; + + public abstract String getKeyName(String keyId, X509Certificate certificate); + + public static XmlKeyInfoKeyNameTransformer from(String name, XmlKeyInfoKeyNameTransformer defaultValue) { + if (name == null) { + return defaultValue; + } + try { + return valueOf(name); + } catch (IllegalArgumentException ex) { + return defaultValue; + } + } +} + diff --git a/saml-core/src/main/java/org/keycloak/saml/processing/api/saml/v2/sig/SAML2Signature.java b/saml-core/src/main/java/org/keycloak/saml/processing/api/saml/v2/sig/SAML2Signature.java index 49c8df84cc..ef3e3bd736 100755 --- a/saml-core/src/main/java/org/keycloak/saml/processing/api/saml/v2/sig/SAML2Signature.java +++ b/saml-core/src/main/java/org/keycloak/saml/processing/api/saml/v2/sig/SAML2Signature.java @@ -121,7 +121,7 @@ public class SAML2Signature { * @throws MarshalException * @throws GeneralSecurityException */ - public Document sign(Document doc, String referenceID, String keyId, KeyPair keyPair, String canonicalizationMethodType) throws ParserConfigurationException, + public Document sign(Document doc, String referenceID, String keyName, KeyPair keyPair, String canonicalizationMethodType) throws ParserConfigurationException, GeneralSecurityException, MarshalException, XMLSignatureException { String referenceURI = "#" + referenceID; @@ -130,7 +130,7 @@ public class SAML2Signature { if (sibling != null) { SignatureUtilTransferObject dto = new SignatureUtilTransferObject(); dto.setDocumentToBeSigned(doc); - dto.setKeyId(keyId); + dto.setKeyName(keyName); dto.setKeyPair(keyPair); dto.setDigestMethod(digestMethod); dto.setSignatureMethod(signatureMethod); @@ -143,7 +143,7 @@ public class SAML2Signature { return XMLSignatureUtil.sign(dto, canonicalizationMethodType); } - return XMLSignatureUtil.sign(doc, keyId, keyPair, digestMethod, signatureMethod, referenceURI, canonicalizationMethodType); + return XMLSignatureUtil.sign(doc, keyName, keyPair, digestMethod, signatureMethod, referenceURI, canonicalizationMethodType); } /** @@ -154,11 +154,11 @@ public class SAML2Signature { * * @throws org.keycloak.saml.common.exceptions.ProcessingException */ - public void signSAMLDocument(Document samlDocument, String keyId, KeyPair keypair, String canonicalizationMethodType) throws ProcessingException { + public void signSAMLDocument(Document samlDocument, String keyName, KeyPair keypair, String canonicalizationMethodType) throws ProcessingException { // Get the ID from the root String id = samlDocument.getDocumentElement().getAttribute(ID_ATTRIBUTE_NAME); try { - sign(samlDocument, id, keyId, keypair, canonicalizationMethodType); + sign(samlDocument, id, keyName, keypair, canonicalizationMethodType); } catch (ParserConfigurationException | GeneralSecurityException | MarshalException | XMLSignatureException e) { throw new ProcessingException(logger.signatureError(e)); } diff --git a/saml-core/src/main/java/org/keycloak/saml/processing/core/util/SignatureUtilTransferObject.java b/saml-core/src/main/java/org/keycloak/saml/processing/core/util/SignatureUtilTransferObject.java index f8181fe222..c9dd5bd272 100755 --- a/saml-core/src/main/java/org/keycloak/saml/processing/core/util/SignatureUtilTransferObject.java +++ b/saml-core/src/main/java/org/keycloak/saml/processing/core/util/SignatureUtilTransferObject.java @@ -33,7 +33,7 @@ public class SignatureUtilTransferObject { private Document documentToBeSigned; - private String keyId; + private String keyName; private KeyPair keyPair; @@ -115,11 +115,11 @@ public class SignatureUtilTransferObject { this.x509Certificate = x509Certificate; } - public String getKeyId() { - return keyId; + public String getKeyName() { + return keyName; } - public void setKeyId(String keyId) { - this.keyId = keyId; + public void setKeyName(String keyName) { + this.keyName = keyName; } } \ No newline at end of file diff --git a/saml-core/src/main/java/org/keycloak/saml/processing/core/util/XMLSignatureUtil.java b/saml-core/src/main/java/org/keycloak/saml/processing/core/util/XMLSignatureUtil.java index 193af19dff..6ad6513602 100755 --- a/saml-core/src/main/java/org/keycloak/saml/processing/core/util/XMLSignatureUtil.java +++ b/saml-core/src/main/java/org/keycloak/saml/processing/core/util/XMLSignatureUtil.java @@ -216,7 +216,7 @@ public class XMLSignatureUtil { * @throws MarshalException * @throws GeneralSecurityException */ - public static Document sign(Document doc, Node nodeToBeSigned, String keyId, KeyPair keyPair, String digestMethod, + public static Document sign(Document doc, Node nodeToBeSigned, String keyName, KeyPair keyPair, String digestMethod, String signatureMethod, String referenceURI, X509Certificate x509Certificate, String canonicalizationMethodType) throws ParserConfigurationException, GeneralSecurityException, MarshalException, XMLSignatureException { @@ -238,7 +238,7 @@ public class XMLSignatureUtil { if (!referenceURI.isEmpty()) { propagateIDAttributeSetup(nodeToBeSigned, newDoc.getDocumentElement()); } - newDoc = sign(newDoc, keyId, keyPair, digestMethod, signatureMethod, referenceURI, x509Certificate, canonicalizationMethodType); + newDoc = sign(newDoc, keyName, keyPair, digestMethod, signatureMethod, referenceURI, x509Certificate, canonicalizationMethodType); // if the signed element is a SAMLv2.0 assertion we need to move the signature element to the position // specified in the schema (before the assertion subject element). @@ -279,10 +279,10 @@ public class XMLSignatureUtil { * @throws MarshalException * @throws XMLSignatureException */ - public static void sign(Element elementToSign, Node nextSibling, String keyId, KeyPair keyPair, String digestMethod, + public static void sign(Element elementToSign, Node nextSibling, String keyName, KeyPair keyPair, String digestMethod, String signatureMethod, String referenceURI, String canonicalizationMethodType) throws GeneralSecurityException, MarshalException, XMLSignatureException { - sign(elementToSign, nextSibling, keyId, keyPair, digestMethod, signatureMethod, referenceURI, null, canonicalizationMethodType); + sign(elementToSign, nextSibling, keyName, keyPair, digestMethod, signatureMethod, referenceURI, null, canonicalizationMethodType); } /** @@ -301,7 +301,7 @@ public class XMLSignatureUtil { * @throws XMLSignatureException * @since 2.5.0 */ - public static void sign(Element elementToSign, Node nextSibling, String keyId, KeyPair keyPair, String digestMethod, + public static void sign(Element elementToSign, Node nextSibling, String keyName, KeyPair keyPair, String digestMethod, String signatureMethod, String referenceURI, X509Certificate x509Certificate, String canonicalizationMethodType) throws GeneralSecurityException, MarshalException, XMLSignatureException { PrivateKey signingKey = keyPair.getPrivate(); @@ -309,7 +309,7 @@ public class XMLSignatureUtil { DOMSignContext dsc = new DOMSignContext(signingKey, elementToSign, nextSibling); - signImpl(dsc, digestMethod, signatureMethod, referenceURI, keyId, publicKey, x509Certificate, canonicalizationMethodType); + signImpl(dsc, digestMethod, signatureMethod, referenceURI, keyName, publicKey, x509Certificate, canonicalizationMethodType); } /** @@ -343,9 +343,9 @@ public class XMLSignatureUtil { * @throws XMLSignatureException * @throws MarshalException */ - public static Document sign(Document doc, String keyId, KeyPair keyPair, String digestMethod, String signatureMethod, String referenceURI, String canonicalizationMethodType) + public static Document sign(Document doc, String keyName, KeyPair keyPair, String digestMethod, String signatureMethod, String referenceURI, String canonicalizationMethodType) throws GeneralSecurityException, MarshalException, XMLSignatureException { - return sign(doc, keyId, keyPair, digestMethod, signatureMethod, referenceURI, null, canonicalizationMethodType); + return sign(doc, keyName, keyPair, digestMethod, signatureMethod, referenceURI, null, canonicalizationMethodType); } /** @@ -363,7 +363,7 @@ public class XMLSignatureUtil { * @throws MarshalException * @since 2.5.0 */ - public static Document sign(Document doc, String keyId, KeyPair keyPair, String digestMethod, String signatureMethod, String referenceURI, + public static Document sign(Document doc, String keyName, KeyPair keyPair, String digestMethod, String signatureMethod, String referenceURI, X509Certificate x509Certificate, String canonicalizationMethodType) throws GeneralSecurityException, MarshalException, XMLSignatureException { logger.trace("Document to be signed=" + DocumentUtil.asString(doc)); @@ -372,7 +372,7 @@ public class XMLSignatureUtil { DOMSignContext dsc = new DOMSignContext(signingKey, doc.getDocumentElement()); - signImpl(dsc, digestMethod, signatureMethod, referenceURI, keyId, publicKey, x509Certificate, canonicalizationMethodType); + signImpl(dsc, digestMethod, signatureMethod, referenceURI, keyName, publicKey, x509Certificate, canonicalizationMethodType); return doc; } @@ -390,7 +390,7 @@ public class XMLSignatureUtil { public static Document sign(SignatureUtilTransferObject dto, String canonicalizationMethodType) throws GeneralSecurityException, MarshalException, XMLSignatureException { Document doc = dto.getDocumentToBeSigned(); - String keyId = dto.getKeyId(); + String keyName = dto.getKeyName(); KeyPair keyPair = dto.getKeyPair(); Node nextSibling = dto.getNextSibling(); String digestMethod = dto.getDigestMethod(); @@ -404,7 +404,7 @@ public class XMLSignatureUtil { DOMSignContext dsc = new DOMSignContext(signingKey, doc.getDocumentElement(), nextSibling); - signImpl(dsc, digestMethod, signatureMethod, referenceURI, keyId, publicKey, dto.getX509Certificate(), canonicalizationMethodType); + signImpl(dsc, digestMethod, signatureMethod, referenceURI, keyName, publicKey, dto.getX509Certificate(), canonicalizationMethodType); return doc; } @@ -694,7 +694,7 @@ public class XMLSignatureUtil { throw logger.unsupportedType(key.toString()); } - private static void signImpl(DOMSignContext dsc, String digestMethod, String signatureMethod, String referenceURI, String keyId, PublicKey publicKey, + private static void signImpl(DOMSignContext dsc, String digestMethod, String signatureMethod, String referenceURI, String keyName, PublicKey publicKey, X509Certificate x509Certificate, String canonicalizationMethodType) throws GeneralSecurityException, MarshalException, XMLSignatureException { dsc.setDefaultNamespacePrefix("dsig"); @@ -718,22 +718,22 @@ public class XMLSignatureUtil { KeyInfo ki; if (includeKeyInfoInSignature) { - ki = createKeyInfo(keyId, publicKey, x509Certificate); + ki = createKeyInfo(keyName, publicKey, x509Certificate); } else { - ki = createKeyInfo(keyId, null, null); + ki = createKeyInfo(keyName, null, null); } XMLSignature signature = fac.newXMLSignature(si, ki); signature.sign(dsc); } - private static KeyInfo createKeyInfo(String keyId, PublicKey publicKey, X509Certificate x509Certificate) throws KeyException { + private static KeyInfo createKeyInfo(String keyName, PublicKey publicKey, X509Certificate x509Certificate) throws KeyException { KeyInfoFactory keyInfoFactory = fac.getKeyInfoFactory(); List items = new LinkedList<>(); - if (keyId != null) { - items.add(keyInfoFactory.newKeyName(keyId)); + if (keyName != null) { + items.add(keyInfoFactory.newKeyName(keyName)); } if (x509Certificate != null) { diff --git a/services/src/main/java/org/keycloak/broker/saml/SAMLEndpoint.java b/services/src/main/java/org/keycloak/broker/saml/SAMLEndpoint.java index 951d03ae8c..95afb09064 100755 --- a/services/src/main/java/org/keycloak/broker/saml/SAMLEndpoint.java +++ b/services/src/main/java/org/keycloak/broker/saml/SAMLEndpoint.java @@ -302,11 +302,12 @@ public class SAMLEndpoint { boolean postBinding = config.isPostBindingResponse(); if (config.isWantAuthnRequestsSigned()) { KeyManager.ActiveKey keys = session.keys().getActiveKey(realm); - binding.signWith(keys.getKid(), keys.getPrivateKey(), keys.getPublicKey(), keys.getCertificate()) + String keyName = config.getXmlSigKeyInfoKeyNameTransformer().getKeyName(keys.getKid(), keys.getCertificate()); + binding.signWith(keyName, keys.getPrivateKey(), keys.getPublicKey(), keys.getCertificate()) .signatureAlgorithm(provider.getSignatureAlgorithm()) .signDocument(); if (! postBinding && config.isAddExtensionsElementWithKeyInfo()) { // Only include extension if REDIRECT binding and signing whole SAML protocol message - builder.addExtension(new KeycloakKeySamlExtensionGenerator(keys.getKid())); + builder.addExtension(new KeycloakKeySamlExtensionGenerator(keyName)); } } try { diff --git a/services/src/main/java/org/keycloak/broker/saml/SAMLIdentityProvider.java b/services/src/main/java/org/keycloak/broker/saml/SAMLIdentityProvider.java index f96f15a07f..d7ea04287d 100755 --- a/services/src/main/java/org/keycloak/broker/saml/SAMLIdentityProvider.java +++ b/services/src/main/java/org/keycloak/broker/saml/SAMLIdentityProvider.java @@ -107,11 +107,12 @@ public class SAMLIdentityProvider extends AbstractIdentityProvider