SAML Saml2WebSsoAuthenticationFilter SHOULD wrap AuthenticationConverter convert exception to AuthenticationException

[GitHanter]

Saml2WebSsoAuthenticationFilter doesn't catch AuthenticationConverter convert exception, we should wrapper the exception as a Saml2AuthenticationException(Saml2ErrorCodes.INTERNAL_VALIDATION_ERROR, ex.getMessage(), ex).

May be we can add a configure method to Saml2LoginConfigurer to configure property authenticationConverter of Saml2WebSsoAuthenticationFilter , e.g through getSharedOrBean so that developer can customize it as a Bean

[jzheaux]

Thanks, @GitHanter, for the suggestion.

Can you elaborate on what problems you are experiencing from the code the way that it is? The reason I ask is that it's probably a bit too aggressive to state that any exception from AuthenticationConverter is an INTERNAL_VALIDATION_ERROR. There may be ways to improve the error handling here, but I'll need more specifics to understand your situation.

[spring-projects-issues]

If you would like us to look at this issue, please provide the requested information. If the information is not provided within the next 7 days this issue will be closed.

[GitHanter]

Thanks, @GitHanter, for the suggestion.

Can you elaborate on what problems you are experiencing from the code the way that it is? The reason I ask is that it's probably a bit too aggressive to state that any exception from AuthenticationConverter is an INTERNAL_VALIDATION_ERROR. There may be ways to improve the error handling here, but I'll need more specifics to understand your situation.

Because the Saml2AuthenticationTokenConverter can throw Saml2Exception or any other non-AuthenticationException Exception, in case of that, the AbstractAuthenticationProcessingFilter will not able to catch the Exception. I think we need wrap the exception as a AuthenticationException , it can be Saml2AuthenticationException(Saml2ErrorCodes.INTERNAL_VALIDATION_ERROR...) or it can be Saml2AuthenticationException with another ErrorCode, so we can handle the exception in AuthenticationFailureHandler, currently I have to do following decorate

@Configuration
public class WebSecurityConfiguration extends WebSecurityConfigurerAdapter {

    @Override
    @SuppressWarnings("unchecked")
    protected void configure(HttpSecurity http) throws Exception {
        //make all exception thrown from Saml2WebSsoAuthenticationFilter AuthenticationException, so the
        //AuthenticationFailureHandler will take over control
        ObjectPostProcessor<Saml2WebSsoAuthenticationFilter> objectPostProcessor = new ObjectPostProcessor<Saml2WebSsoAuthenticationFilter>() {
            @Override
            public <O extends Saml2WebSsoAuthenticationFilter> O postProcess(O object) {
                Field field = ReflectionUtils.findField(Saml2WebSsoAuthenticationFilter.class, "authenticationConverter");
                ReflectionUtils.makeAccessible(Objects.requireNonNull(field));
                Saml2AuthenticationTokenConverter originalConverter = (Saml2AuthenticationTokenConverter) ReflectionUtils.getField(field, object);
                AuthenticationConverter decorator = (request) -> {
                    try {
                        return Objects.requireNonNull(originalConverter).convert(request);
                    } catch (AuthenticationException ex) {
                        throw ex;
                    } catch (Exception ex) {
                        throw new Saml2AuthenticationException(new Saml2Error(SAML_SSO_INTERNAL_ERROR, ex.getMessage()), ex);
                    }
                };
                ReflectionUtils.setField(field, object, decorator);

                return object;
            }
        };
}

[jzheaux]

Thanks for the extra detail, @GitHanter.

I wonder if the following would be simpler:

AuthenticationConverter converter = new Saml2AuthenticationTokenConverter(...);
http
    .saml2Login((saml2) -> saml2
        .authenticationConverter(new MyExceptionWrappingAuthenticationConverter(converter))
    );

Would that work to remove the reflection?

It may be valuable to change Saml2AuthenticationTokenConverter to use Saml2AuthenticationException since otherwise the unsuccessfulAuthentication method won't get invoked.

To better understand the right error code, under what circumstances are you getting an exception? Specifically, we need to be clear whether errors are the client's fault or the server's fault.

[GitHanter]

@jzheaux
Thanks for the authenticationConverter tips, we use spring-security v5.3.6.RELEASE so we didn't see the authenticationConverter configure method in class Saml2LoginConfigurer, after upgrade to latest version, we can do exactly you specified in the comment to remove the reflection.

For Saml2AuthenticationException issue, you can ref org.springframework.security.web.authentication.www.BasicAuthenticationConverter, this AuthenticationConverter handled convert exception internally and wrapper it to a BadCredentialsException

@Override
	public UsernamePasswordAuthenticationToken convert(HttpServletRequest request) {
		String header = request.getHeader(AUTHORIZATION);
		if (header == null) {
			return null;
		}

		header = header.trim();
		if (!StringUtils.startsWithIgnoreCase(header, AUTHENTICATION_SCHEME_BASIC)) {
			return null;
		}

		byte[] base64Token = header.substring(6).getBytes(StandardCharsets.UTF_8);
		byte[] decoded;
		try {
			decoded = Base64.getDecoder().decode(base64Token);
		}
		catch (IllegalArgumentException e) {
			throw new BadCredentialsException(
					"Failed to decode basic authentication token");
		}

		String token = new String(decoded, getCredentialsCharset(request));

		int delim = token.indexOf(":");

		if (delim == -1) {
			throw new BadCredentialsException("Invalid basic authentication token");
		}
		UsernamePasswordAuthenticationToken result  = new UsernamePasswordAuthenticationToken(token.substring(0, delim), token.substring(delim + 1));
		result.setDetails(this.authenticationDetailsSource.buildDetails(request));
		return result;
	}

For Saml2AuthenticationTokenConverter

@Override
	public Saml2AuthenticationToken convert(HttpServletRequest request) {
		RelyingPartyRegistration relyingPartyRegistration = this.relyingPartyRegistrationResolver.convert(request);
		if (relyingPartyRegistration == null) {
			return null;
		}
		String saml2Response = request.getParameter("SAMLResponse");
		if (saml2Response == null) {
			return null;
		}
		byte[] b = samlDecode(saml2Response);
		saml2Response = inflateIfRequired(request, b);
		return new Saml2AuthenticationToken(relyingPartyRegistration, saml2Response);
	}

	private String inflateIfRequired(HttpServletRequest request, byte[] b) {
		if (HttpMethod.GET.matches(request.getMethod())) {
			return samlInflate(b);
		}
		return new String(b, StandardCharsets.UTF_8);
	}

	private byte[] samlDecode(String s) {
		return BASE64.decode(s);
	}

If saml2Response is not a valid BASE64 encoded string or not deflated properly (client's fault), the covert method will throw the exception directly which will not handle by unsuccessfulAuthentication method.

[jzheaux]

Sounds great, @GitHanter. Would you be able to submit a PR that wraps the deflation and decoding and throws a Saml2AuthenticationException that uses Saml2ErrorCodes.INVALID_RESPONSE?

It would be great to include some tests: First, in Saml2AuthenticationTokenConverterTests for showing that when the converter can't deflate and decode the response, it throws an exception. Second, in Saml2LoginConfigurerTests that demonstrates that the authentication failed handler gets invoked.

[GitHanter]

@jzheaux Sure, I'll submit the PR with #9317

[jzheaux]

Sounds good, @GitHanter. In that case, please make sure that the two changes are in separate commits as this simplifies maintenance.