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

[A2-833] Generalize project introspection #448

Merged
merged 6 commits into from
May 31, 2019

Conversation

msorens
Copy link
Contributor

@msorens msorens commented May 29, 2019

🔩 Description

IntrospectAllProjects was evaluating a subset of endpoints in determining the set of allowed projects--not taking into account parameterized endpoints. It turns out that for determining projects we can safely ignore action/resource pairs, which makes the parameterized question moot to boot.

👍 Definition of Done

The global projects filter will populate with all allowed projects, irrespective of whether they are on parameterized endpoints or not.

image

👟 Demo Script / Repro Steps

export TOK=`chef-automate admin-token`

chef-automate iam upgrade-to-v2 --beta2.1 --skip-policy-migration

# Create a project
jo id='proj3' name='Project 3' | \
curl -sSkH "api-token: $TOK" https://localhost/apis/iam/v2beta/projects --data @-  | jq -c

# Create a non-admin user
jo name=coolbean id=coolbean password=chefautomate | \
curl -sSkH "api-token: $TOK" https://localhost/apis/iam/v2beta/users --data @-  | jq -c

# save below as policy.json
{
  "id": "test",
  "name": "testing global project filter",
  "members": ["user:local:coolbean"],
  "projects": ["proj3"],
  "statements": [
    {
      "effect": "ALLOW",
      "actions": ["iam:teams:get"],
      "projects": ["proj3"]
    }
  ]
}
curl -sSkH "api-token: $TOK" -d @policy.json https://localhost/apis/iam/v2beta/policies | jq

(Above leverages sample repro steps on the jira card--thanks @bcmdarroch!)

With all that in place:

  • login as user coolbean and you will see project-p1 as the one and only project in the global project filter.
  • login as user admin and you will see that plus (unassigned) plus any others you happen to have in your system.

⛓️ Related Resources

✅ Checklist

  • Necessary tests added/updated?
  • Necessary docs added/updated?
  • Code actually executed?
  • Vetting performed (unit tests, lint, etc.)?

@msorens msorens requested a review from a team May 29, 2019 22:38
@msorens msorens self-assigned this May 29, 2019
}

for desc, e := range engines {
t.Run(desc, func(t *testing.T) {

t.Run("no matching policies", func(t *testing.T) {
policy0 := map[string]interface{}{
"members": engine.Subject(sub),
"members": engine.Subject("team:local:foo"),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Test now needs to fail to find matches on the subject only.

actual_projects = authorized_project with data.policies.polid as {
"members": ["bob"],
"statements": {
# Allowed with single project
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This complicated block is now more digestibly covered in the couple new tests.

msorens added 6 commits May 29, 2019 16:04
We can safely ignore action/resource pairs when
trying to answer the question of what projects a
user has access to.
The algorithm does not apply the same allow/deny rules.
Denied statements can simply be ignored, because access
to any bit of a project by some statement is sufficient to
say a user can access a given project.

Signed-off-by: michael sorens <[email protected]>
Signed-off-by: michael sorens <[email protected]>
Signed-off-by: michael sorens <[email protected]>
@msorens msorens force-pushed the a2-833/ms/generalize-project-introspection branch from 9574fd1 to 67fc036 Compare May 29, 2019 23:04
Copy link
Contributor

@bcmdarroch bcmdarroch left a comment

Choose a reason for hiding this comment

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

This is a nice improvement!

Copy link
Contributor

@srenatus srenatus left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@@ -46,7 +46,8 @@ authorized_pair[pair] {

allowed_project[project] {
project := policies[pol_id].statements[statement_id].projects[_]
match_pair[["allow", _, pol_id, statement_id]]
"allow" == policies[pol_id].statements[statement_id].effect
authz.has_member[pol_id]
not policies[pol_id].type == const_system_type
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this still necessary? If it's a system policy allowing something, a why exclude it? (but I hadn't been around when this was introduced)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed it is: system policies have wildcard projects, which means a normal user will get literally all projects as their allowed list. This line prevents that from happening.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm we could exclude policies with wildcard projects then, rather, couldn't we? 😃

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, we do not want to do that if it is not a system policy. The above effect should result in all projects if the user has that access.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I see. Thanks!

@msorens msorens merged commit d4e50d8 into master May 31, 2019
@chef-ci chef-ci deleted the a2-833/ms/generalize-project-introspection branch May 31, 2019 16:39
@susanev susanev added the auth-team anything that needs to be on the auth team board label Jul 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auth-team anything that needs to be on the auth team board automate-auth
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants