Skip to content

Commit bb18cf2

Browse files
miszobimiszobi
committed
#78: simplified validateSign, made sure always checks the right document
- validateSign now simpler, takes an xpath for the Signature to verify - added some tests for encrypted assertions, that require either the Response and/or Message sign, to verify
1 parent 40eacc6 commit bb18cf2

4 files changed

Lines changed: 138 additions & 126 deletions

File tree

‎core/src/main/java/com/onelogin/saml2/authn/SamlResponse.java‎

Lines changed: 8 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -160,6 +160,8 @@ public boolean isValid(String requestId) {
160160

161161
ArrayList<String> signedElements = processSignedElements();
162162

163+
final boolean hasSignedAssertion = signedElements.contains("Assertion");
164+
final boolean hasSignedResponse = signedElements.contains("Response");
163165
if (settings.isStrict()) {
164166
if (settings.getWantXMLValidation()) {
165167
if (!Util.validateXML(samlResponseDocument, SchemaFactory.SAML_SCHEMA_PROTOCOL_2_0)) {
@@ -244,11 +246,11 @@ public boolean isValid(String requestId) {
244246

245247
validateSubjectConfirmation(responseInResponseTo);
246248

247-
if (settings.getWantAssertionsSigned() && !signedElements.contains("Assertion")) {
249+
if (settings.getWantAssertionsSigned() && !hasSignedAssertion) {
248250
throw new Exception("The Assertion of the Response is not signed and the SP requires it");
249251
}
250252

251-
if (settings.getWantMessagesSigned() && !signedElements.contains("Response")) {
253+
if (settings.getWantMessagesSigned() && !hasSignedResponse) {
252254
throw new Exception("The Message of the Response is not signed and the SP requires it");
253255
}
254256
}
@@ -260,18 +262,12 @@ public boolean isValid(String requestId) {
260262
String fingerprint = settings.getIdpCertFingerprint();
261263
String alg = settings.getIdpCertFingerprintAlgorithm();
262264

263-
Document documentToValidate;
264-
if (signedElements.contains("Response")) {
265-
documentToValidate = samlResponseDocument;
266-
} else {
267-
if (encrypted) {
268-
documentToValidate = decryptedDocument;
269-
} else {
270-
documentToValidate = samlResponseDocument;
271-
}
265+
if (hasSignedResponse && !Util.validateSign(samlResponseDocument, cert, fingerprint, alg, Util.RESPONSE_SIGNATURE_XPATH)) {
266+
throw new Exception("Signature validation failed. SAML Response rejected");
272267
}
273268

274-
if (!Util.validateSign(documentToValidate, cert, fingerprint, alg, settings.getWantMessagesSigned(), settings.getWantAssertionsSigned())) {
269+
final Document documentToCheckAssertion = encrypted ? decryptedDocument : samlResponseDocument;
270+
if (hasSignedAssertion && !Util.validateSign(documentToCheckAssertion, cert, fingerprint, alg, Util.ASSERTION_SIGNATURE_XPATH)) {
275271
throw new Exception("Signature validation failed. SAML Response rejected");
276272
}
277273
}

‎core/src/main/java/com/onelogin/saml2/util/Util.java‎

Lines changed: 15 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -782,52 +782,26 @@ public static String signatureAlgConversion(String sign) {
782782
return convertedSignatureAlg;
783783
}
784784

785-
/**
786-
* Validate signature (Message or Assertion).
787-
*
788-
* @param doc
789-
* The document we should validate
790-
* @param cert
791-
* The public certificate
792-
* @param fingerprint
793-
* The fingerprint of the public certificate
794-
* @param alg
795-
* The signature algorithm method
796-
* @param wantAssertionSigned whether the caller requires assertions to be signed
797-
* @param wantResponseSigned whether the caller requires responses to be signed
798-
*
799-
* @return True if the required signatures are present and the present signatures are valid, false otherwise.
800-
*/
801-
public static Boolean validateSign(Document doc, X509Certificate cert, String fingerprint, String alg,
802-
boolean wantResponseSigned, boolean wantAssertionSigned) {
785+
/**
786+
* Validate the signature pointed to by the xpath
787+
*
788+
* @param doc The document we should validate
789+
* @param cert The public certificate
790+
* @param fingerprint The fingerprint of the public certificate
791+
* @param alg The signature algorithm method
792+
* @param xpath the xpath of the ds:Signture node to validate
793+
*
794+
* @return True if the signature exists and is valid, false otherwise.
795+
*/
796+
public static boolean validateSign(final Document doc, final X509Certificate cert, final String fingerprint,
797+
final String alg, final String xpath) {
803798
try {
804-
boolean validResponseSignature = checkSignature(doc, cert, fingerprint, alg, wantResponseSigned, RESPONSE_SIGNATURE_XPATH);
805-
boolean validAssertionSignature = checkSignature(doc, cert, fingerprint, alg, wantAssertionSigned, ASSERTION_SIGNATURE_XPATH);
806-
807-
return validResponseSignature && validAssertionSignature;
799+
final NodeList signatures = query(doc, xpath);
800+
return signatures.getLength() == 1 && validateSignNode(signatures.item(0), cert, fingerprint, alg);
808801
} catch (XPathExpressionException e) {
809802
log.warn("Failed to find signature nodes", e);
810-
}
811-
return false;
812-
}
813-
814-
private static boolean checkSignature(Document doc, X509Certificate cert, String fingerprint, String alg, boolean required, final String xpath) throws XPathExpressionException {
815-
final NodeList responseSignature = query(doc, xpath);
816-
final int signatureCount = responseSignature.getLength();
817-
if (signatureCount > 1) {
818-
log.warn("Unexpected number of signatures found matching " + xpath + ": " + signatureCount);
819803
return false;
820804
}
821-
822-
if (signatureCount == 0) {
823-
if (required) {
824-
log.warn("No signature matching " + xpath + ", found but was required");
825-
return false;
826-
}
827-
return true;
828-
}
829-
830-
return validateSignNode(responseSignature.item(0), cert, fingerprint, alg);
831805
}
832806

833807
/**

‎core/src/test/java/com/onelogin/saml2/test/authn/AuthnResponseTest.java‎

Lines changed: 29 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -1449,39 +1449,44 @@ public void testIsValid2() throws Exception {
14491449
* @see com.onelogin.saml2.authn.SamlResponse#isValid
14501450
*/
14511451
@Test
1452-
public void testIsValidEnc() throws Exception {
1452+
public void testIsValid_doubleSignedEncrypted() throws Exception {
14531453
Saml2Settings settings = new SettingsBuilder().fromFile("config/config.my.properties").build();
1454-
settings.setWantAssertionsSigned(false);
1455-
settings.setWantMessagesSigned(false);
1454+
settings.setWantAssertionsSigned(true);
1455+
settings.setWantMessagesSigned(true);
14561456
String samlResponseEncoded = Util.getFileAsString("data/responses/double_signed_encrypted_assertion.xml.base64");
14571457

1458-
settings.setStrict(false);
1459-
SamlResponse samlResponse = new SamlResponse(settings, newHttpRequest(samlResponseEncoded));
1460-
assertTrue(samlResponse.isValid());
1461-
1462-
settings.setStrict(true);
1463-
samlResponse = new SamlResponse(settings, newHttpRequest(samlResponseEncoded));
1464-
assertTrue(samlResponse.isValid());
1458+
assertResponseValid(settings, samlResponseEncoded, false, true, null);
1459+
assertResponseValid(settings, samlResponseEncoded, true, true, null);
1460+
}
14651461

1466-
samlResponseEncoded = Util.getFileAsString("data/responses/signed_message_encrypted_assertion.xml.base64");
1462+
@Test
1463+
public void testIsValid_signedResponseEncryptedAssertion() throws Exception {
1464+
Saml2Settings settings = new SettingsBuilder().fromFile("config/config.my.properties").build();
1465+
settings.setWantAssertionsSigned(false);
1466+
settings.setWantMessagesSigned(true);
14671467

1468-
settings.setStrict(false);
1469-
samlResponse = new SamlResponse(settings, newHttpRequest(samlResponseEncoded));
1470-
assertTrue(samlResponse.isValid());
1468+
String samlResponseEncoded = Util.getFileAsString("data/responses/signed_message_encrypted_assertion.xml.base64");
14711469

1472-
settings.setStrict(true);
1473-
samlResponse = new SamlResponse(settings, newHttpRequest(samlResponseEncoded));
1474-
assertTrue(samlResponse.isValid());
1470+
assertResponseValid(settings, samlResponseEncoded, false, true, null);
1471+
assertResponseValid(settings, samlResponseEncoded, true, true, null);
1472+
settings.setWantAssertionsSigned(true);
1473+
assertResponseValid(settings, samlResponseEncoded, false, true, null);
1474+
assertResponseValid(settings, samlResponseEncoded, true, false, "The Assertion of the Response is not signed and the SP requires it");
1475+
}
14751476

1476-
samlResponseEncoded = Util.getFileAsString("data/responses/signed_encrypted_assertion.xml.base64");
1477+
@Test
1478+
public void testIsValid_signedEncryptedAssertion() throws Exception {
1479+
Saml2Settings settings = new SettingsBuilder().fromFile("config/config.my.properties").build();
1480+
settings.setWantAssertionsSigned(true);
1481+
settings.setWantMessagesSigned(false);
14771482

1478-
settings.setStrict(false);
1479-
samlResponse = new SamlResponse(settings, newHttpRequest(samlResponseEncoded));
1480-
assertTrue(samlResponse.isValid());
1483+
String samlResponseEncoded = Util.getFileAsString("data/responses/signed_encrypted_assertion.xml.base64");
14811484

1482-
settings.setStrict(true);
1483-
samlResponse = new SamlResponse(settings, newHttpRequest(samlResponseEncoded));
1484-
assertTrue(samlResponse.isValid());
1485+
assertResponseValid(settings, samlResponseEncoded, false, true, null);
1486+
assertResponseValid(settings, samlResponseEncoded, true, true, null);
1487+
settings.setWantMessagesSigned(true);
1488+
assertResponseValid(settings, samlResponseEncoded, false, true, null);
1489+
assertResponseValid(settings, samlResponseEncoded, true, false, "The Message of the Response is not signed and the SP requires it");
14851490
}
14861491

14871492
/**

0 commit comments

Comments
 (0)