#20628 Enable implicit flow in Gitlab as OAuth Provider

What does this MR do?

Enables implicit flow in Gitlab as OAuth provider.

Why was this MR needed?

This will enable users to use Gitlab OAuth provider for user-agent-based application (e.g. running on GitLab Pages).

Does this MR meet the acceptance criteria?

• Changelog entry added, if necessary
• Documentation created/updated
• API support added
• Tests
  • Added for this feature/bug - _I'm not sure if this can be unit-tested, application for manual tests provided
  • All builds are passing
• Conform by the merge request performance guides
• Conform by the style guides
• Branch has no merge conflicts with master (if it does - rebase it please)
• Squashed related commits together

What are the relevant issue numbers?

Closes #20628 (closed) - Enable OAuth Implicit Grant, with that enabled it also resolves #2716 (closed) and #19470 (closed) as all the browser clients can apply best practices that don't require CORS for /oauth/token.

I followed up the work done by @medokin (kudos) in !6078 (closed), rebased and added some documentation.

Simple Angular app for oauth testing can be found at empe/gitlab-implicit-grant-demo.

Concerns:

• Commit 5194214e disabled TTL on tokens - the whole point of using access tokens is their limited timespan. Are there any arguments not to limit the token TTLs to at least one week?
• As an OAuthProvider client in most cases I'm only interested in user authentication, but it seems that when using implicit flow and obtaining identity via /api/v4/user endpoint requires api scope, which it an overkill for most of non GitLab related apps. Wouldn't read_user be sufficient?

marked the checklist item Squashed related commits together as completed

marked the checklist item Squashed related commits together as incomplete

changed the description

marked the checklist item Squashed related commits together as completed

added 1 commit

