Oauth flow for Google, GitHub and GitLab #272

Skemba · 2021-07-05T23:37:00Z

This version:

Needs (google, github, gitlab) apps to be turned for verification to be rolled into production
~~Google parameter for allowed_domain was not included as it doesn't provide much more security but can be returned back to improve UX for GSuite users~~
Includes HTML files that can be replaced with a better JS-based frontend code

…irect route

CLAassistant · 2021-07-05T23:37:05Z

All committers have signed the CLA.

kolesnikovae

Awesome! I left a couple of comments, most of them are just nits – please take a look.

kolesnikovae · 2021-07-06T13:49:25Z

pkg/config/config.go

+	GoogleClientID     string `def:"<yourClientID>" desc:"client ID generated for Google API"`
+	GoogleClientSecret string `def:"<yourClientSecret>" desc:"client secret generated for Google API"`
+	GoogleRedirectURL  string `def:"<pyroscope.mycompany.org/google/callback>" desc:"url that google will redirect to after logging in. Has to be in form <pathToPyroscopeServer/google/callback>"`
+	GoogleScopes       string `def:"https://www.googleapis.com/auth/userinfo.email" desc:"scopes for Google API"`


Do we actually need scopes to be configurable?

At this point probably no, I left it in just in case someone could think of a use-case, would be easy to remove if you don't think there's anything useful we could offer?

Yeah, I'd say let's remove them. Less flags is always better.

pkg/config/config.go

pkg/server/controller.go

kolesnikovae · 2021-07-06T14:07:53Z

pkg/server/controller.go

