Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add GitLab provider #515

Closed
wants to merge 7 commits into from
Closed

Conversation

cbjartli
Copy link

@cbjartli cbjartli commented Dec 19, 2022

This patch adds direct support of GitLab as identity provider. The provider gets information about the user's groups from the groups claim, and makes it possible to whitelist users by GitLab group membership. This patch extends the teamWhitelist keyword, which until now was only valid for the GitHub provider, to also be valid when used with the new GitLab provider.

Resolves #514.

@bnfinet
Copy link
Member

bnfinet commented Dec 19, 2022

@cbjartli this looks pretty good without testing it personally.

I think this would accommodate gitlab self-hosted, is that right?

@cbjartli
Copy link
Author

cbjartli commented Dec 20, 2022

@bnfinet, it should, by providing the AuthURL, TokenURL and UserInfoURL for the self-hosted instance. I haven't tried it out myself yet, but I'll see if I can verify it.

@cbjartli
Copy link
Author

@bnfinet I've tested it against a self-hosted instance, and it seems to work as expected. I modified the example config so that it contains the relevant configuration details for the case of self-hosted instances too.

@bnfinet
Copy link
Member

bnfinet commented Dec 20, 2022

@cbjartli thanks for testing self hosted!

@gregorg

This comment was marked as off-topic.

@bnfinet

This comment was marked as off-topic.

@bnfinet
Copy link
Member

bnfinet commented Feb 2, 2023

@cbjartli sorry for the delay in reviewing. Thank you for the contribution to VP.

I have setup a self hosted instance of GitLab and I have tested VP with GitLab. The basic login functionality works well.

It does not appear that any logic has been added in order to validate that a user's group is present on the configured vouch.teamWhitelist. The user's group is never checked. Any logged in user can pass.

Something similar to the check in pkg/providers/github/github.go should be present.

Also it could use some tests.

@bnfinet
Copy link
Member

bnfinet commented Feb 2, 2023

@cbjartli also please follow conventions for log.Infof(..) instead of using cfg.Logging.Logger.Infof(...) etc

@cbjartli
Copy link
Author

cbjartli commented Feb 2, 2023

@bnfinet Thank you for the review, will go through the issues raised asap.

It does not appear that any logic has been added in order to validate that a user's group is present on the configured vouch.teamWhitelist. The user's group is never checked. Any logged in user can pass.

This is strange, we have this in internal use, and it certainly only allows users whose group belongs to the whitelist through. I distinctly remember adding a check for this. I am not in front of my computer now, but I'll have to get back to you. (Could simply be an issue of a missing push.)

@bnfinet bnfinet closed this Mar 7, 2023
@nrukavkov
Copy link

Hi. Why pr was closed?

@bnfinet
Copy link
Member

bnfinet commented Mar 22, 2023

@nrukavkov please see conversation in #514

@puttehi
Copy link

puttehi commented Mar 30, 2023

There does not appear to be any conversation with the author after opening the PR

@bnfinet
Copy link
Member

bnfinet commented Mar 30, 2023

@puttehi that's just not the case. Please take a closer look.

This PR doesn't work, it's incomplete, it's missing code that the author used in his environment.

@cbjartli asked his colleague to submit an alternate solution (see #514). I'm still hopeful that will result in a working solution.

If anyone else would care to submit a working PR I'll happily test it out. GitLab group based auth would be a nice addition to VP.

@cbjartli
Copy link
Author

cbjartli commented Mar 30, 2023

Apologies for being away due to life and work events. Will try to give this matter some attention asap.

I just want to note that @ritmanda is not a colleague of mine as far as I know, so we have not communicated about a solution to this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Authorizing GitLab users based on group membership
5 participants