Mandus Elfving (Migrated from SES-58) said:

In SAMLLogoutProcessingFilter on line 123 the incoming message is retrieved by calling samlMessageContext.getInboundSAMLMessage() but six lines down (line 129) the message is retrieved by using samlMessageContext.getInboundMessage() instead. This makes automatic logout fail if the Logout is initialized by the IDP (i.e. it sends a LogoutRequest message) if I use the SOAP binding which when the message is retrieved using getInboundMessage returns the SOAP Envelope instead of the LogoutRequest message.

Migrated from SES-26

Phil Varner (Migrated from SES-37) said:

According to the saml-metadata-2.0-os spec, multiple EntityDescriptor elements must be enclosed by an EntitiesDescriptor element. The currently implementation only handles a single EntityDescriptor at the root of the metadata, in MetadataManager#parseProvider.

Jim Sellers (Migrated from SES-52) said:

in SAMLObject and SAMLCollection, they have lines like:
throw new IOException("Error serializing SAML object", e);

Even though the compiler is set up as java 5, targeting java 5, it will still allow you to put in invalid calls.

http://java.sun.com/j2se/1.5.0/docs/api/java/io/IOException.html
http://java.sun.com/javase/6/docs/api/java/io/IOException.html

Brian Parent (Migrated from SES-59) said:

Could you implement the second constructor from LogoutFilter? The current SAMLLogoutFilter does not allow you to use LogoutSuccessHandlers.

Rob Moore (Migrated from SES-49) said:

The index.jsp and idpSelection.jsp do not produce proper HTML and some browsers (notably Safari) doesn't render them correctly. I'm attaching a patch to address this.

Mandus Elfving (Migrated from SES-8) said:

Implement functionality for accessing the attributes that may be available in the response from the IDP.

Vladimir Schäfer (Migrated from SES-75) said:

Metadata manager should be able to automatically refresh in case underlaying metadata source changes.

Martin Rösel (Migrated from SES-28) said:

I wonder if it would be possible to extend the SAMLAuthenticationProvider so that it optionally would create the AuthenticationToken with the principal containing the UserDetails instead of the String representation of the username.

This feature could be implemented like in the class
org.springframework.security.authentication.dao.AbstractUserDetailsAuthenticationProvider

I have attached my suggestion for the SAMLAuthenticationProvider.

Perhaps you could consider this.

Anyway I would like to thank you Vladimir for contributing your SAML Security Extension to the public.

Martin

Vladimir Schäfer (Migrated from SES-57) said:

Bindings supported by the extension are currently configured inside AbstractProfileBase and use default implementations. In some cases behavior of the bindings may need to be customized. The customization should be available in the spring configuration together with list of available bindings.

Rob Moore (Migrated from SES-39) said:

We're accessing our application via a reverse-proxy (Pound) and running within a Jetty container. It appears that the Jetty code is returning the correct value for HttpServletRequest.getScheme() but not for HttpServletRequest.isSecure(). The MetadataGenerator class uses two separate ways of generating URLs that are nearly identical except for the case that getServerUrl() uses HttpServletRequest.isSecure() to determine the scheme to use while getEntityID() uses HttpServletRequest.getScheme(). As a result, we have metadata with a correct entity ID but incorrect location URLs. I'd like to propose that HttpServletRequest.getScheme() be used in both cases to retrieve the scheme to use. Perhaps I'm missing a nuance regarding why different approaches were used in each case but I'm hoping not.

I'm providing a patch with this change but I believe a better solution might be to use common code in both cases. So perhaps a method like this:

private String buildUrl(HttpServletRequest request) {
    StringBuilder url = new StringBuilder();
    url.append(request.getScheme()).append("://");
    url.append(request.getServerName()).append(":").append(request.getServerPort());
    url.append(request.getContextPath());
    return url.toString();
}

William Miller (Migrated from SES-72) said:

The metadata XML is "prettyPrintXML"-ed before being served. This invalids the signature in the metadata.
In src\org\springframework\security\saml\metadata\MetadataDisplayFilter.java, changing line 121 from:
writer.print(XMLHelper.prettyPrintXML(element));
To:
writer.print(XMLHelper.nodeToString(element));
fixes the issue.
This might also be fixed by computing the signature ignoring whitespace but I'm not strong in the area of XML sigs.

