Gitlab oauth support #305

miksir · 2020-06-04T13:51:24Z

Support of gitlab authentication
https://github.com/miksir/script-server/wiki/Authentication#gitlab-oauth

bugy · 2020-06-04T14:08:54Z

Wow, that's mighty! thanks a lot!

Just a couple of questions before I start digging into the code:

1

dump - filename where to persist user/group information
session_expire_min - user inactivity time before logged out

do you think it's GitLab specific? Or it can be re-used for all authentications?

2

dump

does it have default value? I think it would be ok to save it to temp folder by default

3

Do you store user information anywhere, to perform additional requests (for ttl, for example). I tried avoiding to store any user information, to reduce user credentials (or tokens) exposure.
So if someone gets access to Script server or the user cookie, they still won't be able to access 3rd party auth services (i.e. google, gitlab or ldap)

miksir · 2020-06-04T14:56:00Z

do you think it's GitLab specific? Or it can be re-used for all authentications?

both implemented only for gitlab auth at this moment
session_expire_min can be useful for all auth providers and can be implemented overriding new is_active method from Authenticator
dump - depend on where we want to to store some session data... I decided to store in memory and dump to file, but may be someone will decide to use secure cookie or database :)
if your question about naming - its ok to rename

does it have default value? I think it would be ok to save it to temp folder by default

nope, if dump missed - it's totally disabled and restart of app will logout all users

Do you store user information anywhere, to perform additional requests (for ttl, for example). I tried avoiding to store any user information, to reduce user credentials (or tokens) exposure.
So if someone gets access to Script server or the user cookie, they still won't be able to access 3rd party auth services (i.e. google, gitlab or ldap)

Well, for sure it's stored in memory
dump store it in file, but this options can be skiped

Also I think to implement special type of authentication without group support, in this case I can ask read_user scope from gitlab, it's read only, so user api token can be used only for reading some information.

I will think how to hide data in dump file. May be to encrypt it with key passed via ENV. But this problem is wider... for example, how to stop accessing logs because user can remove logs with script.

miksir · 2020-06-04T15:04:35Z

Added information about dump to wiki page

bugy · 2020-06-04T15:28:10Z

Thanks for your answers!

if your question about naming - its ok to rename

no, not really. My question was about reusing it for all authentication :) If it would be easily achieved in future

I will think how to hide data in dump file. May be to encrypt it with key passed via ENV.

If an attacker has access to the server, he can access the key as well. So I don't think it would be much more secure.
I think it's fine to store it as it is, but feature users should be aware of this potential security risk.

But this problem is wider... for example, how to stop accessing logs

I think if the server is compromised, there is not much you can do about server data itself. But if you dump user credentials (or gitlab user token), then the attacker would be able to access GitLab as well.

What kind of user data/credentials do you store in the moment? By the way, did you consider storing it in user secured cookies?
For the current authentication mechanism, I perform user authentication once, and if it's successful, I just store that it was authenticated and discard any other credentials/tokens. It has some drawbacks (e.g. if user groups got updated in the meantime or the user is removed), but I thought, that doesn't happen so often and I can provide stronger security guarantees in this case.

PS sorry if these questions sound as critique (or request for changes), it's definitely not intended this way. I'd like to discuss and understand the chosen approach. And we can try to find all potential flaws and benefits, so Script server usage will be secure (first) and convenient.

miksir · 2020-06-04T16:27:42Z

What kind of user data/credentials do you store in the moment?

Only OAuth2 token but with API scope, so it's a lot. There is read_user scope, its safe, but can't fetch user's groups with it.

By the way, did you consider storing it in user secured cookies?

well, it's possible, can do it

I think if the server is compromised, there is not much you can do about server data itself.

I am thinking about "script wrapper" for run script with sudo or in docker container. It can help to restrict access to script server files.

…rmation + group_support on/off, is off - read_user scope used

miksir · 2020-06-04T20:50:54Z

Gitlab oauth key now stored in secure cookie. For better safety now it's possible to auth with read_user gitlab scope - it's read only scope with very limited access, but gitlab groups will not be used.

dump -> state_dump_file, ttl -> auth_info_ttl, session_expire_min -> session_expire_minutes

bugy

Wow, impressive work! Thanks a lot!

