owncloud / ocis

:atom_symbol: ownCloud Infinite Scale Stack
https://doc.owncloud.com/ocis/next/
Apache License 2.0
1.4k stars 182 forks source link

iOS/Android/Desktop app verifies token using web oidc client #6479

Open legalgig opened 1 year ago

legalgig commented 1 year ago

Bug description

Owncloud desktop/mobile clients are verifying OIDC token using web oidc issuer provided in OCIS_OIDC_ISSUER environment variable. This bug prevents using some OIDC providers with token verification turned on (without PROXY_OIDC_ACCESS_TOKEN_VERIFY_METHOD: "none")

Specs

OCIS Version: 3.0.0 Deployment type: docker OIDC Provider: Authentik

Possible fix

Add possibility to define OIDC issuer for each oidc client i.e.

OCIS_OIDC_WEB_ISSUER="https://auth.provider.com/application/o/ocis-web/
OCIS_OIDC_DESKTOP_ISSUER="https://auth.provider.com/application/o/ocis-desktop/
OCIS_OIDC_IOS_ISSUER="https://auth.provider.com/application/o/ocis-ios/
OCIS_OIDC_ANDROID_ISSUER="https://auth.provider.com/application/o/ocis-android/
rhafer commented 1 year ago

@legalgig I don't have a working authentik deployment at hand. Would you mind pasting some example access tokens issued for the various clients. I am somewhat surprised that the iss claim differs based on the client.

The current implementation of the access token verification was initially developed against keycloak and libregraph/lico.

legalgig commented 1 year ago

Sure, here is one for ocis web interface (it was taken from the response)

{
  "iss": "https://auth.provider.com/application/o/ocis-web/",
  "sub": "4de6ff6d497cf4c1697a6557cfb0d115458c58b764dd7a43749bcd9a1d939547",
  "aud": "ocis-web",
  "exp": 1686240697,
  "iat": 1686240097,
  "auth_time": 1686240095,
  "acr": "goauthentik.io/providers/oauth2/default",
  "email": "test@localhost.local",
  "email_verified": true,
  "name": "test",
  "given_name": "test",
  "preferred_username": "test",
  "nickname": "test",
  "groups": [
    "ocis-ios",
    "ocis-android",
    "ocis-web",
    "ocis-desktop"
  ],
  "azp": "ocis-web",
  "uid": "xzr4bW5FNa2XVLtsY9sfHz4WOOseVd7YHj3Cug3j"
}

And this one was taken from authentk interface because I'm not sure how to capture token for desktop application

{
  "iss": "https://auth.provider.com/application/o/ocis-desktop/",
  "sub": "de900b6e77b61c3a92ec730e59cbbde9b22bfd2180d6e74048ad3f65827da05b",
  "aud": "xdXOt13JKxym1B1QcEncf2XDkLAexMBFwiT9j6EfhhHFJhs2KM9jbjTmf8JBXE69",
  "exp": 1686242426,
  "iat": 1686240626,
  "auth_time": 1686240626,
  "acr": "goauthentik.io/providers/oauth2/default",
  "amr": [
      "pwd"
  ],
  "email": "test@localhost.local",
  "email_verified": true,
  "name": "test",
  "given_name": "test",
  "preferred_username": "test-admin",
  "nickname": "test-admin",
  "groups": [
      "authentik Admins"
  ]
}
bobslaede commented 1 year ago

+1

stale[bot] commented 1 year ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 10 days if no further activity occurs. Thank you for your contributions.

mpldr commented 7 months ago

Currently looking at the code that throws the error and was thinking whether or not a relatively simple OCIS_OIDC_ISSUER_SKIP_VALIDATION would be an option. It could just use the existing skipIssuerValidation field of the oidcClient struct. The required changes would require:

This would make the required change significantly smaller and less error prone than introducing multiple OIDC clients.

euh2 commented 5 months ago

PROXY_OIDC_ACCESS_TOKEN_VERIFY_METHOD: "none"

Are there any security concerns about setting this to none?

rhafer commented 5 months ago

Are there any security concerns about setting this to none?

