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

Provide debug hook point to trace original saml callback response #5401

Closed
imasahiro opened this issue Jan 22, 2024 · 2 comments · Fixed by #5622
Closed

Provide debug hook point to trace original saml callback response #5401

imasahiro opened this issue Jan 22, 2024 · 2 comments · Fixed by #5622
Labels
new feature sprint Issues for OSS Sprint participants

Comments

@imasahiro
Copy link
Member

Motivations:

Suggestions:

  • Passes already parsed MessageContext to loginFailed
  • Make HttpPostBindingUtil to public to make it easy to decode at user side
  • or provide a debug hook point to trace callback response on SamlAssertionConsumerFunction
@jrhee17 jrhee17 added the sprint Issues for OSS Sprint participants label Apr 1, 2024
@jrhee17
Copy link
Contributor

jrhee17 commented Apr 1, 2024

Background

Armeria provides a saml integration module, which allows users to easily set up a Service Provider.

The basic flow is as follows:

  1. A SamlDecorator checks if a request is authenticated or not
  • If it is already authenticated, then the request is served without an issue
  • If it is not authenticated, a SAML redirect or form is returned depending on the configuration
    • if (endpoint.bindingProtocol() == SamlBindingProtocol.HTTP_REDIRECT) {
      return responseWithLocation(toRedirectionUrl(
      arg.messageContext.getMessage(),
      endpoint.toUriString(), SAML_REQUEST,
      signingCredential, sp.signatureAlgorithm(),
      relayState));
      } else {
      // signing can incur a blocking call
      final String value = toSignedBase64(
      arg.messageContext.getMessage(),
      signingCredential,
      sp.signatureAlgorithm());
      final HttpData body = getSsoForm(endpoint.toUriString(),
      SAML_REQUEST, value,
      relayState);
      return HttpResponse.of(HttpStatus.OK, MediaType.HTML_UTF_8,
      body);
      }
  1. The user logs in to the Identity Provider
  2. The Idp redirects the user to send a request to the SamlService

There is also an integration test which can be analyzed for this behavior:

ref: https://github.com/line/armeria/blob/main/saml/src/test/java/com/linecorp/armeria/server/saml/SamlServiceProviderTest.java

In terms of this issue

It can be difficult to debug why an assertion is not being processed correctly due to complex the inherent complexity of the SAML protocol.

I think for normal cases, deserializing SAML messages is not a big pain point since 1) the SAML library provides good enough logging, and 2) the SAML message format is relatively straightforward.

The bigger pain point is probably determining why a message refused to be processed. This would probably involve passing the parsed message in the following line:

Passes already parsed MessageContext to loginFailed

So in my opinion this suggestion by @imasahiro should be good enough for starters.

i.e.

return ssoHandler.loginFailed(ctx, req, messageContext, e);

Note that this is simply how I would handle this issue if I were assigned - please feel free to think of any alternative ways to let users more easily debug this issue

@yeojin-dev
Copy link
Contributor

I'm looking this issue. 👀

minwoox pushed a commit that referenced this issue May 20, 2024
…y in SamlAssertionConsumerFunction (#5622)

Motivation:

This PR addresses a code enhancement in the SamlAssertionConsumerFunction by initializing the messageContext variable at the beginning of the method. This change is intended to streamline the assignment and handling of messageContext, ensuring that it can be consistently used throughout the method, particularly in exception handling scenarios.

Modifications:

Declared messageContext at the method start to allow its use across the entire method scope, including within try and catch blocks.

Result:

- Closes #5401 
- The refactor ensures messageContext is available for error handling.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature sprint Issues for OSS Sprint participants
Projects
None yet
4 participants