+
+func (ctrl *Controller) oauthLoginHandler(oauthConf *oauth2.Config) http.HandlerFunc {
+	return func(w http.ResponseWriter, r *http.Request) {
+		URL, err := url.Parse(oauthConf.Endpoint.AuthURL)


We should perform all the validation steps before creating oauth2.Config instance. Consider use of shallow copy (golang/go#41733).

URL would be authURL (url collides with the package name).

I'm not sure I understand what should I use shallow copy for? Also if we were to call url.Parse before building oauth2.Config we would have to either pass some sort of identifier and the create it in a switch or remove url parsing to above function which would triple the amount of code. URL parsing should rarely fail so it won't influence performance that much that we created instance before if it fails.

Or did I misunderstand something?

I agree. My point is that when we create *oauth2.Config we should validate user input (fail fast).

BTW, am I right that scopes are passed as a space delimited string? I'm wondering if it works - CLI may not interpret that in the right way.

Next are just my subjective thoughts :) Given that we already have parsed URL at validation, should we do that again for every request? For oauthLoginHandler we use *oauth2.Config as a data container - instead we could introduce our own type to bear prepared data. But it's up to you ;)

Removed scope as config flag so that won't be a problem :)

I really like the idea. Will include this.

pkg/server/controller.go

codecov · 2021-07-07T13:34:10Z

Codecov Report

Merging #272 (8e1a01d) into main (8c4e0c2) will decrease coverage by 3.56%.
The diff coverage is 3.70%.

@@            Coverage Diff             @@
##             main     #272      +/-   ##
==========================================
- Coverage   53.77%   50.22%   -3.55%     
==========================================
  Files          89       92       +3     
  Lines        3638     3981     +343     
==========================================
+ Hits         1956     1999      +43     
- Misses       1482     1775     +293     
- Partials      200      207       +7

Impacted Files	Coverage Δ
pkg/cli/server.go	`0.00% <0.00%> (ø)`
pkg/server/handler.go	`0.43% <0.43%> (ø)`
pkg/server/controller.go	`24.56% <10.53%> (-6.35%)`	⬇️
pkg/storage/tree/flamebearer.go	`100.00% <0.00%> (ø)`
scripts/decode-resp/decode.go	`100.00% <0.00%> (ø)`
scripts/decode-resp/main.go	`0.00% <0.00%> (ø)`
pkg/agent/session.go	`66.40% <0.00%> (+2.46%)`	⬆️
pkg/storage/tree/tree.go	`81.40% <0.00%> (+18.61%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8c4e0c2...8e1a01d. Read the comment docs.

kolesnikovae · 2021-07-08T11:53:59Z

pkg/server/controller.go

 		if err != nil {
-			logrus.Errorf("failed to read file at %s", extraMetadataPath)
+			ctrl.log.Errorf("Error parsing jwt token: %v", err)


Suggested change

ctrl.log.Errorf("Error parsing jwt token: %v", err)

ctrl.log.WithError(err).Error("parsing jwt token")

Totally forgot about logrus WithError, will add in next commit

kolesnikovae · 2021-07-08T12:00:14Z

pkg/server/handler.go

+
+		token, err := oauthConf.Exchange(r.Context(), code)
+		if err != nil {
+			ctrl.logAndRedirect(w, r, "Exchanging auth code for token failed with "+err.Error(), true)


Well, we either can use fmt.Sprintf, or slightly modify the signature:

func (ctrl *Controller) logAndRedirect(w http.ResponseWriter, r *http.Request, format string, args ...interface{})

I think it's okay to invalidate state cookies on error regardless of anything (even if those are missing).

In general only error can be passed so I'll remove this boolean parameter, pass error when it's available and log it with WithError

petethepig · 2021-07-08T18:56:15Z

pkg/config/config.go

+	GoogleEnabled      bool   `def:"false" desc:"enables Google Oauth"`
+	GoogleClientID     string `def:"<yourClientID>" desc:"client ID generated for Google API"`
+	GoogleClientSecret string `def:"<yourClientSecret>" desc:"client secret generated for Google API"`
+	GoogleRedirectURL  string `def:"<pyroscope.mycompany.org/google/callback>" desc:"url that google will redirect to after logging in. Has to be in form <pathToPyroscopeServer/google/callback>"`


Can we have a way of determining this automatically from request Host header?

I think ideally:

it should still be configurable

by default just be an empty string

and if it's an empty string the server just guesses it from host header and current request scheme (http / https)

and we could add something to desc mentioning that you should only set it if pyroscope is under some fancy load balancer

Hm interesting idea ... In general Host header shouldn't be trusted as they can be spoofed, but in this case I can't think of a problem that would cause us. Worst case scenario I can think of is spoofing the Host header which leads to wrong redirect URL and "400" error page from google. I'll look into doing this.

…n and url parsing to controller so that it's only done once. Fixed error handling. Added redirect url generation based on host header. Other PR fixes

Skemba · 2021-07-16T20:56:01Z

pkg/config/config.go

+	GoogleTokenURL     string `skip:"true" def:"https://accounts.google.com/o/oauth2/token" desc:"token url for Google API (usually present in credentials.json file)"`
+
+	GitlabEnabled bool `skip:"true" def:"false" desc:"enables Gitlab Oauth"`
+	// TODO: why is this one ApplicationID and not ClientID ?


Gitlab calls it Application ID while others call it Client ID, but it's used in the same manner in both cases

Skemba · 2021-07-16T21:02:33Z

pkg/config/config.go

+	GithubAuthURL      string `skip:"true" def:"https://github.com/login/oauth/authorize" desc:"auth url for Github API"`
+	GithubTokenURL     string `skip:"true" def:"https://github.com/login/oauth/access_token" desc:"token url for Github API"`
+
+	// TODO: can we generate these automatically if it's empty?


We can, we would just have to write it in some kind of persistent storage so that users don't get logged out if pyroscope is restarted. It's either that or making users explicitly generate it, which I think should be a good security practice (but it does make less of the things out-of-the-box)

Datasource: Prevent error on empty query

Skemba added 3 commits July 5, 2021 00:19

Setup for general auth flow and google oauth

495ab4c

Added github and gitlab oauth flows. Added CSRF protection. Added red…

aaf118f

…irect route

final changes

bb5aea0

Skemba requested a review from petethepig July 5, 2021 23:37

github-actions bot requested a review from Rperry2174 July 5, 2021 23:37

Skemba linked an issue Jul 5, 2021 that may be closed by this pull request

OAuth Authentication #262

Closed

3 tasks

Skemba added the enhancement New feature or request label Jul 5, 2021

petethepig requested review from kolesnikovae and removed request for petethepig and Rperry2174 July 5, 2021 23:55

kolesnikovae reviewed Jul 6, 2021

View reviewed changes

Some PR fixes

d2cbe6f

Skemba force-pushed the feature/oauth branch from 87ec3a1 to d2cbe6f Compare July 7, 2021 13:37

Added forbidden page. Extracted logAndRedirect function

b868f66

Skemba requested a review from kolesnikovae July 7, 2021 18:02

kolesnikovae reviewed Jul 8, 2021

View reviewed changes

petethepig reviewed Jul 8, 2021

View reviewed changes

Skemba and others added 3 commits July 10, 2021 01:16

Removed scopes and default for redirect url. Extracted config creatio…

f2bd26f

…n and url parsing to controller so that it's only done once. Fixed error handling. Added redirect url generation based on host header. Other PR fixes

config changes, no auth by default

2ab4653

makes templates work via webpack

7f97cc0

Skemba commented Jul 16, 2021

View reviewed changes

petethepig added 5 commits July 18, 2021 01:44

Fixes a bug where handler kept updating the same redirectURL variable

eda5de6

makes frontend prettier, adds sign out button

9e50fb7

adds an svg version of the logo

be15ff0

tests fixes

ec07a93

js lint fix

8e1a01d

petethepig merged commit 66ea269 into main Jul 19, 2021

petethepig deleted the feature/oauth branch July 19, 2021 05:53

Rperry2174 mentioned this pull request Sep 1, 2021

Add Documentation for Authentication Flow #367

Closed

korniltsev pushed a commit that referenced this pull request Jul 18, 2023

Datasource: Prevent error on empty query #272

ecbd046

Datasource: Prevent error on empty query

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Oauth flow for Google, GitHub and GitLab #272

Oauth flow for Google, GitHub and GitLab #272

Skemba commented Jul 5, 2021 •

edited by Rperry2174

Loading

CLAassistant commented Jul 5, 2021 •

edited

Loading

kolesnikovae left a comment

kolesnikovae Jul 6, 2021

Skemba Jul 6, 2021

petethepig Jul 8, 2021

kolesnikovae Jul 6, 2021 •

edited

Loading

Skemba Jul 7, 2021

kolesnikovae Jul 8, 2021 •

edited

Loading

Skemba Jul 9, 2021

codecov bot commented Jul 7, 2021 •

edited

Loading

kolesnikovae Jul 8, 2021

Skemba Jul 9, 2021

kolesnikovae Jul 8, 2021

Skemba Jul 9, 2021

petethepig Jul 8, 2021

petethepig Jul 8, 2021

Skemba Jul 9, 2021

Skemba Jul 16, 2021

Skemba Jul 16, 2021

	ctrl.log.Errorf("Error parsing jwt token: %v", err)
	ctrl.log.WithError(err).Error("parsing jwt token")

Oauth flow for Google, GitHub and GitLab #272

Oauth flow for Google, GitHub and GitLab #272

Conversation

Skemba commented Jul 5, 2021 • edited by Rperry2174 Loading

CLAassistant commented Jul 5, 2021 • edited Loading

kolesnikovae left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kolesnikovae Jul 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kolesnikovae Jul 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jul 7, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Skemba commented Jul 5, 2021 •

edited by Rperry2174

Loading

CLAassistant commented Jul 5, 2021 •

edited

Loading

kolesnikovae Jul 6, 2021 •

edited

Loading

kolesnikovae Jul 8, 2021 •

edited

Loading

codecov bot commented Jul 7, 2021 •

edited

Loading