Add policy IDs to RBAC and return matched policy name

- Replace Vec with BTreeMap for policies in HttpRbac and NetworkRbac
- Update is_permitted to return (bool, Option<CompactString>) with matched policy
- Update tests and conversions to use policy IDs
- Include policy ID in EventKind::RbacAccessDenied and response code details

Signed-off-by: Nicola Bonelli <nicola.bonelli@huawei-partners.com>

Author: awgn

orion-configuration/src/config/network_filters/http_connection_manager/http_filters/http_rbac.rs

@@ -15,18 +15,18 @@
 //
 //
 
+use std::collections::BTreeMap;
+
 use crate::config::network_filters::{http_connection_manager::header_matcher::HeaderMatcher, network_rbac::Action};
+use compact_str::CompactString;
 use http::Request;
 use serde::{Deserialize, Serialize};
 use tracing::debug;
 
 #[derive(Debug, Clone, Deserialize, Serialize, PartialEq, Eq)]
 pub struct HttpRbac {
     pub action: Action,
-    //todo(hayley): replace vec with std::collections::BTreeMap
-    // and include the policy name as Envoy says to apply them
-    // in lexical order
-    pub policies: Vec<Policy>,
+    pub policies: BTreeMap<CompactString, Policy>,
 }
 
 //since we support different permission and principals for http vs network rbac, this struct is different from the network rbac
@@ -78,13 +78,16 @@ impl Policy {
 }
 
 impl HttpRbac {
-    pub fn is_permitted<B>(&self, req: &Request<B>) -> bool {
-        let is_enforced = self.policies.iter().any(|p| p.enforce(req));
-        debug!("Rule is enforced {is_enforced}");
-        match self.action {
-            Action::Allow => is_enforced,
-            Action::Deny => !is_enforced,
-        }
+    pub fn is_permitted<B>(&self, req: &Request<B>) -> (bool, Option<CompactString>) {
+        // policies are applied in the lexicographical order of their keys
+        let enforced_policy = self.policies.iter().find(|(_, p)| p.enforce(req)).map(|(name, _)| name.clone());
+        let permitted = match self.action {
+            Action::Allow => enforced_policy.is_some(),
+            Action::Deny => enforced_policy.is_none(),
+        };
+
+        debug!("HttpRbac: rule is enforced by {enforced_policy:?} with action: {:?} -> permitted {permitted}", self.action);
+        (permitted, enforced_policy)
     }
 }
 
@@ -115,17 +118,21 @@ mod rbac_tests {
         let permission = Permission::Any;
         let principal = Principal::Any;
         let policy = Policy { permissions: vec![permission], principals: vec![principal] };
-        let rbac_rule = HttpRbac { action: Action::Allow, policies: vec![policy] };
-        assert!(rbac_rule.is_permitted(&create_host_request("blah.com")));
+        let rbac_rule = HttpRbac { action: Action::Allow, policies: vec![("my-id".into(), policy)].into_iter().collect() };
+        let (permitted, rule) = rbac_rule.is_permitted(&create_host_request("blah.com"));
+        assert!(permitted);
+        assert_eq!(rule, Some("my-id".into()));
     }
     #[test]
     fn rule_test_allow_host_permission_any_principal() {
         let host = "blah.com";
         let permission = Permission::Header(create_host_matcher(host));
         let principal = Principal::Any;
         let policy = Policy { permissions: vec![permission], principals: vec![principal] };
-        let rbac_rule = HttpRbac { action: Action::Allow, policies: vec![policy] };
-        assert!(rbac_rule.is_permitted(&create_host_request(host)));
+        let rbac_rule = HttpRbac { action: Action::Allow, policies: vec![("my-id".into(), policy)].into_iter().collect() };
+        let (permitted, rule) = rbac_rule.is_permitted(&create_host_request(host));
+        assert!(permitted);
+        assert_eq!(rule, Some("my-id".into()));
     }
 
     #[test]
@@ -135,8 +142,10 @@ mod rbac_tests {
         let permission2 = Permission::Any;
         let principal = Principal::Any;
         let policy = Policy { permissions: vec![permission1, permission2], principals: vec![principal] };
-        let rbac_rule = HttpRbac { action: Action::Allow, policies: vec![policy] };
-        assert!(rbac_rule.is_permitted(&create_host_request(host)));
+        let rbac_rule = HttpRbac { action: Action::Allow, policies: vec![("my-id".into(), policy)].into_iter().collect() };
+        let (permitted, rule) = rbac_rule.is_permitted(&create_host_request(host));
+        assert!(permitted);
+        assert_eq!(rule, Some("my-id".into()));
     }
 
     #[test]
@@ -145,26 +154,34 @@ mod rbac_tests {
         let permission = Permission::Header(create_host_matcher(host));
         let principal = Principal::Header(create_host_matcher(host));
         let policy = Policy { permissions: vec![permission], principals: vec![principal] };
-        let rbac_rule = HttpRbac { action: Action::Allow, policies: vec![policy] };
-        assert!(rbac_rule.is_permitted(&create_host_request(host)));
+        let rbac_rule = HttpRbac { action: Action::Allow, policies: vec![("my-id".into(), policy)].into_iter().collect() };
+        let (permitted, rule) = rbac_rule.is_permitted(&create_host_request(host));
+        assert!(permitted);
+        assert_eq!(rule, Some("my-id".into()));
     }
 
     #[test]
     fn rule_test_deny_any() {
+        let host = "blah.com";
         let permission = Permission::Any;
         let principal = Principal::Any;
         let policy = Policy { permissions: vec![permission], principals: vec![principal] };
-        let rbac_rule = HttpRbac { action: Action::Deny, policies: vec![policy] };
-        assert!(!rbac_rule.is_permitted(&create_host_request("blah.com")));
+        let rbac_rule = HttpRbac { action: Action::Deny, policies: vec![("my-id".into(), policy)].into_iter().collect() };
+        let (permitted, rule) = rbac_rule.is_permitted(&create_host_request(host));
+        assert!(!permitted);
+        assert_eq!(rule, Some("my-id".into()));
     }
+
     #[test]
     fn rule_test_deny_host_permission_any_principal() {
         let host = "blah.com";
         let permission = Permission::Header(create_host_matcher(host));
         let principal = Principal::Any;
         let policy = Policy { permissions: vec![permission], principals: vec![principal] };
-        let rbac_rule = HttpRbac { action: Action::Deny, policies: vec![policy] };
-        assert!(!rbac_rule.is_permitted(&create_host_request(host)));
+        let rbac_rule = HttpRbac { action: Action::Deny, policies: vec![("my-id".into(), policy)].into_iter().collect() };
+        let (permitted, rule) = rbac_rule.is_permitted(&create_host_request(host));
+        assert!(!permitted);
+        assert_eq!(rule, Some("my-id".into()));
     }
 
     #[test]
@@ -173,8 +190,10 @@ mod rbac_tests {
         let permission = Permission::Header(create_host_matcher(host));
         let principal = Principal::Header(create_host_matcher(host));
         let policy = Policy { permissions: vec![permission], principals: vec![principal] };
-        let rbac_rule = HttpRbac { action: Action::Deny, policies: vec![policy] };
-        assert!(!rbac_rule.is_permitted(&create_host_request(host)));
+        let rbac_rule = HttpRbac { action: Action::Deny, policies: vec![("my-id".into(), policy)].into_iter().collect() };
+        let (permitted, rule) = rbac_rule.is_permitted(&create_host_request(host));
+        assert!(!permitted);
+        assert_eq!(rule, Some("my-id".into()));
     }
 }
 
@@ -223,7 +242,10 @@ mod envoy_conversions {
             let EnvoyHttpRbac { action, policies, audit_logging_options } = envoy;
             unsupported_field!(audit_logging_options)?;
             let action = Action::try_from(action).with_node("action")?;
-            let policies = required!(policies)?.into_values().map(Policy::try_from).collect::<Result<_, _>>()?;
+            let policies = required!(policies)?
+                .into_iter()
+                .map(|(id, pol)| Policy::try_from(pol).map(|value| (id.into(), value)))
+                .collect::<Result<_, _>>()?;
             Ok(HttpRbac { action, policies })
         }
     }

