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

Switch to new repository golib/auth/userinfo. #4

Merged
merged 2 commits into from
Jan 22, 2020

Conversation

rgooch
Copy link
Member

@rgooch rgooch commented Dec 4, 2019

No description provided.

@rgooch rgooch requested a review from cviecco December 4, 2019 20:04
@cviecco
Copy link
Contributor

cviecco commented Dec 5, 2019

While this would work, the interface you need for ldap groups in general needs also a group filter see: https://github.com/Cloud-Foundations/keymaster/blob/d003619c9bde950e5935ce8f74bcdc2b4b327a45/lib/authutil/authutil.go#L294. To prevent future changes to the api for the ldap userinfo can you also add those to the library so that when we fix the library we dont need to backport much?

@rgooch
Copy link
Member Author

rgooch commented Dec 5, 2019

Which API are you suggesting to change? I think the generic interface should not change.
Are you suggesting adding parameters to userinfo/ldap.New? That's OK in principle.
I note that the Keymaster code seems more complicated. Why is the Cloud-Gate group lookup code simpler?

If we're considering changing the API, how about removing the pointer for the groupPrefix parameter to the GetUserGroups method? Also, I'd prefer to remove that parameter entirely and just have a tiny support function that performs this operation. It feels like the wrong layer the way it is now.

Another suggested API change is to return a map[string]struct{} rather than []string, and again have a utility function that converts to []string if the caller needs it.

@cviecco
Copy link
Contributor

cviecco commented Dec 10, 2019

We need a way to filter groups at the server side.. a group filter (generic) would be OK for the ldap backend. But I agree that the removal of the groupprefix would be the right thing to do. The correct implementation actually the one one from keymaster.

@rgooch
Copy link
Member Author

rgooch commented Jan 17, 2020

I have restored the LDAP group search and filtering capability.
Since this PR removes the memberOf:mail attribute filtering and instead relies on group search base DNs and filter, this is not a backwards-compatible change, so I've bump the release to 1.0.0 (about time).

PTAL.

broker/staticconfiguration/api.go Show resolved Hide resolved
if config.AWS.GroupPrefix == "" {
b.userInfo = b.rawUserInfo
} else {
ui, err := filter.NewUserGroupsFilter(b.rawUserInfo,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in the case of having a GroupPrefix.. wont this make a new filter wrapping the filtered data already?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rawUserInfo does not have a group prefix filter already. I've abstracted that functionality into a common package so that the low-level implementations don't have to duplicate that.

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.

2 participants