Skip to content

Fix OAuth documentation and tests for Resource Owner Password Credentials Grant

Nikita Bulai requested to merge nikitabulai/gitlab:fix-oauth-resource-owner-docs into master

What does this MR do?

Hi. This is the maintainer of the Doorkeeper gem used by GitLab. There is some point in the GitLab documentation that doesn't conform with RFC (as well as Doorkeeper implementation for it).

Section 4.3.2. Access Token Request for Resource Owner Password Credentials Grant of the OAuth RFC 6749 states next:

If the client type is confidential or the client was issued client credentials (or assigned other authentication requirements), the client MUST authenticate with the authorization server as described in Section 3.2.1. [... by providing client_id for private clients and client_id + client_secret for private clients ...]

Also from the same RFC:

The authorization server MUST:

o require client authentication for confidential clients or for any client that was issued client credentials (or with other authentication requirements),

All the Doorkeeper clients prior to 5.0 version were always private. Then (since 5.0) we introduced a confidential database column that now indicates if client is a public or private one. Though Doorkeeper always generate the client_id and client_secret for any application record. GitLab also migrated to a 5.x version of Doorkeeper not so long ago (maybe in February). Anyway all GitLab client applications (instances of Doorkeeper::Application) have client id and client secret filled. So anyone who works with GitLab using OAuth Resource Owner Password Credentials Grant must provide client credentials in the request to follow the RFC requirements.

However, recently I discovered that Doorkeeper implementation for Resource Owner Password Credentials Grant allows to perform a request without client authentication (anyone could just skip providing HTTP Basic auth or passing client id and client secret in the request body). This doesn't impact resource owner authentication however, and almost makes no sense for Web applications (just because it's super hard to hide client credentials from anyone).

This is not a security issue actually, but this behavior doesn't conform with the OAuth RFC. I'm planning to fix this issue in a next release of the gem. I still not sure if I will save current behavior as a configuration option to allow existing applications not to break this flow, or I will deprecate it first and then remove in a major version - I'll decide it later.

But what we can do now is to fix the documentation so the developers will use the proper way of dealing with Resource Owner Password Credentials Grant. Those why I'm starting from the GitLab docs for OAuth.

/cc @jhyson, @toupeira, @marcia

/cc @gitlab-com/gl-security/appsec

P.S. This is my first contribution to GitLab so please point me in a right direction if I need to fix something in this PR description like the info below.

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

NA

Security

If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:

  • Label as security and @ mention @gitlab-com/gl-security/appsec
  • [-] The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
  • [-] Security reports checked/validated by a reviewer from the AppSec team
Edited by Craig Norris

Merge request reports