Add HSM Support for Decrypting Assertions

[jzheaux]

It would be nice to allow for a custom decryption strategy in OpenSamlAuthenticationProvider. This would simplify delegating the assertion and principal decryption to a separate service.

[jzheaux]

Currently, OpenSamlAuthenticationProvider has a private field Converter<Saml2AuthenticationToken, Decrypter>. I'm not convinced that Decrypter is the correct thing to expose, though. I'd prefer that it be an interface. Since OpenSAML doesn't provide an interface for decryption, we could possibly use Consumer.

So, instead of:

Decrypter decrypter = // ... your custom decrypter
provider.setDecrypter((token) -> decrypter);

you'd do:

YourDecrypter decrypter = // ... your custom decrypter
provider.setResponseDecrypter((tuple) -> {
    EncryptedAssertion encrypted = tuple.getResponse().getEncryptedAssertions().get(0);
    Assertion assertion = decrypter.decrypt(encrypted);
    tuple.getResponse().getAssertions().add(assertion);
});

Or, for decrypting an assertion's subject, you'd do:

YourDecrypter decrypter = // ... your custom decrypter
provider.setAssertionDecrypter((tuple) -> {
    EncryptedID encrypted = tuple.getAssertion().getSubject().getEncryptedID();
    NameID name = decrypter.decrypt(encrypted);
    tuple.getAssertion().getSubject().setNameID(name);
});

While a touch more verbose, it provides a great deal more flexibility and encourages composition over inheritance.

[ryan13mt]

@jzheaux would something like this meet your explanation above? #9055

[jzheaux]

Thanks for the PR, @ryan13mt, I've left my feedback there.

[ryan13mt]

Hello @jzheaux thanks for closing this issue and merging to master.

I am finding a problem with the polishing commit. You added a check in d0581c9#diff-e1434c9a229d033aa3fe922ab7b9637a54ba5a2521711e3b362e70bc7f787833R511 where you are checking if a response is signed and if true, doing the elements decryption. Is there a reason this check was added?

I believe this is not correct since there are use cases where the idp will sign and encrypt the assertion but will leave the response unsigned as is in our case. Thus having an unsigned SAML Response with an encrypted signed Assertion should still decrypt the assertion and then checking that it has been signed before throwing the error "Either the response or one of the assertions is unsigned. Please either sign the response or all of the assertions.". or in my case it's throwing "No assertions found in response." since the encrypted assertions have not being decrypted yet.

With this new check, the library is now requiring that a response must be signed if it contains an encrypted assertion.

Thanks

[jzheaux]

Thanks, @ryan13mt, for the feedback. If the response is unsigned, there is no guarantee that the encryption was not altered in transit (see section 6.2), which appears unsafe to me.

Can you explain why you need the response to remain unsigned? Or, that is, what's the rationale for signing and then encrypting the assertions instead of encrypting the assertions and then signing the response?

[ryan13mt]

@jzheaux i agree completely. Saying that, i dont feel the Spring library should block something that is still technically allowed by the SAML protocol.

I found that these scenarios are all allowed by the protocol although some should never be used due to the obvious security concerns:

1. An unsigned SAML Response with an unsigned Assertion
2. An unsigned SAML Response with a signed Assertion
3. A signed SAML Response with an unsigned Assertion
4. A signed SAML Response with a signed Assertion
5. An unsigned SAML Response with an encrypted Assertion
6. An unsigned SAML Response with an encrypted signed Assertion
7. A signed SAML Response with an encrypted Assertion
8. A signed SAML Response with an encrypted signed Assertion

My main concern about this check is that it will technically be a breaking change for users(like me) that have already onboarded clients and will then force us to go to each individual Asserting Party and tell them to change configuration for SSO to work.

In the Spring-Security Documentation 13.1.3 point 6 of the diagram states:

