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.

Sign up for GitHub

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

Jump to bottom

Illegal base64 character d when using OneLogin SAML SSO (43+) #23451

Closed
paoliniluis opened this issue Jun 20, 2022 · 2 comments
Closed

Illegal base64 character d when using OneLogin SAML SSO (43+) #23451

paoliniluis opened this issue Jun 20, 2022 · 2 comments
Assignees
@howonlee
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
0.44

Comments

@paoliniluis
Copy link
Contributor

Describe the bug
When using Onelogin SSO with 43+, you can't log in. Can't reproduce on Keycloak, just on OneLogin. < 43 works fine

Logs

``` metabase-keycloak | 2022-06-20 23:29:44,027 ERROR api.sso :: Error logging in metabase-keycloak | java.lang.IllegalArgumentException: Illegal base64 character d metabase-keycloak | at java.base/java.util.Base64$Decoder.decode0(Unknown Source) metabase-keycloak | at java.base/java.util.Base64$Decoder.decode(Unknown Source) metabase-keycloak | at java.base/java.util.Base64$Decoder.decode(Unknown Source) metabase-keycloak | at ring.util.codec$base64_decode.invokeStatic(codec.clj:87) metabase-keycloak | at ring.util.codec$base64_decode.invoke(codec.clj:84) metabase-keycloak | at metabase_enterprise.sso.integrations.saml$base64_decode.invokeStatic(saml.clj:170) metabase-keycloak | at metabase_enterprise.sso.integrations.saml$base64_decode.invoke(saml.clj:168) metabase-keycloak | at metabase_enterprise.sso.integrations.saml$fn__80242.invokeStatic(saml.clj:181) metabase-keycloak | at metabase_enterprise.sso.integrations.saml$fn__80242.invoke(saml.clj:172) metabase-keycloak | at clojure.lang.MultiFn.invoke(MultiFn.java:229) metabase-keycloak | at metabase_enterprise.sso.api.sso$fn__80300$fn__80302.invoke(sso.clj:51) metabase-keycloak | at metabase_enterprise.sso.api.sso$fn__80300.invokeStatic(sso.clj:50) metabase-keycloak | at metabase_enterprise.sso.api.sso$fn__80300.invoke(sso.clj:46) metabase-keycloak | at compojure.core$wrap_response$fn__28029.invoke(core.clj:160) metabase-keycloak | at compojure.core$wrap_route_middleware$fn__28013.invoke(core.clj:132) metabase-keycloak | at compojure.core$wrap_route_info$fn__28018.invoke(core.clj:139) metabase-keycloak | at compojure.core$wrap_route_matches$fn__28022.invoke(core.clj:151) metabase-keycloak | at compojure.core$routes$fn__28041$f__28042.invoke(core.clj:198) metabase-keycloak | at compojure.core$routes$fn__28041.invoke(core.clj:200) metabase-keycloak | at compojure.core$routes$fn__28041$f__28042.invoke(core.clj:198) metabase-keycloak | at compojure.core$routes$fn__28041.invoke(core.clj:200) metabase-keycloak | at compojure.core$make_context$handler__28069.invoke(core.clj:289) metabase-keycloak | at compojure.core$make_context$fn__28073.invoke(core.clj:299) metabase-keycloak | at compojure.core$routes$fn__28041$f__28042.invoke(core.clj:198) metabase-keycloak | at compojure.core$routes$fn__28041.invoke(core.clj:200) metabase-keycloak | at compojure.core$routes$fn__28041$f__28042.invoke(core.clj:198) metabase-keycloak | at compojure.core$routes$fn__28041.invoke(core.clj:200) metabase-keycloak | at compojure.core$make_context$handler__28069.invoke(core.clj:289) metabase-keycloak | at compojure.core$make_context$fn__28073.invoke(core.clj:299) metabase-keycloak | at compojure.core$routes$fn__28041$f__28042.invoke(core.clj:198) metabase-keycloak | at compojure.core$routes$fn__28041.invoke(core.clj:200) metabase-keycloak | at compojure.core$routes$fn__28041$f__28042.invoke(core.clj:198) metabase-keycloak | at compojure.core$routes$fn__28041.invoke(core.clj:200) metabase-keycloak | at metabase.server.middleware.exceptions$catch_uncaught_exceptions$fn__75710.invoke(exceptions.clj:98) metabase-keycloak | at metabase.server.middleware.exceptions$catch_api_exceptions$fn__75707.invoke(exceptions.clj:86) metabase-keycloak | at metabase.server.middleware.log$log_api_call$fn__80967.invoke(log.clj:200) metabase-keycloak | at metabase.server.middleware.browser_cookie$ensure_browser_id_cookie$fn__86509.invoke(browser_cookie.clj:40) metabase-keycloak | at metabase.server.middleware.security$add_security_headers$fn__61080.invoke(security.clj:148) metabase-keycloak | at metabase.server.middleware.json$wrap_json_body$fn__83808.invoke(json.clj:65) metabase-keycloak | at metabase.server.middleware.json$wrap_streamed_json_response$fn__83826.invoke(json.clj:99) metabase-keycloak | at metabase.server.middleware.offset_paging$handle_paging$fn__61104.invoke(offset_paging.clj:42) metabase-keycloak | at ring.middleware.keyword_params$wrap_keyword_params$fn__86776.invoke(keyword_params.clj:55) metabase-keycloak | at ring.middleware.params$wrap_params$fn__86795.invoke(params.clj:77) metabase-keycloak | at metabase.server.middleware.misc$maybe_set_site_url$fn__35170.invoke(misc.clj:59) metabase-keycloak | at metabase.server.middleware.session$bind_current_user$fn__46635$fn__46636.invoke(session.clj:291) metabase-keycloak | at metabase.server.middleware.session$do_with_current_user.invokeStatic(session.clj:270) metabase-keycloak | at metabase.server.middleware.session$do_with_current_user.invoke(session.clj:259) metabase-keycloak | at metabase.server.middleware.session$bind_current_user$fn__46635.invoke(session.clj:290) metabase-keycloak | at metabase.server.middleware.session$wrap_current_user_info$fn__46617.invoke(session.clj:240) metabase-keycloak | at metabase.server.middleware.session$wrap_session_id$fn__46601.invoke(session.clj:173) metabase-keycloak | at metabase.server.middleware.auth$wrap_api_key$fn__67402.invoke(auth.clj:27) metabase-keycloak | at ring.middleware.cookies$wrap_cookies$fn__86696.invoke(cookies.clj:216) metabase-keycloak | at metabase.server.middleware.misc$add_content_type$fn__35153.invoke(misc.clj:27) metabase-keycloak | at metabase.server.middleware.misc$disable_streaming_buffering$fn__35178.invoke(misc.clj:76) metabase-keycloak | at ring.middleware.gzip$wrap_gzip$fn__86738.invoke(gzip.clj:86) metabase-keycloak | at metabase.server.middleware.misc$bind_request$fn__35181.invoke(misc.clj:93) metabase-keycloak | at metabase.server.middleware.ssl$redirect_to_https_middleware$fn__86525.invoke(ssl.clj:38) metabase-keycloak | at metabase.server$async_proxy_handler$fn__80739.invoke(server.clj:73) metabase-keycloak | at metabase.server.proxy$org.eclipse.jetty.server.handler.AbstractHandler$ff19274a.handle(Unknown Source) metabase-keycloak | at org.eclipse.jetty.server.handler.HandlerWrapper.handle(HandlerWrapper.java:127) metabase-keycloak | at org.eclipse.jetty.server.Server.handle(Server.java:516) metabase-keycloak | at org.eclipse.jetty.server.HttpChannel.lambda$handle$1(HttpChannel.java:400) metabase-keycloak | at org.eclipse.jetty.server.HttpChannel.dispatch(HttpChannel.java:645) metabase-keycloak | at org.eclipse.jetty.server.HttpChannel.handle(HttpChannel.java:392) metabase-keycloak | at org.eclipse.jetty.server.HttpConnection.onFillable(HttpConnection.java:277) metabase-keycloak | at org.eclipse.jetty.io.AbstractConnection$ReadCallback.succeeded(AbstractConnection.java:311) metabase-keycloak | at org.eclipse.jetty.io.FillInterest.fillable(FillInterest.java:105) metabase-keycloak | at org.eclipse.jetty.io.ChannelEndPoint$1.run(ChannelEndPoint.java:104) metabase-keycloak | at org.eclipse.jetty.util.thread.strategy.EatWhatYouKill.runTask(EatWhatYouKill.java:338) metabase-keycloak | at org.eclipse.jetty.util.thread.strategy.EatWhatYouKill.doProduce(EatWhatYouKill.java:315) metabase-keycloak | at org.eclipse.jetty.util.thread.strategy.EatWhatYouKill.tryProduce(EatWhatYouKill.java:173) metabase-keycloak | at org.eclipse.jetty.util.thread.strategy.EatWhatYouKill.run(EatWhatYouKill.java:131) metabase-keycloak | at org.eclipse.jetty.util.thread.ReservedThreadExecutor$ReservedThread.run(ReservedThreadExecutor.java:409) metabase-keycloak | at org.eclipse.jetty.util.thread.QueuedThreadPool.runJob(QueuedThreadPool.java:883) metabase-keycloak | at org.eclipse.jetty.util.thread.QueuedThreadPool$Runner.run(QueuedThreadPool.java:1034) metabase-keycloak | at java.base/java.lang.Thread.run(Unknown Source) ```

