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

feat(authz): added groups mechanism #1123

Merged
merged 1 commit into from
Mar 8, 2023

Conversation

aokirisaki
Copy link
Contributor

What type of PR is this?

Which issue does this PR fix:
#983

What does this PR do / Why do we need it:

If an issue # is not available please add repro steps and logs showing the issue:

Testing done on this change:

Automation added to e2e:

Will this break upgrades or downgrades?

Does this PR introduce any user-facing change?:


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@aokirisaki aokirisaki changed the title Groups new feat(authz): added groups mechanism Jan 19, 2023
@rchincha rchincha linked an issue Jan 19, 2023 that may be closed by this pull request
@rchincha rchincha added this to the v2.0.0 milestone Jan 19, 2023
@aokirisaki aokirisaki force-pushed the groups_new branch 7 times, most recently from 3d73ffb to ce9e017 Compare January 20, 2023 14:57
@peusebiu
Copy link
Collaborator

to summarize: group policies have priority over user policies which have priority over default policy.

can you add a test please in controller_test?

@codecov
Copy link

codecov bot commented Jan 27, 2023

Codecov Report

Merging #1123 (5205edf) into main (79783b4) will increase coverage by 0.03%.
The diff coverage is 92.00%.

❗ Current head 5205edf differs from pull request most recent head 708ca49. Consider uploading reports for the commit 708ca49 to get more accurate results

@@            Coverage Diff             @@
##             main    #1123      +/-   ##
==========================================
+ Coverage   89.97%   90.00%   +0.03%     
==========================================
  Files          93       93              
  Lines       20426    20469      +43     
==========================================
+ Hits        18379    18424      +45     
+ Misses       1542     1541       -1     
+ Partials      505      504       -1     
Impacted Files Coverage Δ
pkg/api/config/config.go 89.18% <ø> (-0.37%) ⬇️
pkg/api/ldap.go 57.82% <58.33%> (+3.97%) ⬆️
pkg/api/authz.go 95.87% <91.37%> (-2.42%) ⬇️
pkg/api/authn.go 90.04% <100.00%> (+4.32%) ⬆️
pkg/api/controller.go 90.87% <100.00%> (-0.02%) ⬇️
pkg/api/routes.go 94.42% <100.00%> (-0.01%) ⬇️
pkg/cli/root.go 95.44% <100.00%> (-0.05%) ⬇️
pkg/requestcontext/context.go 100.00% <100.00%> (ø)
pkg/extensions/extension_search.go 95.61% <0.00%> (-4.39%) ⬇️
... and 3 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

examples/config-policy.json Outdated Show resolved Hide resolved
pkg/api/authz.go Outdated Show resolved Hide resolved
pkg/api/authz.go Outdated Show resolved Hide resolved
pkg/api/authz.go Outdated Show resolved Hide resolved
pkg/api/controller_test.go Show resolved Hide resolved
@Skiepp
Copy link

Skiepp commented Feb 1, 2023

Quick note: right now in the config, groups memberships are only hardcoded into the config file and not taken from LDAP.
This approach will not the suitable for deployment made for LDAP only user federation

Also, the issue #983 suggest to take the group membershipt from another source

@andaaron
Copy link
Contributor

andaaron commented Feb 1, 2023