Vladimir Schäfer (Migrated from SES-60) said:

In some cases important events in the SAML processing might need to be logged for auditing purposes. Integrators should be able to plug a custom implementation enabling filtering of the messages and sending them to a custom storage.

Mandus Elfving (Migrated from SES-43) said:

To make the SAML implementation more complete artifact resolution should be added. Supplied is both support for retrieving artifacts through a back channel as well as a start of an implementation of the Artifact Resolution profile.

Because OpenSAML doesn't support artifact decoding an implementation for doing just that is included in the file HTTPArtifactDecoder and supporting files can be found in the org.opensaml package. For now it only supports incoming GET requests and needs to be extended to support POST requests as well. The code has been submitted to the OpenSAML issue tracker (https://bugs.internet2.edu/jira/browse/JOST-90), but there seem to be little interest in committing it and as such I suggest that the code be added to the repository for now and once we get the functionality in OpenSAML we can start using that instead.

The Artifact Resolution profile is implemented as specified in the standard documents and supplied is a basic filter that handles incoming artifact resolution messages and responds with the artifact. The filter supports some white listing of allowed clients and authentication using a supplied username and password, however this should be integrated with the rest of Spring Security using the supplied authentication and other security facilities.

The code still needs to be reviewed to assert that it conforms to the SAML standard, both the decoder and the profile. It has been tested to work with one third party SAML IDP implementation but still needs to be tested with other implementations. And last but not least, unit tests still needs to be written for the code.

Vladimir Schäfer (Migrated from SES-79) said:

It should be possible to autowire saml beans to simplify configuration and simplify future refactoring.

Phil Varner (Migrated from SES-51) said:

In AbstractProfileBase, this method is used to verify the signature of a message:

protected void verifySignature(Signature signature, String IDPEntityID) throws org.opensaml.xml.security.SecurityException, ValidationException {
SAMLSignatureProfileValidator validator = new SAMLSignatureProfileValidator();
validator.validate(signature);
CriteriaSet criteriaSet = new CriteriaSet();
criteriaSet.add(new EntityIDCriteria(IDPEntityID));
criteriaSet.add(new MetadataCriteria(IDPSSODescriptor.DEFAULT_ELEMENT_NAME, SAMLConstants.SAML20P_NS));
criteriaSet.add(new UsageCriteria(UsageType.SIGNING));
log.debug("Verifying signature", signature);
trustEngine.validate(signature, criteriaSet);
}

However, trustEngine.validate (SignatureTrustEngine.validate) returns "false" if the signature is invalid, rather than throwing a ValidationException as I believe this method is expecting. According to the javadoc for this method:

 * @return true if the signature was valid for the provided content
 * 
 * @throws SecurityException thrown if there is a problem attempting to verify the signature such as the signature
 *             algorithim not being supported

Mandus Elfving (Migrated from SES-48) said:

In the initialize method of JKSKeyManmager an InputStream is opened but never closed when the method is done with it. Attached is a patch to close the stream when the method is done using it.

Vladimir Schäfer (Migrated from SES-56) said:

Currently maximum interval between assertion create time and current time is hardcoded to 60s in the past and 3s in the future. The value should be made configurable.
Also values maxAssertionTime maxAuthenticationDelay should be made configurable.

Vladimir Schäfer (Migrated from SES-61) said:

It should be possible to specify requested AuthnContext and comparison method to be used in AuthnRequest sent to IDP.

Rob Moore (Migrated from SES-16) said:

I do not see this issue with the original zip file that was provided in SEC-1004 (SpringSecurity SAML.zip) but see it consistently with the war built from Subversion source (revision 53).

I'm getting this exception when I make a request to the application via a browser:

