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

RedirectURL's for public clients #1300

Closed
RichardLindhout opened this issue Sep 19, 2018 · 22 comments
Closed

RedirectURL's for public clients #1300

RichardLindhout opened this issue Sep 19, 2018 · 22 comments

Comments

@RichardLindhout
Copy link

I'm currently adding a feature to our webapp where we want to login with the microsoft connector.

    - id: webapp
      secret: ********************************
      public: true
      name: 'Webapp'
      redirectURIs:
      - 'https://dexserver.com/accounts/callback' 
      - 'http://localhost:3000/connected-login'
      - 'https://webapp.com/connected-login'
    connectors:
    - type: microsoft
      id: microsoft
      name: Microsoft
      config:
        clientID: ***********************
        clientSecret: ***********************
        redirectURI: https://dexserver.com/accounts/callback
        tenant: ********.onmicrosoft.com

The windows connect redirects from the microsoft login to our dex server and the dex server redirects it to the public client with a code as parameter.

In our webapp we get our access_token with the code which is send to us via dex when authenticated with microsoft.

It works perfect when the redirectURI=http://localhost:3000/connected-login
But not when the redirectURI=https://webapp.com/connected-login

According to this code redirects are only allowed for non-public

	if !client.Public {
		for _, uri := range client.RedirectURIs {
			if redirectURI == uri {
				return true
			}
		}
		return false
	}

I don't get that exactly since the redirectURI is checked when obtaining the access_tokens:

handleAuthCode (server/handler.go:643)

authCode.ClientID != client.ID > error
authCode.RedirectURI != redirectURI > error

Since the redirect and client id are checked why not allow redirectURI for public-clients.

flow_dex_swimminglane
flow-dex

@RichardLindhout
Copy link
Author

I also can't obtain access_tokens with the microsoft connector login with a non-public client since it would expose the secret when obtaining the access_token via the authorization_code grant.

@srenatus
Copy link
Contributor

srenatus commented Sep 19, 2018

Have you seen the docs on "public clients"? They're trying to provide some colour re: how it's intended to be used.

I also can't obtain access_tokens with the microsoft connector login with a non-public client since it would expose the secret when obtaining the access_token via the authorization_code grant.

I'm not sure I'm following you here. Where would the secret be exposed?

Update: I wasn't clear there, sorry, I would think a non-public client should work fine for you there. Trying to understand why it doesn't 😃

@RichardLindhout
Copy link
Author

@srenatus
I need to send the client_secret in the basic_auth when obtaining my access_token and thus leaking the secret for a non-public client

@srenatus
Copy link
Contributor

Ah, so, this is from a web app and those notoriously can't keep secrets. Sorry I've missed that bit.

Have you considered using the implicit flow (auth grant "token+id_token" IIRC) with a non-public client? It might be comparable in security features to having a public client.

(Also #1114 might be relevant?)

@RichardLindhout
Copy link
Author

Implicit flow would work. Except I want to also have the refresh-token and it recommended to use the authorization_code flow.

But there aren't any security issues when allowing more redirectURL's than the 'urn:ietf:wg:oauth:2.0:oob' right, or am I overlooking something.

@srenatus
Copy link
Contributor

srenatus commented Sep 19, 2018 via email

@RichardLindhout
Copy link
Author

Since we only redirect to specified redirectURI's it could not be the case that an attacker could obtain the refresh_token right?

@srenatus
Copy link
Contributor

srenatus commented Sep 19, 2018 via email

@RichardLindhout
Copy link
Author

RichardLindhout commented Sep 19, 2018

Ah ok, but it would also if we use the 'urn:ietf:wg:oauth:2.0:oob' only with more actions required.
-> no redirect url -> login on dex/microsoft -> user sees code on dex page -> user fills code in webapp -> webapp obtains token+refreshtoken
-> redirect url -> login on dex/microsoft -> dex redirects to the webapp with code-> webapp obtains token+refreshtoken

@srenatus
Copy link
Contributor

srenatus commented Sep 19, 2018 via email

@RichardLindhout
Copy link
Author

Ah that's right! But I also need this in my mobile-app but with a custom protocol like this:

native-app://redirectUrl

So it's needed for mobile apps when you are using a connector and don't your users to fill in the code manually since it is not really good UX.

@srenatus
Copy link
Contributor

Hehe, I feel like we're getting somewhere 😄 So, it's two problems.

For the mobile app, would an additional restriction to the public clients' redirect URI, say, "scheme != http[s]" near here work?

For the web app, an alternative road could be something like #990, where the web-app can do a interaction-less "click-through" to get another id_token, based on the connector data in the old id_token provided in the request. I think this would be preferable to handing out refresh tokens, and in my reading is well within spec for OIDC.

What do you think?

@RichardLindhout
Copy link
Author

Some background information: We're using the same flow in my webapp for username and password. To skip the user code part.

const reqId = getRequestCodeFromHTML(res);

  var formData = new FormData();
  formData.append("login", username);
  formData.append("password", password);

  const authRes = yield call(api, {
    method: "POST",
    basePath: authPath,
    path: `auth/local?req=${reqId}`,
    body: formData,
    type: "text",
    noAuth: true
  });
  // get the auth code
  const authTokenMatch = authRes.match('value="(.*)"');
  const accessTokenRes = yield call(askTokensWithCode, authTokenMatch[1]);
  return authResToObject(accessTokenRes)

We run the dex server on the same address as the webapp so we skip CORS issues.


According to your suggestion: scheme https:// should work so we can allow the dex server to send the code to trusted url's or trusted mobile url's.

@ericchiang
Copy link
Contributor

But there aren't any security issues when allowing more redirectURL's than the 'urn:ietf:wg:oauth:2.0:oob' right, or am I overlooking something.

For 'urn:ietf:wg:oauth:2.0:oob' the client secret isn't secret. When using a redirectURL, the secret is supposed to be secret. They have different security profiles and that's why we haven't historically let the same client use both.

Refresh tokens probably make this even worse. We never figured out the revocation story there.

@ericchiang
Copy link
Contributor

Why do you need the client to support both? Why not just use two clients?

@RichardLindhout
Copy link
Author

Features needed:
Endless login (so refresh-token support)
Connector login
No user actions required regarding to a token copy-pasting
Needs to work in client-side apps

But it's probably that im describing the following flow: https://tools.ietf.org/html/rfc7636
But it has some extra security measures in place.

According to this flow we can use redirectUrl's

@RichardLindhout
Copy link
Author

@ericchiang I can't get my access_token + refresh_token via the auth_code without exposing my secret when I'm using a NON-public client.

@ericchiang
Copy link
Contributor

Okay, so beyond the general discussion of issues around mobile flows, it sounds like the right way to approach this issue is to implement #1114. Is that right?

@RichardLindhout
Copy link
Author

RichardLindhout commented Sep 19, 2018

I think it's the latest and safest approach to mobile authentication in OAuth. So yes this would fix issues with client-side applications.

@RichardLindhout
Copy link
Author

@ericchiang

For 'urn:ietf:wg:oauth:2.0:oob' the client secret isn't secret. When using a redirectURL, the secret is supposed to be secret. They have different security profiles and that's why we haven't historically let the same client use both.

Refresh tokens probably make this even worse. We never figured out the revocation story there.

If the user fills in the code in the webapp the refresh token is still in possession of the webapp. I don't think it makes a difference?

@tkleczek
Copy link
Contributor

@sagikazarmark I believe this issue is fixed by PR #1822

@sagikazarmark
Copy link
Member

I think you are right. Thanks!

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

No branches or pull requests

5 participants