I don't think so. Even if we don't verify the token locally. It is still being validated as we use it for retrieving the userinfo from the IDP. Which cannot work with an invalid token.

.

danthonywalker commented 2 months ago

I am similarly trying to setup OCIS with Authentik. I can successfully login to the Web client, however, trying to connect on the Android app gets me all the way through until I get an "Unsuccessful authorization" error in the app. I have tried PROXY_OIDC_ACCESS_TOKEN_VERIFY_METHOD to none, but it hasn't changed the result.

I have 2 providers/applications in Authentik, one for Web and one for Android as mentioned and configured in this article: https://helgeklein.com/blog/owncloud-infinite-scale-with-openid-connect-authentication-for-home-networks/

I am also going to call attention to this issue, which mentions OCIS quite a bit: https://github.com/goauthentik/authentik/issues/7251

I have uploaded the logs as seen from Android, except domain changed: owncloud.2024-08-24_12.42.07.log

danthonywalker commented 2 months ago

I think the fix as described in the issue would be the best approach to resolving this while still being able to maintain backwards compatibility. OCIS_OIDC_WEB_ISSUER, OCIS_OIDC_DESKTOP_ISSUER, OCIS_OIDC_IOS_ISSUER, OCIS_OIDC_ANDROID_ISSUER can just fallback to OCIS_OIDC_ISSUER if they are not set for their respective client.

micbar commented 2 months ago

@rhafer @butonic Do we need to do something here?

butonic commented 2 months ago

tl;dr no, close this issue. PROXY_OIDC_ACCESS_TOKEN_VERIFY_METHOD=none does not reduce security.

Long version:

Authentik now supports the oidc webfinger endpoint: https://github.com/goauthentik/authentik/pull/10400

oCIS supports a rewrite of that endpoint because our clients currently try the oidc discovery at the ocis domain.

What I do not understand is why the different ocis clients lead to multiple providers/applications in Authentik?

👀

Ok after reading https://helgeklein.com/blog/owncloud-infinite-scale-with-openid-connect-authentication-for-home-networks/#authentik-configure-openid-connect-idp I now understand that OIDC clients are called providers in authentik and providers have a 1:1 relationship with applications.

AFAICT a single OpenID Connect Identity provider should support multiple clients. That is the whole idea of OpenID Connect: multiple clients being able to externalize the user and credential management to an identity provider.

And it seems to me that authentik already does support multiple clients as per the yaml config in https://helgeklein.com/blog/owncloud-infinite-scale-with-openid-connect-authentication-for-home-networks/#authelia-configure-openid-connect-idp

Setting PROXY_OIDC_ACCESS_TOKEN_VERIFY_METHOD=none is perfectly fine. The proxy then queries the userinfo using the access token and can prevents access based on the response. PROXY_OIDC_ACCESS_TOKEN_VERIFY_METHOD=jwt can be used with PROXY_OIDC_SKIP_USER_INFO=true to support ADFS.

Also, OIDC actually recomments the access token to be opaque to prevent leaking data that might find its way into a JWT. PROXY_OIDC_ACCESS_TOKEN_VERIFY_METHOD=jwt may be used with PROXY_OIDC_SKIP_USER_INFO=true to save a trip to the IdP, which helps performance during login.

That being said, oCIS wants to support multiple identity providers in the future at least for migration purposes. However, we need to support assigning multiple identities to users, which is not a priority right now as it touches a lot of code.

So ... no, I don't think we should do something here. And I am against adding multiple env vars that influence the output of the wellknown lookup. We would have to look at the user agent to change the issuer url in the /.well-known/openid-configuration endpoint. That is the wrong approach. The correct fix would be to let our clients implement openid-connect discovery. Which might put even more burden and work on the hoster because he has to ensure that clients can look up the IdP based on their email.

That clients ask oCIS for the /.well-known/openid-configuration is IMO just a fallback when users try to login with a username and not an email address.

danthonywalker commented 2 months ago

And it seems to me that authentik already does support multiple clients as per the yaml config in https://helgeklein.com/blog/owncloud-infinite-scale-with-openid-connect-authentication-for-home-networks/#authelia-configure-openid-connect-idp

That is for Authelia, not authentik.