java.lang.IllegalArgumentException: org.opensaml.xml.signature.impl.KeyInfoImpl cannot be added - it is already the child of another SAML Object
at org.opensaml.xml.AbstractXMLObject.prepareForAssignment(AbstractXMLObject.java:272)
at org.opensaml.saml2.metadata.impl.KeyDescriptorImpl.setKeyInfo(KeyDescriptorImpl.java:79)
at org.springframework.security.saml.metadata.MetadataGenerator.getKeyDescriptor(MetadataGenerator.java:125)
at org.springframework.security.saml.metadata.MetadataGenerator.buildSPSSODescriptor(MetadataGenerator.java:116)
at org.springframework.security.saml.metadata.MetadataGenerator.generateMetadata(MetadataGenerator.java:96)
at org.springframework.security.saml.metadata.MetadataDisplayFilter.initializeSystemMetadata(MetadataDisplayFilter.java:134)
at org.springframework.security.saml.metadata.MetadataDisplayFilter.doFilterHttp(MetadataDisplayFilter.java:88)
at org.springframework.security.ui.SpringSecurityFilter.doFilter(SpringSecurityFilter.java:53)
at org.springframework.security.util.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:390)
at org.springframework.security.saml.SAMLEntryPoint.doFilterHttp(SAMLEntryPoint.java:102)
at org.springframework.security.ui.SpringSecurityFilter.doFilter(SpringSecurityFilter.java:53)
at org.springframework.security.util.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:390)
at org.springframework.security.ui.logout.LogoutFilter.doFilterHttp(LogoutFilter.java:89)
at org.springframework.security.ui.SpringSecurityFilter.doFilter(SpringSecurityFilter.java:53)
at org.springframework.security.util.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:390)
at org.springframework.security.context.HttpSessionContextIntegrationFilter.doFilterHttp(HttpSessionContextIntegrationFilter.java:235)
at org.springframework.security.ui.SpringSecurityFilter.doFilter(SpringSecurityFilter.java:53)
at org.springframework.security.util.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:390)
at org.springframework.security.util.FilterChainProxy.doFilter(FilterChainProxy.java:175)
at org.springframework.web.filter.DelegatingFilterProxy.invokeDelegate(DelegatingFilterProxy.java:236)
at org.springframework.web.filter.DelegatingFilterProxy.doFilter(DelegatingFilterProxy.java:167)

Vladimir Schäfer (Migrated from SES-80) said:

In order to comply with http://kantarainitiative.org/confluence/display/eGov/eGovernment+Implementation+Profile+of+SAML+V2.0 the IDP Discovery (http://docs.oasis-open.org/security/saml/Post2.0/sstc-saml-idp-discovery.pdf) must be implemented.

Vladimir Schäfer (Migrated from SES-77) said:

It should be possible to supply additional processing instructions as part of the metadata. At least the following options should be available: signing key, encryption key, logout request/response signatures, artifact resolution signatures. Extension should be available for both local and remote service/identity providers.

William Miller (Migrated from SES-73) said:

Invalid signatures on SAML assertions DO NOT prevent the assertion from being accepted. This is a major security issue. This may also affect other signature validations on other SAML message types.

The code assumes that the signature trust engine will throw an exception when the signature is invalid when the semantics of the validate function states that false will be returned.

The return value is NOT being checked.

Line 237 in spring-security-saml/src/org/springframework/security/saml/websso/AbstractProfileBase.java must be changed to something such as:
if (! trustEngine.validate(signature, criteriaSet)) {
throw new ValidationException("Signature is not trusted or invalid");
}
or the return value from validate must be returned from the verifySignature funciton and checked by the caller.

Mandus Elfving (Migrated from SES-40) said:

When catching exceptions and throwing a new exception append the catched exception to the thrown exception to make it easier when debugging when an exception is thrown. Attached is a patch that fixes it where it is missing in the current code base.

Mandus Elfving (Migrated from SES-69) said:

We are using SAML for our production environment and username & password authentication for development. In production we an implementation of SAMLUserDetailsService to get information about the user and in development we use the corresponding functionality by implementing UserDetailsService (both return the same kind of object with information about the user). However the result returned from these two services is put in to different fields of the Authentication object depending on the authentication scheme used. When using SAML the object is put in to the details field and when using username & password authentication it is put into the principal field.

