From a45b5dcd90b2fe14698e518da156f4f0df80366e Mon Sep 17 00:00:00 2001 From: Daniel Kobras Date: Thu, 9 Mar 2023 10:13:04 +0100 Subject: [PATCH] Prefer cert over pubkey in SAML metadata If SAML key material was given as a certificate, consistently expose the certificate rather than just the public key when presenting SAML metadata info. This change ensures that the client obtains sufficient information (eg. issuer) to close the trust chain. Closes: #17549 Signed-off-by: Daniel Kobras --- .../api/saml/v2/sig/SAML2Signature.java | 9 ++-- .../core/util/XMLSignatureUtil.java | 4 +- .../broker/saml/SAMLIdentityProvider.java | 5 ++- .../testsuite/admin/IdentityProviderTest.java | 42 ++++++++++++++++++- 4 files changed, 48 insertions(+), 12 deletions(-) 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 09ce9d62f8..dd21333e3b 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 @@ -134,14 +134,11 @@ public class SAML2Signature { dto.setSignatureMethod(signatureMethod); dto.setReferenceURI(referenceURI); dto.setNextSibling(sibling); - - if (x509Certificate != null) { - dto.setX509Certificate(x509Certificate); - } + dto.setX509Certificate(x509Certificate); return XMLSignatureUtil.sign(dto, canonicalizationMethodType); } - return XMLSignatureUtil.sign(doc, keyName, keyPair, digestMethod, signatureMethod, referenceURI, canonicalizationMethodType); + return XMLSignatureUtil.sign(doc, keyName, keyPair, digestMethod, signatureMethod, referenceURI, x509Certificate, canonicalizationMethodType); } /** @@ -224,4 +221,4 @@ public class SAML2Signature { element.setIdAttribute(JBossSAMLConstants.ID.get(), true); } -} \ 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 b8a6ad6c2f..b9ea4d622c 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 @@ -723,9 +723,7 @@ public class XMLSignatureUtil { if (x509Certificate != null) { items.add(keyInfoFactory.newX509Data(Collections.singletonList(x509Certificate))); - } - - if (publicKey != null) { + } else if (publicKey != null) { items.add(keyInfoFactory.newKeyValue(publicKey)); } 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 07f2c727da..7bea27e1cc 100755 --- a/services/src/main/java/org/keycloak/broker/saml/SAMLIdentityProvider.java +++ b/services/src/main/java/org/keycloak/broker/saml/SAMLIdentityProvider.java @@ -89,6 +89,7 @@ import javax.xml.stream.XMLStreamWriter; import java.io.StringWriter; import java.net.URI; import java.security.KeyPair; +import java.security.cert.X509Certificate; import java.util.ArrayList; import java.util.Arrays; import java.util.Iterator; @@ -457,13 +458,15 @@ public class SAMLIdentityProvider extends AbstractIdentityProviderStian Thorgersen */ @@ -1044,7 +1055,7 @@ public class IdentityProviderTest extends AbstractAdminTest { } @Test - public void testSamlExportSignatureOn() throws URISyntaxException, IOException, ConfigurationException, ParsingException, ProcessingException { + public void testSamlExportSignatureOn() throws URISyntaxException, IOException, ConfigurationException, ParsingException, ProcessingException, CertificateEncodingException { // Use import-config to convert IDPSSODescriptor file into key value pairs // to use when creating a SAML Identity Provider MultipartFormDataOutput form = new MultipartFormDataOutput(); @@ -1059,6 +1070,7 @@ public class IdentityProviderTest extends AbstractAdminTest { // Explicitly enable SP Metadata Signature result.put(SAMLIdentityProviderConfig.SIGN_SP_METADATA, "true"); + result.put(SAMLIdentityProviderConfig.XML_SIG_KEY_INFO_KEY_NAME_TRANSFORMER, XmlKeyInfoKeyNameTransformer.CERT_SUBJECT.name()); // Create new SAML identity provider using configuration retrieved from import-config IdentityProviderRepresentation idpRep = createRep("saml", "saml", true, result); @@ -1073,6 +1085,32 @@ public class IdentityProviderTest extends AbstractAdminTest { Document document = DocumentUtil.getDocument(body); Element signatureElement = DocumentUtil.getDirectChildElement(document.getDocumentElement(), XMLDSIG_NSURI.get(), "Signature"); - Assert.assertNotNull(signatureElement); + Assert.assertThat("Signature not null", signatureElement, notNullValue()); + + Element keyInfoElement = DocumentUtil.getDirectChildElement(signatureElement, XMLDSIG_NSURI.get(), "KeyInfo"); + Assert.assertThat("KeyInfo not null", keyInfoElement, notNullValue()); + + Element x509DataElement = DocumentUtil.getDirectChildElement(keyInfoElement, XMLDSIG_NSURI.get(), "X509Data"); + Assert.assertThat("X509Data not null", x509DataElement, notNullValue()); + + Element x509CertificateElement = DocumentUtil.getDirectChildElement(x509DataElement, XMLDSIG_NSURI.get(), "X509Certificate"); + Assert.assertThat("X509Certificate not null", x509CertificateElement, notNullValue()); + + Element keyNameElement = DocumentUtil.getDirectChildElement(keyInfoElement, XMLDSIG_NSURI.get(), "KeyName"); + Assert.assertThat("KeyName not null", keyNameElement, notNullValue()); + + String activeSigCert = KeyUtils.findActiveSigningKey(realm, Constants.DEFAULT_SIGNATURE_ALGORITHM).getCertificate(); + Assert.assertThat("activeSigCert not null", activeSigCert, notNullValue()); + + X509Certificate activeX509SigCert = XMLSignatureUtil.getX509CertificateFromKeyInfoString(activeSigCert); + Assert.assertThat("KeyName matches subject DN", + keyNameElement.getTextContent().trim(), equalTo(activeX509SigCert.getSubjectDN().getName())); + + Assert.assertThat("Signing cert matches active realm cert", + x509CertificateElement.getTextContent().trim(), equalTo(Base64.getEncoder().encodeToString(activeX509SigCert.getEncoded()))); + + PublicKey activePublicSigKey = activeX509SigCert.getPublicKey(); + Assert.assertThat("Metadata signature is valid", + new SAML2Signature().validate(document, new HardcodedKeyLocator(activePublicSigKey)), is(true)); } }