Generalizing template placeholders

[cdisselkoen]

This RFC was created from (migrated from) cedar-policy/cedar#81.

Rendered

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[mwhicks1]

Could clarify the proposal, per the comments.

[cdisselkoen]

Since I was the one who wrote up this proposal, I can't actually add my approval on GitHub. But, as a maintainer, I do approve of this RFC and think it would add value to the language. (My writing up the proposal from @WanderingStar was perhaps already an implicit endorsement, but I'm making that an explicit endorsement here.)

[john-h-kastner-aws]

This RFC makes the limitations in the validator with respect to templates placeholders more of an issue. We always type a template placeholder as AnyEntity which was fine when it could only occur in an in or == in the policy scope. The only problem then was that the validator could not short-circuit based on that comparison since it would always have type Boolean, but that doesn't present a problem most of the time.

With this extension, a valid policy could contain ?principal.attr, but the current validator will never be able to typecheck this access.

[cdisselkoen]

@john-h-kastner-aws do you envision there being a solution to the validator problem you describe above, or is that a blocking concern for this RFC? For instance, is there a road toward being able to type template placeholders more precisely than AnyEntity?

Alternately, does this RFC need to wait until we have "typed holes", i.e., explicitly typed template placeholders? (That would be its own RFC, of course.)

Alternately, do we need to slim down this RFC so that the feature is restricted in some way such that it doesn't run afoul of the validator issue? (Restricting which operations are legal on template placeholders, for instance)

[john-h-kastner-aws]

is there a road toward being able to type template placeholders more precisely than AnyEntity?

The possible extension to the typechecker mentioned in RFC #5 is one path by which we could use is to have de-facto typed holds. A specific mechanism for typed holes could also work, of course.

With RFC #8 it would become possible to unsoundly access attributes on template placeholders under partial schema validation rules.

Alternatively this doesn't have to block if we're happy to let the typechecker reject attribute access expressions while allowing them for customers who don't use a schema.

[mwhicks1]

With this extension, a valid policy could contain ?principal.attr, but the current validator will never be able to typecheck this access.

Could you typecheck it if you wrote ?principal has attr && ?principal.attr ? If so, I think that little bit of overhead should be Ok, at least for now.

[john-h-kastner-aws]

Could you typecheck it if you wrote ?principal has attr && ?principal.attr ?

No. ?principal : AnyEntity, so even with the effect set {?princpal has attr} we can't typecheck ?principal.attr. (expect as top, of course, but that's still an error).

This could work if we defined the type of a placeholder as the union of all defined entity types and then updated entity LUBs so they have an optional attribute if an attribute does not exist for every element of the LUB, but the type of that attribute is consistent everywhere that it is defined. We would need to think about that more since it might not be sound. This would amount to {} <: { attr :? T } for entities, which isn't sound in general, but it could perhaps be sound for entities because they never lose information about the type of attributes in LUB computation .

[cdisselkoen]

In response to @john-h-kastner-aws's comment farther above about the misleadingness of what's labeled Example 2 in the current RFC text:

It was proposed in some offline discussion that we restrict ?principal and ?resource from appearing in the policy conditions unless they also appear in the policy scope. This would ensure that the thing named ?principal or ?resource remains at least somewhat connected semantically to the actual principal or resource respectively, rather than being some potentially completely unrelated entity (as in Example 2).

This restriction sounds good to me on its face. However, it does end up disallowing the Example 1 that motivated this RFC in the first place. It also disallows the non-numbered example under "Non-proposals" which was suggested as a "fix" for the bad policy there. In short, we have no examples where this RFC actually helps, if we implement this restriction.

Further, in the case where the policy scope has principal == ?principal, it's meaningless to allow ?principal to appear in the when/unless conditions, since the policy author could (should?) have just written principal instead -- they're known to be equal at that point. So the only case where the restricted version of this RFC would help is in the principal in ?principal case, where you could perhaps write additional logic that depends on properties of ?principal (and likewise for resource in ?resource). We don't currently have any such examples, but maybe they could hypothetically exist.