To Reproduce
Set up an account on OneLogin

  • Go To Application -> Add Application -> SAML Custom Connector (Advanced)
  • In the application you added complete the following sections: “Recipient”, “ACS (Consumer) URL*” “and ACS (Consumer) URL Validator*”: the URL that Metabase provides in “URL the IdP should redirect back to”
  • In the Parameters section: Add a new parameter, field name Email, include it in the Assertion, and the value should be “Email”
  • In the SSO section:
    “Issuer URL” should be the value that goes into Metabase “SAML Identity Provider URL”
    “Issuer URL” should be the value that goes into Metabase “SAML Identity Provider Issuer”
    Click on “View details” under the certificate, and copy the certificate to “SAML Identity Provider Certificate” in Metabase

Now try to authenticate Metabase with SSO, you'll get the error

Expected behavior
Work in the same way as 42, users are authenticated

Screenshots
image

Information about your Metabase Installation:

  • Your browser and the version: Brave latest
  • Your operating system: Pop 22.04
  • Your databases: Postgres 14.3
  • Metabase version: 1.43.3
  • Metabase hosting environment: Docker
  • Metabase internal database: Postgres 14.3

Severity
P1

Additional context
NA
@paoliniluis paoliniluis 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 Jun 20, 2022
@paoliniluis paoliniluis changed the title Illegal base64 character d when using OneLogin SAML SSO Illegal base64 character d when using OneLogin SAML SSO (43+) Jun 20, 2022
@flamber
Copy link
Contributor

