search

davido / gerrit-oauth-provider

OAuth2 authentication provider for Gerrit Code Review. Please upload changes for review to: https://gerrit-review.googlesource.com/#/admin/projects/plugins/oauth
Apache License 2.0
140 stars 84 forks source link

github sending alert emails that they will disallow access tokens in the URL query string #135

Closed zzzeek closed 4 years ago

zzzeek commented 4 years ago

Hi there -

Very few hits on google for this and I think this is all new. Github is sending these emails:

[GitHub API] Deprecation notice for authentication via URL query parameter

Hi @sqla-tester,

On February 3rd, 2020 at 23:36 (UTC) your application (sqlalchemy-gerrit) used an access token (with the User-Agent Java/1.8.0_212) as part of a query parameter to access an endpoint through the GitHub API:

https://api.github.com/user

Please use the Authorization HTTP header instead as using the access_token query parameter is deprecated.

Depending on your API usage, we'll be sending you this email reminder once every 3 days for each token and User-Agent used in API calls made on your behalf. Just one URL that was accessed with a token and User-Agent combination will be listed in the email reminder, not all.

Visit https://developer.github.com/changes/2019-11-05-deprecated-passwords-and-authorizations-api/#authenticating-using-query-parameters for more information.

The gerrit oauth provider accesses the https://api.github.com/user URL right here: https://github.com/davido/gerrit-oauth-provider/blob/0f807f680d3b204e0d376b473d8a84076a67f138/src/main/java/com/googlesource/gerrit/plugins/oauth/GitHubOAuthService.java#L79


  public OAuthUserInfo getUserInfo(OAuthToken token) throws IOException {
    OAuthRequest request = new OAuthRequest(Verb.GET, PROTECTED_RESOURCE_URL);
    Token t = new Token(token.getToken(), token.getSecret(), token.getRaw());
    service.signRequest(t, request);
    Response response = request.send();

and I believe the access_token is hardwired to be in the query string assuming the scribe library in use is here: https://github.com/dsyer/scribe-java/blob/master/src/main/java/org/scribe/oauth/OAuth20ServiceImpl.java#L59


 public void signRequest(Token accessToken, OAuthRequest request)
  {
    request.addQuerystringParameter(OAuthConstants.ACCESS_TOKEN, accessToken.getToken());
  }

I apologize if I'm getting this all wrong, I have a lot of github / gerrit integration going on but I wrote all of it in Python and it's using access_token in the headers; the oauth plugin is the only tool I have installed in my gerrit server that would report Java as the user agent,the api.github.com/user URL hardwired in the oauth plugin here and the behavior of "scribe" would seem to be the source of the issue. It just seems surprising to me that nobody has reported this yet!

vhelke commented 4 years ago

Just saw this same thing, an hour ago.

davido commented 4 years ago

Thanks for the report. I will try to look into it. GitLab seems to be affected as well, though: [1].

@lucamilanesio: Is gerrithub.io also affected?

[1] https://gitlab.com/gitlab-org/gitter/webapp/issues/2442

lucamilanesio commented 4 years ago

@davido thanks for the feedback, I'll check if we use it and make a fix.

heikkilavanti commented 4 years ago

We ran into the same issue, and the fix we came up seemed easy enough (not sure if it's the correct one though, didn't go digging around too much).

When calling out to ServiceBuilder() in GitHubOAuthService.java, just added signatureType, like so:

    service =
        new ServiceBuilder()
            .provider(new GitHub2Api(rootUrl))
            .apiKey(cfg.getString(InitOAuth.CLIENT_ID))
            .apiSecret(cfg.getString(InitOAuth.CLIENT_SECRET))
            .callback(canonicalWebUrl + "oauth")
            .scope(SCOPE)
            .signatureType(SignatureType.Header)
            .build();

Deployed the modified jar to our instances, and everything seems to be working.

zzzeek commented 4 years ago

Deployed the modified jar to our instances, and everything seems to be working.

Looking at https://github.com/dsyer/scribe-java/blob/master/src/main/java/org/scribe/builder/ServiceBuilder.java the default signature type is already "SignatureType.Header" and this doesn't seem to have any effect if it were something else either.

The email says it's only sent every three days have you watched the actual HTTP requests go out ?

davido commented 4 years ago

I think that in https://github.com/scribejava/scribejava/commit/d82112bb4d41c39265c3447c4d51ad97dff5ed2f the ability to actually sign request in header was added.

And in https://github.com/scribejava/scribejava/commit/73c2ef4a3984336dfa3474a52e6e390170bc3fdc#diff-163a0f46f10a315138f60858fc8981f8R118 it was switched per default in DefaultApi20.java.

So I think one non invasive approach to fix it would be to add in currently used scribe 1.3.7 version the ability to sign request in header first and then switch GitHub OAuth service implementation to using it.

zzzeek commented 4 years ago

yeah I noticed there's two really different versions of this "scribe" thing, not sure of the history there.

davido commented 4 years ago

Yeah. Another approach would be to just use the new release 6.9.0. It could even be used in the same time, given that the package name was changed between major version bump, I think 2.0 or something.

davido commented 4 years ago

I think there is one complication. If I read the spec correctly, they are saying also, that client_id/client_secret also should be sent using basic auth, quoting:

GitHub is deprecating authentication to the GitHub API using query parameters,

such as using a access_token query parameter for OAuth user authentication

or a client_id/client_secret query parameter for OAuth application authentication.

While the request to retrieve the access token is executed in the oauth plugin itself, the application authorization request is issued from gerrit core, using this interface:

https://gerrit.googlesource.com/gerrit/+/refs/heads/master/java/com/google/gerrit/extensions/auth/oauth/OAuthServiceProvider.java#29

in OAuthSession class, here: https://gerrit.googlesource.com/gerrit/+/refs/heads/master/java/com/google/gerrit/httpd/auth/oauth/OAuthSession.java#123

If I am not missing something very obvious, then I think we cannot fix it without extending Gerrit core. We would need to retrieve client api and client secret and be able to put them as basic auth in request header. This would have to happen, at least partly in gerrit core.

Of course this would have to be done conditionally, depending on specific service provider.

What a mess?!

zzzeek commented 4 years ago

My vague recollection of Oauth coupled with the fact that those are redirect URLs suggests those would not have client_id / client_secret in them. the client_id/secret is only in the server-to-server POST call.

on my current gerrit, looking at browser calls the redirect has client id in it:

https://github.com/login/oauth/authorize?client_id=<FOO BAR>&redirect_uri=https%3A%2F%2Fgerrit.sqlalchemy.org%2Foauth&state=<FOO BAR>

which is described in step 1 at https://developer.github.com/apps/building-oauth-apps/authorizing-oauth-apps/#web-application-flow, which is the flow that the deprecation notice is saying that they're supporting.

they're being a little lose with the terms "authentication" and "authorization" however if I recall the terminology correctly, gerrit is only doing the first part of "authentication". its that "api.github.com/user" request that requires "authorization".

github has another form of app authentication which is the client_id/client_secret one I think they are referring towards, that's not web flow and has no access token, that's this one: https://developer.github.com/v3/guides/basics-of-authentication/#accepting-user-authorization

heikkilavanti commented 4 years ago

the default signature type is already "SignatureType.Header" and this doesn't seem to have any effect if it were something else either.

I knew it couldn't be this simple, but one can always hope. And I should have dug a bit deeper, apologies for butting in!

davido commented 4 years ago

I have a working prototype and will upload a new CL in a moment and provide plugin artifact to test.

davido commented 4 years ago

New scribe patch release is here: [1] and the CL that switched GitHub OAuth provider to use client basic auth scheme and bearer token request signing is here: [2]. Plugin artifact is here: [3].

SHA1 is:

  $ sha1sum oauth-g0e144214cc.jar
  4419ef34936e0c9c75c869ce8bcdd1c9040217db  oauth-g0e144214cc.jar

Can someone test it?

[1] https://github.com/davido/scribejava/releases/tag/v1.3.8 [2] https://gerrit-review.googlesource.com/c/plugins/oauth/+/253926 [3] https://ostrovsky.org/gerrit/oauth-g0e144214cc.jar

zzzeek commented 4 years ago

will this plugin work with Gerrit 2.15.4 ? that's what I'm running right now

davido commented 4 years ago

It would not, unfortunately, because of this change: [1].

We would have to move that change on stable-2.15 branch then. Note, though, that support for 2.15 was discontinued and you would have to upgrade to at least 2.16.

[1] https://gerrit-review.googlesource.com/c/gerrit/+/208043

davido commented 4 years ago

I moved that change to stable-2.15 branch: [1]. To test the plugin on stable-2.15 branch: [2].

[1] https://gerrit-review.googlesource.com/c/plugins/oauth/+/253926 [2] https://ostrovsky.org/gerrit/oauth-g470058ed4a.jar

davido commented 4 years ago

I think that in scribejava/scribejava@d82112b the ability to actually sign request in header was added.

In this major overhaul of this plugin dependency: [1], I bumped the scribe version from 1.3.7 to 6.9.0, so that the both features: client basic authentication scheme and bearer access token authorization should be fixed now.

[1] https://gerrit-review.googlesource.com/c/plugins/oauth/+/253929

zzzeek commented 4 years ago

OK I can try out that .jar file now, what will the actual release be, will there be a 2.14.6.3 ? I currently use 2.14.6.2. OTOH If I have to upgrade my gerrit just let me know how far I have to go up.

zzzeek commented 4 years ago

welp I have to upgrade to gerrit 2.16 anyway beacuse none of the plugins are available at https://gerrit-ci.gerritforge.com/view/Plugins-stable-2.15/ anymore and I blew away my docker image

zzzeek commented 4 years ago

OK I used the .jar file at https://ostrovsky.org/gerrit/oauth-g0e144214cc.jar against gerrit-2.16.16, replacing it for the gerrit-oauth-provider.jar file, got the stack trace below.

is that .jar usable for 2.16 or do I need a new build for that.

[2020-02-10 03:30:53,615] [HTTP-93] WARN  org.eclipse.jetty.server.HttpChannel : /oauth
java.lang.NoClassDefFoundError: com/google/gerrit/json/OutputFormat
    at com.googlesource.gerrit.plugins.oauth.GitHubOAuthService.getUserInfo(GitHubOAuthService.java:107)
    at com.google.gerrit.httpd.auth.oauth.OAuthSession.login(OAuthSession.java:106)
    at com.google.gerrit.httpd.auth.oauth.OAuthWebFilter.doFilter(OAuthWebFilter.java:105)
    at com.google.gerrit.httpd.RequireSslFilter.doFilter(RequireSslFilter.java:72)
    at com.google.gerrit.httpd.RunAsFilter.doFilter(RunAsFilter.java:121)
    at com.google.gerrit.httpd.GwtCacheControlFilter.doFilter(GwtCacheControlFilter.java:72)
    at com.google.gerrit.httpd.SetThreadNameFilter.doFilter(SetThreadNameFilter.java:62)
    at com.google.gerrit.httpd.AllRequestFilter$FilterProxy$1.doFilter(AllRequestFilter.java:133)
    at com.google.gerrit.httpd.AllRequestFilter$FilterProxy.doFilter(AllRequestFilter.java:135)
    at com.google.gerrit.httpd.RequestCleanupFilter.doFilter(RequestCleanupFilter.java:60)
    at com.google.gerrit.httpd.RequestMetricsFilter.doFilter(RequestMetricsFilter.java:57)
    at com.google.gerrit.httpd.RequestContextFilter.doFilter(RequestContextFilter.java:64)
    at com.google.inject.servlet.ManagedFilterPipeline.dispatch(ManagedFilterPipeline.java:121)
    at com.google.inject.servlet.GuiceFilter.doFilter(GuiceFilter.java:133)
    at org.eclipse.jetty.servlet.ServletHandler$CachedChain.doFilter(ServletHandler.java:1610)
    at org.eclipse.jetty.servlet.ServletHandler.doHandle(ServletHandler.java:540)
    at org.eclipse.jetty.server.handler.ScopedHandler.nextHandle(ScopedHandler.java:255)
    at org.eclipse.jetty.server.session.SessionHandler.doHandle(SessionHandler.java:1588)
    at org.eclipse.jetty.server.handler.ScopedHandler.nextHandle(ScopedHandler.java:255)
    at org.eclipse.jetty.server.handler.ContextHandler.doHandle(ContextHandler.java:1345)
    at org.eclipse.jetty.server.handler.ScopedHandler.nextScope(ScopedHandler.java:203)
    at org.eclipse.jetty.servlet.ServletHandler.doScope(ServletHandler.java:480)
    at org.eclipse.jetty.server.session.SessionHandler.doScope(SessionHandler.java:1557)
    at org.eclipse.jetty.server.handler.ScopedHandler.nextScope(ScopedHandler.java:201)
    at org.eclipse.jetty.server.handler.ContextHandler.doScope(ContextHandler.java:1247)
    at org.eclipse.jetty.server.handler.ScopedHandler.handle(ScopedHandler.java:144)
    at org.eclipse.jetty.server.handler.HandlerWrapper.handle(HandlerWrapper.java:132)
    at org.eclipse.jetty.server.Server.handle(Server.java:502)
    at org.eclipse.jetty.server.HttpChannel.handle(HttpChannel.java:364)
    at org.eclipse.jetty.server.HttpConnection.onFillable(HttpConnection.java:260)
    at org.eclipse.jetty.io.AbstractConnection$ReadCallback.succeeded(AbstractConnection.java:305)
    at org.eclipse.jetty.io.FillInterest.fillable(FillInterest.java:103)
    at org.eclipse.jetty.io.ChannelEndPoint$2.run(ChannelEndPoint.java:118)
    at org.eclipse.jetty.util.thread.strategy.EatWhatYouKill.runTask(EatWhatYouKill.java:333)
    at org.eclipse.jetty.util.thread.strategy.EatWhatYouKill.doProduce(EatWhatYouKill.java:310)
    at org.eclipse.jetty.util.thread.strategy.EatWhatYouKill.tryProduce(EatWhatYouKill.java:168)
    at org.eclipse.jetty.util.thread.strategy.EatWhatYouKill.run(EatWhatYouKill.java:126)
    at org.eclipse.jetty.util.thread.ReservedThreadExecutor$ReservedThread.run(ReservedThreadExecutor.java:366)
    at org.eclipse.jetty.util.thread.QueuedThreadPool.runJob(QueuedThreadPool.java:765)
    at org.eclipse.jetty.util.thread.QueuedThreadPool$2.run(QueuedThreadPool.java:683)
    at java.lang.Thread.run(Thread.java:748)
Caused by: java.lang.ClassNotFoundException: com.google.gerrit.json.OutputFormat
    at java.net.URLClassLoader.findClass(URLClassLoader.java:382)
    at java.lang.ClassLoader.loadClass(ClassLoader.java:419)
    at java.net.FactoryURLClassLoader.loadClass(URLClassLoader.java:817)
    at java.lang.ClassLoader.loadClass(ClassLoader.java:352)
    ... 41 more
[2020-02-10 03:30:53,621] [HTTP-93] ERROR com.google.gerrit.pgm.http.jetty.HiddenErrorHandler : Error in GET /oauth?code=a113984f67679c59965f&state=K4bYgyJwlcVbBGgKdl3vHOFoc1CWas0fY0EdsEXPAF0
java.lang.NoClassDefFoundError: com/google/gerrit/json/OutputFormat
    at com.googlesource.gerrit.plugins.oauth.GitHubOAuthService.getUserInfo(GitHubOAuthService.java:107)
    at com.google.gerrit.httpd.auth.oauth.OAuthSession.login(OAuthSession.java:106)
    at com.google.gerrit.httpd.auth.oauth.OAuthWebFilter.doFilter(OAuthWebFilter.java:105)
    at com.google.gerrit.httpd.RequireSslFilter.doFilter(RequireSslFilter.java:72)
    at com.google.gerrit.httpd.RunAsFilter.doFilter(RunAsFilter.java:121)
    at com.google.gerrit.httpd.GwtCacheControlFilter.doFilter(GwtCacheControlFilter.java:72)
    at com.google.gerrit.httpd.SetThreadNameFilter.doFilter(SetThreadNameFilter.java:62)
    at com.google.gerrit.httpd.AllRequestFilter$FilterProxy$1.doFilter(AllRequestFilter.java:133)
    at com.google.gerrit.httpd.AllRequestFilter$FilterProxy.doFilter(AllRequestFilter.java:135)
    at com.google.gerrit.httpd.RequestCleanupFilter.doFilter(RequestCleanupFilter.java:60)
    at com.google.gerrit.httpd.RequestMetricsFilter.doFilter(RequestMetricsFilter.java:57)
    at com.google.gerrit.httpd.RequestContextFilter.doFilter(RequestContextFilter.java:64)
    at com.google.inject.servlet.ManagedFilterPipeline.dispatch(ManagedFilterPipeline.java:121)
    at com.google.inject.servlet.GuiceFilter.doFilter(GuiceFilter.java:133)
    at org.eclipse.jetty.servlet.ServletHandler$CachedChain.doFilter(ServletHandler.java:1610)
    at org.eclipse.jetty.servlet.ServletHandler.doHandle(ServletHandler.java:540)
    at org.eclipse.jetty.server.handler.ScopedHandler.nextHandle(ScopedHandler.java:255)
    at org.eclipse.jetty.server.session.SessionHandler.doHandle(SessionHandler.java:1588)
    at org.eclipse.jetty.server.handler.ScopedHandler.nextHandle(ScopedHandler.java:255)
    at org.eclipse.jetty.server.handler.ContextHandler.doHandle(ContextHandler.java:1345)
    at org.eclipse.jetty.server.handler.ScopedHandler.nextScope(ScopedHandler.java:203)
    at org.eclipse.jetty.servlet.ServletHandler.doScope(ServletHandler.java:480)
    at org.eclipse.jetty.server.session.SessionHandler.doScope(SessionHandler.java:1557)
    at org.eclipse.jetty.server.handler.ScopedHandler.nextScope(ScopedHandler.java:201)
    at org.eclipse.jetty.server.handler.ContextHandler.doScope(ContextHandler.java:1247)
    at org.eclipse.jetty.server.handler.ScopedHandler.handle(ScopedHandler.java:144)
    at org.eclipse.jetty.server.handler.HandlerWrapper.handle(HandlerWrapper.java:132)
    at org.eclipse.jetty.server.Server.handle(Server.java:502)
    at org.eclipse.jetty.server.HttpChannel.handle(HttpChannel.java:364)
    at org.eclipse.jetty.server.HttpConnection.onFillable(HttpConnection.java:260)
    at org.eclipse.jetty.io.AbstractConnection$ReadCallback.succeeded(AbstractConnection.java:305)
    at org.eclipse.jetty.io.FillInterest.fillable(FillInterest.java:103)
    at org.eclipse.jetty.io.ChannelEndPoint$2.run(ChannelEndPoint.java:118)
    at org.eclipse.jetty.util.thread.strategy.EatWhatYouKill.runTask(EatWhatYouKill.java:333)
    at org.eclipse.jetty.util.thread.strategy.EatWhatYouKill.doProduce(EatWhatYouKill.java:310)
    at org.eclipse.jetty.util.thread.strategy.EatWhatYouKill.tryProduce(EatWhatYouKill.java:168)
    at org.eclipse.jetty.util.thread.strategy.EatWhatYouKill.run(EatWhatYouKill.java:126)
    at org.eclipse.jetty.util.thread.ReservedThreadExecutor$ReservedThread.run(ReservedThreadExecutor.java:366)
    at org.eclipse.jetty.util.thread.QueuedThreadPool.runJob(QueuedThreadPool.java:765)
    at org.eclipse.jetty.util.thread.QueuedThreadPool$2.run(QueuedThreadPool.java:683)
    at java.lang.Thread.run(Thread.java:748)
Caused by: java.lang.ClassNotFoundException: com.google.gerrit.json.OutputFormat
    at java.net.URLClassLoader.findClass(URLClassLoader.java:382)
    at java.lang.ClassLoader.loadClass(ClassLoader.java:419)
    at java.net.FactoryURLClassLoader.loadClass(URLClassLoader.java:817)
    at java.lang.ClassLoader.loadClass(ClassLoader.java:352)
    ... 41 more
[2020-02-10 03:31:31,310] [PluginScanner] WA
davido commented 4 years ago

OK I used the .jar file at https://ostrovsky.org/gerrit/oauth-g0e144214cc.jar against gerrit-2.16.16

This JAR was built against master and as I wrote is not compatible with 2.16 and older gerrit releases. For that I've uploaded plugin version built against 2.15: [1], or even bumped the scribe version to 6.9.0, see the CL link in my previous comment.

[1] https://ostrovsky.org/gerrit/oauth-g470058ed4a.jar

zzzeek commented 4 years ago

OK, it wasn't clear to me if a jar that works with 2.15 would also work with 2.16 since I thought that was where the API changes were.

you're still waiting on a positive test before releasing right?

zzzeek commented 4 years ago

OK so I am running that and it "works", if you know offhand how I can get HTTPS request logging to come out when I run gerrit, I can confirm the headers are correct,otherwise will just see if I don't get an email in the next three days.

davido commented 4 years ago

Thanks for the feedback.

Can you also test the major upgrade of scribe library (based on stable-2.16 branch)? The CL is here: [1] and the final artifact is here: [2].

[1] https://gerrit-review.googlesource.com/c/plugins/oauth/+/253929 [2] https://ostrovsky.org/gerrit/oauth-g5e0febd09d.jar

borospeti commented 4 years ago

I'm running the plugin built from [1] (hash e4457af1) with gerrit 3.1.0 and GitHub OAuth. So far it has been working nicely and I haven't noticed any issues.

[1] https://gerrit-review.googlesource.com/c/plugins/oauth/+/254332

davido commented 4 years ago

@borospeti Thanks for the feedback. Note, though, that for 3.1.x release you shoud rather use the master branch, so for 3.1.x gerrit release the right change to build from is: [1].

[1] https://gerrit-review.googlesource.com/c/plugins/oauth/+/254372

borospeti commented 4 years ago

Indeed. I've just copied the wrong link/hash. :)

gerrit-oauth-provider 801c00b Enabled

Mark-Booth commented 4 years ago

I know this issue is now closed, and I'm probably missing something really obvious, but what to I have to do to my current 2.16.11.1 deployment to stop Google sending out these messages?

Do I just need to download https://ostrovsky.org/gerrit/oauth-g5e0febd09d.jar and replace my existing gerrit-oauth-provider-v2.16.1.jar in my plugins folder?

If so, I assume it would be the same if I update to 2.16.16, but what if I upgrade to 3.0.7 or 3.1.3? Would I then have to build the jar from source?

Once you are sure these fixes are working, @davido, will you be making v2.14.2 and v3.0.1 releases on github? Or should I now be downloading from gerritforge instead?

davido commented 4 years ago

You can not use 2.16 with 3.x and vise versa.

If you use gerrit 2.16 you need the plugin that is built against that gerrit version. Yes, just fetch the JAR from the link in my previous comment.

If you upgrade to 3.x then you would have to use a different JAR, that I also provided in my previous comment.

Once you are sure these fixes are working, @davido, will you be making v2.14.2 and v3.0.1 releases on github? Or should I now be downloading from gerritforge instead?

When the pending changes for review will be merged, then the corresponding build artifacts will show up in GerritForge-CI.

Note, though, that the changes are not merged yet (probably in next days). So, in case someone would like to build them from source, the changes would have to be fetched in the corresponding gerrit release branch:

stable-2.16: https://gerrit-review.googlesource.com/c/plugins/oauth/+/253929 stable-3.0: https://gerrit-review.googlesource.com/c/plugins/oauth/+/254332 stable-3.1 and master: https://gerrit-review.googlesource.com/c/plugins/oauth/+/254372

Mark-Booth commented 4 years ago

Thanks for the clarifications @davido. If its a matter of a few days I'll wait for a build (I've starred the change in gerrit, and it saves making my upgrade documentation more complex than it needs to be). I guess I should also apologise the the people getting the nag e-mails. *8')

zzzeek commented 4 years ago

sorry to keep adding to the bother here but the jar file at https://gerrit-ci.gerritforge.com/job/plugin-oauth-bazel-stable-2.16/ is not updated yet, right? I get all my other plugins from this site so if oauth is there now that would be more convenient than getting this one from the local github download.

davido commented 4 years ago

sorry to keep adding to the bother here but the jar file at https://gerrit-ci.gerritforge.com/job/plugin-oauth-bazel-stable-2.16/ is not updated yet, right?

Right. The changes linked in my previous comment are not merged yet.

davido commented 4 years ago

All changes were merged. New plugin release based on Gerrit 3.1.3 plugin API is here: [1].

For other releases, please consult GeritForge-CI, e.g. for 2.16 branch: [2].

[1] https://github.com/davido/gerrit-oauth-provider/releases/tag/v3.1.3 [2] https://gerrit-ci.gerritforge.com/job/plugin-oauth-bazel-stable-2.16