Skip to content

Commit

Permalink
Fix invalid Cedar 'in' operators in policy scope
Browse files Browse the repository at this point in the history 
It is not valid to have a set of entities for the 'in' operator in either the
principal or the resource clause.

This fixes an issue where the Terraform provider would output invalid Cedar policies
where 'principal_in' or 'resource_in' was used.
  • Loading branch information
@chrnorm
chrnorm committed Jul 31, 2024
1 parent 30ff8b8 commit 61e0150
Show file tree
Hide file tree
Showing 5 changed files with 81 additions and 70 deletions.
12 changes: 4 additions & 8 deletions internal/provider/policy_data_source.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,12 +74,10 @@ You may also optionally provide one or more 'when' and 'unless' conditions as bl
MarkdownDescription: "Specifies the principal component of the policy scope. Equivalent to writing 'principal in'",
Optional: true,
},
"principal_in": schema.ListAttribute{
"principal_in": schema.ObjectAttribute{
MarkdownDescription: "Specifies the principal component of the policy scope. Equivalent to writing 'principal =='",
Optional: true,
ElementType: basetypes.ObjectType{
AttrTypes: eid.EIDAttrsForDataSource,
},
AttributeTypes: eid.EIDAttrsForDataSource,
},

"any_action": schema.BoolAttribute{
Expand Down Expand Up @@ -112,12 +110,10 @@ You may also optionally provide one or more 'when' and 'unless' conditions as bl
MarkdownDescription: "Specifies the resource component of the policy scope. Equivalent to writing 'resource is'",
Optional: true,
},
"resource_in": schema.ListAttribute{
"resource_in": schema.ObjectAttribute{
MarkdownDescription: "Specifies the resource component of the policy scope. Equivalent to writing 'resource in'",
Optional: true,
ElementType: basetypes.ObjectType{
AttrTypes: eid.EIDAttrsForDataSource,
},
AttributeTypes: eid.EIDAttrsForDataSource,
},
},
Blocks: map[string]schema.Block{
Expand Down
41 changes: 41 additions & 0 deletions internal/provider/policy_data_source_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,3 +71,44 @@ func TestPolicyDataSource_Annotations(t *testing.T) {
},
})
}

func TestPolicyDataSource_InOperator(t *testing.T) {
resource.UnitTest(t, resource.TestCase{
TerraformVersionChecks: []tfversion.TerraformVersionCheck{
tfversion.SkipBelow(tfversion.Version1_5_0),
},
ProtoV6ProviderFactories: testAccProtoV6ProviderFactories,
Steps: []resource.TestStep{
{
Config: `
data "cedar_policyset" "test" {
policy {
effect = "permit"
principal_in = {
type = "Group"
id = "test"
}
action_in = [
{
type = "Action"
id = "Read"
}
]
resource_in = {
type = "Folder"
id = "example"
}
}
}
output "test" {
value = data.cedar_policyset.test.text
}
`,
Check: resource.ComposeAggregateTestCheckFunc(
resource.TestCheckOutput("test", "permit (\n\tprincipal in Group::\"test\",\n\taction in [Action::\"Read\"],\n\tresource in Folder::\"example\"\n);\n"),
),
},
},
})
}
4 changes: 2 additions & 2 deletions pkg/cedarpolicy/policy.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ type Policy struct {

AnyPrincipal types.Bool `tfsdk:"any_principal"`
Principal *eid.EID `tfsdk:"principal"`
PrincipalIn *[]eid.EID `tfsdk:"principal_in"`
PrincipalIn *eid.EID `tfsdk:"principal_in"`
PrincipalIs types.String `tfsdk:"principal_is"`

AnyAction types.Bool `tfsdk:"any_action"`
Expand All @@ -29,7 +29,7 @@ type Policy struct {

AnyResource types.Bool `tfsdk:"any_resource"`
Resource *eid.EID `tfsdk:"resource"`
ResourceIn *[]eid.EID `tfsdk:"resource_in"`
ResourceIn *eid.EID `tfsdk:"resource_in"`
ResourceIs types.String `tfsdk:"resource_is"`

When []Condition `tfsdk:"when"`
Expand Down
46 changes: 18 additions & 28 deletions pkg/cedarpolicy/render_string.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,24 +53,19 @@ func (p Policy) RenderString() (string, error) {
line := fmt.Sprintf("\tprincipal == %s::%q,", principalType, principalID)
output = append(output, line)
} else if p.PrincipalIn != nil {
// principal == [<entity>, <entity>],
// principal in <entity>,

entities := make([]string, len(*p.PrincipalIn))
typ := p.PrincipalIn.Type.ValueString()
id := p.PrincipalIn.ID.ValueString()

for i, ent := range *p.PrincipalIn {
typ := ent.Type.ValueString()
id := ent.ID.ValueString()

if typ == "" {
return "", fmt.Errorf("principal_in entry %v: type must be specified", i)
}
if id == "" {
return "", fmt.Errorf("principal_in entry %v: ID must be specified", i)
}
entities[i] = fmt.Sprintf(`%s::"%s"`, typ, id)
if typ == "" {
return "", fmt.Errorf("principal_in: type must be specified")
}
if id == "" {
return "", fmt.Errorf("principal_in: ID must be specified")
}

line := fmt.Sprintf("\tprincipal in [%s],", strings.Join(entities, ", "))
line := fmt.Sprintf("\tprincipal in %s::%q,", typ, id)
output = append(output, line)
} else if p.PrincipalIs.ValueString() != "" {
// principal is <entity type>,
Expand Down Expand Up @@ -139,24 +134,19 @@ func (p Policy) RenderString() (string, error) {
line := fmt.Sprintf("\tresource == %s::%q", resourceType, resourceID)
output = append(output, line)
} else if p.ResourceIn != nil {
// resource == [<entity>, <entity>],
// resource in <entity>, <entity>,

entities := make([]string, len(*p.ResourceIn))
typ := p.ResourceIn.Type.ValueString()
id := p.ResourceIn.ID.ValueString()

for i, ent := range *p.ResourceIn {
typ := ent.Type.ValueString()
id := ent.ID.ValueString()

if typ == "" {
return "", fmt.Errorf("resource_in entry %v: type must be specified", i)
}
if id == "" {
return "", fmt.Errorf("resource_in entry %v: ID must be specified", i)
}
entities[i] = fmt.Sprintf(`%s::"%s"`, typ, id)
if typ == "" {
return "", fmt.Errorf("resource_in: type must be specified")
}
if id == "" {
return "", fmt.Errorf("resource_in: ID must be specified")
}

line := fmt.Sprintf("\tresource in [%s]", strings.Join(entities, ", "))
line := fmt.Sprintf("\tresource in %s::%q", typ, id)
output = append(output, line)
} else if p.ResourceIs.ValueString() != "" {
// resource is <entity type>,
Expand Down
48 changes: 16 additions & 32 deletions pkg/cedarpolicy/render_string_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -139,11 +139,9 @@ unless {
name: "principal_action_resource_in",
policy: Policy{
Effect: types.StringValue("permit"),
PrincipalIn: &[]eid.EID{
{
Type: types.StringValue("CF::User"),
ID: types.StringValue("user1"),
},
PrincipalIn: &eid.EID{
Type: types.StringValue("CF::User"),
ID: types.StringValue("user1"),
},

ActionIn: &[]eid.EID{
Expand All @@ -153,32 +151,24 @@ unless {
},
},

ResourceIn: &[]eid.EID{
{
Type: types.StringValue("Test::Vault"),
ID: types.StringValue("test1"),
},
ResourceIn: &eid.EID{
Type: types.StringValue("Test::Vault"),
ID: types.StringValue("test1"),
},
},
want: `permit (
principal in [CF::User::"user1"],
principal in CF::User::"user1",
action in [Action::Access::"Request"],
resource in [Test::Vault::"test1"]
resource in Test::Vault::"test1"
);`,
},
{
name: "in_condition_multiple_values",
policy: Policy{
Effect: types.StringValue("permit"),
PrincipalIn: &[]eid.EID{
{
Type: types.StringValue("CF::User"),
ID: types.StringValue("user1"),
},
{
Type: types.StringValue("CF::User"),
ID: types.StringValue("user2"),
},
PrincipalIn: &eid.EID{
Type: types.StringValue("CF::User"),
ID: types.StringValue("user1"),
},

ActionIn: &[]eid.EID{
Expand All @@ -192,21 +182,15 @@ unless {
},
},

ResourceIn: &[]eid.EID{
{
Type: types.StringValue("Test::Vault"),
ID: types.StringValue("test1"),
},
{
Type: types.StringValue("Test::Vault"),
ID: types.StringValue("test2"),
},
ResourceIn: &eid.EID{
Type: types.StringValue("Test::Vault"),
ID: types.StringValue("test1"),
},
},
want: `permit (
principal in [CF::User::"user1", CF::User::"user2"],
principal in CF::User::"user1",
action in [Action::Access::"Request", Action::Access::"Close"],
resource in [Test::Vault::"test1", Test::Vault::"test2"]
resource in Test::Vault::"test1"
);`,
},
{
Expand Down

0 comments on commit 61e0150

Please sign in to comment.