[cdisselkoen]

Just to enumerate them, we have approximately three courses of action here as I see it: (given that the restriction proposed above makes the feature meaningless and fails to address the motivating use case)

1. Accept this RFC as-is, despite the concerns about policy readability arising from placeholder names [we still have to address the validation concern above, but that's orthogonal to the current discussion]
2. Greatly expand the scope of this RFC (perhaps such that it deserves to be a new RFC altogether) to also allow template placeholders with names other than principal and resource. This probably entails also allowing an arbitrary number of placeholders to appear in a template.
3. Reject this RFC and do nothing.

[mwhicks1]

Thanks for the further discussion. Thinking a little more about your option 2, I think it works as long as ?principal and ?resource are still treated specially, i.e.,

1. ?principal must appear in the scope if it appears anywhere, and it must be on the RHS of principal in ... or principal == .... It can only be instantiated by an entity.
2. Same restriction for ?resource
3. Arbitrary names can be used but only in the when condition, in addition to ?principal and ?resource.

One question is whether arbitrary names must be instantiated with entities, or whether primitive values are also allowed.

A related question is whether templates are validated on their own, or whether they are validated only when they are linked. The latter approach is simplest, but it might be nice to provide more assurance at template creation time. (E.g., you could label slots with types, validate assuming those types, and then check that links provide values of the expected type.)

[cdisselkoen]

I think all of this is good discussion and probably belongs in a separate RFC. I'm perfectly OK with the scope restrictions you propose for ?principal and ?resource, and also your restriction 3.

I was envisioning that the new arbitrary names would only be instantiated with entities, as that still enables some interesting use cases without additional implementation/validation difficulty of allowing other types; and allowing other types could always be a follow-up RFC later, as it would be a backward-compatible change if we write the APIs correctly. Perhaps we should specifically stipulate that the APIs should be written this way.

I agree that it would be nice to provide some validation at template creation time, as far as possible. We might not even require typed slots to at least catch some errors (unrelated to the slots). Typed slots sounds like a separate RFC to me, unless you feel it should be included in the same RFC. Note we could do typed slots either before or after the arbitrary-names change.

[john-h-kastner-aws]

After some in person discussion yesterday, I have a proposal to support precise typechecking for template placeholders. This builds on the observation that an extension to the typechecker mentioned in the RFC for is (#5) would allow more precise typechecking.

As proposed there, an expression ?principal is User would generate an effect mapping ?principal to {User} to encode that we know the expression ?principal is one of the entity types in the set {User}. The is expression is the most obvious way to generate this effect, but it could just as easily be generated by an == or in expression.

Without first accepting RFC #5 and implementing is, we can extend of effect system to generate an effect mapping ?principal to {User} for an expression ?principal == principal when principal has type {User}. Similarly principal in ?principal might generate {User, Group} if principal is {User} because those are the entity types a User could be in when the expression evaluates to true.

As long as the template placeholder appears in the policy scope we would generate some effect constraining its entity type. It would then be possible to access attributes on the template placeholder.

[mwhicks1]

I have a proposal to support precise typechecking for template placeholders.

Looks great! Do you propose to consider doing this as part of this RFC, as it is stated, or perhaps your proposal should be a separate improvement that needs to be implemented before this RFC's implementation can proceed?

[cdisselkoen]

@mwhicks1 your update to the RFC text looks great to me. May I propose a tweak to the syntax for discussion: instead of

template ?bound
permit(...)

I propose

template (?bound)
permit(...)

I find this more readable particularly in the multivariable case, where instead of

template ?user, ?resourcebound
permit(...)

we would have

template (?user, ?resourcebound)

I feel that the first one might on first glance mentally parse as "template ?user", "?resourcebound" which is misleading. I also suspect that the ()s might leave more doors open to add more syntax for other features later.

[mwhicks1]

May I propose a tweak to the syntax for discussion:

I like it. RFC updated. However, whilst updating the text, I thought of another syntax that I actually like a little better. It's given at the bottom under alternatives. What do you think?

[cdisselkoen]

I think the permit<?user,?resourcebound> syntax, while it might be more familiar to C++ or Java programmers, is less readable for less-technically-savvy users, so I'm against it. One of Cedar's goals is to be readable for non-experts and non-programmers.

While we're proposing syntax tweaks, another possible tweak would be

template (?user, ?resourcebound) => permit(...)

(note the addition of the double-arrow). Not sure if this adds anything vs the existing proposal.

[cdisselkoen]

One more question: should we allow including ?principal and ?resource in the list of template variables, even if we don't take the "Type annotations" alternative?

Pros:

• Users might try this, or even expect that this is required, and be confused that it's not allowed
• Declaring all variables in the same place seems pleasing to me and perhaps also to some users
• Might give us a better path toward the typed syntax if we want to adopt it in the future

Cons:

• Users might not realize that ?principal and ?resource are special names, and eg come with special restrictions (must appear in the policy scope)
• The change is not necessary and doesn't improve expressiveness

[mwhicks1]

This is not quite clear: is this RFC proposing that the new variables must be entities, or that they can be any type (string, bool, etc)?

It is proposing they can be any type. Since we are validating only linked policies, and not templates, there is no reason not to do this, that I can see.

[mwhicks1]

should we allow including ?principal and ?resource in the list of template variables, even if we don't take the "Type annotations" alternative?

For backward compatibility I'd prefer them to be optional. I don't have a problem if they are included. I lean a little toward warning the user that they are optional, perhaps only in a "verbose warnings" mode, if we have that.

[john-h-kastner-aws]

This all sounds good to me.

[john-h-kastner-aws]

Partial schema validation offers a way around this. We'd still do link-time typechecking, but would be able to report some errors earlier.

[mwhicks1]

Nice! If/when we accept this one, we can update the partial-schema RFC to mention this.

[khieta]

+1 partial schema validation could make this a non-issue.

[emina]

Partial schema validation won't help if we want to be able to automatically analyze policies though. We need precise types and would need something stronger here (type inference).

[john-h-kastner-aws]

We could analyze a fully linked policy template though, right?

[emina]

Yes, but we also want to analyze templates in their own right. For example, you might want to know if there is a way to instantiate one of your templates so that this instantiation breaks a property you care about.

[john-h-kastner-aws]

To be clear, this RFC is proposing that we typecheck templates only when linking?

[mwhicks1]

Yes. So we don't typecheck templates, just template instances. Is that a change from what we do now?

[john-h-kastner-aws]

Yes. We currently typecheck the template and then when checking a linked template we only need to see that linking won't cause the policy to have type False.

[mwhicks1]

I see. Then that's a breaking change. Does the change concern you?

[john-h-kastner-aws]

I don't think any existing templates/instantiations will fail to validate after this change: templates typechecking is obviously more permissive, and then typechecking the instantiations (for existing templates with the placeholders in the head) should not be able to cause any errors since it previously typechecked with AnyEntity, and now were typechecking with a specific entity type.

[mwhicks1]

Note that we could still do creation-time validation when templates involve only ?principal and ?resource. I just updated the RFC to make this point.

[john-h-kastner-aws]

We could, but I'm not convinced that we need to to maintain backwards compatibility.

[khieta]

I added some comments from the perspective of someone who hasn't been involved in the discussion so far. Aside from some minor differences of opinion, I approve of this RFC.

[khieta]

"this list" what list?

[khieta]

Ah, I see later it's the list in template(...). Would be nice to clean up wording here or show an example of the syntax.

[khieta]

delete "to be used"

[khieta]

I slightly prefer this alternative syntax. I feel like template(...) will be confused with @template(...) since prior to this proposed change, annotations were the only construct allowed before permit or forbid.

Of course, we could have special error messages to help guide users in that case. But it still feels a little ugly to me to add a new keyword.

[cdisselkoen]

I made a comment above which I should have made inline on this text. Adding it here:

I think the permit<?user,?resourcebound> syntax, while it might be more familiar to C++ or Java programmers, is less readable for less-technically-savvy users, so I'm against it. One of Cedar's goals is to be readable for non-experts and non-programmers.

[cdisselkoen]

I also don't quite understand the worry about template(...) being confused with @template(...). Is it any different than possible confusion between permit(...) and @permit(...), which could happen today?

[khieta]

Fair point about @permit(...). Also a fair point that our target audience may not be familiar with templates in other languages -- I am probably biasing towards permit<?user,?resourcebound> due to C++ experience.

[khieta]

+1 partial schema validation could make this a non-issue.

[khieta]

"there is no reason" are arbitrary-typed placeholder variables in scope for this RFC, or is that a proposed future extension? If this will be included as part of the current RFC it would be nice to see some concrete examples. The string and long cases are easy, but I wonder if there's anything difficult about supporting placeholder variables that have type record or set.

[cdisselkoen]

The final comment period (FCP) for this RFC is starting now, with intent to accept this RFC. There seems to be broad support for the RFC, and in the core team's judgment the few outstanding questions remaining above are minor and it seems likely they can be resolved before the end of the FCP. The FCP will end Wednesday 2023-07-05 at noon PT / 3pm ET / 7pm UTC. If no new unresolved major objections are voiced by then, this RFC will be merged.

For more on the RFC process, see https://github.com/cedar-policy/rfcs.

[emina]

This RFC makes the limitations in the validator with respect to templates placeholders more of an issue. We always type a template placeholder as AnyEntity which was fine when it could only occur in an in or == in the policy scope.

This also presents a problem for any tools that need to perform automated reasoning over templates. Doing so requires precise type information, which is lost with the fully generalized approach for templates.

I think there is independent value in allowing ?principal and ?resource placeholders to be used freely in policy conditions, whenever they appear in the policy scope.

I propose that we adopt this extension alone.

Arbitrary placeholders can be added independently later, if there is need and we have had time to think through their ramifications for type checking analysis.

[cdisselkoen]

... allowing ?principal and ?resource placeholders to be used freely in policy conditions, whenever they appear in the policy scope.

I propose that we adopt this extension alone.

This was the original proposal in this RFC, and was rejected in favor of this expanded version. See the comments above, especially this one and the discussion surrounding it

[emina]

... allowing ?principal and ?resource placeholders to be used freely in policy conditions, whenever they appear in the policy scope.

I propose that we adopt this extension alone.

This was the original proposal in this RFC, and was rejected in favor of this expanded version. See the comments above, especially this one and the discussion surrounding it

Ack. I missed this part (the thread has gotten long!).

In that case, I vote for option 3. Reject this RFC and do nothing. from above, at least for the current proposal.

I do like the idea of generalized templates, but we may also want to be able to analyze them, which requires supporting some way to constrain the type of arbitrary placeholders.

[cdisselkoen]

I missed this part (the thread has gotten long!)

In retrospect I think we should have made a new RFC after that discussion, rather than just updating this one and greatly expanding its scope :) Lesson learned for the future

[mwhicks1]

In that case, I vote for option 3. Reject this RFC and do nothing. from above, at least for the current proposal.

We have been iterating on this for a long time, and the need for generalizing templates seems clear: We have a particular use-case that seems well justified, and we have a general principle that all dynamically-created policies should be templatizable so that we can (a) avoid code injection attacks, and (b) make sense of the overall possible permissions states by storing policies in one place.

I take the point that we will want to analyze templates in advance. That speaks to having creation-time (strict) validation. What about going with the proposal at the end of the RFC that involves labeling placeholder variables with their expected type?

[mattmccutchen-amazon]

After reading the RFC, I think it could benefit from saying something about the new format of a template link. I'm pretty sure the intent is to make the obvious generalization of the current design where the link contains a map from placeholder name to value, but now there can be placeholder names other than just ?principal and ?resource. The value for ?principal or ?resource (if present in the template) must be an entity UID, while the value for any other placeholder can be any Cedar value (modulo any future changes to the RFC regarding stricter validation). But IMO, it would be good to be explicit about this in the RFC and not just assume people know it.

The next related question is what the API to add a template link will look like. Currently, we have PolicySet::link(&mut self, template_id: PolicyId, new_id: PolicyId, vals: HashMap<SlotId, EntityUid>). I imagine we'll keep that method for backward compatibility and add a new method where vals can contain arbitrary Cedar values, presumably in the form of a RestrictedExpression? And then we'll have a new kind of LinkingError, "?principal or ?resource was not an EntityUid", since that's no longer enforced by the Rust type system? I think it's useful to work out this level of API detail as part of the process of evaluating an RFC. What do others think?

[emina]

That speaks to having creation-time (strict) validation. What about going with the proposal at the end of the RFC that involves labeling placeholder variables with their expected type?

Yes, I think the alternative should address creation-time strict validation.

Note though that we might want to have a more detailed discussion about the syntax etc. in a separate RFC, and also to address Matt's comments above.

Re syntax: it might be redundant / confusing to support this form of type annotation for templates, and then also support the is operator separately, if we adopt the is RFC.

[cdisselkoen]

might want to have a more detailed discussion about the syntax etc. in a separate RFC

why in a separate RFC? This is the RFC that's about exactly this change, shouldn't the discussion be here?

[emina]

why in a separate RFC? This is the RFC that's about exactly this change, shouldn't the discussion be here?

I'm fine with either. Was hoping that we can figure out an easier way to keep track of all the conversation threads in the new RFC :)

[cdisselkoen]

We (Cedar maintainers) are aborting the FCP for this RFC due to additional concerns raised above and at the maintainers meeting. It will be returned to Pending state for revisions; the intent is to revert this RFC back to closer to its original form (more narrow in scope). The other generalizations (arbitrary template placeholders and/or arbitrary types for placeholders) can be discussed in separate RFCs if anyone wants to open them.

[cdisselkoen]

More status updates:

I'm personally abandoning this RFC and will be closing it. That said, I hope this general topic and use-case will be taken up in a future RFC, and there's a chance that it could even be me that opens a new RFC on this topic in the future.

The proposal in the most recent comment -- to revert this RFC back to closer to its original form, but with the restriction that ?principal and ?resource can't be used unless they also appear in the policy scope -- fails to address any of the motivating use cases we've found for this RFC. See this comment and the surrounding comments for more on why I find this version of the proposal unsatisfactory.

I hope that someday we find a way to introduce the broader feature, with arbitrary template placeholders of arbitrary types, but this has some fundamental issues that came up during FCP, and needs more thought. At this point I think everyone agrees that should be a new RFC which may or may not look different from this one. Rather than push through a very restricted version of this RFC that doesn't solve the actual motivating problems, I'd rather hold out for that future better RFC that actually solves problems.

[WanderingStar]

For what it's worth, we've implemented a proof of concept that uses an external templating mechanism to accomplish this and cedar-policy/cedar#106 without relying on template linking. The resulting policies are obviously less space efficient than the template links, but we have full control over them. Annotations let us link the populated policies back to their template sources.

[apandays]

I have a couple of more usecases for this feature, both stepping from Association constraints for constrained roles.

Usecase
Association constraints require there to be a association between the principal and the resource. The association is recorded as an attribute on the principal that references a set of resources, or a condition that point to a set of resources. For example, a timesheet approver may be associated to projects in one or more locations . This would be represented as an AssociatedProjectsLocations attribute for an Employee that contains a set of references to locations. The condition shown below constrains Approvers to acting only on the projects that are in their associated location.

Without this feature, one could implement this pattern is based on creating a static policy per role and loading the associated projects from a database. an example of this is below

// Constrained Approver Role - Associated Projects

permit(
  principal in TimesheetManager::Role::"Approver",
  action in [TimesheetManager::Action::"ViewProjectDetails"],
  resource in TimesheetManager::Project::"ALL"
) when {
  principal.AssociatedProjectsLocations.contains(resource.location)
};  

The problem with this option is that permissions management is being stored in the database storing associated projects. This does not fulfil the purpose of AVP/cedar, as a part of your authz data lies with the DB.

A better way to solve this if this feature was available is to create a template linked policy when a user is granted a role and a project is assigned to the user.

The template would like lie

permit(
  principal == ?principal",
  action in [TimesheetManager::Action::"ViewProjectDetails"],    // actions will be different for each role 
  resource in TimesheetManager::Project::"ALL"
) when {
  ?AssociatedProjectsLocations.contains(resource.location)   
};  

and the corresponding template linked policy that gets created when Joe is assigned "timesheet viewer" role in paris and london would be like below.

permit(
  principal == TimesheetManager::User::"Joe",
  action in [TimesheetManager::Action::"ViewProjectDetails"],    // actions will be different for each role 
  resource in TimesheetManager::Project::"ALL"
) when {
  ["london", "paris"].contains(resource.location)   
};  

[mwhicks1]

The problem with this option is that permissions management is being stored in the database storing associated projects. This does not fulfil the purpose of AVP/cedar, as a part of your authz data lies with the DB.

I don't know that I believe this statement, that information relevant to permissions management should not live outside of your policy store. Reasons that I think it would make sense:

1. It might be useful to your application for reasons other that permissions management. For example, an organizational hierarchy (managers and those they manage) is relevant to both permissions (e.g., employees and their managers can see their dossier, but not others) and to other functions (e.g., having a clear contact point for escalations). Similarly, you might organize your resources in a hierarchy of "folders;" the folder's permissions might affect those of its contents, so that's permissions-relevant, but the folder structure might also affect how data is stored. In general, the whole premise of ABAC is that attributes of a particular principal or resource affect permissions, and these attributes are stored with that principal/resource.
2. Having the data outside of AVP can ease implementation of some operations, such as listing who has access to what resources. For our TinyTodo sample application, it's easy to enumerate a task list's readers and/or editors because those values are stored directly in the DB, rather than indirectly stored as template links in AVP.
3. Policies are easier to understand. Looking at your template above, I have a hard time understanding the structure of permissions that it authorizes. The high-level reasoning that ?AssociatedProjectsLocations is effectively an attribute of ?principal is not self-evident in the template, but it is self-evident in the first example that you gave. Policy auditors are going to have an easier time with your first policy than the template.

This is not to say that what you propose is not a reasonable approach, and thus justification for this RFC. I am pointing out that there are other ways to do it, and reason to believe that customers might prefer those alternatives.

[craihamm]

I am in favor of this proposal.

It would be ideal to have a template which can take an arbitrary slot like

permit (
            principal == ?principal,
            action in [Action1, Action2],
            resource == ?resource
        )
        when { context.someContext == ?slot1 };

The only options I have are to create different templates for the different slot values, but we have too many values for that approach. So, we could not use templates and just write policies directly - but we like the ability to modify aspects of the template. So, what we do is we fetch that slot value and pass it into the authorization request. Its extra latency on every auth call, and worse - its a fragmented permissions environment. Introduction of a bug into our secondary "supplemental policy" store is a possibility.

[craihamm]

What would it take for this proposal to be reopened? Its also unclear why this proposal is rejected based upon the comment thread.

I don't know that I believe this statement, that information relevant to permissions management should not live outside of your policy store

As a customer I disagree with this statement. I want all information relevant to permissions management to live within the policy store. If I fetch information from other places, then that introduces consistency concerns and opens vulnerabilities. Do any other permission stores rely on user input to verify permissions? That seems like an easy way to spoof permissions.