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 for load balancer and maxAuthenticationAge in SAML #32

Closed
wants to merge 2 commits into from

Conversation

johannestang
Copy link
Contributor

This pull request covers two issues I've come across when using SAML authentication.

First, when ShinyProxy/ContainerProxy is running behind a load balancer (or in my case a Kubernetes ingress controller) and SSL traffic is terminated at the load balancer, then authentication fails with the following error:

2019-10-14 09:23:05.704 ERROR 1 --- [  XNIO-2 task-2] o.o.c.b.decoding.BaseSAMLMessageDecoder  : SAML message intended destination endpoint 'https://shinyapp.host.domain.xyz/saml/SSO' did not match the recipient endpoint 'http://shinyapp.host.domain.xyz/saml/SSO'

The issue is fixed by using the SAMLContextProviderLB context provider.

The implementation exposes the following new configuration options under proxy.saml:

  • lb-server-name: Server name of the load balancer. By setting this option, load balancer support is enabled.
  • lb-context-path: Context path of the load balancer. Optional. Default value /.
  • lb-port-in-url: Include server port in construction of load balancer request URL. Optional. Default value false.
  • lb-scheme: Scheme of the load balancer - either http or https. Optional. Default value https.
  • lb-server-port: Port of the load balancer server. Optional. Default value 443.

Second, if the IDP issues tokens that are valid longer than 7200 seconds, then the following error can occur CredentialsExpiredException: Authentication statement is too old to be used.

This issue is fixed by exposing the following configuration setting:

  • proxy.saml.max-auth-age: Maximum time (in seconds) between users authentication and processing of an authentication statement.

and setting it to a sufficiently high value.

More details and a build of the ShinyProxy jar which incorporates these changes can be found here.

@jat255
Copy link

jat255 commented Jan 21, 2021

This is something we need in our deployment. Is there any consideration for this PR from the developers?

@LEDfan
Copy link
Member

LEDfan commented Jan 22, 2021

Hi @jat255

Yes, we are planning to integrate this in the next release. I see you are already compiling your own version with these changes. Can I ask you to let us know whether these changes work correctly in your setup? Thanks!

@LEDfan LEDfan added this to the Next milestone Jan 22, 2021
@jat255
Copy link

jat255 commented Jan 22, 2021

@LEDfan I incorporated these changes in a custom build, and it does look like it fixed the http/https mismatch error that I was seeing. I have another error now (org.springframework.security.saml.SAMLStatusException: Response has invalid status code urn:oasis:names:tc:SAML:2.0:status:Responder, status message is null), although I'm working through that is an error on the IdP side or my side (I don't control the IdP, so I have to work with some colleagues to debug all this).

@johannestang
Copy link
Contributor Author

@LEDfan: I have been using a custom build based on this pull request the last year for several of our solutions without any issues. We're running Shinyproxy on AKS with Azure AD as IDP. It would be great to see this incorporated in the next release.

@LEDfan LEDfan added the enhancement New feature or request label Feb 10, 2021
LEDfan pushed a commit that referenced this pull request Mar 2, 2021
@LEDfan
Copy link
Member

LEDfan commented Mar 2, 2021

Hi @johannestang

We just released ShinyProxy 2.5.0 which now includes your PR. I mentioned https://github.com/johannestang in our release notes (https://shinyproxy.io/downloads/#250). Please let me know if that's not the proper account or if you prefer to not be mentioned.

Thanks again for your contribution!

@LEDfan LEDfan closed this Mar 2, 2021
@johannestang
Copy link
Contributor Author

That's great, thanks a lot 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants