Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support signing SAML metadata #14916

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

CrazyParanoid
Copy link

Closes gh-14801

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Apr 17, 2024
Copy link
Contributor

@jzheaux jzheaux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @CrazyParanoid! I've left some feedback inline.

@@ -238,6 +244,15 @@ private String serialize(EntitiesDescriptor entities) {
}
}

/**
* Configure whether to sign the metadata.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please indicate the default value in the JavaDoc.

/**
* Configure whether to sign the metadata.
*
* @since 6.3
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that we are in the RC phase, this will go into the 6.4 release instead. Please change the @since attribute to reflect this.

@jzheaux jzheaux changed the title Add support sign SAML metadata Add signing SAML metadata Apr 17, 2024
@jzheaux jzheaux changed the title Add signing SAML metadata Support signing SAML metadata Apr 17, 2024
@CrazyParanoid
Copy link
Author

Hi @jzheaux ! Thanks for your feedback. All your comments have been resolved.

@jzheaux jzheaux added this to the 6.4.0-M1 milestone Apr 18, 2024
@jzheaux jzheaux added type: enhancement A general enhancement in: saml2 An issue in SAML2 modules and removed status: waiting-for-triage An issue we've not yet triaged labels Apr 18, 2024
@longgt
Copy link

longgt commented May 2, 2024

@CrazyParanoid thanks for your contribution.
Regarding to Signing with OpenSAML here, the final hash should be computed right after we did marshal the EntityDescriptor at below method

If we sign EntityDescriptor before calling OpenSamlMetadataResolver.serialize, the marshaller in there will create a brand new XMLSignature object.
It causes the 2 signature elements in below serialized with empty content

<ds:DigestValue/>
<ds:SignatureValue/>

Where it should be

<ds:DigestValue>digest signature</ds:DigestValue>
<ds:SignatureValue>signature value</ds:SignatureValue>

https://github.com/alexo/SAML-2.0/blob/96363df652cff76cd7a04f200a7f58b988b7bfe7/java-opensaml/opensaml-xmlsec-impl/src/main/java/org/opensaml/xml/signature/impl/SignatureMarshaller.java#L99

Beside that, the test case is check only whether DigestValue and SignatureValue is existing or not.
It's better to check whether it contains value or not, in my opinion.

I noted all debug steps at my environment (with Spring Security 6.2.2) here
#13661 (comment)

Please correct if I misunderstood this situation.
Thanks.

@CrazyParanoid
Copy link
Author

Hi @longgt ! In the resolveWhenRelyingPartyAndSignMetadataSetThenMetadataMatches test the xml is serialized with:

<ds:DigestValue>K411jmOwS83oyZ5LDpixHzwmvi6dbmd/GAT2sW2Pdyc=</ds:DigestValue>
<ds:SignatureValue>
DQfd3Gl+04LYqDYEk3O7zwuDbA940PAA3F+BVQxSF+7QdmIEykaSL5Kj+kg3DGtpUgbQPwC7SyRz&#13;
hkTZ78v9JzNu1SP8rx5DxyrdRNS51LOQLQP1qFMeSjzEi1TXlHtp0Z35qF2dAycnIty4oZFk2zNk&#13;
Uvm2N1DkAtC5XSqKssI=
</ds:SignatureValue>

I can add a value check to the test, but apparently there is no problem here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: saml2 An issue in SAML2 modules type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sign SP Metadata
4 participants