This makes it hard to have the same code to get at the object returned from the SAMLUserDetailsService/UserDetailsService as we have to know which authentication scheme was used. My personal opinion is that the SAML module should behave as the username & password authentication and put the data in the principal field.

Mandus Elfving (Migrated from SES-33) said:

When specifying a logout success URL to the SAMLLogoutProcessingFilter it is not picked up by the SimpleUrlLogoutSuccessHandler. The problem is that it doesn't use the getDefaultTargetUrl method when getting the default URL but the field defaultTargetUrl which fails with the custom implementation in SAMLProcessingLogoutFilter.

If the response.isCommited() check isn't necessary in the SimpleUrlLogoutSuccessHandler.onLogoutSuccess method then the constructor of SAMLProcessingLogoutFilter can be changed to super(logoutSuccessUrl, handlers) to fix this problem.

Mandus Elfving (Migrated from SES-41) said:

Attached is a patch that fixes most of the spelling mistakes that I have found in the current code base and cleans up the javadoc style a bit.

Rob Moore (Migrated from SES-42) said:

The current implementation caches the first request for metadata. As a result, requests subsequent to the initial request receive metadata referencing an invalid SP hostname in the event that the request is to a different host than the initial request. That is, if the first request is to server1 then all requests for metadata will reference server1 even if server2 was requested.

William Miller (Migrated from SES-74) said:

Many parts reference the metadata manager's getHostSPName function. However, depending on the order of requests, this value may not be available.

When not specifically configured, the value is derived from the generated metadata. The generation of the metadata occurs in the metadata display filter. It's possible that filters installed before the metadata display filter will reference getHostSPName before it has been set by the metadata display filter. Specifically, if you request /saml/login which is handled by SAMLEntryPoint and occurs before MetadataDisplayFilter in the filter change, you will receive an error. If /saml/metadata is requested first, requesting /saml/login works as expect.

Unfortunately, since the generation of the metadata is based in the request, it's difficult to refactor it's initialization out of a filter. One fix is to move the initialization into SAMLEntryPoint. Ideally, it should be completely refactored out of any filters into either MetadataGenerator and/or MetadataManager.

Mandus Elfving (Migrated from SES-7) said:

Implement support for Single Logout.

According to the thread at http://jira.springframework.org/browse/SEC-1004 Aslak Knutsen has already implemented it and could maybe be contacted for a working implementation?

Mandus Elfving (Migrated from SES-5) said:

The current implementation makes Velocity log to the file velocity.log in the project root folder. Functionality should be added to make it log to SLF4J or any other logging library instead.

Supplied is a LogChute class that can be used with Velocity to make it log to SLF4J if the line:

velocityEngine.setProperty(VelocityEngine.RUNTIME_LOG_LOGSYSTEM, new SLF4JLogChute());

is added to the constructor for CustomWebSSOProfileImpl.

Phil Varner (Migrated from SES-38) said:

In WebSSOProfileConsumerImpl#verifyAssertionConditions, if the local entity is not in the first AudienceRestriction, an exception is thrown. The relevant code is:

340 audience:
341 for (AudienceRestriction rest : conditions.getAudienceRestrictions()) {
342 if (rest.getAudiences().size() == 0) {
343 log.debug("No audit audience specified for the assertion");
344 throw new SAMLException("SAML response is invalid");
345 }
346 for (Audience aud : rest.getAudiences()) {
347 if (context.getLocalEntityId().equals(aud.getAudienceURI())) {
348 continue audience;
==> this should return rather than continue.

349 }
350 }

==> If it's not in the first AR, we get here and throw an exception

351 log.debug("Our entity is not the intended audience of the assertion");
352 throw new SAMLException("SAML response is not intended for this entity");
353 }

Mandus Elfving (Migrated from SES-4) said:

Add functionality to end a users session and make him/her reauthenticate with the IDP when the session expires according to the session notOnOrAfter in the SAML response message.

Vladimir Schäfer (Migrated from SES-76) said:

Operations like parsing of relevant descriptor based on hash from artifact should use a lookup map with cached values. Also lookups of descriptors by entity id should be cached instead of re-parsing XML on each request.

