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

Allow Azure AAD auth provider to use AAD group ids instead of display name for authn/authz #154

Merged
merged 7 commits into from Jun 20, 2018

Conversation

@amanohar
Copy link
Collaborator

commented Jun 20, 2018

Allow Azure AAD auth provider to use AAD group ids instead of display name for authentication

This PR will allow Azure AAD auth provider to be set up to use AAD group ids instead of display names for authentication. More details in issue #153

amanohar and others added 5 commits Apr 30, 2018
Add paging to get around directoryObjects.getByIds limit of 1000
When trying to detailed description of AAD membership groups using API getbyids: https://github.com/microsoftgraph/microsoft-graph-docs/blob/master/api-reference/v1.0/api/directoryobject_getbyids.md if AAD member has more than 1000 group memerships

AAD returns:
    "code": "Request_BadRequest",
    "message": "Number of included identifiers cannot exceed '1000'.",

Therefore, guard needs to do page queries to get details of all groups the user belong to. But getbyids does not support query options like $top and $expand and returns error:

    "code": "Request_BadRequest",
    "message": "The following query options are not supported by this request method or cannot be applied to the requested resource: $filter,$expand,$orderby,$count,$inlinecount,$select,$skiptoken,$skip,$top"

So in this case guard cannot rely on AAD to do paging and needs to do paging itself to fetch membership group details from AAD.

Github issue: #132
Some cleanup
- Avoid re-allocations for groupNames array
- Simplify the loop over groupIDs
- glog.Infof appends \n automatically
Azure AAD auth provider is using AAD group's displayName instead of u…
…nique objectId for auth

Azure AAD auth provider is using AAD group's displayName instead of unique objectId for auth

@amanohar amanohar changed the title Allow Azure AAD auth provider to use AAD group ids instead of display name for authentication Allow Azure AAD auth provider to use AAD group ids instead of display name for authn/authz Jun 20, 2018

@@ -29,6 +30,7 @@ func (o *Options) AddFlags(fs *pflag.FlagSet) {
fs.StringVar(&o.ClientID, "azure.client-id", o.ClientID, "MS Graph application client ID to use")
fs.StringVar(&o.ClientSecret, "azure.client-secret", o.ClientSecret, "MS Graph application client secret to use")
fs.StringVar(&o.TenantID, "azure.tenant-id", o.TenantID, "MS Graph application tenant id to use")
fs.BoolVar(&o.UseGroupUID, "azure.use-group-uid", false, "MS Graph application tenant id to use")

This comment has been minimized.

Copy link
@colemickens

colemickens Jun 20, 2018

Update the flag usage text, it's copy pasted from the tenant Id.

amanohar and others added 2 commits Jun 20, 2018
}

func NewOptions() Options {
return Options{
ClientSecret: os.Getenv("AZURE_CLIENT_SECRET"),
UseGroupUID: true,

This comment has been minimized.

Copy link
@tamalsaha

tamalsaha Jun 20, 2018

Member

I have changed it to default true , since that is the safer option.

This comment has been minimized.

Copy link
@amanohar

amanohar Jun 20, 2018

Author Collaborator

I left it false for backward compatibility reasons so that upgrading existing Guard deployments are not impacted.

@tamalsaha tamalsaha merged commit 9161264 into appscode:master Jun 20, 2018

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/travis-ci/push The Travis CI build failed
Details

@amanohar amanohar deleted the amanohar:aadgroupid branch Jul 7, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.