Some notes:

  1. We need a mechanism to be able to use groups also for local users, not just ldap, so even if the PR does not resolve [Feat]: Add a group mechanism for authZ #983, some of the group related code is the same.
  2. For LDAP (as in https://github.com/project-zot/zot/blob/main/examples/config-example.yaml#L8) there will be extra settings needed, but I'm not sure how the config would look (for example attribute of the LDAP group to match against policies, basically the group name).

Latest edit:
3. For OpenID, we don't have the authN code yet, after we have that we can also look at authZ

@Skiepp
Copy link

Skiepp commented Feb 1, 2023

Thanks @andaaron, I'm editing my post to reflect those notes

@rchincha
Copy link
Contributor

rchincha commented Feb 1, 2023

wrt LDAP (served by Windows AD backend at least),
"sAMAccountName" and "memberOf" return the two pieces of info relevant here.
Pls. enhance LDAP config to also include GroupAttribute to allow for @Skiepp's ask.

@Skiepp
Copy link

Skiepp commented Feb 1, 2023

Thank you @rchincha, but as @andaaron suggested the attribute should be part of config.

Your proposal is perfect for us (we are using AD), but many other people use other services like OpenLDAP and samba, which uses different attributes.

Restrict to those filelds only would be limitative.
Thanks to all for the interest

@rchincha
Copy link
Contributor

rchincha commented Feb 1, 2023

@Skiepp understood, these attributes are not hard-coded, and you are allowed to configure what the attributes are called and zot would just query of those attributes. If there is sample backend config that you can share, we will be happy to include it in our CI/CD tests.

examples/config-anonymous-authz.json Outdated Show resolved Hide resolved
examples/config-anonymous-authz.json Outdated Show resolved Hide resolved
examples/config-ldap.json Outdated Show resolved Hide resolved
andaaron
andaaron previously approved these changes Mar 2, 2023
andaaron
andaaron previously approved these changes Mar 2, 2023
andaaron
andaaron previously approved these changes Mar 2, 2023
Copy link
Contributor

@andaaron andaaron left a comment

Choose a reason for hiding this comment

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

Looks good.

Copy link
Contributor

@rchincha rchincha left a comment

Choose a reason for hiding this comment

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

the commit msg has not been fixed yet?

@rchincha
Copy link
Contributor

rchincha commented Mar 3, 2023

"BREAKING CHANGE: A groups mechanism has been added, with policies for groups"

^ this is NOT the breaking change.

andaaron
andaaron previously approved these changes Mar 6, 2023
@rchincha
Copy link
Contributor

rchincha commented Mar 7, 2023

feat(groups)!: added "groups" mechanism for authZ

BREAKING CHANGE: repository paths are now specified under a new config key called "repositories" under "accessControl" section in order to handle "groups" feature. Previously the repository paths were specified directly under "accessControl".

This PR adds the ability to create groups of users which can be used for authZ policies, instead of just users.

{
"http": {
   "accessControl": {
       "groups": {
 
Just like the users, groups can be part of repository policies/default policies/admin policies. The 'groups' field in accessControl can be missing if there are no groups. The permissions priority is user>group>default>admin policy, verified in this order (in authz.go), and permissions are cumulative. It works with LDAP too, and the group attribute name is configurable. The DN of the group is used as the group name and the functionality is the same. All groups for the given user are added to the context in authn.go. Repository paths are now specified under a new keyword called "repositories" under "accessControl" section in order to handle "groups" feature.

BREAKING CHANGE: repository paths are now specified under a new config key called "repositories" under "accessControl" section in order to handle "groups" feature. Previously the repository paths were specified directly under "accessControl".

This PR adds the ability to create groups of users which can be used for authZ policies, instead of just users.

{
"http": {
   "accessControl": {
       "groups": {

Just like the users, groups can be part of repository policies/default policies/admin policies. The 'groups' field in accessControl can be missing if there are no groups. The permissions priority is user>group>default>admin policy, verified in this order (in authz.go), and permissions are cumulative. It works with LDAP too, and the group attribute name is configurable. The DN of the group is used as the group name and the functionality is the same. All groups for the given user are added to the context in authn.go. Repository paths are now specified under a new keyword called "repositories" under "accessControl" section in order to handle "groups" feature.

Signed-off-by: Ana-Roberta Lisca <ana.kagome@yahoo.com>
Copy link
Contributor

@rchincha rchincha left a comment

Choose a reason for hiding this comment

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

lgtm

@rchincha rchincha merged commit 3365260 into project-zot:main Mar 8, 2023
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.

[Feat]: Add a group mechanism for authZ
5 participants