I have a couple of questions (since I don't know how Gitlab auth works) and some suggestions about extracting common code

src/auth/auth_gitlab.py

src/tests/auth/test_auth_gitlab.py

web-src/src/login/login.js

src/web/server.py

src/tests/auth/test_auth_gitlab.py

bugy · 2020-06-05T11:50:15Z

Hi @miksir, I checked the PR. At the moment there is a lot of duplication between Google oauth and GitLab oauth. What do you think about extracting common part to some generic OauthAuthenticator? And then let google and GitLab implementations implement only the part, which is different?

I think it would be fine if you move your new logic (with logout, dump, ttl, etc.) to this common class as well, so it will be used by google authenticator also.

miksir · 2020-06-05T13:06:32Z

Both is OAuth, but lot of small difference. logout, dump, ttl needs only because user validity recheck implemented, but in Google OAuth not. I see what you want but it's require massive refactoring of Authenticator :(

bugy · 2020-06-05T13:39:13Z

but lot of small difference. logout, dump, ttl needs only because user validity recheck implemented

But all of those can be used for google as well, cannot they? The only difference is how you parse the response. For Gitlab you check the state (and probably groups), but for google you just get email field.
If there is an error or email is missing, the use is also inactive and can/should be logged out

bugy · 2020-06-09T21:32:23Z

Hi @miksir should I try to help you with the refactoring? I should have time on Thursday, I think.

miksir · 2020-06-10T00:36:43Z

I pushed some changes. Please, check unresolved with my answers - do you need more info? I will try to refactor unit tests today.

If you want to refactor google oauth too - yes, I need you help because I has other priority tasks from users.

…rmation + group_support on/off, is off - read_user scope used

dump -> state_dump_file, ttl -> auth_info_ttl, session_expire_min -> session_expire_minutes

bugy · 2020-06-11T09:58:51Z

Hi @miksir, I'm working on the refactoring, hope to finish until end of the day
Are you going to do any changes here, today? Because it would be quite hard to merge them

miksir · 2020-06-11T10:08:43Z

Nop, no changes planned. But I rebased branch (no changes, just rebase), so rebease too :)

bugy · 2020-06-12T09:15:08Z

Hi @miksir I did common behaviour extraction to a separate class. Unfortunately, I couldn't test Gitlab auth, so if you could check (and probably fix it), it would be cool.

I'll still work on unit tests

I also changed current behaviour a bit:

I'm not storing user state anymore. If it becomes disabled, I just remove user's state completely and he has to re-login. I thought, that storing a state was a bit redundant
A user is validated only during his validate_user phase and only this user
Access_token is stored only if auth_info_ttl is enabled
Dump file is taken every 30 seconds, to avoid potential clashes when multiple users are being updated simultaneously. I think, I'll also add checksum checking, to avoid unnecessary writes, if nothing has changed
Custom OAuth implementations have to define fetch_user_info and fetch_groups (i.e. how to load the data). Everything else is managed by a common class
I changed user state from a dictionary to a dedicated class. For me, it's easier to work in PyCharm, when the IDE knows about existing fields and can suggest/validate them :)
The dump file structure was changed a bit, to address 1. and 6., so I believe you would have to delete the dump file before testing

I think I went to deep and probably you won't recognize your code anymore 😅 But what do you think about the new flow?

miksir · 2020-06-12T20:22:51Z

I'm not storing user state anymore. If it becomes disabled, I just remove user's state completely and he has to re-login. I thought, that storing a state was a bit redundant

Dump file is taken every 30 seconds, to avoid potential clashes when multiple users are being updated simultaneously. I think, I'll also add checksum checking, to avoid unnecessary writes, if nothing has changed

Sorry, I am confused :) Dump and storing user state was the same :) It was dump of users state.

bugy · 2020-06-12T20:46:05Z

I meant state['state'] :) Like 'active', 'error', etc.

miksir · 2020-06-12T21:03:00Z

I C. Great work. It's look like you can refactor ldap auth too :)

bugy · 2020-06-13T09:00:02Z

Ye, it would be nice... But i think it's too much for now :) One question: for session expiry you chose minutes, do you have a real case for it? Usually it's days. I would think that hours would be more than enough And in this case, it would be way easier to implement it, by setting expiry time for the username cookie miksir <[email protected]> schrieb am Fr., 12. Juni 2020, 23:03:

…

I C. Great work. It's look like you can refactor ldap auth too :) — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#305 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJXPJPH3JFQFHRNMG7RGVLRWKJZDANCNFSM4NSUU22A> .

miksir · 2020-06-13T16:04:43Z

One question: for session expiry you chose minutes, do you have a real case for it? Usually it's days. I would think that hours would be more than enough

Well, may be someone want 30 min session. So just got minimal usable. But, imho, best way is to use human readable form. Like 30m or 1h30m

bugy · 2020-06-13T17:44:42Z

Hi @miksir I'm done with the refactoring.

Let's keep the session behaviour as per your implementation for now: I realized, that using cookie expire time would have different behaviour: the time wouldn't be reset on user actions and would just measure time after login.

