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

Cannot login with SAML when IdP is OneLogin or OpenAM since 1.38.3 #15567

Closed
flamber opened this issue Apr 12, 2021 · 3 comments · Fixed by #17785
Closed

Cannot login with SAML when IdP is OneLogin or OpenAM since 1.38.3 #15567

flamber opened this issue Apr 12, 2021 · 3 comments · Fixed by #17785
Assignees
Labels
Administration/Auth/SSO Enterprise SSO like SAML and JWT Priority:P1 Security holes w/o exploit, crashing, setup/upgrade, login, broken common features, correctness .Regression Bugs that were previously fixed and/or bugs unintentionally shipped with new features. Type:Bug Product defects
Milestone

Comments

@flamber
Copy link
Contributor

flamber commented Apr 12, 2021

Describe the bug
After upgrading from 1.38.2 to 1.38.3, it's no longer possible to login via OpenAM SAML. Errors with SAML info does not contain user attributes. There's no difference in the responses (besides timings and signature of course). Likely caused by #15410.

To Reproduce

  1. Configure Metabase to use SAML via OpenAM
  2. Try to login as a user via Metabase, after completing the IdP login, then the SAML response to POST /auth/sso error with:
2021-04-12 11:41:29,406 ERROR api.sso :: Error logging in
clojure.lang.ExceptionInfo: Unable to log in: SAML info does not contain user attributes. {:status-code 401}
	at metabase_enterprise.sso.integrations.saml$saml_response__GT_attributes.invokeStatic(saml.clj:149)
	at metabase_enterprise.sso.integrations.saml$saml_response__GT_attributes.invoke(saml.clj:145)
	at metabase_enterprise.sso.integrations.saml$fn__61655.invokeStatic(saml.clj:168)
	at metabase_enterprise.sso.integrations.saml$fn__61655.invoke(saml.clj:157)
	at clojure.lang.MultiFn.invoke(MultiFn.java:229)
	at metabase_enterprise.sso.api.sso$fn__58209$fn__58211.invoke(sso.clj:92)
	at metabase_enterprise.sso.api.sso$fn__58209.invokeStatic(sso.clj:91)
	at metabase_enterprise.sso.api.sso$fn__58209.invoke(sso.clj:87)
	at compojure.core$wrap_response$fn__11947.invoke(core.clj:160)
	at compojure.core$wrap_route_middleware$fn__11931.invoke(core.clj:132)
	at compojure.core$wrap_route_info$fn__11936.invoke(core.clj:139)
	at compojure.core$wrap_route_matches$fn__11940.invoke(core.clj:151)
	at compojure.core$routes$fn__11959$f__11960.invoke(core.clj:198)
	at compojure.core$routes$fn__11959.invoke(core.clj:200)
	at compojure.core$routes$fn__11959$f__11960.invoke(core.clj:198)
	at compojure.core$routes$fn__11959.invoke(core.clj:200)
	at compojure.core$make_context$handler__11987.invoke(core.clj:287)
	at compojure.core$make_context$fn__11989.invoke(core.clj:296)
	at compojure.core$routes$fn__11959$f__11960.invoke(core.clj:198)
	at compojure.core$routes$fn__11959.invoke(core.clj:200)
	at compojure.core$routes$fn__11959$f__11960.invoke(core.clj:198)
	at compojure.core$routes$fn__11959.invoke(core.clj:200)
	at compojure.core$make_context$handler__11987.invoke(core.clj:287)
	at compojure.core$make_context$fn__11989.invoke(core.clj:296)
	at compojure.core$routes$fn__11959$f__11960.invoke(core.clj:198)
	at compojure.core$routes$fn__11959.invoke(core.clj:200)
	at compojure.core$routes$fn__11959$f__11960.invoke(core.clj:198)
	at compojure.core$routes$fn__11959.invoke(core.clj:200)
	at metabase.server.middleware.exceptions$catch_uncaught_exceptions$fn__79054.invoke(exceptions.clj:98)
	at metabase.server.middleware.exceptions$catch_api_exceptions$fn__79051.invoke(exceptions.clj:86)
	at metabase.server.middleware.log$log_api_call$fn__80892.invoke(log.clj:187)
	at metabase.server.middleware.security$add_security_headers$fn__79016.invoke(security.clj:142)
	at metabase.server.middleware.json$wrap_json_body$fn__80673.invoke(json.clj:64)
	at metabase.server.middleware.json$wrap_streamed_json_response$fn__80691.invoke(json.clj:98)
	at ring.middleware.keyword_params$wrap_keyword_params$fn__81233.invoke(keyword_params.clj:55)
	at ring.middleware.params$wrap_params$fn__81249.invoke(params.clj:69)
	at metabase.server.middleware.misc$maybe_set_site_url$fn__35107.invoke(misc.clj:58)
	at metabase.server.middleware.session$bind_current_user$fn__42539$fn__42540.invoke(session.clj:277)
	at metabase.server.middleware.session$do_with_current_user.invokeStatic(session.clj:258)
	at metabase.server.middleware.session$do_with_current_user.invoke(session.clj:250)
	at metabase.server.middleware.session$bind_current_user$fn__42539.invoke(session.clj:276)
	at metabase.server.middleware.session$wrap_current_user_info$fn__42526.invoke(session.clj:236)
	at metabase.server.middleware.session$wrap_session_id$fn__42512.invoke(session.clj:182)
	at metabase.server.middleware.auth$wrap_api_key$fn__55051.invoke(auth.clj:27)
	at ring.middleware.cookies$wrap_cookies$fn__81153.invoke(cookies.clj:216)
	at metabase.server.middleware.misc$add_content_type$fn__35090.invoke(misc.clj:27)
	at metabase.server.middleware.misc$disable_streaming_buffering$fn__35115.invoke(misc.clj:75)
	at ring.middleware.gzip$wrap_gzip$fn__81195.invoke(gzip.clj:86)
	at metabase.server.middleware.misc$bind_request$fn__35118.invoke(misc.clj:92)
	at metabase.server.middleware.ssl$redirect_to_https_middleware$fn__80909.invoke(ssl.clj:38)
	at metabase.server$async_proxy_handler$fn__80506.invoke(server.clj:71)
	at metabase.server.proxy$org.eclipse.jetty.server.handler.AbstractHandler$ff19274a.handle(Unknown Source)
	at org.eclipse.jetty.server.handler.HandlerWrapper.handle(HandlerWrapper.java:127)
	at org.eclipse.jetty.server.Server.handle(Server.java:516)
	at org.eclipse.jetty.server.HttpChannel.lambda$handle$1(HttpChannel.java:383)
	at org.eclipse.jetty.server.HttpChannel.dispatch(HttpChannel.java:556)
	at org.eclipse.jetty.server.HttpChannel.handle(HttpChannel.java:375)
	at org.eclipse.jetty.server.HttpConnection.onFillable(HttpConnection.java:273)
	at org.eclipse.jetty.io.AbstractConnection$ReadCallback.succeeded(AbstractConnection.java:311)
	at org.eclipse.jetty.io.FillInterest.fillable(FillInterest.java:105)
	at org.eclipse.jetty.io.ChannelEndPoint$1.run(ChannelEndPoint.java:104)
	at org.eclipse.jetty.util.thread.strategy.EatWhatYouKill.runTask(EatWhatYouKill.java:336)
	at org.eclipse.jetty.util.thread.strategy.EatWhatYouKill.doProduce(EatWhatYouKill.java:313)
	at org.eclipse.jetty.util.thread.strategy.EatWhatYouKill.tryProduce(EatWhatYouKill.java:171)
	at org.eclipse.jetty.util.thread.strategy.EatWhatYouKill.produce(EatWhatYouKill.java:135)
	at org.eclipse.jetty.util.thread.QueuedThreadPool.runJob(QueuedThreadPool.java:773)
	at org.eclipse.jetty.util.thread.QueuedThreadPool$Runner.run(QueuedThreadPool.java:905)
	at java.base/java.lang.Thread.run(Thread.java:834)
  1. There's no difference in the responses:
    SAML Response 1.38.2.txt
    SAML Response 1.38.3.txt

