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

Generic OAuth2 authentication: incorrect redirect_uri during access token request, does not match authorization request due to query parameters being appended #1642

Closed
lpar opened this issue Nov 14, 2017 · 6 comments

Comments

@lpar
Copy link

lpar commented Nov 14, 2017

Infos:

Using zammad-2.1.0-1510157740.86d5b882.centos7.x86_64
Firefox 57

I've been trying to work out how to get the generic OAuth2 authentication flow to be compatible with OpenID Connect, since the latter is a subset of the former.

During this process, I've been using a Go HTTP server to act as a proxy OAuth server and dump the requests and responses, and I believe I've found a bug.

Expected behavior vs actual behavior

(Sections cited below refer to RFC 6749, OAuth2 Authorization Framework.)

I click the OAuth2 login button, and Zammad initiates an authorization request as per section 4.1.1.

My browser is redirected to the OAuth2 server, which receives a GET request:

GET /authorization/path?client_id=CLIENT_ID&redirect_uri=https%3A%2F%2Fzammad.example.com%2Fauth%2Foauth2%2Fcallback&response_type=code&state=f40876d758810fd5cb6f3e1c9a0bbf6596f1da63e2b0789c HTTP/1.1

where CLIENT_ID is the client ID registered with the server. Note that the redirect_uri parameter passed is https://zammad.example.com/auth/oauth2/callback.

Next, I log in on the OAuth2 server successfully. My browser is redirected back to the redirect_uri with a one-time code and the matching state parameter from the original GET request as per section 4.1.2. e.g.

https://zammad.example.com/auth/oauth2/callback?code=123CODE789&state=f40876d758810fd5cb6f3e1c9a0bbf6596f1da63e2b0789c

Next, Zammad needs to use the code it has received, and make an access token request to the token endpoint. It makes an HTTP POST request to the OAuth2 server token endpoint:

POST /token HTTP/1.1
accept-encoding: gzip;q=1.0,deflate;q=0.6,identity;q=0.3
accept: */*
connection: close
content-length: 264
user-agent: Faraday v0.11.0
content-type: application/x-www-form-urlencoded

client_id=OTU2NzYyYmItNWZiNS00&client_secret=ZjE4MjEwNGYtNWMyMi00&code=123CODE789&grant_type=authorization_code&redirect_uri=https%3A%2F%2Fzammad.example.com%2Fauth%2Foauth2%2Fcallback%3Fcode%3D123CODE789%26state%3Df40876d758810fd5cb6f3e1c9a0bbf6596f1da63e2b0789c

And this is where the bug occurs. The decoded redirect_uri parameter is https://zammad.example.com/auth/oauth2/callback?code=123CODE789&state=f40876d758810fd5cb6f3e1c9a0bbf6596f1da63e2b0789c

This is incorrect. As section 4.1.3 states:

 redirect_uri
         REQUIRED, if the "redirect_uri" parameter was included in the
         authorization request as described in Section 4.1.1, and their
         values MUST be identical.

In this case they are not identical, because the token request POST is including the code and state as query parameters.

The OAuth2 server therefore refuses to issue a token, because the redirect_uri doesn't match what it was during the initial authentication request.

I assume that this hasn't been noticed before because other OAuth2 servers ignore any query parameters when comparing the redirect_uri between authorization request and access token request, but apparently ours is pedantic.

@lpar
Copy link
Author

lpar commented Dec 5, 2017

Still a problem in zammad-2.1.0-1512403885.b993a3ef.centos7.x86_64. Is there any other information you need?

@thorsteneckel
Copy link
Contributor

Currently not. Just more time and/or some Ruby devs applying for our opening... Pull Request would work too. I guess 🤓

@lpar
Copy link
Author

lpar commented Dec 20, 2017

Looks like this is omniauth-oauth2 bug #93.

@lpar
Copy link
Author

lpar commented Dec 20, 2017

Can confirm that changing /opt/zammad/config/initializers/omniauth.rb to explicitly set token_params...

# oauth2 database connect
  provider :oauth2_database, 'not_change_will_be_set_by_database', 'not_change_will_be_set_by_database', {
    setup: SETUP_PROC,
    client_options: {
      site: 'https://not_change_will_be_set_by_database',
      authorize_url: '/oauth/authorize',
      token_url: '/oauth/token',
    },
    token_params: {
      redirect_uri: 'https://zammad.example.com/auth/oauth2/callback'
    }
  }

...fixes the problem. Presumably you could make it set the appropriate value from the config in the database, but I don't know Zammad's code well enough to know how to do that and supply a patch.

@thorsteneckel
Copy link
Contributor

Hi @lpar - good news! We managed to get a working solution for all affected OAuth2 providers. In our case the Office365 integration was affected. The issue was caused by using NGINX as a reverse proxy for handling the HTTPS stuff. It redirects the requests internally as HTTP which causes OmniAuth to detect requests as those. This leads to the generation of a wrong HTTP redirect_uri as you described and we encountered.

Luckily OmniAuth allows to set a Proc as OmniAuth.config.full_host which allows us to define the full_host at runtime. So this is what we do and it works in my tests for Office365 and generic OAuth2 👍

The fix is available in stable in ~30 minutes from now. Feedback is highly appreciated. Closing now. 🚀

@martini
Copy link
Collaborator

martini commented Apr 20, 2018

JFI: I solved this (or similar) issue by using a correct nginx #723 (comment) (just a side info)

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

No branches or pull requests

3 participants