• 1f911a54 - [#20628 (closed)] Enable implicit flow in Gitlab as OAuth Provider

Compare with previous version

changed the description

marked the checklist item Changelog entry added, if necessary as completed

added 1 commit

• 088359b5 - #20628 (closed) Enable implicit flow in Gitlab as OAuth Provider

Compare with previous version

unmarked as a Work In Progress

changed title from WIP: [#20628] Enable implicit flow in Gitlab as OAuth Provider to #20628 Enable implicit flow in Gitlab as OAuth Provider

changed the description

marked the checklist item Branch has no merge conflicts with master (if it does - rebase it please) as completed

@rymai, @smcgivern - It's a clone of !6078 (closed) in which you took a huge part. Can you please review / advise on who can I involve in this MR? Thanks in advance!

added 81 commits

• 088359b5...bf0b3d83 - 80 commits from branch gitlab-org:master
• 6a0129af - #20628 (closed) Enable implicit flow in Gitlab as OAuth Provider

Compare with previous version

changed the description

marked the checklist item All builds are passing as completed

@empe can you help me understand:

all the browser clients can apply best practices that don't require CORS for /oauth/token

? The original merge request (commit https://gitlab.com/medokin/gitlab-ce/commit/a960414b960a327b2c5617f0ae1cf8dc21d7ebdf) was changing some CORS stuff, too. Does the above sentence mean that we don't have to do that? Can you explain why?

Commit 5194214e disabled TTL on tokens - the whole point of using access tokens is their limited timespan. Are there any arguments not to limit the token TTLs to at least one week?

@vsizov do you maybe remember why we had to disable it? I know that it was over two years ago... 😞

As an OAuthProvider client in most cases I'm only interested in user authentication, but it seems that when using implicit flow and obtaining identity via /api/v4/user endpoint requires api scope, which it an overkill for most of non GitLab related apps. Wouldn't read_user be sufficient?

@empe why do we have to obtain identity via /api/v4/user endpoint? (this may be a basic question, but I'm not familiar with the problems this merge request is trying to solve so I'm asking basic questions 😄).

added Community Contribution ~480950 labels

assigned to @briann

@empe thanks for picking up the work! 💚

@briann can you take a look?

added 1 commit

• d7e9c453 - #20628 (closed) Enable implicit flow in Gitlab as OAuth Provider

Compare with previous version

@empe Thank you so much for picking this up! ❤️ ✨

changed the description

marked the checklist item API support added as completed

marked the checklist item Conform by the merge request performance guides as completed

marked the checklist item Conform by the style guides as completed

resolved all discussions

I'm still trying to find time to go through all the possible dangers of enabling implicit grants. I doubt I'll be able to finish testing in the next day or two. In addition to leaks I'm particularly concerned about token fixation.

I'll try to get to this as soon as I can.

@empe thanks, I'll leave this to @briann to mull over for now. Just a couple of questions on the docs.

added 1 commit

• f65ce536 - #20628 (closed) Enable implicit flow in Gitlab as OAuth Provider

Compare with previous version

resolved all discussions

If it relieves your concerns, Bitbucket supports implicit grant

@MariusGundersen As does Google. I just want to make sure we do it properly. 😄

@MariusGundersen I'm comfortable with enabling this grant flow so long as the documentation includes warnings about its proper use for applications using GitLab as a provider and that might store their own data. In particular I'd like to see the warnings shown here: http://technotes.iangreenleaf.com/posts/closing-a-nasty-security-hole-in-oauth.html

To close the security hole, you need to add one extra step. After you receive the OAuth callback with the token, you need to verify the token. Don’t save it or do anything else with the token until it’s verified.

Here’s how you verify the token:

1. Find your own hardcoded client ID. This is what you sent as client_id earlier when you kicked off the whole OAuth flow.
2. Find the verification endpoint offered by the OAuth provider. It may be called “verification”, or it may be called “token info” or “token debug” or something along those lines. Google offers tokeninfo. Facebook offers debug_token. Doorkeeper, an OAuth provider for Rails, offers token/info. The two important traits of this endpoint are that it must require a token, and it must return the application id.
3. Send a request to this endpoint with the token you received as authorization.
4. Find the application ID in the response. Facebook calls this app_id. Google calls it audience. Doorkeeper calls it application.uid. Compare this ID to your own client ID. Do they match? Then you’re all set! Do they not match? Throw away the token and fail ungracefully3.

That’s it! Verify the tokens and the security hole is closed.

As well as the usual info about verifying the state parameter to protect against CSRF.

Or alternatively there could simply be a warning about using the implicit flow for authenticating users for access to data outside of the GitLab instance.

@douwe what do you think?

assigned to @DouweM

• Commit 5194214e disabled TTL on tokens - the whole point of using access tokens is their limited timespan. Are there any arguments not to limit the token TTLs to at least one week?

@empe I think we can and should do that. The reason we currently don't seems to be that a GitLab instance will store the token for the user's GitLab.com account in the GitLab instance's DB, and may not currently be "smart" enough to ask the user to authenticate again if/when it expires. That's something we'd need to investigate and fix.

• As an OAuthProvider client in most cases I'm only interested in user authentication, but it seems that when using implicit flow and obtaining identity via /api/v4/user endpoint requires api scope, which it an overkill for most of non GitLab related apps. Wouldn't read_user be sufficient?

It seems to be like read_user should already be sufficient: http://gitlab.com/gitlab-org/gitlab-ce/blob/master/lib/api/users.rb#L6. Isn't it working for you?

@briann good that you are taking it seriously 👍

Hi @DouweM I did a small refactor of the page to mostly to remove duplications and include security warnings @briann mentioned. Let me know what you think, I'll squash the commits once you give a green light or do the mvp version with content duplication otherwise.

Regarding user scope - it doesn't work for me even with private tokens. I believe this may be a regression or I misunderstood the use case.

Regarding user scope - it doesn't work for me even with private tokens. I believe this may be a regression or I misunderstood the use case.

@empe Sounds like this issue: https://gitlab.com/gitlab-org/gitlab-ce/issues/33022 which was fixed in: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/12300 which will be in the next 9.3 patch release :)

As for this MR, looks good to me! Please squash, and I'll go ahead with the merge.

added 1 commit

• a7c571ae - #20628 (closed) Enable implicit flow in Gitlab as OAuth Provider

Compare with previous version

@DouweM done 👍

resolved all discussions

approved this merge request

merged

mentioned in commit 9a708aec

@DouweM FYI milestone was missing (it's important to set one so that we know how many Community Contribution we merged for each milestone! :)).

changed milestone to %9.4