Information about your Metabase Installation:
Tested 1.38.2 (working) and 1.38.3 (failing)

Additional context
I'm not a fan of OpenAM. Took a long time to understand how it works and how to setup with Metabase, not even sure if I did it correctly, but I can reproduce the issue.

@flamber flamber added Type:Bug Product defects Priority:P1 Security holes w/o exploit, crashing, setup/upgrade, login, broken common features, correctness .Regression Bugs that were previously fixed and/or bugs unintentionally shipped with new features. Administration/Auth/SSO Enterprise SSO like SAML and JWT labels Apr 12, 2021
@camsaul camsaul self-assigned this May 3, 2021
@camsaul
Copy link
Member

camsaul commented Jun 7, 2021

SAML info does not contain user attributes was in the code before #15410 so not sure that PR was the culprit

@camsaul camsaul removed their assignment Jun 7, 2021
@pawit-metabase
Copy link
Contributor

pawit-metabase commented Sep 7, 2021

The new base64-decode method in #15410 is the culprit.

The encoded SAML from OpenAM contains line breaks every 76 characters which doesn't match u/base64-string?

@pawit-metabase pawit-metabase self-assigned this Sep 7, 2021
pawit-metabase added a commit that referenced this issue Sep 7, 2021
OpenAM SAMLResponse contains "\r\n" every 76 characters
leading our SAML code to think it's not base64-encoded.

