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

ACL for mcs #123

Merged
merged 1 commit into from
May 19, 2020
Merged

ACL for mcs #123

merged 1 commit into from
May 19, 2020

Conversation

Alevsk
Copy link
Contributor

@Alevsk Alevsk commented May 15, 2020

This PR sets the initial version of the ACL for mcs, the idea behind
this is to start using the principle of least privileges when assigning
policies to users, currently mcsAdmin policy uses admin:* and s3:*, we
are assuming users have access to everything which is not good.

We need to start validating explicitly if users has acccess to an
specific endpoint based on IAM policy actions.

In this first version every endpoint (you can see it as a page to),
defines a set of well defined admin/s3 actions to work properly, ie:

// corresponds to /groups endpoint used by the groups page
var groupsActionSet = iampolicy.NewActionSet(
    iampolicy.ListGroupsAdminAction,
    iampolicy.AddUserToGroupAdminAction,
    //iampolicy.GetGroupAdminAction,
    iampolicy.EnableGroupAdminAction,
    iampolicy.DisableGroupAdminAction,
)

// corresponds to /policies endpoint used by the policies page
var iamPoliciesActionSet = iampolicy.NewActionSet(
    iampolicy.GetPolicyAdminAction,
    iampolicy.DeletePolicyAdminAction,
    iampolicy.CreatePolicyAdminAction,
    iampolicy.AttachPolicyAdminAction,
    iampolicy.ListUserPoliciesAdminAction,
)

With that said, for this initial version, now the sessions endpoint will
return a list of authorized pages to be render on the UI, on subsequent
prs we will add this verification of authorization via a server
middleware.

@Alevsk Alevsk self-assigned this May 15, 2020
@bexsoft
Copy link
Collaborator

bexsoft commented May 15, 2020

Please run make assets

@cesnietor
Copy link
Collaborator

I'm getting this error.
Screen Shot 2020-05-15 at 2 12 38 PM

@dvaldivia
Copy link
Collaborator

This policy causes the reducer to panic on null returned by the server, avoid NPE

{
    "name": "mcsTestUserAddOnly",
    "Statement": [
        {
            "Action": [
                "admin:CreateUser"
            ],
            "Effect": "Allow",
            "Resource": [
                "arn:aws:s3:::*"
            ]
        }
    ],
    "version": "2012-10-17"
}

Screen Shot 2020-05-15 at 2 49 48 PM

@Alevsk Alevsk requested a review from cesnietor May 15, 2020 22:19
@Alevsk
Copy link
Contributor Author

Alevsk commented May 15, 2020

This policy causes the reducer to panic on null returned by the server, avoid NPE

{
    "name": "mcsTestUserAddOnly",
    "Statement": [
        {
            "Action": [
                "admin:CreateUser"
            ],
            "Effect": "Allow",
            "Resource": [
                "arn:aws:s3:::*"
            ]
        }
    ],
    "version": "2012-10-17"
}
Screen Shot 2020-05-15 at 2 49 48 PM

fixed

@dvaldivia
Copy link
Collaborator

Can you remove menu groups if not a single options is available?

Screen Shot 2020-05-15 at 4 05 57 PM

dvaldivia
dvaldivia previously approved these changes May 15, 2020
@cesnietor
Copy link
Collaborator

Just some comments, probably here we shouldn't show that Dashboard window also.
Or remove that Access denied
Screen Shot 2020-05-15 at 5 21 39 PM

@cesnietor
Copy link
Collaborator

Can we remove this logs? or are they needed?
Screen Shot 2020-05-15 at 5 22 33 PM

@Alevsk Alevsk force-pushed the permissions branch 2 times, most recently from 109d505 to 531be75 Compare May 18, 2020 20:25
@Alevsk
Copy link
Contributor Author

Alevsk commented May 18, 2020

Screen Shot 2020-05-18 at 1 25 35 PM

@cesnietor @dvaldivia

@Alevsk Alevsk requested a review from dvaldivia May 18, 2020 20:27
dvaldivia
dvaldivia previously approved these changes May 18, 2020
bexsoft
bexsoft previously approved these changes May 19, 2020
Copy link
Collaborator

@bexsoft bexsoft left a comment

Choose a reason for hiding this comment

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

I think we might change the text in README.md to avoid confusions. Also we could add a link to the page where all the privileges list for mcs. Everything else looks good.

@Alevsk Alevsk dismissed stale reviews from bexsoft and dvaldivia via 9534f30 May 19, 2020 00:12
@Alevsk Alevsk requested review from dvaldivia and bexsoft May 19, 2020 00:17
Copy link
Collaborator

@bexsoft bexsoft left a comment

Choose a reason for hiding this comment

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

Please rebase

@Alevsk
Copy link
Contributor Author

Alevsk commented May 19, 2020

Please rebase

i don't see any new changes

@Alevsk Alevsk requested a review from bexsoft May 19, 2020 00:56
This PR sets the initial version of the ACL for mcs, the idea behind
this is to start using the principle of least privileges when assigning
policies to users when creating users through mcs, currently mcsAdmin policy uses admin:*
and s3:* and by default a user with that policy will have access to everything, if want to limit
that we can create a policy with least privileges.

We need to start validating explicitly if users has acccess to an
specific endpoint based on IAM policy actions.

In this first version every endpoint (you can see it as a page to),
defines a set of well defined admin/s3 actions to work properly, ie:

```
// corresponds to /groups endpoint used by the groups page
var groupsActionSet = iampolicy.NewActionSet(
    iampolicy.ListGroupsAdminAction,
    iampolicy.AddUserToGroupAdminAction,
    //iampolicy.GetGroupAdminAction,
    iampolicy.EnableGroupAdminAction,
    iampolicy.DisableGroupAdminAction,
)

// corresponds to /policies endpoint used by the policies page
var iamPoliciesActionSet = iampolicy.NewActionSet(
    iampolicy.GetPolicyAdminAction,
    iampolicy.DeletePolicyAdminAction,
    iampolicy.CreatePolicyAdminAction,
    iampolicy.AttachPolicyAdminAction,
    iampolicy.ListUserPoliciesAdminAction,
)
```
With that said, for this initial version, now the sessions endpoint will
return a list of authorized pages to be render on the UI, on subsequent
prs we will add this verification of authorization via a server
middleware.
Copy link
Collaborator

@bexsoft bexsoft left a comment

Choose a reason for hiding this comment

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

LGTM

@Alevsk Alevsk merged commit 732e0ef into minio:master May 19, 2020
@Alevsk Alevsk deleted the permissions branch May 19, 2020 01:15
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.

4 participants