flamber commented Jun 21, 2022

Seems like OneLogin and OpenAM are behaving similarly, since the same problem occurs with OpenAM, and that was also the case a while ago in #15567.

Perhaps caused by upstream upgrade of SAML dependency: https://github.com/metabase/saml20-clj

@howonlee howonlee self-assigned this Jun 24, 2022
@howonlee howonlee mentioned this issue Jun 30, 2022
Merged
howonlee added a commit that referenced this issue Jul 13, 2022
@howonlee
Deal with SAML responses having whitespace (#23451) (#23633)
a9ca102 
Pursuant to #23451.

The end effect of whitespace existing in a SAML response is us choking on it as reported in #23451. Two possible interpretations of causes of this bug:

There was an upstream change in our fork of the clojure SAML lib as flamber noted,
The decoding of base64 in our SAML endpoint (which uses the SAML lib) chokes on whitespace.
The proximate cause is the second one and ultimate cause is the first. However, I tend to believe that fixing the second one would be the better fix. For comparison, onelogin's first party SAML thing for java decodes base64 (https://github.com/onelogin/java-saml/blob/master/core/src/main/java/com/onelogin/saml2/util/Util.java) via apache's lib, which seems to do the thing that a lot of base64 decoders do of skipping whitespace.
github-actions bot pushed a commit that referenced this issue Jul 13, 2022
@howonlee
Deal with SAML responses having whitespace (#23451) (#23633)
51bc334 
Pursuant to #23451.

The end effect of whitespace existing in a SAML response is us choking on it as reported in #23451. Two possible interpretations of causes of this bug:

There was an upstream change in our fork of the clojure SAML lib as flamber noted,
The decoding of base64 in our SAML endpoint (which uses the SAML lib) chokes on whitespace.
The proximate cause is the second one and ultimate cause is the first. However, I tend to believe that fixing the second one would be the better fix. For comparison, onelogin's first party SAML thing for java decodes base64 (https://github.com/onelogin/java-saml/blob/master/core/src/main/java/com/onelogin/saml2/util/Util.java) via apache's lib, which seems to do the thing that a lot of base64 decoders do of skipping whitespace.
@rlotun rlotun added this to the 0.44 milestone Jul 13, 2022
@rlotun
Copy link
Contributor

rlotun commented Jul 13, 2022

fixed by: #23633

@rlotun rlotun closed this as completed Jul 13, 2022
howonlee added a commit that referenced this issue Jul 13, 2022
@github-actions @howonlee
Deal with SAML responses having whitespace (#23451) (#23633) (#23884)
c31c912 
Pursuant to #23451.

The end effect of whitespace existing in a SAML response is us choking on it as reported in #23451. Two possible interpretations of causes of this bug:

There was an upstream change in our fork of the clojure SAML lib as flamber noted,
The decoding of base64 in our SAML endpoint (which uses the SAML lib) chokes on whitespace.
The proximate cause is the second one and ultimate cause is the first. However, I tend to believe that fixing the second one would be the better fix. For comparison, onelogin's first party SAML thing for java decodes base64 (https://github.com/onelogin/java-saml/blob/master/core/src/main/java/com/onelogin/saml2/util/Util.java) via apache's lib, which seems to do the thing that a lot of base64 decoders do of skipping whitespace.

Co-authored-by: Howon Lee <hlee.howon@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@howonlee howonlee

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
Milestone
0.44
Development

No branches or pull requests
4 participants
@rlotun @howonlee @flamber @paoliniluis