Fix by relaxing our valid base64 check to ignore all spaces.

Fixes #15567
@rlotun rlotun added this to the 0.40.4 milestone Sep 7, 2021
pawit-metabase added a commit that referenced this issue Sep 8, 2021
OpenAM SAMLResponse contains "\r\n" every 76 characters
leading our SAML code to think it's not base64-encoded.

Fix by relaxing our valid base64 check to ignore all spaces.

Fixes #15567
github-actions bot pushed a commit that referenced this issue Sep 8, 2021
OpenAM SAMLResponse contains "\r\n" every 76 characters
leading our SAML code to think it's not base64-encoded.

Fix by relaxing our valid base64 check to ignore all spaces.

Fixes #15567
pawit-metabase added a commit that referenced this issue Sep 8, 2021
OpenAM SAMLResponse contains "\r\n" every 76 characters
leading our SAML code to think it's not base64-encoded.

Fix by relaxing our valid base64 check to ignore all spaces.

Fixes #15567

Co-authored-by: pawit-metabase <pawit@metabase.com>
@pawit-metabase
Copy link
Contributor

Minimal XML file to setup Metabase inside OpenAM for testing

<?xml version="1.0" encoding="UTF-8" standalone="yes"?>
<EntityDescriptor entityID="http://localhost:3000" xmlns="urn:oasis:names:tc:SAML:2.0:metadata">
    <SPSSODescriptor protocolSupportEnumeration="urn:oasis:names:tc:SAML:2.0:protocol">
        <NameIDFormat>urn:oasis:names:tc:SAML:1.1:nameid-format:emailAddress</NameIDFormat>
        <AssertionConsumerService index="0" Binding="urn:oasis:names:tc:SAML:2.0:bindings:HTTP-POST" Location="http://localhost:3000/auth/sso"/>
    </SPSSODescriptor>
</EntityDescriptor>

@flamber flamber changed the title Cannot login with OpenAM SAML since 1.38.3 Cannot login with SAML when IdP is OneLogin or OpenAM since 1.38.3 Jun 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Administration/Auth/SSO Enterprise SSO like SAML and JWT Priority:P1 Security holes w/o exploit, crashing, setup/upgrade, login, broken common features, correctness .Regression Bugs that were previously fixed and/or bugs unintentionally shipped with new features. Type:Bug Product defects
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants