Azure Provider will now query for groups by tschechniker · Pull Request #1514 · oauth2-proxy/oauth2-proxy

tschechniker · 2022-01-12T08:40:56Z

Description

The Azure Provider did not query for groups. I added the azure graph api call to resolve the groups of the logged in user

Motivation and Context

How Has This Been Tested?

Tested with Azure environment.

Checklist:

[ X ] My change requires a change to the documentation or CHANGELOG.
I have updated the documentation/CHANGELOG accordingly.
I have created a feature (non-master) branch for my PR.

JoelSpeed

Would be keen to see if anyone who knows Azure a bit better has any input on this, eg @pierluigilenoci perhaps

JoelSpeed · 2022-02-14T10:31:31Z

providers/azure.go

+	// Default Groipe URL for Azure.
+	// Pre-parsed URL of https://graph.microsoft.com/v1.0/me.
+	azureDefaultGroupURL = &url.URL{
+		Scheme: "https",
+		Host:   "graph.microsoft.com",
+		Path:   "/v1.0/me/transitiveMemberOf/microsoft.graph.group",
+	}


Is any configuration of the Azure application required to enable this to work or should it work with default configurations (is this likely to be a breaking change for anyone)

This Path is also odd. You should probably just be calling /me here.
I'm pretty sure you'll need at least one of the permissions here: https://docs.microsoft.com/en-us/graph/permissions-reference#application-permissions-19

JoelSpeed · 2022-02-14T10:33:58Z

providers/provider_data.go

 	ProfileURL        *url.URL
 	ProtectedResource *url.URL
 	ValidateURL       *url.URL
+	GroupURL          *url.URL


This isn't a common pattern for other providers, most of the time the groups should be included within the profile or ID tokens returned, do we really want to add this for all providers?

pierluigilenoci · 2022-02-14T10:50:57Z

@JoelSpeed unfortunately Azure Provider has a lot of problems but I am on the side of those who suffer from it.
Who has the information to solve, in this case, I think is @weinong.

Ref: #1231, #1505 and #1144

braunsonm · 2022-02-19T01:23:34Z

I use Azure frequently. I can provide some thoughts on this.

braunsonm · 2022-02-19T01:34:18Z

providers/azure.go

+	var groups []string
+	for url != "" {
+		logger.Printf("Calling Group API: %s", url)
+		json, err := requests.New(url).


What scopes did you test this with? Calling the graph API requires a graph scoped access token unless I'm wrong.
This would mean you used the scope https://graph.microsoft.com/.default which would generate a token with the graph audience which isn't necessarily correct for upstream applications.

adriananeci · 2022-02-19T17:26:07Z

providers/azure.go

+	for _, doc := range json.Get("value").MustArray() {
+		for k, v := range doc.(map[string]interface{}) {
+			if k == "displayName" {
+				groups = append(groups, v.(string))


This will panic if v is nil with

http: panic serving [::1]:58156: interface conversion: interface {} is nil, not string

adriananeci · 2022-02-19T17:34:31Z

providers/azure.go

+	azureDefaultGroupURL = &url.URL{
+		Scheme: "https",
+		Host:   "graph.microsoft.com",
+		Path:   "/v1.0/me/transitiveMemberOf/microsoft.graph.group",


This path requires high privileges like Directory.Read.All otherwise it won't work(e.g. will retrieve nil as value for displayName key).

One approach that might not need extended permissions could be to use /v1.0/me/getMemberGroups instead, but in this case there are no trivial ways to find if a group is part of another group. Authentication might fail in this case when it shouldn't(e.g. a user will be denied even if he is part of a group that is part of the group configured in the oauth2-proxy allowed-group list).

Even /v1.0/me/getMemberGroups requires Directory.Read.All permissions based on my testing 😞

Why not using id instead of displayName(in getGroupsFromJSON method) when checking if a user is part of group or not? In this way we don't need elevated permissions anymore.

Or put the groups field selection(id or displayName) behind a config flag and let users choose which one fits best for their needs.

github-actions · 2022-04-21T00:07:00Z

This pull request has been inactive for 60 days. If the pull request is still relevant please comment to re-activate the pull request. If no action is taken within 7 days, the pull request will be marked closed.

pierluigilenoci · 2022-04-25T10:33:14Z

This is still an issue

github-actions · 2022-06-27T00:06:30Z

This pull request has been inactive for 60 days. If the pull request is still relevant please comment to re-activate the pull request. If no action is taken within 7 days, the pull request will be marked closed.

pierluigilenoci · 2022-06-27T06:30:58Z

This is still an issue

pierluigilenoci · 2022-06-27T06:31:49Z

@tschechniker could you take a look?

github-actions · 2022-08-27T00:07:09Z

This pull request has been inactive for 60 days. If the pull request is still relevant please comment to re-activate the pull request. If no action is taken within 7 days, the pull request will be marked closed.

pierluigilenoci · 2022-08-29T07:21:35Z

@tschechniker could you please fix the PR?

github-actions · 2022-10-29T00:06:51Z

This pull request has been inactive for 60 days. If the pull request is still relevant please comment to re-activate the pull request. If no action is taken within 7 days, the pull request will be marked closed.

mvalenzisi · 2022-11-08T08:43:21Z

This is still an issue.

pierluigilenoci · 2022-11-15T15:52:19Z

@adriananeci your PR solve also this?

pierluigilenoci · 2022-11-15T15:52:57Z

@JoelSpeed could you please reopen this?

pierluigilenoci · 2022-11-15T15:53:53Z

@tschechniker could you try if the latest version of the proxy works for your problem?

braunsonm · 2022-11-16T00:02:22Z

This should be resolved in the latest update, please advise otherwise in the related issue if that's not the case

vittoriocanilli · 2022-11-17T12:59:47Z

Hello,
we are using an AKS cluster with managed AAD, so we have to use the resource attribute set to 6dae42f8-4368-4678-94ff-3960e28e3630 as described here.

The resource attribute can not be used with Azure OAuth v2.0, so we have to use Azure OAuth v1.0, otherwise the groups won't be used properly per the permissions, even if they are retrieved correctly.

Anyway if we use Azure OAuth v1.0 with OAuth2 Proxy in a version greater or equal to the 7.2.1, the groups are not retrieved at all. So we are still using OAuth2 Proxy 7.2.0 at the moment.

Azure Provider will now query for groups

208ac6a

tschechniker requested a review from a team as a code owner January 12, 2022 08:40

Tschech, Tobias added 3 commits January 13, 2022 15:18

follow odata nextlink

bd16d71

fixed codeclimate issue

8d2a153

fixed codeclimate issue

1d57fc4

JoelSpeed reviewed Feb 14, 2022

View reviewed changes

JoelSpeed added the enhancement label Feb 14, 2022

braunsonm reviewed Feb 19, 2022

View reviewed changes

adriananeci suggested changes Feb 19, 2022

View reviewed changes

adriananeci mentioned this pull request Feb 23, 2022

Add Azure groups support and Azure OAuth v2.0 #1574

Merged

3 tasks

github-actions bot added the Stale label Apr 21, 2022

JoelSpeed removed the Stale label Apr 27, 2022

github-actions bot added the Stale label Jun 27, 2022

JoelSpeed removed the Stale label Jun 27, 2022

github-actions bot added the Stale label Aug 27, 2022

JoelSpeed removed the Stale label Aug 29, 2022

github-actions bot added the Stale label Oct 29, 2022

github-actions bot closed this Nov 5, 2022

Uh oh!

Conversation

tschechniker commented Jan 12, 2022

Description

Motivation and Context

How Has This Been Tested?

Checklist:

Uh oh!

JoelSpeed left a comment

Choose a reason for hiding this comment

Uh oh!

JoelSpeed Feb 14, 2022

Choose a reason for hiding this comment

Uh oh!

braunsonm Feb 19, 2022

Choose a reason for hiding this comment

Uh oh!

JoelSpeed Feb 14, 2022

Choose a reason for hiding this comment

Uh oh!

pierluigilenoci commented Feb 14, 2022

Uh oh!

braunsonm commented Feb 19, 2022

Uh oh!

braunsonm Feb 19, 2022

Choose a reason for hiding this comment

Uh oh!

adriananeci Feb 19, 2022

Choose a reason for hiding this comment

Uh oh!

adriananeci Feb 19, 2022

Choose a reason for hiding this comment

Uh oh!

adriananeci Feb 19, 2022

Choose a reason for hiding this comment

Uh oh!

adriananeci Feb 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Apr 21, 2022

Uh oh!

pierluigilenoci commented Apr 25, 2022

Uh oh!

github-actions bot commented Jun 27, 2022

Uh oh!

pierluigilenoci commented Jun 27, 2022

Uh oh!

pierluigilenoci commented Jun 27, 2022

Uh oh!

github-actions bot commented Aug 27, 2022

Uh oh!

pierluigilenoci commented Aug 29, 2022

Uh oh!

github-actions bot commented Oct 29, 2022

Uh oh!

mvalenzisi commented Nov 8, 2022

Uh oh!

pierluigilenoci commented Nov 15, 2022

Uh oh!

pierluigilenoci commented Nov 15, 2022

Uh oh!

pierluigilenoci commented Nov 15, 2022

Uh oh!

braunsonm commented Nov 16, 2022

Uh oh!

vittoriocanilli commented Nov 17, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

adriananeci Feb 19, 2022 •

edited

Loading