"After that, the provider verifies the signature of each Assertion. If any signature is invalid, authentication fails. Also, if neither the response nor the assertions have signatures, authentication fails. It is required that either the response or the assertions have signatures."

Not sure on the way forward for this. Maybe we can set an explicit configuration on the OpenSamlAuthenticationProvider class to allow this scenario? That way the users will have to configure it manually and thus knowing the risks involved in doing so. It will of course not allow it by default but at least provide a way for users to still be able to do so if they choose.

Would like to thank you again Josh for all your time and knowledge on this subject. I am fairly new to this and have learned a lot from these discussions with you.

[jzheaux]

Good comments, @ryan13mt

I don't feel the Spring library should block something that is still technically allowed by the SAML protocol.

It's important to remember that some of those configurations are for other exchange mechanisms, like over TLS on the backchannel. It's not meant that they be allowed universally.

For example, Spring Security supports the WebSSO profile POST binding and probably won't support others for a while yet, so having everything unsigned is not a scenario we want to try and simplify.

My main concern about this check is that it will technically be a breaking change for users(like me) that have already onboarded clients and will then force us to go to each individual Asserting Party and tell them to change configuration for SSO to work.

How common of a setup (sign-then-encrypt) is this for you and what was the motivation for choosing that order? The reason I ask is two-fold:

• First, if it's insecure, it's probably in your best interest to migrate those clients as soon as possible. The 5.5 upgrade could be a motivating factor to prioritize that, especially if there's no specific reason you need it that way.
• Second, there are some encryption algorithms that may be acceptable, but I'll need some more time to research them. For example, the SAML spec warns specifically against CBC encryption. It may be safe to accept unsigned responses with GCM-encrypted, signed assertions. What encryption algorithm are you using?

[ryan13mt]

Thanks for the reply @jzheaux. Most of our clients use RSA-OAEP or RSA1_5

[jzheaux]

Sorry, I wasn't clear with my question, @ryan13mt. I think asymmetric algorithms are intended for encrypting the session key (see the <EncryptedKey>/<EncryptionAlgorithm> element).

What is the symmetric algorithm used to encrypt the assertion itself? (see the <EncryptedData>/<EncryptionAlgorithm> element)

[sarod]

Hi @jzheaux

In our app we expose a configuration UI that allows our customers to configure their SSO (SAML, OpenID) and we internally rely on spring-security to do the authentication.

Between two versions of our app we upgraded spring-security from 5.4 to 5.5 to benefit from some of the new features but it broke the authentication for some of our clients that did configure their SAML server to sign the Encrypted Assertion but did not sign the SAML response.
I don't know what is the motivation for those clients, it may be some limitation in their SAML server.

I understand that spring-security wants to prevent unsafe configuration and that's probably a good thing.

However in this case this is a breaking change so

• having a way to re-enable the previous behavior would help? Workaround provided in issue 10162 is good but cannot be applied to 5.5.x. Maybe backporting support for setResponseValidator to branch 5.5 would be the way to go?
• having a breaking change section in the release note for such case would have helped but I couldn't find any.

Also if this decision is made I think that when response is not signed but contains EncryptedAssertion authentication should fail with a clear message instead of "No assertions found in response." because this message is very misleading.

[jzheaux]

@sarod, thanks for the extra explanation. Agreed that additional documentation about it being a breaking change would help, and I've added #10219 to address this.

Since decrypting an unsigned response is demonstrably insecure in common cases, I'm hesitant to further simplify doing this. But there are some things I think we can do:

First, if AES-GCM encryption was used, I believe that Spring Security code could be modified to allow decryption without a response signature. Is that your case?

Second, if not, you can make the allowance by decrypting the response yourself like so:

public class MyAuthenticationProvider implements AuthenticationProvider {
    OpenSaml4AuthenticationProvider delegate = new OpenSaml4AuthenticationProvider();

