From 47d05f6b59ae979d98bba0fb945d5c722cc52710 Mon Sep 17 00:00:00 2001 From: John Kastner <130772734+john-h-kastner-aws@users.noreply.github.com> Date: Fri, 6 Oct 2023 14:35:54 -0400 Subject: [PATCH 1/8] Return `Err` from `Context::from_expr` on non-record expr (#345) --- cedar-policy-core/src/ast/request.rs | 15 ++++++++++----- cedar-policy-core/src/authorizer.rs | 3 ++- cedar-policy-core/src/entities/json/context.rs | 13 +++++-------- cedar-policy-core/src/evaluator.rs | 10 ++++++---- 4 files changed, 23 insertions(+), 18 deletions(-) diff --git a/cedar-policy-core/src/ast/request.rs b/cedar-policy-core/src/ast/request.rs index c4014a090..0f4c1a70d 100644 --- a/cedar-policy-core/src/ast/request.rs +++ b/cedar-policy-core/src/ast/request.rs @@ -164,11 +164,16 @@ impl Context { Self::from_pairs([]) } - /// Create a `Context` from a `RestrictedExpr`, which must be a `Record` - /// INVARIANT: `from_expr` must only be called with an `expr` that is a `Record` - pub fn from_expr(expr: RestrictedExpr) -> Self { - debug_assert!(matches!(expr.expr_kind(), ExprKind::Record { .. })); - Self { context: expr } + /// Create a `Context` from a `RestrictedExpr`, which must be a `Record`. + /// If it is not a `Record`, then this function returns `Err` (returning + /// ownership of the non-record expression), otherwise it returns `Ok` of + /// a context for that record. + pub fn from_expr(expr: RestrictedExpr) -> Result { + match expr.expr_kind() { + // INVARIANT: `context` must be a `Record`, which is guaranteed by the match case. + ExprKind::Record { .. } => Ok(Self { context: expr }), + _ => Err(expr), + } } /// Create a `Context` from a map of key to `RestrictedExpr`, or a Vec of diff --git a/cedar-policy-core/src/authorizer.rs b/cedar-policy-core/src/authorizer.rs index 770df62f6..d008eb33a 100644 --- a/cedar-policy-core/src/authorizer.rs +++ b/cedar-policy-core/src/authorizer.rs @@ -523,7 +523,8 @@ mod test { let context = Context::from_expr(RestrictedExpr::record([( "test".into(), RestrictedExpr::new(Expr::unknown("name")).unwrap(), - )])); + )])) + .unwrap(); let a = Authorizer::new(); let q = Request::new( EntityUID::with_eid("p"), diff --git a/cedar-policy-core/src/entities/json/context.rs b/cedar-policy-core/src/entities/json/context.rs index 5478ddac5..826174ec4 100644 --- a/cedar-policy-core/src/entities/json/context.rs +++ b/cedar-policy-core/src/entities/json/context.rs @@ -15,7 +15,7 @@ */ use super::{JsonDeserializationError, JsonDeserializationErrorContext, SchemaType, ValueParser}; -use crate::ast::{Context, ExprKind}; +use crate::ast::Context; use crate::extensions::Extensions; use std::collections::HashMap; @@ -80,14 +80,11 @@ impl<'e, 's, S: ContextSchema> ContextJsonParser<'e, 's, S> { let rexpr = vparser.val_into_rexpr(json, expected_ty.as_ref(), || { JsonDeserializationErrorContext::Context })?; - // INVARIANT `Context::from_exprs` requires that `rexpr` is a `Record`. - // This is checked by the `match` expression - match rexpr.expr_kind() { - ExprKind::Record { .. } => Ok(Context::from_expr(rexpr)), - _ => Err(JsonDeserializationError::ExpectedContextToBeRecord { + Context::from_expr(rexpr).map_err(|rexpr| { + JsonDeserializationError::ExpectedContextToBeRecord { got: Box::new(rexpr), - }), - } + } + }) } /// Parse context JSON (in `std::io::Read` form) into a `Context` object diff --git a/cedar-policy-core/src/evaluator.rs b/cedar-policy-core/src/evaluator.rs index 3c2760569..d8f1a34cd 100644 --- a/cedar-policy-core/src/evaluator.rs +++ b/cedar-policy-core/src/evaluator.rs @@ -4040,7 +4040,7 @@ pub mod test { let euid: EntityUID = r#"Test::"test""#.parse().unwrap(); let rexpr = RestrictedExpr::new(context_expr) .expect("Context Expression was not a restricted expression"); - let context = Context::from_expr(rexpr); + let context = Context::from_expr(rexpr).unwrap(); let q = Request::new(euid.clone(), euid.clone(), euid, context); let es = Entities::new(); let exts = Extensions::none(); @@ -4163,7 +4163,8 @@ pub mod test { let context = Context::from_expr(RestrictedExpr::new_unchecked(Expr::record([ ("a".into(), Expr::val(3)), ("b".into(), Expr::unknown("b".to_string())), - ]))); + ]))) + .unwrap(); let euid: EntityUID = r#"Test::"test""#.parse().unwrap(); let q = Request::new(euid.clone(), euid.clone(), euid, context); let es = Entities::new(); @@ -4202,7 +4203,7 @@ pub mod test { Expr::unknown("cell".to_string()), )])) .expect("should qualify as restricted"); - let context = Context::from_expr(c_expr); + let context = Context::from_expr(c_expr).unwrap(); let q = Request::new(p, a, r, context); let exts = Extensions::none(); @@ -4275,7 +4276,8 @@ pub mod test { Context::from_expr(RestrictedExpr::new_unchecked(Expr::record([( "condition".into(), Expr::unknown("unknown_condition"), - )]))), + )]))) + .unwrap(), ); let eval = Evaluator::new(&q, &es, &exts).unwrap(); From 0a2ffb51c936d8890b6b15461bbe51cc569ab790 Mon Sep 17 00:00:00 2001 From: John Kastner <130772734+john-h-kastner-aws@users.noreply.github.com> Date: Wed, 11 Oct 2023 09:14:12 -0400 Subject: [PATCH 2/8] Use `Display` instead of `Debug` for error message in `json_is_authorized` (#354) Also make two small tweaks to error text. --- cedar-policy/src/frontend/is_authorized.rs | 4 ++-- cedar-policy/src/frontend/validate.rs | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/cedar-policy/src/frontend/is_authorized.rs b/cedar-policy/src/frontend/is_authorized.rs index 836092c9a..46be42c19 100644 --- a/cedar-policy/src/frontend/is_authorized.rs +++ b/cedar-policy/src/frontend/is_authorized.rs @@ -289,7 +289,7 @@ fn parse_policy_set_from_individual_policies( Ok(p) => match policy_set.add(p) { Ok(_) => {} Err(err) => { - errs.push(format!("couldn't add policy to set due to error: {err:?}")); + errs.push(format!("couldn't add policy to set due to error: {err}")); } }, Err(pes) => errs.extend( @@ -305,7 +305,7 @@ fn parse_policy_set_from_individual_policies( Ok(p) => match policy_set.add_template(p) { Ok(_) => {} Err(err) => { - errs.push(format!("couldn't add policy to set due to error: {err:?}")); + errs.push(format!("couldn't add policy to set due to error: {err}")); } }, Err(pes) => errs.extend( diff --git a/cedar-policy/src/frontend/validate.rs b/cedar-policy/src/frontend/validate.rs index a286a6ae8..269c738d8 100644 --- a/cedar-policy/src/frontend/validate.rs +++ b/cedar-policy/src/frontend/validate.rs @@ -38,7 +38,7 @@ fn validate(call: &ValidateCall) -> Result { parse_errors.extend( policy_set_parse_errs .into_iter() - .map(|pe| format!("{pe:?}")), + .map(|pe| format!("parse error in policy: {pe}")), ); } }, @@ -68,7 +68,7 @@ fn validate(call: &ValidateCall) -> Result { .schema .clone() .try_into() - .map_err(|e| format!("couldn't construct schema - {e}"))?; + .map_err(|e| format!("could not construct schema: {e}"))?; let validator = Validator::new(schema); let notes: Vec = validator From 1eaaf17dcf4b0720a6b3cbb8f7d7c418bf102ff9 Mon Sep 17 00:00:00 2001 From: Andrew Wells <130512013+andrewmwells-amazon@users.noreply.github.com> Date: Mon, 16 Oct 2023 12:02:15 -0700 Subject: [PATCH 3/8] add note about version pinning to README (#364) --- README.md | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/README.md b/README.md index 5aa90b3c5..6de112b26 100644 --- a/README.md +++ b/README.md @@ -103,6 +103,24 @@ To build, simply run `cargo build` (or `cargo build --release`). We maintain changelogs for our public-facing crates: [cedar-policy](./cedar-policy/CHANGELOG.md) and [cedar-policy-cli](./cedar-policy-cli/CHANGELOG.md). For a list of the current and past releases, see [crates.io](https://crates.io/crates/cedar-policy) or [Releases](https://github.com/cedar-policy/cedar/releases). +## Backward Compatibility Considerations + +Cedar is written in Rust and you will typically depend on Cedar via Cargo. Cargo makes sane choices for the majority of project, but your needs may differ. If you don't want automatic updates to Cedar replace + +``` +[dependencies] +cedar-policy = "2.3.3" +``` + +with + +``` +[dependencies] +cedar-policy = "=2.3.3" +``` + +in your `Cargo.toml` file. + ## Security See [SECURITY](SECURITY.md) for more information. From 1b868f39dea345c03fc8cc2058394a3938af2a07 Mon Sep 17 00:00:00 2001 From: Craig Disselkoen Date: Tue, 17 Oct 2023 13:13:10 -0700 Subject: [PATCH 4/8] small refactoring and docs (#367) --- cedar-policy-validator/src/lib.rs | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/cedar-policy-validator/src/lib.rs b/cedar-policy-validator/src/lib.rs index dfa540e83..871504107 100644 --- a/cedar-policy-validator/src/lib.rs +++ b/cedar-policy-validator/src/lib.rs @@ -76,25 +76,26 @@ impl Validator { Self { schema } } - /// Validate all templates in a policy set (which includes static policies) and - /// return an iterator of policy notes associated with each policy id. + /// Validate all templates, links, and static policies in a policy set. + /// Return an iterator of policy notes associated with each policy id. pub fn validate<'a>( &'a self, policies: &'a PolicySet, mode: ValidationMode, ) -> ValidationResult<'a> { - let template_errs = policies + let template_and_static_policy_errs = policies .all_templates() .flat_map(|p| self.validate_policy(p, mode)); - let instantiation_errs = policies.policies().flat_map(|p| { + let link_errs = policies.policies().flat_map(|p| { self.validate_slots(p.env()) .map(move |note| ValidationError::with_policy_id(p.id(), None, note)) }); - ValidationResult::new(template_errs.chain(instantiation_errs)) + ValidationResult::new(template_and_static_policy_errs.chain(link_errs)) } - /// Run all validations against a single policy, gathering all validation - /// notes from together in the returned iterator. + /// Run all validations against a single static policy or template (note + /// that Core `Template` includes static policies as well), gathering all + /// validation notes from together in the returned iterator. fn validate_policy<'a>( &'a self, p: &'a Template, @@ -108,8 +109,10 @@ impl Validator { } /// Construct a Typechecker instance and use it to detect any type errors in - /// the argument policy in the context of the schema for this validator. Any - /// detected type errors are wrapped and returned as `ValidationErrorKind`s. + /// the argument static policy or template (note that Core `Template` + /// includes static policies as well) in the context of the schema for this + /// validator. Any detected type errors are wrapped and returned as + /// `ValidationErrorKind`s. fn typecheck_policy<'a>( &'a self, t: &'a Template, From 84b176ab335555001f805a9bc28119066edeb5a2 Mon Sep 17 00:00:00 2001 From: Andrew Wells <130512013+andrewmwells-amazon@users.noreply.github.com> Date: Tue, 17 Oct 2023 13:55:09 -0700 Subject: [PATCH 5/8] should_panic CI check (#368) --- panic_safety.sh | 25 ++++++++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-) diff --git a/panic_safety.sh b/panic_safety.sh index 04620fb3a..b5802c41f 100755 --- a/panic_safety.sh +++ b/panic_safety.sh @@ -19,6 +19,8 @@ # Clippy is automatically run against PRs on GitHub. Clippy lints are # controlled for all of our crates by `./.cargo/config.toml`. + +# Check that panics are prefixed with a line that has "PANIC SAFETY" in it total_panics=0 failed=0 @@ -28,9 +30,9 @@ panic_markers=("panic unwrap_used expect_used fallible_impl_from unreachable ind for crate in ${crates[@]}; do crate_panics=0 for panic_marker in ${panic_markers[@]}; do - while read -r filename linenum ; do + while read -r filename linenum ; do msg_line=$(($linenum - 1)) - if sed "$msg_line!d" $filename | grep 'PANIC SAFETY' > /dev/null ; then + if sed "$msg_line!d" $filename | grep 'PANIC SAFETY' > /dev/null ; then crate_panics=$(($crate_panics + 1)) else echo "Unchecked panic at $filename:$linenum" @@ -45,4 +47,21 @@ done echo "Total Panics: $total_panics" -exit $failed +if ((failed > 0)) +then + exit $failed +else + # Check for "should_panic"s without "expected = ..." + num_should_panic=$(grep -inr --include \*.rs should_panic | wc -l) + echo "num 'should_panic's " $num_should_panic + num_should_panic_lparen=$(grep -inr --include \*.rs should_panic\( | wc -l) + echo "num 'should_panic('s " $num_should_panic_lparen + if ((num_should_panic == num_should_panic_lparen)) + then + exit 0 + else + echo "failed should_panic( test" + echo "every 'should_panic' should also be a 'should_panic('" + exit 1 + fi +fi \ No newline at end of file From f1d28c67c59168fa2b5fe69ba9376aa04c572323 Mon Sep 17 00:00:00 2001 From: Kesha Hietala Date: Thu, 19 Oct 2023 10:29:01 -0400 Subject: [PATCH 6/8] Fix validation of template-linked policies (#371) Co-authored-by: Craig Disselkoen --- cedar-policy-validator/src/lib.rs | 86 ++++++++-- cedar-policy-validator/src/rbac.rs | 249 ++++++++++++----------------- cedar-policy/CHANGELOG.md | 5 + 3 files changed, 184 insertions(+), 156 deletions(-) diff --git a/cedar-policy-validator/src/lib.rs b/cedar-policy-validator/src/lib.rs index 871504107..7dc91bbd6 100644 --- a/cedar-policy-validator/src/lib.rs +++ b/cedar-policy-validator/src/lib.rs @@ -19,7 +19,7 @@ use std::collections::HashSet; -use cedar_policy_core::ast::{PolicySet, Template}; +use cedar_policy_core::ast::{Policy, PolicySet, Template}; mod err; mod str_checks; @@ -86,16 +86,16 @@ impl Validator { let template_and_static_policy_errs = policies .all_templates() .flat_map(|p| self.validate_policy(p, mode)); - let link_errs = policies.policies().flat_map(|p| { - self.validate_slots(p.env()) - .map(move |note| ValidationError::with_policy_id(p.id(), None, note)) - }); + let link_errs = policies + .policies() + .filter_map(|p| self.validate_slots(p)) + .flatten(); ValidationResult::new(template_and_static_policy_errs.chain(link_errs)) } /// Run all validations against a single static policy or template (note /// that Core `Template` includes static policies as well), gathering all - /// validation notes from together in the returned iterator. + /// validation notes together in the returned iterator. fn validate_policy<'a>( &'a self, p: &'a Template, @@ -103,11 +103,39 @@ impl Validator { ) -> impl Iterator + 'a { self.validate_entity_types(p) .chain(self.validate_action_ids(p)) - .chain(self.validate_action_application(p)) + .chain(self.validate_action_application( + p.principal_constraint(), + p.action_constraint(), + p.resource_constraint(), + )) .map(move |note| ValidationError::with_policy_id(p.id(), None, note)) .chain(self.typecheck_policy(p, mode)) } + /// Run relevant validations against a single template-linked policy, + /// gathering all validation notes together in the returned iterator. + fn validate_slots<'a>( + &'a self, + p: &'a Policy, + ) -> Option + 'a> { + // Ignore static policies since they are already handled by `validate_policy` + if p.is_static() { + return None; + } + // For template-linked policies `Policy::principal_constraint()` and + // `Policy::resource_constraint()` return a copy of the constraint with + // the slot filled by the appropriate value. + Some( + self.validate_entity_types_in_slots(p.env()) + .chain(self.validate_action_application( + &p.principal_constraint(), + p.action_constraint(), + &p.resource_constraint(), + )) + .map(move |note| ValidationError::with_policy_id(p.id(), None, note)), + ) + } + /// Construct a Typechecker instance and use it to detect any type errors in /// the argument static policy or template (note that Core `Template` /// includes static policies as well) in the context of the schema for this @@ -316,19 +344,51 @@ mod test { ) .expect("Linking failed!"); let result = validator.validate(&set, ValidationMode::default()); - - let pid = ast::PolicyID::from_string("link2"); - let resource_err = ValidationError::with_policy_id( - &pid, + assert!(!result.validation_passed()); + assert_eq!(result.validation_errors().count(), 2); + let id = ast::PolicyID::from_string("link2"); + let undefined_err = ValidationError::with_policy_id( + &id, None, ValidationErrorKind::unrecognized_entity_type( "some_namespace::Undefined".to_string(), Some("some_namespace::User".to_string()), ), ); + let invalid_action_err = ValidationError::with_policy_id( + &id, + None, + ValidationErrorKind::invalid_action_application(false, false), + ); + assert!(result.validation_errors().any(|x| x == &undefined_err)); + assert!(result.validation_errors().any(|x| x == &invalid_action_err)); + + // this is also an invalid instantiation (not a valid resource type for any action in the schema) + let mut values = HashMap::new(); + values.insert( + ast::SlotId::resource(), + ast::EntityUID::from_components( + "some_namespace::User".parse().unwrap(), + ast::Eid::new("foo"), + ), + ); + set.link( + ast::PolicyID::from_string("template"), + ast::PolicyID::from_string("link3"), + values, + ) + .expect("Linking failed!"); + let result = validator.validate(&set, ValidationMode::default()); assert!(!result.validation_passed()); - println!("{:?}", result.validation_errors().collect::>()); - assert!(result.validation_errors().any(|x| x == &resource_err)); + // `result` contains the two prior error messages plus one new one + assert_eq!(result.validation_errors().count(), 3); + let id = ast::PolicyID::from_string("link3"); + let invalid_action_err = ValidationError::with_policy_id( + &id, + None, + ValidationErrorKind::invalid_action_application(false, false), + ); + assert!(result.validation_errors().any(|x| x == &invalid_action_err)); Ok(()) } diff --git a/cedar-policy-validator/src/rbac.rs b/cedar-policy-validator/src/rbac.rs index d0288a896..8344a4739 100644 --- a/cedar-policy-validator/src/rbac.rs +++ b/cedar-policy-validator/src/rbac.rs @@ -17,8 +17,8 @@ //! Contains the validation logic specific to RBAC policy validation. use cedar_policy_core::ast::{ - self, ActionConstraint, EntityReference, EntityUID, Name, PrincipalOrResourceConstraint, - SlotEnv, Template, + self, ActionConstraint, EntityReference, EntityUID, Name, PrincipalConstraint, + PrincipalOrResourceConstraint, ResourceConstraint, SlotEnv, Template, }; use std::{collections::HashSet, sync::Arc}; @@ -131,7 +131,7 @@ impl Validator { /// Generate UnrecognizedEntityType or UnspecifiedEntity notes for every /// entity type in the slot environment that is either not in the schema, /// or unspecified. - pub(crate) fn validate_slots<'a>( + pub(crate) fn validate_entity_types_in_slots<'a>( &'a self, slots: &'a SlotEnv, ) -> impl Iterator + 'a { @@ -166,21 +166,29 @@ impl Validator { }) } - fn check_if_in_fixes_principal(&self, template: &Template) -> bool { + fn check_if_in_fixes_principal( + &self, + principal_constraint: &PrincipalConstraint, + action_constraint: &ActionConstraint, + ) -> bool { self.check_if_in_fixes( - template.principal_constraint().as_inner(), + principal_constraint.as_inner(), &self - .get_apply_specs_for_action(template) + .get_apply_specs_for_action(action_constraint) .collect::>(), &|spec| Box::new(spec.applicable_principal_types()), ) } - fn check_if_in_fixes_resource(&self, template: &Template) -> bool { + fn check_if_in_fixes_resource( + &self, + resource_constraint: &ResourceConstraint, + action_constraint: &ActionConstraint, + ) -> bool { self.check_if_in_fixes( - template.resource_constraint().as_inner(), + resource_constraint.as_inner(), &self - .get_apply_specs_for_action(template) + .get_apply_specs_for_action(action_constraint) .collect::>(), &|spec| Box::new(spec.applicable_resource_types()), ) @@ -268,20 +276,26 @@ impl Validator { // Check that there exists a (action id, principal type, resource type) // entity type pair where the action can be applied to both the principal - // and resource. + // and resource. This function takes the three scope constraints as input + // (rather than a template) to facilitate code reuse. pub(crate) fn validate_action_application( &self, - template: &Template, + principal_constraint: &PrincipalConstraint, + action_constraint: &ActionConstraint, + resource_constraint: &ResourceConstraint, ) -> impl Iterator { - let mut apply_specs = self.get_apply_specs_for_action(template); - let resources_for_head: HashSet = - self.get_resources_satisfying_constraint(template).collect(); + let mut apply_specs = self.get_apply_specs_for_action(action_constraint); + let resources_for_head: HashSet = self + .get_resources_satisfying_constraint(resource_constraint) + .collect(); let principals_for_head: HashSet = self - .get_principals_satisfying_constraint(template) + .get_principals_satisfying_constraint(principal_constraint) .collect(); - let would_in_fix_principal = self.check_if_in_fixes_principal(template); - let would_in_fix_resource = self.check_if_in_fixes_resource(template); + let would_in_fix_principal = + self.check_if_in_fixes_principal(principal_constraint, action_constraint); + let would_in_fix_resource = + self.check_if_in_fixes_resource(resource_constraint, action_constraint); Some(ValidationErrorKind::invalid_action_application( would_in_fix_principal, @@ -321,15 +335,15 @@ impl Validator { } /// Gather all ApplySpec objects for all actions in the schema. The `applies_to` - /// field is optional, so any actions lacking this field are omitted from the - /// result. + /// field is optional in the schema. In the case that it was not supplied, the + /// `applies_to` field will contain `UnspecifiedEntity`. pub(crate) fn get_apply_specs_for_action<'a>( &'a self, - template: &'a Template, + action_constraint: &'a ActionConstraint, ) -> impl Iterator + 'a { - self.get_actions_satisfying_constraint(template) + self.get_actions_satisfying_constraint(action_constraint) // Get the action type if the id string exists, and then the - // applies_to list for the action type, if that exists. + // applies_to list. .filter_map(|action_id| self.schema.get_action_id(&action_id)) .map(|action| &action.applies_to) } @@ -338,10 +352,10 @@ impl Validator { /// head constraint of the policy. pub(crate) fn get_principals_satisfying_constraint<'a>( &'a self, - template: &'a Template, + principal_constraint: &'a PrincipalConstraint, ) -> impl Iterator + 'a { self.get_entities_satisfying_constraint( - HeadConstraint::from(template.principal_constraint().as_inner()), + HeadConstraint::from(principal_constraint.as_inner()), PrincipalOrResourceHeadVar::PrincipalOrResource, ) } @@ -350,10 +364,10 @@ impl Validator { /// action head constraint of the policy. pub(crate) fn get_actions_satisfying_constraint<'a>( &'a self, - template: &'a Template, + action_constraint: &'a ActionConstraint, ) -> impl Iterator + 'a { self.get_entities_satisfying_constraint( - HeadConstraint::from(template.action_constraint()), + HeadConstraint::from(action_constraint), ActionHeadVar::Action, ) } @@ -362,10 +376,10 @@ impl Validator { /// head constraint of the policy. pub(crate) fn get_resources_satisfying_constraint<'a>( &'a self, - template: &'a Template, + resource_constraint: &'a ResourceConstraint, ) -> impl Iterator + 'a { self.get_entities_satisfying_constraint( - HeadConstraint::from(template.resource_constraint().as_inner()), + HeadConstraint::from(resource_constraint.as_inner()), PrincipalOrResourceHeadVar::PrincipalOrResource, ) } @@ -536,20 +550,19 @@ mod test { let bin_eid = EntityUID::with_eid_and_type(bin_type, "bin").expect(""); - let id = "id"; - let p = Template::new( - PolicyID::from_string(id), - BTreeMap::new(), - Effect::Permit, - PrincipalConstraint::is_eq(group_eid), - ActionConstraint::is_eq(action_eid), - ResourceConstraint::is_eq(bin_eid), - Expr::val(true), - ); + let principal_constraint = PrincipalConstraint::is_eq(group_eid); + let action_constraint = ActionConstraint::is_eq(action_eid); + let resource_constraint = ResourceConstraint::is_eq(bin_eid); let v = Validator::new(schema); - let notes = v.validate_action_application(&p).collect::>(); + let notes = v + .validate_action_application( + &principal_constraint, + &action_constraint, + &resource_constraint, + ) + .collect::>(); assert_eq!( vec![ValidationErrorKind::invalid_action_application(true, true)], @@ -729,18 +742,10 @@ mod test { [], ); let schema = schema_file.try_into().unwrap(); - let template = Template::new( - PolicyID::from_string("id"), - BTreeMap::new(), - Effect::Forbid, - PrincipalConstraint::is_eq_slot(), - ActionConstraint::any(), - ResourceConstraint::any(), - Expr::val(true), - ); + let principal_constraint = PrincipalConstraint::is_eq_slot(); let validator = Validator::new(schema); let entities = validator - .get_principals_satisfying_constraint(&template) + .get_principals_satisfying_constraint(&principal_constraint) .collect::>(); assert_eq!(entities.len(), 1); let name = &entities[0]; @@ -761,18 +766,10 @@ mod test { [], ); let schema = schema_file.try_into().unwrap(); - let template = Template::new( - PolicyID::from_string("id"), - BTreeMap::new(), - Effect::Forbid, - PrincipalConstraint::any(), - ActionConstraint::any(), - ResourceConstraint::is_in_slot(), - Expr::val(true), - ); + let principal_constraint = PrincipalConstraint::any(); let validator = Validator::new(schema); let entities = validator - .get_principals_satisfying_constraint(&template) + .get_principals_satisfying_constraint(&principal_constraint) .collect::>(); assert_eq!(entities.len(), 1); let name = &entities[0]; @@ -800,7 +797,8 @@ mod test { let env = HashMap::from([(ast::SlotId::principal(), undefined_euid)]); let validator = Validator::new(schema); - let notes: Vec = validator.validate_slots(&env).collect(); + let notes: Vec = + validator.validate_entity_types_in_slots(&env).collect(); assert_eq!(1, notes.len()); match notes.get(0) { @@ -1012,15 +1010,7 @@ mod test { let foo_name = "foo_name"; let euid_foo = EntityUID::with_eid_and_type("Action", foo_name).expect("should be a valid identifier"); - let policy = Template::new( - PolicyID::from_string("policy0"), - BTreeMap::new(), - Effect::Permit, - PrincipalConstraint::any(), - ActionConstraint::is_eq(euid_foo.clone()), - ResourceConstraint::any(), - Expr::val(true), - ); + let action_constraint = ActionConstraint::is_eq(euid_foo.clone()); let schema_file = NamespaceDefinition::new( [], @@ -1037,7 +1027,7 @@ mod test { let validate = Validator::new(singleton_schema); let actions = validate - .get_actions_satisfying_constraint(&policy) + .get_actions_satisfying_constraint(&action_constraint) .collect(); assert_eq!(HashSet::from([euid_foo]), actions); @@ -1049,15 +1039,7 @@ mod test { let foo_name = "foo_name"; let euid_foo = EntityUID::with_eid_and_type("Action", foo_name).expect("should be a valid identifier"); - let policy = Template::new( - PolicyID::from_string("policy0"), - BTreeMap::new(), - Effect::Permit, - PrincipalConstraint::any(), - ActionConstraint::is_in(vec![euid_foo.clone()]), - ResourceConstraint::any(), - Expr::val(true), - ); + let action_constraint = ActionConstraint::is_in(vec![euid_foo.clone()]); let schema_file = NamespaceDefinition::new( [], @@ -1074,7 +1056,7 @@ mod test { let validate = Validator::new(singleton_schema); let actions = validate - .get_actions_satisfying_constraint(&policy) + .get_actions_satisfying_constraint(&action_constraint) .collect(); assert_eq!(HashSet::from([euid_foo]), actions); @@ -1086,15 +1068,7 @@ mod test { let foo_name = "foo_name"; let euid_foo = EntityUID::with_eid_and_type("Action", foo_name).expect("should be a valid identifier"); - let policy = Template::new( - PolicyID::from_string("policy0"), - BTreeMap::new(), - Effect::Permit, - PrincipalConstraint::any(), - ActionConstraint::is_in(vec![euid_foo.clone()]), - ResourceConstraint::any(), - Expr::val(true), - ); + let action_constraint = ActionConstraint::is_in(vec![euid_foo.clone()]); let schema_file = NamespaceDefinition::new( [], @@ -1111,7 +1085,7 @@ mod test { let validate = Validator::new(singleton_schema); let actions = validate - .get_actions_satisfying_constraint(&policy) + .get_actions_satisfying_constraint(&action_constraint) .collect(); assert_eq!(HashSet::from([euid_foo]), actions); @@ -1123,15 +1097,7 @@ mod test { let foo_type = "foo_type"; let euid_foo = EntityUID::with_eid_and_type(foo_type, "foo_name") .expect("should be a valid identifier"); - let policy = Template::new( - PolicyID::from_string("policy0"), - BTreeMap::new(), - Effect::Permit, - PrincipalConstraint::is_eq(euid_foo.clone()), - ActionConstraint::any(), - ResourceConstraint::any(), - Expr::val(true), - ); + let principal_constraint = PrincipalConstraint::is_eq(euid_foo.clone()); let schema_file = NamespaceDefinition::new( [( @@ -1147,7 +1113,7 @@ mod test { let validate = Validator::new(singleton_schema); let principals = validate - .get_principals_satisfying_constraint(&policy) + .get_principals_satisfying_constraint(&principal_constraint) .map(cedar_policy_core::ast::EntityType::Concrete) .collect::>(); assert_eq!(HashSet::from([euid_foo.components().0]), principals); @@ -1232,19 +1198,18 @@ mod test { fn validate_action_apply_incorrect_principal() -> Result<()> { let (_, action, resource, schema) = schema_with_single_principal_action_resource(); - let policy = Template::new( - PolicyID::from_string("policy0"), - BTreeMap::new(), - Effect::Permit, - PrincipalConstraint::is_eq(resource.clone()), - ActionConstraint::is_eq(action), - ResourceConstraint::is_eq(resource), - Expr::val(true), - ); + let principal_constraint = PrincipalConstraint::is_eq(resource.clone()); + let action_constraint = ActionConstraint::is_eq(action); + let resource_constraint = ResourceConstraint::is_eq(resource); let validate = Validator::new(schema); - let notes: Vec = - validate.validate_action_application(&policy).collect(); + let notes: Vec = validate + .validate_action_application( + &principal_constraint, + &action_constraint, + &resource_constraint, + ) + .collect(); assert_eq!(1, notes.len()); match notes.get(0) { Some(ValidationErrorKind::InvalidActionApplication(_)) => (), @@ -1258,19 +1223,18 @@ mod test { fn validate_action_apply_incorrect_resource() -> Result<()> { let (principal, action, _, schema) = schema_with_single_principal_action_resource(); - let policy = Template::new( - PolicyID::from_string("policy0"), - BTreeMap::new(), - Effect::Permit, - PrincipalConstraint::is_eq(principal.clone()), - ActionConstraint::is_eq(action), - ResourceConstraint::is_eq(principal), - Expr::val(true), - ); + let principal_constraint = PrincipalConstraint::is_eq(principal.clone()); + let action_constraint = ActionConstraint::is_eq(action); + let resource_constraint = ResourceConstraint::is_eq(principal); let validate = Validator::new(schema); - let notes: Vec = - validate.validate_action_application(&policy).collect(); + let notes: Vec = validate + .validate_action_application( + &principal_constraint, + &action_constraint, + &resource_constraint, + ) + .collect(); assert_eq!(1, notes.len()); match notes.get(0) { Some(ValidationErrorKind::InvalidActionApplication(_)) => (), @@ -1284,19 +1248,18 @@ mod test { fn validate_action_apply_incorrect_principal_and_resource() -> Result<()> { let (principal, action, resource, schema) = schema_with_single_principal_action_resource(); - let policy = Template::new( - PolicyID::from_string("policy0"), - BTreeMap::new(), - Effect::Permit, - PrincipalConstraint::is_eq(resource), - ActionConstraint::is_eq(action), - ResourceConstraint::is_eq(principal), - Expr::val(true), - ); + let principal_constraint = PrincipalConstraint::is_eq(resource); + let action_constraint = ActionConstraint::is_eq(action); + let resource_constraint = ResourceConstraint::is_eq(principal); let validate = Validator::new(schema); - let notes: Vec = - validate.validate_action_application(&policy).collect(); + let notes: Vec = validate + .validate_action_application( + &principal_constraint, + &action_constraint, + &resource_constraint, + ) + .collect(); assert_eq!(1, notes.len()); match notes.get(0) { Some(ValidationErrorKind::InvalidActionApplication(_)) => (), @@ -1335,18 +1298,18 @@ mod test { fn validate_used_as_incorrect() -> Result<()> { let (principal, _, resource, schema) = schema_with_single_principal_action_resource(); - let policy = Template::new( - PolicyID::from_string("policy0"), - BTreeMap::new(), - Effect::Permit, - PrincipalConstraint::is_eq(resource), - ActionConstraint::any(), - ResourceConstraint::is_eq(principal), - Expr::val(true), - ); + let principal_constraint = PrincipalConstraint::is_eq(resource); + let action_constraint = ActionConstraint::any(); + let resource_constraint = ResourceConstraint::is_eq(principal); let validate = Validator::new(schema); - let notes: Vec<_> = validate.validate_action_application(&policy).collect(); + let notes: Vec<_> = validate + .validate_action_application( + &principal_constraint, + &action_constraint, + &resource_constraint, + ) + .collect(); assert_eq!( notes, vec![ValidationErrorKind::invalid_action_application( diff --git a/cedar-policy/CHANGELOG.md b/cedar-policy/CHANGELOG.md index a75f69ca4..ab1bbdd52 100644 --- a/cedar-policy/CHANGELOG.md +++ b/cedar-policy/CHANGELOG.md @@ -1,5 +1,10 @@ # Changelog +## 2.4.2 + +### Changed +- Fixed bug (#370) related to how the validator handles template-linked policies + ## 2.4.1 ### Added - New experimental API to construct queries with `Unknown` fields for partial evaluation. From e3d74cb94a4ad7576b41d107df4aef09e0845cc0 Mon Sep 17 00:00:00 2001 From: Kesha Hietala Date: Thu, 19 Oct 2023 11:59:54 -0400 Subject: [PATCH 7/8] bump to 2.4.2 --- CONTRIBUTING.md | 2 +- cedar-policy-cli/CHANGELOG.md | 2 ++ cedar-policy-cli/Cargo.toml | 6 +++--- cedar-policy-core/Cargo.toml | 2 +- cedar-policy-formatter/CHANGELOG.md | 2 ++ cedar-policy-formatter/Cargo.toml | 4 ++-- cedar-policy-validator/Cargo.toml | 4 ++-- cedar-policy/Cargo.toml | 6 +++--- 8 files changed, 16 insertions(+), 12 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 3cf24c831..875a09e02 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -94,7 +94,7 @@ The following examples assume the *next major* release on main is 3.0, then *nex * **Add a new feature to release in next minor:** Add a changelog entry to `[Unreleased 2.x]` on main, then backport to 2.x (including the changelog entry). * **Introduce a breaking API change to release in next major:** Add a changelog entry to `[Unreleased 3.0]` on main, do not backport. -* **Upgrade a dependency to fix a CVE:** Add a changelog entry to `[Unreleased 2.x]` on main, then backport to 2.x (including the changelog entry), then backport to 2.4 and ensure the changelog entry is added to `[Unreleased 2.4.1]`. +* **Upgrade a dependency to fix a CVE:** Add a changelog entry to `[Unreleased 2.x]` on main, then backport to 2.x (including the changelog entry), then backport to 2.4 and ensure the changelog entry is added to `[Unreleased 2.4.2]`. ## Review Process diff --git a/cedar-policy-cli/CHANGELOG.md b/cedar-policy-cli/CHANGELOG.md index fe1e9bfe8..1b4a8c851 100644 --- a/cedar-policy-cli/CHANGELOG.md +++ b/cedar-policy-cli/CHANGELOG.md @@ -1,5 +1,7 @@ # Changelog +## 2.4.2 + ## 2.4.1 ## 2.4.0 diff --git a/cedar-policy-cli/Cargo.toml b/cedar-policy-cli/Cargo.toml index 1681a4f73..ca36db4e4 100644 --- a/cedar-policy-cli/Cargo.toml +++ b/cedar-policy-cli/Cargo.toml @@ -2,7 +2,7 @@ name = "cedar-policy-cli" edition = "2021" -version = "2.4.1" +version = "2.4.2" license = "Apache-2.0" categories = ["compilers", "config"] description = "CLI interface for the Cedar Policy language." @@ -11,8 +11,8 @@ homepage = "https://cedarpolicy.com" repository = "https://github.com/cedar-policy/cedar" [dependencies] -cedar-policy = { version = "=2.4.1", path = "../cedar-policy" } -cedar-policy-formatter = { version = "=2.4.1", path = "../cedar-policy-formatter" } +cedar-policy = { version = "=2.4.2", path = "../cedar-policy" } +cedar-policy-formatter = { version = "=2.4.2", path = "../cedar-policy-formatter" } clap = { version = "4", features = ["derive", "env"] } serde = { version = "1.0", features = ["derive"] } serde_json = "1.0" diff --git a/cedar-policy-core/Cargo.toml b/cedar-policy-core/Cargo.toml index 58b6b67f7..83f7e2104 100644 --- a/cedar-policy-core/Cargo.toml +++ b/cedar-policy-core/Cargo.toml @@ -3,7 +3,7 @@ name = "cedar-policy-core" edition = "2021" build = "build.rs" -version = "2.4.1" +version = "2.4.2" license = "Apache-2.0" categories = ["compilers", "config"] description = "Core implemenation of the Cedar Policy language." diff --git a/cedar-policy-formatter/CHANGELOG.md b/cedar-policy-formatter/CHANGELOG.md index cecdc9685..19e6154d9 100644 --- a/cedar-policy-formatter/CHANGELOG.md +++ b/cedar-policy-formatter/CHANGELOG.md @@ -1,5 +1,7 @@ # Changelog +## 2.4.2 + ## 2.4.1 - Bumped `pretty`, `logos`, and `regex` dependencies diff --git a/cedar-policy-formatter/Cargo.toml b/cedar-policy-formatter/Cargo.toml index 2dd720539..12afa6cac 100644 --- a/cedar-policy-formatter/Cargo.toml +++ b/cedar-policy-formatter/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "cedar-policy-formatter" -version = "2.4.1" +version = "2.4.2" edition = "2021" license = "Apache-2.0" categories = ["compilers", "config"] @@ -10,7 +10,7 @@ homepage = "https://cedarpolicy.com" repository = "https://github.com/cedar-policy/cedar" [dependencies] -cedar-policy-core = { version = "=2.4.1", path = "../cedar-policy-core" } +cedar-policy-core = { version = "=2.4.2", path = "../cedar-policy-core" } pretty = "0.12.1" logos = "0.13.0" itertools = "0.10" diff --git a/cedar-policy-validator/Cargo.toml b/cedar-policy-validator/Cargo.toml index 2a3d79de4..4d99e1feb 100644 --- a/cedar-policy-validator/Cargo.toml +++ b/cedar-policy-validator/Cargo.toml @@ -2,7 +2,7 @@ name = "cedar-policy-validator" edition = "2021" -version = "2.4.1" +version = "2.4.2" license = "Apache-2.0" categories = ["compilers", "config"] description = "Validator for the Cedar Policy language." @@ -11,7 +11,7 @@ homepage = "https://cedarpolicy.com" repository = "https://github.com/cedar-policy/cedar" [dependencies] -cedar-policy-core = { version = "=2.4.1", path = "../cedar-policy-core" } +cedar-policy-core = { version = "=2.4.2", path = "../cedar-policy-core" } serde = { version = "1.0", features = ["derive"] } serde_json = { version = "1.0", features = ["preserve_order"] } serde_with = "3.0" diff --git a/cedar-policy/Cargo.toml b/cedar-policy/Cargo.toml index cc3ce6e0f..4acf1994d 100644 --- a/cedar-policy/Cargo.toml +++ b/cedar-policy/Cargo.toml @@ -2,7 +2,7 @@ name = "cedar-policy" edition = "2021" -version = "2.4.1" +version = "2.4.2" license = "Apache-2.0" categories = ["compilers", "config"] description = "Cedar is a language for defining permissions as policies, which describe who should have access to what." @@ -11,8 +11,8 @@ homepage = "https://cedarpolicy.com" repository = "https://github.com/cedar-policy/cedar" [dependencies] -cedar-policy-core = { version = "=2.4.1", path = "../cedar-policy-core" } -cedar-policy-validator = { version = "=2.4.1", path = "../cedar-policy-validator" } +cedar-policy-core = { version = "=2.4.2", path = "../cedar-policy-core" } +cedar-policy-validator = { version = "=2.4.2", path = "../cedar-policy-validator" } ref-cast = "1.0" serde = { version = "1.0", features = ["derive", "rc"] } serde_json = "1.0" From 9bb3209986a8c5c89749fa0058355f9451513615 Mon Sep 17 00:00:00 2001 From: Andrew Wells <130512013+andrewmwells-amazon@users.noreply.github.com> Date: Thu, 19 Oct 2023 12:14:26 -0700 Subject: [PATCH 8/8] Fix CI (#374) --- .github/workflows/ci.yml | 14 +------------- 1 file changed, 1 insertion(+), 13 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 649e6025a..da67d8cf0 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -31,19 +31,7 @@ jobs: - run: cd cedar-policy-core ; cargo test --no-default-features --verbose - run: cd cedar-policy-formatter ; cargo test --no-default-features --verbose - run: cd cedar-policy-validator ; cargo test --no-default-features --verbose - - cargo_audit: - name: Cargo Audit - runs-on: ubuntu-latest - strategy: - matrix: - toolchain: - - stable - steps: - - uses: actions/checkout@v3 - - run: rustup update ${{ matrix.toolchain }} && rustup default ${{ matrix.toolchain }} - - run: cargo install cargo-audit - - run: cargo audit --deny warnings + - run: cargo audit --deny warnings # For some reason this hangs if you don't cargo build first cargo_semver_checks: name: Cargo SemVer Checks