If gitlab auth is still working for you, and you are fine with my changes, we can merge the PR

miksir · 2020-06-15T09:56:34Z

Will check today. But i think you can register on gitlab.com and add application here https://gitlab.com/profile/applications for test

bugy · 2020-08-09T11:51:37Z

Alright, I finally tested it and it seems to work :)
Thanks a lot @miksir for the great work!

Miksir added 2 commits June 4, 2020 01:11

Gitlab OAuth authentication

7543230

Gitlab Oauth tests

4a02f49

+ Store Gitlab keys to cookes so dump file now free from private info…

b420238

…rmation + group_support on/off, is off - read_user scope used

Rename config keys:

0ca6270

dump -> state_dump_file, ttl -> auth_info_ttl, session_expire_min -> session_expire_minutes

bugy requested changes Jun 5, 2020

View reviewed changes

miksir closed this Jun 9, 2020

miksir reopened this Jun 9, 2020

Refactoring of gitlab auth class

45a8987

MiksIr added 7 commits June 10, 2020 13:16

Removing unnecessary persists

02bc005

Gitlab OAuth authentication

1c1a2e6

Gitlab Oauth tests

662e252

+ Store Gitlab keys to cookes so dump file now free from private info…

b4623f1

…rmation + group_support on/off, is off - read_user scope used

Rename config keys:

e1e08ce

dump -> state_dump_file, ttl -> auth_info_ttl, session_expire_min -> session_expire_minutes

Refactoring of gitlab auth class

88b2c48

Removing unnecessary persists

29c0c75

miksir force-pushed the gitlab_oauth branch from 02bc005 to 29c0c75 Compare June 11, 2020 09:52

Merge remote-tracking branch 'origin/gitlab_oauth' into gitlab_oauth

956b650

bugy#305 moved gitlab and google oauth shared code to a common class

67c105d

bugy#305 added oauth authenticate and validate_user tests

c6c8201

bugy added 2 commits June 13, 2020 17:10

bugy#305 added oauth update_auth, logout and dump tests

fa3d664

bugy#305 updated gitlab oauth tests to reflect new structure

dd1ff23

bugy#305 fixed flickering and <3.7 incompatible tests

c53dfd1

bugy added 2 commits August 9, 2020 13:18

Merge branch 'master' into gitlab_oauth

7d3c7c1

bugy#305 added groups logging for OAuth

63d02ef

bugy merged commit e39523f into bugy:master Aug 9, 2020

bugy added bug feature labels Aug 9, 2020

bugy added this to the 1.16.0 milestone Aug 9, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Gitlab oauth support #305

Gitlab oauth support #305

miksir commented Jun 4, 2020

bugy commented Jun 4, 2020

miksir commented Jun 4, 2020 •

edited

Loading

miksir commented Jun 4, 2020

bugy commented Jun 4, 2020

miksir commented Jun 4, 2020

miksir commented Jun 4, 2020

bugy left a comment

bugy commented Jun 5, 2020

miksir commented Jun 5, 2020

bugy commented Jun 5, 2020

bugy commented Jun 9, 2020

miksir commented Jun 10, 2020

bugy commented Jun 11, 2020

miksir commented Jun 11, 2020

bugy commented Jun 12, 2020

miksir commented Jun 12, 2020

bugy commented Jun 12, 2020

miksir commented Jun 12, 2020

bugy commented Jun 13, 2020 via email

miksir commented Jun 13, 2020

bugy commented Jun 13, 2020

miksir commented Jun 15, 2020

bugy commented Aug 9, 2020

Gitlab oauth support #305

Gitlab oauth support #305

Conversation

miksir commented Jun 4, 2020

bugy commented Jun 4, 2020

1

2

3

miksir commented Jun 4, 2020 • edited Loading

miksir commented Jun 4, 2020

bugy commented Jun 4, 2020

miksir commented Jun 4, 2020

miksir commented Jun 4, 2020

bugy left a comment

Choose a reason for hiding this comment

bugy commented Jun 5, 2020

miksir commented Jun 5, 2020

bugy commented Jun 5, 2020

bugy commented Jun 9, 2020

miksir commented Jun 10, 2020

bugy commented Jun 11, 2020

miksir commented Jun 11, 2020

bugy commented Jun 12, 2020

miksir commented Jun 12, 2020

bugy commented Jun 12, 2020

miksir commented Jun 12, 2020

bugy commented Jun 13, 2020 via email

miksir commented Jun 13, 2020

bugy commented Jun 13, 2020

miksir commented Jun 15, 2020

bugy commented Aug 9, 2020

miksir commented Jun 4, 2020 •

edited

Loading