    @Override
    public Authentication authenticate(Authentication authentication) {
        Saml2AuthenticationToken token = (Saml2AuthenticationToken) authentication;
        Response response = parse(token.getSaml2Response());
        decrypt(response);
        Saml2AuthenticationToken decrypted = new Saml2AuthenticationToken(
            token.getRelyingPartyRegistration(), serialize(response));
        return this.delegate.authenticate(decrypted);
    }

    public boolean supports(Authentication authentication) {
        return this.delegate.supports(authentication);
    }

    private Response parse(String response) {
        try {
            ParserPool parserPool = XMLObjectProviderRegistrySupport.getParserPool();
            InputStream responseStream = new ByteArrayInputStream(response.getBytes(StandardCharsets.UTF_8));
            return (Response) XMLObjectSupport.unmarshallFromInputStream(parserPool, responseStream);
        }
        catch (Exception ex) {
            throw new Saml2AuthenticationException(...);
        }
    }

    private void decrypt(Response response) {
        DecryptionParameters parameters = new DecryptionParameters();
	// ... set parameters as needed
	Decrypter decrypter = new Decrypter(parameters);
  	EncryptedAssertion encrypted = response.getEncryptedAssertions().get(0);
  	try {
            Assertion assertion = decrypter.decrypt(encrypted);
            response.getAssertions().add(assertion);
            response.getEncryptedAssertions().remove(encrypted);
  	} catch (Exception e) {
            throw new Saml2AuthenticationException(...);
  	}
    }

    private String serialize(Response response) {
        try {
            Element element = XMLObjectSupport.marshall(response);
            return SerializeSupport.nodeToString(element);
        }
        catch (MarshallingException ex) {
            throw new Saml2AuthenticationException(...);
        }
    }
}

Finally, yes I agree that the error message should improve, and I've opened #10220 to address that.

[sarod]

Hi @jzheaux,

To answer your question it seems that our client is using AES-CBC and so they are indeed using an insecure setup.

The provided workaround using a wrapping AuthenticationProvider should allow us to re-enable this insecure setup to give time for our customers to migrate to a more secure configuration so that's great.

Thanks for the detailed explanation and workaround.

[fink-artem]

@jzheaux Hi.
I think that if signing the encrypted context became required then it would be correct for this to appear in the metadata. To simplify configuration by metadata. To have something like the WantAssertionsSigned flag.
How do you think?

[thmarti]

Hi @jzheaux

Unfortunately the proposed workaround fails. It results in an Saml2AuthenticationException with message:
"Either the response or one of the assertions is unsigned. Please either sign the response or all of the assertions."
(thrown somewhere around line 434 in OpenSaml4AuthenticationProvider)

@sarod, thanks for the extra explanation. Agreed that additional documentation about it being a breaking change would help, and I've added #10219 to address this.

Since decrypting an unsigned response is demonstrably insecure in common cases, I'm hesitant to further simplify doing this. But there are some things I think we can do:

First, if AES-GCM encryption was used, I believe that Spring Security code could be modified to allow decryption without a response signature. Is that your case?

Second, if not, you can make the allowance by decrypting the response yourself like so:

public class MyAuthenticationProvider implements AuthenticationProvider {
    OpenSaml4AuthenticationProvider delegate = new OpenSaml4AuthenticationProvider();

    @Override
    public Authentication authenticate(Authentication authentication) {
        Saml2AuthenticationToken token = (Saml2AuthenticationToken) authentication;
        Response response = parse(token.getSaml2Response());
        decrypt(response);
        Saml2AuthenticationToken decrypted = new Saml2AuthenticationToken(
            token.getRelyingPartyRegistration(), serialize(response));
        return this.delegate.authenticate(decrypted);
    }

    public boolean supports(Authentication authentication) {
        return this.delegate.supports(authentication);
    }

    private Response parse(String response) {
        try {
            ParserPool parserPool = XMLObjectProviderRegistrySupport.getParserPool();
            InputStream responseStream = new ByteArrayInputStream(response.getBytes(StandardCharsets.UTF_8));
            return (Response) XMLObjectSupport.unmarshallFromInputStream(parserPool, responseStream);
        }
        catch (Exception ex) {
            throw new Saml2AuthenticationException(...);
        }
    }

    private void decrypt(Response response) {
        DecryptionParameters parameters = new DecryptionParameters();
	// ... set parameters as needed
	Decrypter decrypter = new Decrypter(parameters);
  	EncryptedAssertion encrypted = response.getEncryptedAssertions().get(0);
  	try {
            Assertion assertion = decrypter.decrypt(encrypted);
            response.getAssertions().add(assertion);
            response.getEncryptedAssertions().remove(encrypted);
  	} catch (Exception e) {
            throw new Saml2AuthenticationException(...);
  	}
    }

    private String serialize(Response response) {
        try {
            Element element = XMLObjectSupport.marshall(response);
            return SerializeSupport.nodeToString(element);
        }
        catch (MarshallingException ex) {
            throw new Saml2AuthenticationException(...);
        }
    }
}

Finally, yes I agree that the error message should improve, and I've opened #10220 to address that.

[fink-artem]

Hi @jzheaux

Unfortunately the proposed workaround fails. It results in an Saml2AuthenticationException with message: "Either the response or one of the assertions is unsigned. Please either sign the response or all of the assertions." (thrown somewhere around line 434 in OpenSaml4AuthenticationProvider)

@sarod, thanks for the extra explanation. Agreed that additional documentation about it being a breaking change would help, and I've added #10219 to address this.
Since decrypting an unsigned response is demonstrably insecure in common cases, I'm hesitant to further simplify doing this. But there are some things I think we can do:
First, if AES-GCM encryption was used, I believe that Spring Security code could be modified to allow decryption without a response signature. Is that your case?
Second, if not, you can make the allowance by decrypting the response yourself like so:

public class MyAuthenticationProvider implements AuthenticationProvider {
    OpenSaml4AuthenticationProvider delegate = new OpenSaml4AuthenticationProvider();

    @Override
    public Authentication authenticate(Authentication authentication) {
        Saml2AuthenticationToken token = (Saml2AuthenticationToken) authentication;
        Response response = parse(token.getSaml2Response());
        decrypt(response);
        Saml2AuthenticationToken decrypted = new Saml2AuthenticationToken(
            token.getRelyingPartyRegistration(), serialize(response));
        return this.delegate.authenticate(decrypted);
    }

    public boolean supports(Authentication authentication) {
        return this.delegate.supports(authentication);
    }

    private Response parse(String response) {
        try {
            ParserPool parserPool = XMLObjectProviderRegistrySupport.getParserPool();
            InputStream responseStream = new ByteArrayInputStream(response.getBytes(StandardCharsets.UTF_8));
            return (Response) XMLObjectSupport.unmarshallFromInputStream(parserPool, responseStream);
        }
        catch (Exception ex) {
            throw new Saml2AuthenticationException(...);
        }
    }

    private void decrypt(Response response) {
        DecryptionParameters parameters = new DecryptionParameters();
	// ... set parameters as needed
	Decrypter decrypter = new Decrypter(parameters);
  	EncryptedAssertion encrypted = response.getEncryptedAssertions().get(0);
  	try {
            Assertion assertion = decrypter.decrypt(encrypted);
            response.getAssertions().add(assertion);
            response.getEncryptedAssertions().remove(encrypted);
  	} catch (Exception e) {
            throw new Saml2AuthenticationException(...);
  	}
    }

    private String serialize(Response response) {
        try {
            Element element = XMLObjectSupport.marshall(response);
            return SerializeSupport.nodeToString(element);
        }
        catch (MarshallingException ex) {
            throw new Saml2AuthenticationException(...);
        }
    }
}

Finally, yes I agree that the error message should improve, and I've opened #10220 to address that.

