zmartzone / lua-resty-openidc

OpenID Connect Relying Party and OAuth 2.0 Resource Server implementation in Lua for NGINX / OpenResty
Apache License 2.0
963 stars 247 forks source link

Cross-tenant requests are possible #526

Closed markbanierink closed 4 days ago

markbanierink commented 1 week ago

We have a multi-tenant setup where based on the subdomain in the URL a tenant is determined. Each tenant has his own Keycloak realm where he can authenticate. So when you do a request to tenanta.ourdomain.com, you authenticate at realm "tenanta" in Keycloak. We use the default setting with cookies as session storage. When you authenticate at tenanta, you get a cookie that holds the session information. However, if I do a request to tenantb and add this cookie of tenanta to the request, I am still authenticated.

Environment
Configuration

nginx.conf:

server {
    # Example: tenanta.ourdomain.com
    server_name ~ ^ (?!www\.)^(?<tenant>.+)\.ourdomain.com$;

    set $session_secret very_secret;

    # some more configuration..

    access_by_lua_block {
        require("authentication").authenticate()
    }

    # some more configuration..
}

authentication.lua:

function authentication.authenticate()
    local base_url = ngx.var.scheme .. "://" .. ngx.var.http_host
    local tenant = ngx.var.tenant

    local opts = {
        discovery = base_url .. "/auth/realms/" .. tenant .. "/.well-known/openid-configuration"
        -- some more options..
    }

    local res, err = openidc.authenticate(opts)

    -- some more code..
end
Minimized example
  1. Create two realms in Keycloak named "tenanta" and "tenantb".
  2. Do a request to tenantA at tenanta.ourdomain.com and authenticate.
  3. Do the same request, but change the url, host header and origin header to tenantb.ourdomain.com (used Burp Suite).
Expected behaviour

Step 3 resulting in a failed request, responding with a HTTP 302 or 401, depending on the configuration.

Actual behaviour

Step 3 responds with the requested resource, authentication went fine.

Additional

We figured we might make the session secret tenant specific, like in this nginx configuration:

set $session_secret very_secret${tenant};

Any request to tenantA results in a cookie encrypted with a session secret specific for tenantA, since it is part of the secret. A request like the one in step 3 would, when requesting to tenantB, then be decrypted with the wrong secret and automatically the request would fail. This seemed to work at moments, but a lot of times it just authenticated fine for tenantB. So far we couldn't figure out what caused this unexpected behaviour.

Possible solutions:

  1. Add a check that compares the issuer of the cookie data with the current opts.
  2. Fix the tenant specific session secret mechanism.
  3. ?
bodewig commented 6 days ago

Does using opts.cache_segment help in this situation? See https://github.com/zmartzone/lua-resty-openidc?tab=readme-ov-file#caching-of-introspection-and-jwt-verification-results and #399

markbanierink commented 6 days ago

No, it doesn't help. I've added it to my options like this cache_segment = tenant, where tenant is the variable for the tenant name and I can still do cross-tenant requests. This is only possible when using the openidc.authenticate() function. In another flow where we use openidc.bearer_jwt_verify() this is not possible and everything works correctly.

oldium commented 6 days ago

I think you need to limit the cookie scope to subdomain and also verify that the cookie is used on the correct subdomain afterwards somehow, because malicious user could take cookie from tenanta.ourdomain.com and use it on tenantb.ourdomain.com. I think the code simply has no knowledge that the cookie from tenanta is wrong when used on tenantb.

oldium commented 6 days ago

Just an untested idea:

According to https://github.com/bungle/lua-resty-session (lua-resty-session 4 is required, so master branch of lua-resty-openidc is needed) it should be possible to set cookie audience to your tenant for example. Then your code will be responsible to open the session with the correct audience (require "resty.session".open({audience = tenant,})) and you will pass it to lua-resty-openidc as a session_or_opts argument.

The cookie domain can be controlled with cookie_domain parameter, so at the end you could use someting like require "resty.session".open({audience = tenant, cookie_domain=tenant_fqdn})) to open the correct session. lua-resty-openidc will save any session changes.

For the bearer_jwt_verify flow you also need to verify that the JWT is used on the correct domain, but that could be part of other access rights check (same JWT could be used cross-site).

EDIT: openidc.authenticate(opts, target_url, unauth_action, {audience = tenant, cookie_domain=tenant_fqdn}) should work too and lua-resty-openidc will open the correct session.

cookie_domain looks undocumented, but it is in the code of lua-resty-session 😊

markbanierink commented 4 days ago

@zandbelt @oldium @bodewig I have just tried openidc.authenticate(opts, target_url, unauth_action, {audience = tenant, cookie_domain=tenant_fqdn}) with the master lua-resty-openidc with session 4.0.5 and it seems to prevent cross-tenant requests from being possible 👍. Nice, and thank you all for the help!

oldium commented 4 days ago

Just to be sure to prevent any surprises - the tenant_fqdn variable used in my code snippet is not present in your original code, so it needs to be set to the fully qualified domain name of the tenant (the tenant's top-level domain), i.e. tenanta.ourdomain.com.

oldium commented 4 days ago

One additional note - omitting the cookie_domain entirely is not an error, it depends on what you want:

  1. Without cookie_domain (or with cookie_domain=nil), the cookie will be set only for the request's domain (which is hopefully tenanta.ourdomain.com anyway) and not its subdomains.
  2. With cookie_domain="tenanta.ourdomain.com", the cookie will be additionally available also for subdomains, i.e. service.tenanta.ourdomain.com.
  3. With cookie_domain="ourdomain.com", you still get a working cookie separation, but the cookie value will be shared with all tenants. This works because the cookie data has a key-value map and the audience acts like a key to get the audience-specific data (i.e. tenant-specific data in your case). In order to have this working, the session's ikm/secret used to derive cookie data AES encryption key has to be the same for all tenants, otherwise some tenants will be unable to decode the cookie value and get the audience-specific data. Please check lua-resty-session documentation for more info about data encryption. The cookie value will also be bigger in this case, because it will contain all tenants' data, so I do not recommend this.
systemofapwne commented 15 hours ago

Not sure, if this is related, but since the recent update to 1.8 I am getting CORS related errors once my keycloak session needs to be refreshed.

Here for example, grafana wants to pull fresh data but the OIDC session needs to be refreshed. So resty-openid redirects the browser to the keycloak auth backend and then the browser tries to preflight that domain for CORS rules via the 'OPTIONS' method. This then fails with 405 (Not allowed)

Besides of the recent upgrade to resty-openidc 1.8, I did not change my setup:

image

zandbelt commented 14 hours ago

this then looks like something caused by lua-resty-session's behavioral changes between <=3.x and >= 4.x; it may be a setting that is configurable, hopefully someone else can answer that here or you could ask the question in the lua-resty-session project