feat: implement policy templates #48

caiorcferreira · 2024-10-25T20:08:58Z

Description of changes:

One of Cedar's main features is Policy Templates, which allows for managing large sets of policies while keeping the authorization rules centralized.

This PR is an attempt to implement Templates in cedar-go.

Features

Create types.EntityReference (inspired by Rust's implementation) to allow policies to be parsed while deferring variable slot resolution.
Add slot field to JSON representation, following how cedar-policy-cli serializes Policy Templates. Note that JSON's variable slot is currently wrong in the official documentation
Implement LinkTemplateToPolicy, which creates a LinkedPolicy from a Template and variable slot environment.

Limitations

I wanted to gather feedback on the implementation strategy before covering all the bases, so this PR has lots of shortcuts that will be addressed if you believe I'm heading in the right direction.

Few unit tests and no integration test.
I left multiple todo comments in the code with decisions that I delayed or that I would like the team input.
At this stage, variable slot resolution only works for == expressions in principal and resource scope conditions.

Conclusion

Policy templates will soon become a critical feature for my Cedar use case. Hence, I want to try to move this feature forward, and I appreciate any help!

patjakdev · 2024-10-29T16:15:09Z

Thank you very much for the contribution! We'll begin reviewing it today; hopefully we should have some feedback for you soon.

philhassey · 2024-10-29T16:31:02Z

.editorconfig

@@ -2,6 +2,6 @@
 root = true


This spacing change broke your PR, it now seems to change everything. Can you update this? Also what editor does this config file work for? Thanks!

I used this configuration because, in a previous PR, my Goland used 4 spaces and changed multiple lines I didn't touch.
I removed the file, but I can't see the linting job. Let me know if the error persists.

About which editor supports it, the most popular ones do without any plugin: VS Code, IntelliJ, Vim, etc. It's an open-source standard

Signed-off-by: Caio Cavalcante <[email protected]>

patjakdev · 2024-10-29T16:37:59Z

internal/json/json_unmarshal.go

+		case s.Slot != nil:
+			ref = types.VariableSlot{Name: types.String(*s.Slot)}
+		case s.Entity != nil:
+			ref = types.EntityUID(*s.Entity)


It makes me a feel a bit icky that slots could end up being parsed into static policies and not just templates.

I wonder if we want a type Template struct which calls the same underlying code to do the JSON unmarshaling, but passes a flag to this function indicating that slots are allowed.

We may have to imitate Rust implementation and have slots in the Policy type.

The following policy is valid:

@id("AccessVacation") permit( principal, action == Action::"view", resource ); @id("Template") permit( principal == ?principal, action == Action::"view", resource );

It contains both a static policy and a template. Because of that, in Rust, a policy is defined as:

pub struct Policy { /// Reference to the template template: Arc<Template>, /// Id of this link /// None in the case that this is an instance of a Static Policy link: Option<PolicyID>, // INVARIANT (values total map) // All of the slots in `template` MUST be bound by `values` /// values the slots are bound to. /// The constructor `new` is only visible in this module, /// so it is the responsibility of callers to maintain values: HashMap<SlotId, EntityUID>, }

I started splitting the serialization so that slots could only be parsed by a type Template struct. But because the above policy is valid, we couldn't know ahead of time whether to use a Policy or Template.

Unless we create a cedarDTO that could be either policy or template and decide which type to use based on whether there are slots, but I wouldn't say I like this idea.

However, instead of doing exactly like Rust, I think we could compromise to avoid a massive change in the codebase by changing ast.Policy to:

type templateContext struct { linkID types.String slots map[types.SlotID]types.EntityUID } type Policy struct { Effect Effect Annotations []AnnotationType // duplicate keys are prevented via the builders Principal IsPrincipalScopeNode Action IsActionScopeNode Resource IsResourceScopeNode Conditions []ConditionType Position Position templateContext templateContext }

P.S. I'm still trying to think of a better name than templateContext. The case is to encapsulate template-related information in a new field instead of encapsulating the expression fields in a Template field, which would demand changes in all the codebase.

Hi @patjakdev, do you have any insights on this?

patjakdev · 2024-10-29T16:38:28Z

internal/json/json_unmarshal.go

+		case s.Entity != nil:
+			ref = types.EntityUID(*s.Entity)
+		default:
+			return nil, fmt.Errorf("missing entity and slot")


nit: entity or slot

patjakdev · 2024-10-29T16:38:54Z

internal/json/json_unmarshal.go

+		case s.Slot != nil:
+			ref = types.VariableSlot{Name: types.String(*s.Slot)}
+		case s.Entity != nil:


We should error if both Slot and Entity are set.

patjakdev · 2024-10-29T16:47:16Z

internal/parser/template.go

+// todo: add invariant len(slotEnv) == len(template.Slots)
+func LinkTemplateToPolicy(template ast.Template, linkID string, slotEnv map[string]string) ast.LinkedPolicy {
+	p := template.Body
+	templateID, _ := findAnnotation(p, "id")


IDs as annotations are not part of the Cedar spec and are merely a nicety supported by the Cedar CLI. So, we can't be mucking around with annotations at the AST level. Instead, the mapping of IDs to policies or templates should all be done at the PolicySet level.

So, I think we end up with two functions:

ast.LinkTemplateToPolicy(ast.Template, map[string]string) *ast.Policy - this basically just clones the template body and calls the linkScope() functions you've outlined, returning a *ast.Policy.

func (*PolicySet) StoreLinkedPolicy(TemplateID, PolicyID, map[string]types.EntityUID) - this looks up the template in the PolicySet by TemplateID, calls ast.LinkTemplateToPolicy() to get an ast.Policy() and then calls PolicySet.Store() to store the policy with the given ID.

Unfortunately, in order to implement the JSON serialization spec for PolicySet, we'll also have to keep around the information about the template links that were created and whether stored policies were added statically or via a template link.

patjakdev · 2024-10-29T17:01:21Z

internal/parser/template.go

+
+// todo: how to clone policy body to avoid modifying the original template?
+// todo: add invariant len(slotEnv) == len(template.Slots)
+func LinkTemplateToPolicy(template ast.Template, linkID string, slotEnv map[string]string) ast.LinkedPolicy {


I think we want slotEnv to be a map[string]types.EntityUID. Or, even better, a map[types.SlotID]types.EntityUID.

patjakdev · 2024-10-29T17:02:40Z

internal/parser/template.go

+	return linkedScope.(T)
+}
+
+// todo: should we panic on this? or just trust that the interface is correct?


I don't see any harm in panicking.

patjakdev · 2024-10-29T17:04:11Z

internal/parser/template.go

+func parseEntityUID(euid string) types.EntityUID {
+	typeIDseparator := strings.LastIndex(euid, "::")
+
+	etype := euid[:typeIDseparator]
+	eID := strings.Trim(euid[typeIDseparator+2:], `"`)
+
+	return types.NewEntityUID(types.EntityType(etype), types.String(eID))
+}


Yeah, let's just have the value type of the slot env be a types.EntityUID. Then it's up to the caller how it constructs the EntityUID (NewEntityUID(), UnmarshalJSON(), etc).

patjakdev · 2024-10-29T17:07:04Z

types/entity.go

@@ -6,6 +6,8 @@ import (
 	"strings"
 )

+const CedarVariable = EntityType("__cedar::variable")


I don't think we want this leaking out. It was just used for some internal placeholder values during partial evaluation.

patjakdev · 2024-10-29T17:10:07Z

internal/eval/compile.go

@@ -84,3 +84,15 @@ func scopeToNode(varNode ast.NodeTypeVariable, in ast.IsScopeNode) ast.Node {
 		panic(fmt.Sprintf("unknown scope type %T", t))
 	}
 }
+
+// todo: should we panic on this? or just trust that the interface is correct?


panicking seems fine to me

Hopefully the type system prevents us from ever evaling an ast.Template.

caiorcferreira · 2024-10-30T16:59:10Z

Thanks for the thorough review! I'm working on applying your suggestions and extending the design to other operators beyond =.

philhassey · 2024-10-30T17:01:04Z

I don't know if @patjakdev mentioned this or not, but I think having the Slots be just a EntityUID with the Type being "__cedar::Slot" might be a nice way to avoid changing a lot of interfaces.

caiorcferreira · 2024-10-30T17:38:28Z

Using __cedar::Slot would reduce some work, but I don't think it would be much. Although template variables are now restricted to the policy scope, I can see they being extended to when/unless conditions in the near future.
Even more now, with the introduction of tags, producing a policy by replacing a tag in the when clause is an everyday use case.

Hence, I feel that laying the groundwork for variables with their own type is useful.

philhassey reviewed Oct 29, 2024

View reviewed changes

caiorcferreira force-pushed the feat/templates branch from 3e2a17a to 193b04c Compare October 29, 2024 17:42

Caio Cavalcante added 8 commits October 29, 2024 14:52

feat: add entity reference and variable slot

9ab905e

Signed-off-by: Caio Cavalcante <[email protected]>

feat: support slot variables in json

905de6c

Signed-off-by: Caio Cavalcante <[email protected]>

refact: update eval to handle entity reference

5862d09

Signed-off-by: Caio Cavalcante <[email protected]>

feat: implement link template to policy instance

d2764dd

Signed-off-by: Caio Cavalcante <[email protected]>

chore: update todo comment

416e28e

Signed-off-by: Caio Cavalcante <[email protected]>

chore: fix indent

4a4ce12

Signed-off-by: Caio Cavalcante <[email protected]>

chore: fix indent with go fmt

a35d213

Signed-off-by: Caio Cavalcante <[email protected]>

chore: remove editorconfig

198ce1b

Signed-off-by: Caio Cavalcante <[email protected]>

caiorcferreira force-pushed the feat/templates branch from 0d94494 to 198ce1b Compare October 29, 2024 17:53

patjakdev reviewed Oct 29, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: implement policy templates #48

feat: implement policy templates #48

caiorcferreira commented Oct 25, 2024

patjakdev commented Oct 29, 2024

philhassey Oct 29, 2024

caiorcferreira Oct 29, 2024

patjakdev Oct 29, 2024

caiorcferreira Oct 30, 2024

caiorcferreira Nov 13, 2024

patjakdev Oct 29, 2024

patjakdev Oct 29, 2024

patjakdev Oct 29, 2024

patjakdev Oct 29, 2024

patjakdev Oct 29, 2024

patjakdev Oct 29, 2024

patjakdev Oct 29, 2024

patjakdev Oct 29, 2024

caiorcferreira commented Oct 30, 2024

philhassey commented Oct 30, 2024

caiorcferreira commented Oct 30, 2024

feat: implement policy templates #48

Are you sure you want to change the base?

feat: implement policy templates #48

Conversation

caiorcferreira commented Oct 25, 2024

Features

Limitations

Conclusion

patjakdev commented Oct 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caiorcferreira commented Oct 30, 2024

philhassey commented Oct 30, 2024

caiorcferreira commented Oct 30, 2024