I fixed it by changing the configuration of the decrypt

public class MyAuthenticationProvider implements AuthenticationProvider {

    private final OpenSamlAuthenticationProvider delegate = new OpenSamlAuthenticationProvider();
    private static final EncryptedKeyResolver encryptedKeyResolver = new ChainingEncryptedKeyResolver(
            Arrays.asList(new InlineEncryptedKeyResolver(), new EncryptedElementTypeEncryptedKeyResolver(),
                    new SimpleRetrievalMethodEncryptedKeyResolver()));

    @Override
    public Authentication authenticate(Authentication authentication) {
        Saml2AuthenticationToken token = (Saml2AuthenticationToken) authentication;
        Response response = parse(token.getSaml2Response());
        RelyingPartyRegistration registration = token.getRelyingPartyRegistration();
        decrypt(response, registration);
        Saml2AuthenticationToken decrypted = new Saml2AuthenticationToken(
                token.getRelyingPartyRegistration(), serialize(response));
        return this.delegate.authenticate(decrypted);
    }

    @Override
    public boolean supports(Class<?> authentication) {
        return this.delegate.supports(authentication);
    }

    private Response parse(String response) {
        try {
            ParserPool parserPool = XMLObjectProviderRegistrySupport.getParserPool();
            InputStream responseStream = new ByteArrayInputStream(response.getBytes(StandardCharsets.UTF_8));
            return (Response) XMLObjectSupport.unmarshallFromInputStream(parserPool, responseStream);
        } catch (Exception e) {
 throw new Saml2AuthenticationException(...);
        }
    }

    private void decrypt(Response response, RelyingPartyRegistration registration) {
        Decrypter decrypter = decrypter(registration);
        try {
            List<EncryptedAssertion> encryptedAssertions = response.getEncryptedAssertions();
            for (int i = 0; i < encryptedAssertions.size(); i++) {
                EncryptedAssertion encrypted = encryptedAssertions.get(i);
                Assertion assertion = decrypter.decrypt(encrypted);
                response.getAssertions().add(assertion);
                encryptedAssertions.remove(encrypted);
            }
        } catch (Exception e) {
 throw new Saml2AuthenticationException(...);
        }
    }

    private String serialize(Response response) {
        try {
            Element element = XMLObjectSupport.marshall(response);
            return SerializeSupport.nodeToString(element);
        } catch (MarshallingException e) {
               throw new Saml2AuthenticationException(...);
        }
    }

    private static Decrypter decrypter(RelyingPartyRegistration registration) {
        Collection<Credential> credentials = new ArrayList<>();
        for (Saml2X509Credential key : registration.getDecryptionX509Credentials()) {
            Credential cred = CredentialSupport.getSimpleCredential(key.getCertificate(), key.getPrivateKey());
            credentials.add(cred);
        }
        KeyInfoCredentialResolver resolver = new CollectionKeyInfoCredentialResolver(credentials);
        Decrypter decrypter = new Decrypter(null, resolver, encryptedKeyResolver);
        decrypter.setRootInNewDocument(true);
        return decrypter;
    }

[denis111]

@fink-artem I tried the updated workaround but I still have problems with Azure AD:
Some entries in log:
Failed to decrypt EncryptedKey, valid decryption key could not be resolved
Failed to decrypt EncryptedData using either EncryptedData KeyInfoCredentialResolver or EncryptedKeyResolver + EncryptedKey KeyInfoCredentialResolver
SAML Decrypter encountered an error decrypting element content: Failed to decrypt EncryptedData
And exception is:
Caused by: org.opensaml.xmlsec.encryption.support.DecryptionException: Failed to decrypt EncryptedData
at org.opensaml.xmlsec.encryption.support.Decrypter.decryptDataToDOM(Decrypter.java:541)

[denis111]

@fink-artem Sorry, nevermind! I had wrongly configurated decryption credentials in relying party registration. After fixing it the workaround works fine!.