From d3defcc2aaf1f00b51b0f81ade6db4e4a28bf466 Mon Sep 17 00:00:00 2001 From: Vadim Kovalyov Date: Wed, 18 Nov 2020 17:22:44 -0800 Subject: [PATCH] Policy engine proptests (#3985) In this PR I've added one proptest scenario for Policy engine. To make that work I had to introduce a method `PolicyBuilder::from_definition(definition)`, that can accept struct, instead of json string. Proptests guarder by the "proptest" feature. --- mqtt/Cargo.lock | 2 + mqtt/policy/Cargo.toml | 2 + mqtt/policy/src/core/builder.rs | 109 ++++++++++++++++---------- mqtt/policy/src/core/mod.rs | 130 ++++++++++++++++++++++++++------ mqtt/policy/src/lib.rs | 2 +- 5 files changed, 181 insertions(+), 64 deletions(-) diff --git a/mqtt/Cargo.lock b/mqtt/Cargo.lock index 751789728d8..130d2bdaf99 100644 --- a/mqtt/Cargo.lock +++ b/mqtt/Cargo.lock @@ -1546,8 +1546,10 @@ dependencies = [ name = "policy" version = "0.1.0" dependencies = [ + "itertools", "lazy_static", "matches", + "proptest", "regex", "serde", "serde_json", diff --git a/mqtt/policy/Cargo.toml b/mqtt/policy/Cargo.toml index c08edff3a58..74c3b67ca13 100644 --- a/mqtt/policy/Cargo.toml +++ b/mqtt/policy/Cargo.toml @@ -7,6 +7,7 @@ version = "0.1.0" [dependencies] lazy_static = "1" +proptest = { version = "0.9", optional = true } regex = "1" serde = { version = "1.0", features = ["derive"] } serde_json = "1.0" @@ -14,3 +15,4 @@ thiserror = "1.0" [dev-dependencies] matches = "0.1" +itertools = "0.9" \ No newline at end of file diff --git a/mqtt/policy/src/core/builder.rs b/mqtt/policy/src/core/builder.rs index 39f22078923..a9f67a6d370 100644 --- a/mqtt/policy/src/core/builder.rs +++ b/mqtt/policy/src/core/builder.rs @@ -19,12 +19,12 @@ pub struct PolicyBuilder { validator: V, matcher: M, substituter: S, - json: String, + source: Source, default_decision: Decision, } impl PolicyBuilder { - /// Constructs a `PolicyBuilder` from provided json policy definition and + /// Constructs a `PolicyBuilder` from provided json policy definition, with /// default configuration. /// /// Call to this method does not parse or validate the json, all heavy work @@ -33,7 +33,24 @@ impl PolicyBuilder json: impl Into, ) -> PolicyBuilder { PolicyBuilder { - json: json.into(), + source: Source::Json(json.into()), + validator: DefaultValidator, + matcher: DefaultResourceMatcher, + substituter: DefaultSubstituter, + default_decision: Decision::Denied, + } + } + + /// Constructs a `PolicyBuilder` from provided policy definition struct, with + /// default configuration. + /// + /// Call to this method does not validate the definition, all heavy work + /// is done in `build` method. + pub fn from_definition( + definition: PolicyDefinition, + ) -> PolicyBuilder { + PolicyBuilder { + source: Source::Definition(definition), validator: DefaultValidator, matcher: DefaultResourceMatcher, substituter: DefaultSubstituter, @@ -52,7 +69,7 @@ where /// Specifies the `PolicyValidator` to validate the policy definition. pub fn with_validator(self, validator: V1) -> PolicyBuilder { PolicyBuilder { - json: self.json, + source: self.source, validator, matcher: self.matcher, substituter: self.substituter, @@ -63,7 +80,7 @@ where /// Specifies the `ResourceMatcher` to use with `Policy`. pub fn with_matcher(self, matcher: M1) -> PolicyBuilder { PolicyBuilder { - json: self.json, + source: self.source, validator: self.validator, matcher, substituter: self.substituter, @@ -74,7 +91,7 @@ where /// Specifies the `Substituter` to use with `Policy`. pub fn with_substituter(self, substituter: S1) -> PolicyBuilder { PolicyBuilder { - json: self.json, + source: self.source, validator: self.validator, matcher: self.matcher, substituter, @@ -96,13 +113,26 @@ where /// /// Any validation errors are collected and returned as `Error::ValidationSummary`. pub fn build(self) -> Result> { - let mut definition: PolicyDefinition = PolicyDefinition::from_json(&self.json)?; + let PolicyBuilder { + validator, + matcher, + substituter, + source, + default_decision, + } = self; + + let mut definition: PolicyDefinition = match source { + Source::Json(json) => PolicyDefinition::from_json(&json)?, + Source::Definition(definition) => definition, + }; for (order, mut statement) in definition.statements.iter_mut().enumerate() { statement.order = order; } - self.validate(&definition)?; + validator + .validate(&definition) + .map_err(|e| Error::Validation(e.into()))?; let mut static_rules = Identities::new(); let mut variable_rules = Identities::new(); @@ -112,19 +142,13 @@ where } Ok(Policy { - default_decision: self.default_decision, - resource_matcher: self.matcher, - substituter: self.substituter, + default_decision, + resource_matcher: matcher, + substituter, static_rules: static_rules.0, variable_rules: variable_rules.0, }) } - - fn validate(&self, definition: &PolicyDefinition) -> Result<()> { - self.validator - .validate(definition) - .map_err(|e| Error::Validation(e.into())) - } } fn process_statement( @@ -215,11 +239,16 @@ fn is_variable_rule(value: &str) -> bool { VAR_PATTERN.is_match(value) } +enum Source { + Json(String), + Definition(PolicyDefinition), +} + /// Represents a deserialized policy definition. -#[derive(Deserialize)] +#[derive(Debug, Deserialize)] #[serde(rename_all = "camelCase")] pub struct PolicyDefinition { - statements: Vec, + pub(super) statements: Vec, } impl PolicyDefinition { @@ -236,18 +265,18 @@ impl PolicyDefinition { } /// Represents a statement in a policy definition. -#[derive(Deserialize)] +#[derive(Debug, Deserialize)] #[serde(rename_all = "camelCase")] pub struct Statement { #[serde(default)] - order: usize, + pub(super) order: usize, #[serde(default)] - description: String, - effect: Effect, - identities: Vec, - operations: Vec, + pub(super) description: String, + pub(super) effect: Effect, + pub(super) identities: Vec, + pub(super) operations: Vec, #[serde(default)] - resources: Vec, + pub(super) resources: Vec, } impl Statement { @@ -277,7 +306,7 @@ impl Statement { } /// Represents an effect on a statement. -#[derive(Deserialize, Copy, Clone)] +#[derive(Debug, Deserialize, Copy, Clone)] #[serde(rename_all = "camelCase")] pub enum Effect { Allow, @@ -291,7 +320,7 @@ mod tests { use matches::assert_matches; use crate::{ - core::{tests::build_policy, Effect as CoreEffect, EffectOrd}, + core::{tests::build_policy, EffectImpl, EffectOrd}, validator::ValidatorError, }; @@ -544,7 +573,7 @@ mod tests { assert_eq!( EffectOrd { order: 0, - effect: CoreEffect::Allow + effect: EffectImpl::Allow }, policy.static_rules["actor_a"].0["write"].0["events/telemetry"] ); @@ -553,7 +582,7 @@ mod tests { assert_eq!( EffectOrd { order: 2, - effect: CoreEffect::Allow + effect: EffectImpl::Allow }, policy.variable_rules["actor_a"].0["read"].0["{{variable}}/#"] ); @@ -591,28 +620,28 @@ mod tests { assert_eq!( policy.static_rules["actor_a"].0["write"].0["events/telemetry"], EffectOrd { - effect: CoreEffect::Allow, + effect: EffectImpl::Allow, order: 0 } ); assert_eq!( policy.static_rules["actor_a"].0["read"].0["events/telemetry"], EffectOrd { - effect: CoreEffect::Allow, + effect: EffectImpl::Allow, order: 0 } ); assert_eq!( policy.static_rules["actor_b"].0["write"].0["events/telemetry"], EffectOrd { - effect: CoreEffect::Allow, + effect: EffectImpl::Allow, order: 0 } ); assert_eq!( policy.static_rules["actor_b"].0["read"].0["events/telemetry"], EffectOrd { - effect: CoreEffect::Allow, + effect: EffectImpl::Allow, order: 0 } ); @@ -622,42 +651,42 @@ mod tests { assert_eq!( policy.variable_rules["actor_a"].0["write"].0["devices/{{variable}}/#"], EffectOrd { - effect: CoreEffect::Allow, + effect: EffectImpl::Allow, order: 0 } ); assert_eq!( policy.variable_rules["actor_a"].0["read"].0["devices/{{variable}}/#"], EffectOrd { - effect: CoreEffect::Allow, + effect: EffectImpl::Allow, order: 0 } ); assert_eq!( policy.variable_rules["actor_b"].0["write"].0["devices/{{variable}}/#"], EffectOrd { - effect: CoreEffect::Allow, + effect: EffectImpl::Allow, order: 0 } ); assert_eq!( policy.variable_rules["actor_b"].0["read"].0["devices/{{variable}}/#"], EffectOrd { - effect: CoreEffect::Allow, + effect: EffectImpl::Allow, order: 0 } ); assert_eq!( policy.variable_rules["{{var_actor}}"].0["write"].0["devices/{{variable}}/#"], EffectOrd { - effect: CoreEffect::Allow, + effect: EffectImpl::Allow, order: 0 } ); assert_eq!( policy.variable_rules["{{var_actor}}"].0["read"].0["devices/{{variable}}/#"], EffectOrd { - effect: CoreEffect::Allow, + effect: EffectImpl::Allow, order: 0 } ); diff --git a/mqtt/policy/src/core/mod.rs b/mqtt/policy/src/core/mod.rs index d2942875dce..3f753a787ae 100644 --- a/mqtt/policy/src/core/mod.rs +++ b/mqtt/policy/src/core/mod.rs @@ -7,7 +7,7 @@ use crate::errors::Result; use crate::{substituter::Substituter, Error, ResourceMatcher}; mod builder; -pub use builder::{PolicyBuilder, PolicyDefinition, Statement}; +pub use builder::{Effect, PolicyBuilder, PolicyDefinition, Statement}; /// Policy engine. Represents a read-only set of rules and can /// evaluate `Request` based on those rules. @@ -38,12 +38,12 @@ where match self.eval_static_rules(request) { // static rules not defined. Need to check variable rules. Ok(EffectOrd { - effect: Effect::Undefined, + effect: EffectImpl::Undefined, .. }) => match self.eval_variable_rules(request) { // variable rules undefined as well. Return default decision. Ok(EffectOrd { - effect: Effect::Undefined, + effect: EffectImpl::Undefined, .. }) => Ok(self.default_decision), // variable rules defined. Return the decision. @@ -55,7 +55,7 @@ where match self.eval_variable_rules(request) { // variable rules undefined. Proceed with static rule decision. Ok(EffectOrd { - effect: Effect::Undefined, + effect: EffectImpl::Undefined, .. }) => Ok(static_effect.into()), // variable rules defined. Compare priority and return. @@ -304,18 +304,8 @@ pub enum Decision { Denied, } -impl From for Decision { - fn from(effect: Effect) -> Self { - match effect { - Effect::Allow => Decision::Allowed, - Effect::Deny => Decision::Denied, - Effect::Undefined => Decision::Denied, - } - } -} - #[derive(Debug, Copy, Clone, PartialOrd, PartialEq)] -enum Effect { +enum EffectImpl { Allow, Deny, Undefined, @@ -324,18 +314,18 @@ enum Effect { #[derive(Debug, Copy, Clone, PartialEq)] struct EffectOrd { order: usize, - effect: Effect, + effect: EffectImpl, } impl EffectOrd { - pub fn new(effect: Effect, order: usize) -> Self { + pub fn new(effect: EffectImpl, order: usize) -> Self { Self { order, effect } } pub fn undefined() -> Self { Self { order: usize::MAX, - effect: Effect::Undefined, + effect: EffectImpl::Undefined, } } @@ -358,9 +348,9 @@ impl PartialOrd for EffectOrd { impl From for Decision { fn from(effect: EffectOrd) -> Self { match effect.effect { - Effect::Allow => Decision::Allowed, - Effect::Deny => Decision::Denied, - Effect::Undefined => Decision::Denied, + EffectImpl::Allow => Decision::Allowed, + EffectImpl::Deny => Decision::Denied, + EffectImpl::Undefined => Decision::Denied, } } } @@ -368,8 +358,8 @@ impl From for Decision { impl From<&Statement> for EffectOrd { fn from(statement: &Statement) -> Self { match statement.effect() { - builder::Effect::Allow => EffectOrd::new(Effect::Allow, statement.order()), - builder::Effect::Deny => EffectOrd::new(Effect::Deny, statement.order()), + builder::Effect::Allow => EffectOrd::new(EffectImpl::Allow, statement.order()), + builder::Effect::Deny => EffectOrd::new(EffectImpl::Deny, statement.order()), } } } @@ -843,4 +833,98 @@ pub(crate) mod tests { policy.starts_with(input) } } + + #[cfg(feature = "proptest")] + mod proptests { + use crate::{Decision, Effect, PolicyBuilder, PolicyDefinition, Request, Statement}; + use proptest::{collection::vec, prelude::*}; + + proptest! { + /// The goal of this test is to verify the following scenarios: + /// - PolicyBuilder does not crash. + /// - All combinations of identity/operation/resource in a statement in the definition + /// should produce expected result. + /// Since some statements can be overridden by the previous ones, + /// we can only safely verify the very first statement. + #[test] + fn policy_engine_proptest(definition in arb_policy_definition()){ + use itertools::iproduct; + + // take very first statement, which should have top priority. + let statement = &definition.statements()[0]; + let expected = match statement.effect() { + Effect::Allow => Decision::Allowed, + Effect::Deny => Decision::Denied, + }; + + // collect all combos of identity/operation/resource + // in the statement. + let requests = iproduct!( + statement.identities(), + statement.operations(), + statement.resources() + ) + .map(|item| Request::new(item.0, item.1, item.2).expect("unable to create a request")) + .collect::>(); + + let policy = PolicyBuilder::from_definition(definition) + .build() + .expect("unable to build policy from definition"); + + // evaluate and assert. + for request in requests { + assert_eq!(policy.evaluate(&request).unwrap(), expected); + } + } + } + + prop_compose! { + pub fn arb_policy_definition()( + statements in vec(arb_statement(), 1..5) + ) -> PolicyDefinition { + PolicyDefinition { + statements + } + } + } + + prop_compose! { + pub fn arb_statement()( + description in arb_description(), + effect in arb_effect(), + identities in vec(arb_identity(), 1..5), + operations in vec(arb_operation(), 1..5), + resources in vec(arb_resource(), 1..5), + ) -> Statement { + Statement{ + order: 0, + description, + effect, + identities, + operations, + resources, + } + } + } + + pub fn arb_effect() -> impl Strategy { + prop_oneof![Just(Effect::Allow), Just(Effect::Deny)] + } + + pub fn arb_description() -> impl Strategy { + "\\PC+" + } + + pub fn arb_identity() -> impl Strategy { + "(\\PC+)|(\\{\\{\\PC+\\}\\})" + } + + pub fn arb_operation() -> impl Strategy { + "\\PC+" + } + + pub fn arb_resource() -> impl Strategy { + "\\PC+(/(\\PC+|\\{\\{\\PC+\\}\\}))*" + } + } } diff --git a/mqtt/policy/src/lib.rs b/mqtt/policy/src/lib.rs index a43907427ae..40058f69826 100644 --- a/mqtt/policy/src/lib.rs +++ b/mqtt/policy/src/lib.rs @@ -17,7 +17,7 @@ mod matcher; mod substituter; mod validator; -pub use crate::core::{Decision, Policy, Request}; +pub use crate::core::{Decision, Effect, Policy, Request}; pub use crate::core::{PolicyBuilder, PolicyDefinition, Statement}; pub use crate::errors::{Error, Result}; pub use crate::matcher::{DefaultResourceMatcher, ResourceMatcher};