orion-configuration/src/config/network_filters/network_rbac.rs

@@ -16,18 +16,16 @@
 //
 
 use crate::config::core::StringMatcher;
+use compact_str::CompactString;
 use ipnet::IpNet;
 use serde::{Deserialize, Serialize};
-use std::net::SocketAddr;
+use std::{collections::BTreeMap, net::SocketAddr};
 use tracing::debug;
 
 #[derive(Debug, Clone, Deserialize, Serialize, PartialEq, Eq)]
 pub struct NetworkRbac {
     pub action: Action,
-    //fixme(hayley): replace vec with std::collections::BTreeMap
-    // and include the policy name as Envoy says to apply them
-    // in lexical order
-    pub policies: Vec<Policy>,
+    pub policies: BTreeMap<CompactString, Policy>,
 }
 
 #[derive(Debug, Clone, Copy, Deserialize, Serialize, PartialEq, Eq)]
@@ -74,7 +72,7 @@ impl<'a> NetworkContext<'a> {
 }
 
 impl Permission {
-    fn is_applicable(&self, ctx: NetworkContext) -> bool {
+    fn is_applicable(&self, ctx: &NetworkContext) -> bool {
         match self {
             Self::Any => true,
             Self::DestinationIp(ip) => ip.contains(&ctx.destination.ip()),
@@ -89,7 +87,7 @@ impl Permission {
 }
 
 impl Principal {
-    fn has_principal(&self, ctx: NetworkContext) -> bool {
+    fn has_principal(&self, ctx: &NetworkContext) -> bool {
         match self {
             Principal::Any => true,
             Principal::DownstreamRemoteIp(ip) => ip.contains(&ctx.downstream.ip()),
@@ -98,7 +96,7 @@ impl Principal {
 }
 
 impl Policy {
-    fn enforce(&self, ctx: NetworkContext) -> bool {
+    fn enforce(&self, ctx: &NetworkContext) -> bool {
         let has_permission = self.permissions.iter().any(|p| p.is_applicable(ctx));
         let has_principal = self.principals.iter().any(|p| p.has_principal(ctx));
         debug!("Enforcing policy permissions {has_permission} principals {has_principal}");
@@ -107,13 +105,15 @@ impl Policy {
 }
 
 impl NetworkRbac {
-    pub fn is_permitted(&self, ctx: NetworkContext) -> bool {
-        let is_enforced = self.policies.iter().any(|p| p.enforce(ctx));
-        debug!("Rule is enforced {is_enforced}");
-        match self.action {
-            Action::Allow => is_enforced,
-            Action::Deny => !is_enforced,
-        }
+    pub fn is_permitted(&self, ctx: &NetworkContext) -> (bool, Option<CompactString>) {
+        let enforced_policy = self.policies.iter().find(|(_, p)| p.enforce(ctx)).map(|(id, _)| id.clone());
+        let permitted = match self.action {
+            Action::Allow => enforced_policy.is_some(),
+            Action::Deny => enforced_policy.is_none(),
+        };
+
+        debug!("NetworkRbac: rule is enforced by {enforced_policy:?} with action: {:?} -> permitted {permitted}", self.action);
+        (permitted, enforced_policy)
     }
 }
 
@@ -141,16 +141,20 @@ mod tests {
         let permission = Permission::Any;
         let principal = Principal::Any;
         let policy = Policy { permissions: vec![permission], principals: vec![principal] };
-        let rbac_rule = NetworkRbac { action: Action::Allow, policies: vec![policy] };
-        assert!(rbac_rule.is_permitted(create_network_context("127.0.0.1", 8000, "127.0.0.1", 9000, None)));
+        let rbac_rule = NetworkRbac { action: Action::Allow, policies: vec![("my-id".into(), policy)].into_iter().collect() };
+        let (permitted, rule) = rbac_rule.is_permitted(&create_network_context("127.0.0.1", 8000, "127.0.0.1", 9000, None));
+        assert!(permitted);
+        assert_eq!(rule, Some("my-id".into()));
     }
     #[test]
     fn rule_test_allow_dest_ip_permission_any_principal() {
         let permission = Permission::DestinationIp("127.0.0.0/24".parse().unwrap());
         let principal = Principal::Any;
         let policy = Policy { permissions: vec![permission], principals: vec![principal] };
-        let rbac_rule = NetworkRbac { action: Action::Allow, policies: vec![policy] };
-        assert!(rbac_rule.is_permitted(create_network_context("127.0.0.1", 8000, "127.0.0.1", 9000, None)));
+        let rbac_rule = NetworkRbac { action: Action::Allow, policies: vec![("my-id".into(), policy)].into_iter().collect() };
+        let (permitted, rule) = rbac_rule.is_permitted(&create_network_context("127.0.0.1", 8000, "127.0.0.1", 9000, None));
+        assert!(permitted);
+        assert_eq!(rule, Some("my-id".into()));
     }
 
     #[test]
@@ -159,8 +163,10 @@ mod tests {
         let permission1 = Permission::Any;
         let principal = Principal::Any;
         let policy = Policy { permissions: vec![permission1, permission2], principals: vec![principal] };
-        let rbac_rule = NetworkRbac { action: Action::Allow, policies: vec![policy] };
-        assert!(rbac_rule.is_permitted(create_network_context("127.0.0.1", 8000, "127.0.0.1", 9000, None)));
+        let rbac_rule = NetworkRbac { action: Action::Allow, policies: vec![("my-id".into(),policy)].into_iter().collect() };
+        let (permitted, rule) =  rbac_rule.is_permitted(&create_network_context("127.0.0.1", 8000, "127.0.0.1", 9000, None));
+        assert!(permitted);
+        assert_eq!(rule, Some("my-id".into()));
     }
 
     #[test]
@@ -169,17 +175,21 @@ mod tests {
         let permission1 = Permission::Any;
         let principal = Principal::Any;
         let policy = Policy { permissions: vec![permission1, permission2], principals: vec![principal] };
-        let rbac_rule = NetworkRbac { action: Action::Allow, policies: vec![policy] };
-        assert!(rbac_rule.is_permitted(create_network_context("127.0.0.1", 8000, "127.0.0.1", 9000, None)));
+        let rbac_rule = NetworkRbac { action: Action::Allow, policies: vec![("my-id".into(),policy)].into_iter().collect() };
+        let (permitted, rule) = rbac_rule.is_permitted(&create_network_context("127.0.0.1", 8000, "127.0.0.1", 9000, None));
+        assert!(permitted);
+        assert_eq!(rule, Some("my-id".into()));
     }
 
     #[test]
     fn rule_test_allow_dest_ip_permission_src_ip_principal() {
         let permission = Permission::DestinationIp("127.0.0.0/24".parse().unwrap());
         let principal = Principal::DownstreamRemoteIp("127.0.0.0/24".parse().unwrap());
         let policy = Policy { permissions: vec![permission], principals: vec![principal] };
-        let rbac_rule = NetworkRbac { action: Action::Allow, policies: vec![policy] };
-        assert!(rbac_rule.is_permitted(create_network_context("127.0.0.1", 8000, "127.0.0.1", 9000, None)));
+        let rbac_rule = NetworkRbac { action: Action::Allow, policies: vec![("my-id".into(),policy)].into_iter().collect() };
+        let (permitted, rule) = rbac_rule.is_permitted(&create_network_context("127.0.0.1", 8000, "127.0.0.1", 9000, None));
+        assert!(permitted);
+        assert_eq!(rule, Some("my-id".into()));
     }
 }
 
@@ -205,7 +215,10 @@ mod envoy_conversions {
             let EnvoyRbac { action, policies, audit_logging_options } = envoy;
             unsupported_field!(audit_logging_options)?;
             let action = Action::try_from(action).with_node("action")?;
-            let policies = required!(policies)?.into_values().map(Policy::try_from).collect::<Result<_, _>>()?;
+            let policies = required!(policies)?
+                .into_iter()
+                .map(|(id, pol)| Policy::try_from(pol).map(|value| (id.into(), value)))
+                .collect::<Result<_, _>>()?;
             Ok(NetworkRbac { action, policies })
         }
     }

orion-error/src/lib.rs

@@ -227,7 +227,7 @@ impl Context for Error {
     }
 }
 
-impl AsRef<(dyn ErrorTrait + Send + Sync + 'static)> for Error {
+impl AsRef<dyn ErrorTrait + Send + Sync + 'static> for Error {
     fn as_ref(&self) -> &(dyn ErrorTrait + Send + Sync + 'static) {
         &self.0
     }

orion-lib/src/event_error.rs

@@ -17,7 +17,9 @@
 
 use http::Response;
 use orion_format::types::ResponseFlags as FmtResponseFlags;
+use orion_interner::StringInterner;
 use std::error::Error as ErrorTrait;
+use compact_str::CompactString;
 use std::io;
 use tokio::time::error::Elapsed;
 
@@ -54,14 +56,14 @@ pub enum EventKind {
     NoHealthyUpstream,
     RouteNotFound,
     UpgradeFailed,
-    RbacAccessDenied,
+    RbacAccessDenied(CompactString),
     RateLimited,
     ViaUpstream,
 }
 
 impl EventKind {
     pub fn code_details(&self) -> Option<ResponseCodeDetails> {
-        match self {
+         match self {
             EventKind::Error(err) => match err {
                 EventError::IoError(err) => Some(ResponseCodeDetails::from(err)),
                 EventError::ConnectTimeout(_) => Some(ResponseCodeDetails("connect_timeout")),
@@ -79,7 +81,7 @@ impl EventKind {
             EventKind::NoHealthyUpstream => Some(ResponseCodeDetails("no_healthy_upstream")),
             EventKind::RouteNotFound => Some(ResponseCodeDetails("route_not_found")),
             EventKind::UpgradeFailed => Some(ResponseCodeDetails("upgrade_failed")),
-            EventKind::RbacAccessDenied => Some(ResponseCodeDetails("rbac_access_denied")),
+            EventKind::RbacAccessDenied(id) => Some(ResponseCodeDetails(format!("rbac_access_denied[{id}]").to_static_str())),
             EventKind::RateLimited => Some(ResponseCodeDetails("rate_limited")),
             EventKind::ViaUpstream => Some(ResponseCodeDetails("via_upstream")),
         }
@@ -93,7 +95,7 @@ impl EventKind {
                 EventError::RouteTimeout => Some(ConnectionTerminationDetails("route timeout was reached")),
                 _ => None,
             },
-            EventKind::RbacAccessDenied => Some(ConnectionTerminationDetails("rbac_access_denied_matched_policy")),
+            EventKind::RbacAccessDenied(id) => Some(ConnectionTerminationDetails(format!("rbac_access_denied_matched_policy[{id}]").to_static_str())),
             _ => None,
         }
     }

orion-lib/src/listeners/filterchain.rs

@@ -161,7 +161,8 @@ impl FilterchainType {
         let network_context =
             NetworkContext::new(downstream_metadata.local_address(), downstream_metadata.peer_address(), server_name);
         for rbac in rbac_filters {
-            if !rbac.is_permitted(network_context) {
+            let (permitted, _) = rbac.is_permitted(&network_context);
+            if !permitted {
                 return None;
             }
         }

orion-lib/src/listeners/http_connection_manager.rs

@@ -1154,11 +1154,12 @@ fn eval_http_finish_context(
 
 fn apply_authorization_rules<B>(rbac: &HttpRbac, req: &Request<B>) -> FilterDecision {
     debug!("Applying authorization rules {rbac:?} {:?}", &req.headers());
-    if rbac.is_permitted(req) {
+    let (permitted, enforced_policy) = rbac.is_permitted(req);
+    if permitted {
         FilterDecision::Continue
     } else {
         FilterDecision::DirectResponse(
-            SyntheticHttpResponse::forbidden(EventKind::RbacAccessDenied, "RBAC: access denied")
+            SyntheticHttpResponse::forbidden(EventKind::RbacAccessDenied(enforced_policy.unwrap_or(CompactString::new("unknown"))), "RBAC: access denied")
                 .into_response(req.version()),
         )
     }