Rob Moore (Migrated from SES-45) said:

Currently the generated metadata states that requests will be signed. However, the code relies on the IDP's metadata to determine if the requests should be signed (see line 92 of WebSSOProfileImpl). This results in a case where Microsoft ADFS (formerly Geneva) rejects the request because it does not require authentication requests be signed as an IDP but enforces the SP's assertion that it will sign requests it sends to the IDP.

This can be worked around if the IDP is configured to require signed requests (http://social.msdn.microsoft.com/Forums/en/Geneva/thread/88394bb2-9dad-45fd-8dfa-60155d2af37c) but there may be instances where this kind of configuration isn't possible across the board (that is, there may be a mix of SP types -- some that support/require signing and some that do not).

I suggest that the SP's descriptor's value be used instead or that both descriptors be consulted to determine whether signing is required.

Vladimir Schäfer (Migrated from SES-78) said:

Metadata descriptor supports settings which currently require manual changes to put them in place. User interface should provide means to generate the file based on user preferences and sign.

GWA (Migrated from SES-70) said:

If you give a metadata generated by this module to an IDP that validate the metadata signature, the metadata will be rejected

this is because the signature is calculated with a non-formated metadata and metadata filter return a pretty printed xml.

The provided patch remove the pretty print, thus Idp can validate metadata.

(you can always read formated xml through your browser)

Mandus Elfving (Migrated from SES-17) said:

On line 269 in WebSSOProfileConsumerImpl the Recipient of the Subject element in the message is supposed to be checked. However as well as checking for a correct Recipient value the binding used is also checked to be correct. This check is wrong as it checks the communicationProfileId which is never set in the message context and therefore we get a NullPointerException.

From what I can deduce from the code and the SAML standard this binding check has nothing to do with the Subject in question and it is not required to be done, there only needs to be a check that the Recipient is correct. If it however is to be done shouldn't it use the inboundSAMLProtocol field (information found in the message) instead of the communicationProfileId?

Expected code from revision 54:

if (context.getInboundSAMLProtocol().equals(service.getBinding()) && service.getLocation().equals(data.getRecipient())) {

Charles Wang (Migrated from SES-18) said:

Currently there is no SOAP binding implemented for the SAML module. So web service clients will not be able to use Spring Security's SAML support.

Rob Moore (Migrated from SES-44) said:

Apparently Microsoft's ADFS (formerly Geneva) does not support the scoping element in requests (http://social.msdn.microsoft.com/Forums/en-SG/Geneva/thread/537603e7-3457-43bf-9d1e-c0e23fbd3d54).

I have found references (http://saml2int.org/profile/current) that state that the IDP must support the absence of a scoping element but nothing that asserts that the scoping element must be supported by the IDP:

It seems like the element should be optional in cases where the IDP does not support it and/or scoping is not necessary/desired.

GWA (Migrated from SES-67) said:

There is an error if you try to login to an IdP with the Http-POST binding.

The POST request will have a 'SAMLResponse' parameter instead of a 'SAMLRequest' parameter.

When i found this, i first believed that the error was in the HTTPPostEncoder class of OpenSAML library.
I update it to the latest version, (modifying the MetadataMemoryProvider to implements new method, correct the initialization process). It doesn't correct the problem.

After searching a little more, I found out that the HTTPPostEncoder used in the HTTPPostBinding class is from the wrong package (saml1 instead of saml2)...

I attach the patch to correct this.
This patch also includes the code to upgrade openSAML version to 2.4.1

Vladimir Schäfer (Migrated from SES-62) said:

It should be possible to specify required nameID policy in AuthnRequest.

pinot noir (Migrated from SES-14) said:

Key alias 'apollo' is hard-coded in org.springframework.security.saml.metadata.MetadataGenerator.
The spring-security saml extension would be easier to use if you could configure the key alias in the security context.

Martin Rösel (Migrated from SES-66) said:

The verification of Address in SubjectLocality in the AuthnStatement should be optional because some Identity providers send an address that cannot be verified.

GWA (Migrated from SES-68) said:

Actually you need to declare IDP metadata directly into the spring configuration file.

If you override correctly addMetadataProvider (and removeMetadataProvider) method in the MetadataManager class, you can manage idp in a easier way by adding them at runtime. (for instance i've overridden the metadata manager to scan a directory where my idp metadata are stored)

Here is a patch that override those method (and add some test too)

William Blackburn (Migrated from SES-50) said:

The SAML extension works as expected for IdP Initiated SSO with the exception that it does not respect a target-uri contained in the RelayState of the unsolicited Assertion. This means that applications using this method of authentication will always start on the 'defaultTargetURI' page defined in context.

I worked around this by updating SAMLProcessingFilter to add the relay state to the request:

if (samlMessageContext.getRelayState()!=null) {
String relayState = samlMessageContext.getRelayState();
logger.debug("Saving SAML RelayState in Request attribute: " + relayState);
if (isTargetUrl(relayState)) {
request.setAttribute(SAML_RELAYSTATE_TARGET_URL_ATTRIBUTE, relayState);
}
}

Then a custom AuthenticationSuccessHandler can obtain the uri from the request attributes, if set, and redirect accordingly.

I'm sure there is a better way to do this, but I could not find any other place where the RelayState was immediately available.

Rob Moore (Migrated from SES-47) said:

ADFS does not return a subject name identifier by default but can be configured to do so:

http://blogs.msdn.com/card/archive/2010/02/17/name-identifiers-in-saml-assertions.aspx

The issue occurs when ADFS has not been configured this way as no subject name identifier will be provided and the following error will occur:

java.lang.IllegalArgumentException: SAMLBase object can't be created with null object argument
at org.springframework.security.saml.parser.SAMLBase.(SAMLBase.java:64)
at org.springframework.security.saml.parser.SAMLObject.(SAMLObject.java:43)
at org.springframework.security.saml.SAMLCredential.(SAMLCredential.java:66)
at org.springframework.security.saml.websso.WebSSOProfileConsumerImpl.processResponse(WebSSOProfileConsumerImpl.java:206)
at org.springframework.security.saml.SAMLAuthenticationProvider.authenticate(SAMLAuthenticationProvider.java:87)
...

I think it would be useful to have the code in WebSSOProfileConsumerImpl.processResponse() check for the existence of the subject name identifier returned from context.getSubjectNameIdentifier() before attempting to create the SAMLCredential and log an error if it is null.

Martin Rösel (Migrated from SES-65) said:

It would be desirable to be able to configure the variable futureSkew used in AbstractProfileBase.isDateTimeSkewValid().

Mandus Elfving (Migrated from SES-3) said:

In WebSSOProfileConsumer the sessionNotOnOrAfter is checked in the wrong way, it throws an exception if the current time is before the session end time (it should throw an exception if the current time is after or equal to the session end time).

It currently looks like:

// Validate users session is still valid
if (auth.getSessionNotOnOrAfter() != null && auth.getSessionNotOnOrAfter().isAfter(new Date().getTime())) {
log.debug("Authentication session is not valid anymore", auth.getSessionNotOnOrAfter());
throw new CredentialsExpiredException("Users authentication is expired");
}

but should be checked for example as follows:

// Validate users session is still valid
if (auth.getSessionNotOnOrAfter() != null && !(new DateTime()).isBefore(auth.getSessionNotOnOrAfter())) {
logger.debug("Authentication session is not valid anymore", auth.getSessionNotOnOrAfter());
throw new CredentialsExpiredException("Users authentication is expired");
}

Rob Moore (Migrated from SES-46) said:

The current implementation does not check for encrypted assertions. In the case where the SAML reply is encrypted this means that no assertions are found. The code checks for encrypted values in other cases so this leads to unexpected behavior as it appears as if no assertion was found in the response when really the assertion was simply encrypted.

Rather than simply calling response.getAssertions() alone one approach would also call response.getEncryptedAssertions() and add the decrypted assertions to the list returned from the first call (assuming it's modifiable otherwise a fresh, independent list could be created). This two pass approach seems consistent with existing code (see attributes are handled in the code in WebSSOProfileConsumer.processResponse()).