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

Add a deploy command & improve expression translation #17

Merged
merged 26 commits into from
Oct 11, 2022

Conversation

xacrimon
Copy link

@xacrimon xacrimon commented Oct 10, 2022

This PR is intended to go hand-in-hand with gravitational/teleport#17220. It adds a shortcut for exporting the policy YAML and ingesting it into a Teleport cluster (by calling into tctl) and improves the expression translator to be able to translate more complex predicate-lang policies into the resource representation.

With gravitational/teleport#17220 compiled & running and tctl in path. One can now run predicate deploy examples/access.py containing the following policy

class Teleport:
    p = Policy(
        name="access",
        loud=False,
        allow=Rules(
            Node(
                ((Node.login == User.name) & (User.name != "root"))
                | (User.traits["team"] == ("admins",))
            ),
        ),
        options=OptionsSet(Options((Options.max_session_ttl < Duration.new(hours=10)))),
        deny=Rules(
            Node(
                (Node.login == "mike")
                | (Node.login == "jester")
                | (Node.labels["env"] == "prod")
            ),
        ),
    )

    def test_access(self):
        # Alice will be able to login to any machine as herself
        ret, _ = self.p.check(
            Node(
                (Node.login == "alice")
                & (User.name == "alice")
                & (Node.labels["env"] == "dev")
            )
        )
        assert ret is True, "Alice can login with her user to any node"

        # No one is permitted to login as mike
        ret, _ = self.p.query(Node((Node.login == "mike")))
        assert ret is False, "This role does not allow access as mike"

        # No one is permitted to login as jester
        ret, _ = self.p.query(Node((Node.login == "jester")))
        assert ret is False, "This role does not allow access as jester"

This will translate the policy into the Teleport policy format and automatically write it to the cluster. Once that is done, one can run tctl get policy/access and they will see the following YAML policy in the terminal:

kind: policy
metadata:
  id: 1665420264035364000
  name: access
spec:
  allow: scope((((node.login == user.name) && (!(user.name == "root"))) || (user.traits["team"]
    == ["admins"])), "node")
  deny: scope((((node.login == "mike") || (node.login == "jester")) || (node.labels["env"]
    == "prod")), "node")
  options: (options.max_session_ttl < 36000000000000)
version: v1

@xacrimon xacrimon changed the base branch from main to sasha/python October 10, 2022 14:51
@xacrimon xacrimon requested a review from klizhentas October 10, 2022 14:53
@xacrimon xacrimon marked this pull request as ready for review October 10, 2022 17:12
if hasattr(predicate, "scope"):
# if the predicate has an evaluation scope for which it is only enabled,
# case it's expression within a `scope` call to allow defaulting in the expression evaluator
return f'scope({t_expr(predicate.expr)}, "{predicate.scope}")'
Copy link
Collaborator

Choose a reason for hiding this comment

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

This part is not 100% clear to me, can you elaborate why do we need scopes here?

Copy link
Author

@xacrimon xacrimon Oct 11, 2022

Choose a reason for hiding this comment

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

Ah, sure. Let's use the allow rules as an example. An allow section is composed of one or more coarse grained rules applicable to a specific scenario as I understand it, notably Node in current predicate code. That is, the following predicate DSL is a self-contained rule that we need to evaluate as part of the allow expression:

Node(
    ((Node.login == User.name) & (User.name != "root"))
    | (User.traits["team"] == ("admins",))
)

When assembling these both for the Z3 theorem and when generating the Teleport predicate resource, these rules are merged together with the OR logical operator to form a final expression:

# Policy.export
if self.allow.rules:
    allow_rules = functools.reduce(operator.or_, self.allow.rules)
    out["spec"]["allow"] = t_expr(allow_rules)

When Teleport later evaluates the allow expression to determine RBAC to say, an SSH node. We only want to think about the evaluation results of any Node rules. If we support say K8S via a class KubeCluster or the existing class Request rule similar to class Node, we may not fill in all the values needed for that subexpression to evaluate correctly, thus it may evaluate to true, which may cause the overall allow expression to evaluate to true due to the OR operator. Thus, we want to selectively avoid evaluating/discard the result of any scopes we aren't interested in. We still want to use an OR rule since we'd like to apply that if multiple Node rules are present.

Using a wrapper function like scope allows Teleport to discard or avoid evaluating a subexpression when the result doesn't make sense. There are other ways to get this sort of functionality but after some playing around this felt logical for the time being. We could just as easily rename this general scope() function to say Node() and define one per relevant scope.

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK, I understand now.

Let's do this instead:

kind: policy
metadata:
  name: access
spec:
  allow: 
       node: (node.login == user.name) && (!(user.name == "root"))) || (user.traits["team"] == ["admins"])

Pretty much the same as scope, but read better. Let's also replace scope property with a decorator

@serialize(scope)
class Node(ast.Predicate):
    pass

Behind the scenes you can add a property to the class, but we won't have to repeat ourselves and you can derive it from the class name

@klizhentas
Copy link
Collaborator

@xacrimon can you also please resolve conflicts

@klizhentas
Copy link
Collaborator

@xacrimon can you also fix the make lint command so it passes the lint step on the examples folder.

@xacrimon
Copy link
Author

xacrimon commented Oct 11, 2022

@klizhentas I've fixed the conflicts and reformatted make lint runs without issues on my machine. But make check will still fail because the isort tool called by lint-python disagrees with another formatter called by lint-python. I'm not familiar enough with Python tooling to know what we should do here, do you have any preference?

@klizhentas
Copy link
Collaborator

@xacrimon to resolve isort conflict, try telling it to use black profile:

PyCQA/isort#1518 (comment)

@xacrimon
Copy link
Author

xacrimon commented Oct 11, 2022

@klizhentas I have now

  • replaced the explicit scope field with the scoped decorator, wasn't sure if serialize was the intended name or just an example
  • replaced the allow string with an object containing rules grouped by their scope property
  • fixed the isort bug, thanks

@klizhentas klizhentas merged commit 028a938 into sasha/python Oct 11, 2022
@klizhentas klizhentas deleted the joel/deploy branch October 11, 2022 17:58
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