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

svc: Assume access key creation permission to be available by default #3306

Merged
merged 3 commits into from
May 8, 2024

Conversation

vadmeste
Copy link
Member

@vadmeste vadmeste commented Apr 23, 2024

Adding this policy will make the user not able to create a service account anymore:

    {
      "Effect": "Deny",
      "Action": [
              "admin:CreateServiceAccount"
      ],
      "Condition": {
              "NumericGreaterThanIfExists": {"svc:DurationSeconds": "1500"}
      }
    },

The reason is that policy.IsAllowedActions() is called with conditions from the user login.

Assume svc account creation to be possible for now until we come up with a better fix

@prakashsvmx
Copy link
Member

prakashsvmx commented Apr 29, 2024

@vadmeste ,

The Console UI already allows "Access Key " creation, by assuming this permission. but looks like a policy should be associated with the user to make it work.

e.g: a new user without any policy association results in Access denied when we trt to save the Service Account credentials from UI.
but if the user has readOnly policy. it works fine. ( though the policy does not explictly allow the action)

@vadmeste vadmeste force-pushed the add-svc-permission branch from b0efa52 to 0e07e77 Compare May 1, 2024 14:58
@vadmeste
Copy link
Member Author

vadmeste commented May 1, 2024

@prakashsvmx the current implementation does not work all the time. The reason is that we evaluate the default actions during the user's login, however this is not correct. A policy is evaluated during the API request itself. The request that is sent to the server contains sometimes some information (http headers or path or other) that will be transformed to conditions before the policy evaluation, and the policy decision will depend on the passed conditions from the request itself. So we cannot assume anything during the user's login.

But if we want to have a granular approach to this, I think we should move this API, policy.IsAllowedActions(), to Console code, it will be easier for us to tweak it when needed.

@vadmeste vadmeste force-pushed the add-svc-permission branch from 0e07e77 to 9e3df37 Compare May 1, 2024 15:41
@vadmeste vadmeste marked this pull request as ready for review May 1, 2024 15:41
@harshavardhana
Copy link
Member

@prakashsvmx the current implementation does not work all the time. The reason is that we evaluate the default actions during the user's login, however this is not correct. A policy is evaluated during the API request itself. The request that is sent to the server contains sometimes some information (http headers or path or other) that will be transformed to conditions before the policy evaluation, and the policy decision will depend on the passed conditions from the request itself. So we cannot assume anything during the user's login.

But if we want to have a granular approach to this, I think we should move this API, policy.IsAllowedActions(), to Console code, it will be easier for us to tweak it when needed.

@vadmeste we should only add this if there are "conditions" in the policy as part of the admin actions.

we should not out-rightly add permissions for allow locally without consulting the server policy.

Adding this policy will make the user not able to create a service account anymore:

```
    {
      "Effect": "Deny",
      "Action": [
              "admin:CreateServiceAccount"
      ],
      "Condition": {
              "NumericGreaterThanIfExists": {"svc:DurationSeconds": "1500"}
      }
    },

```

The reason is that policy.IsAllowedActions() is called with conditions from the user login.

Assume svc account creation to be possible for now until we come up with a better fix
@vadmeste vadmeste force-pushed the add-svc-permission branch from 9e3df37 to a32449c Compare May 2, 2024 12:07
@harshavardhana harshavardhana merged commit d0f744e into minio:master May 8, 2024
33 checks passed
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.

3 participants