Update Key-value-storage and policy-engine #1131

Xynnn007 · 2025-12-25T04:02:19Z

This PR mainly lays the groundwork for the dependency in #1092; please see the specific commit message.

fitzthum

Nice. A few comments.

The change in the abstraction is a little confusing. I guess the properties of the Rego backend (setting extensions, getting data and input), etc) are too specific to implement in one generic policy trait.

Still, I feel that things could be clarified slightly. Maybe policy isn't the right name for the directory or the trait could be renamed or comments added.

Generally this seems fine though.

fitzthum · 2025-12-30T19:33:23Z

deps/key-value-storage/src/lib.rs

+pub enum SetResult {
+    Inserted,
+    AlreadyExists,
+}


Some std stuff uses an option for this i.e. return Some(val) if val was already there. Might be simpler. Up to you.

You're right, to be honest, I've been thinking about this problem all day. I considered several influencing factors:

Currently, the parameters allow for non-overwriting writes.

How to demonstrate that the value hasn't been overwritten.

If returning the existing value, we need to add another logic to get the current value, and this logic should be atomic with the existence check

The main issue, specifically regarding scenario 3 in the database context, led me to abandon the approach of returning the original value. I believe this is an imperfect outcome, requiring that if the caller wants to retrieve a value, they should call get, not set, but still it meets our needs of following PRs.

Let's leave it as it is in the PR, and welcome any ideas that require changes—as long as the upper-level caller needs.

fitzthum · 2025-12-30T19:36:15Z

deps/key-value-storage/src/local_json/mod.rs

-                source: anyhow::anyhow!("key already exists"),
-                key: key.to_string(),
-            });
+            return Ok(SetResult::AlreadyExists);


What about making a new error variant for a key that already exists? Maybe I am missing the point of this change slightly.

The reason for this is that, in scenarios where no overwriting is required, inserting an existing value should not result in an error, but rather a notification that the value was not written.

This is used when setting the default policy in AS. When the user's policy storage backend is not configured with a policy, the policy configuration is written; if it is configured, the write will not overwrite and will not report an error.

why not just return Ok if the policy already exists (and overwrite is set to false)? Maybe log an error

this is a way for upper caller to know whether the key exists while logging cannot

Why does the caller need to know?

Mostly there are two reasons

The design principle of separation of concerns: how to respond to successful/incomplete write operations should be driven by the caller, and the underlying library should faithfully return the processing result.

A typical scenario, such as when we add Prometheus indicators at a higher level, it also facilitates the statistical analysis of write operations.

Well doesn't really seem necessary to me, especially since overwrite is only set in a tiny number of cases, but I guess it doesn't matter much either way.

fitzthum · 2025-12-30T19:45:26Z

deps/policy-engine/src/lib.rs

+    /// In this case, the policy engine may need to add a suffix to the policy id to distinguish the policy.
+    /// This is also for compatibility with the existing policy setting and getting
+    /// APIs. Concretely, users do not need to specify the `.rego` suffix.
+    fn policy_suffix() -> &'static str {


Is this really necessary? Why don't we just store the policy with whatever name the user provides?

I also do not like this design too much tbo, mainly for compatibility with higher-level systems. Let me describe the scenario and I'd like to hear your opinion:

(Advantage, compatibility) For traditional localFS, if we only use keys for storage, a policy originally named default_cpu will directly use the name default_cpu instead of default_cpu.rego if we do not use this suffix.

(Disadvantage, strange for non-localFS systems) For a database as the backend storage, for example, if we have a policy, it would be strange to represent it as policy.rego in the table.

In abstract terms I would expect every storage implementation to handle that without leaking storage specifics to upper layers. For a database it would be id=$POLICY_NAME in table "policies", for a the filesystem a path /policies/$POLICY_NAME.rego, for redis it might be HGET policies $POLICY_NAME etc.

This implies that the storage has some awareness of the objects it is managing, which is fine, we don't build a generic k/v store here. there are various ways to do this, using generics and putting the suffix in a Trait:

trait Storage { fn get<T: Storable>(&self, id: &str) -> Result<T>; ... } trait Storable { fn collection_name() -> &'static str; fn suffix() -> Option<&'static str> { None } }

I would also recommend fixing the table names (i.e. making the collection names non-configurable: "policies", "reference_values", etc) and, similarly, hashmap names and subfolders on the fs. this will help with migrations later on and I'm not sure there is an upside this. in a database the schema belongs to an application (you'd use database objects, tablespaces or similar namespacing features to host multiple schemas in a DB)

It might be simpler to just escape/replace the . as needed for each backend and not have any suffix abstraction.

Either way, I agree that the we probably don't want to monkey around with the names too much in the policy code. Ideally the storage backend would handle this.

Another approach would be to add method to the storage interface to store rego specifically. This is less generic than what Magnus suggested. We might also use an enum for the collection type rather than allowing any value? Possibly the collection type and the suffix could be combined into one? Either way, I think it's a good suggestion.

I would also recommend fixing the table names (i.e. making the collection names non-configurable: "policies", "reference_values", etc) and, similarly, hashmap names and subfolders on the fs. this will help with migrations later on and I'm not sure there is an upside this. in a database the schema belongs to an application (you'd use database objects, tablespaces or similar namespacing features to host multiple schemas in a DB)

What about still keeping the configurable table names, while providing a standard configuration file template and an SQL file to initialize the entire database? The reason is that the current lib is unaware of the upper layer, and it's somewhat intrusive when we can only define table names from the upper layer.

I believe that for ordinary users, whether using a database (db) or a filesystem (fs), the only thing they need to care about is the actual content of the files stored on the backend. Certainly, it maintains some flexibility.

It might be simpler to just escape/replace the . as needed for each backend and not have any suffix abstraction.

Now users does not directly touch ., it will be automatically added to the <policy-name> as .rego. So we might not need to handle . explicitly, but only maintain a "allowed" alphabet for key names. Then the backend will handle the actual storage things transparently.

Another approach would be to add method to the storage interface to store rego specifically. This is less generic than what Magnus suggested. We might also use an enum for the collection type rather than allowing any value? Possibly the collection type and the suffix could be combined into one? Either way, I think it's a good suggestion.

Another interfae to store rego is not a good idea, tbo. As common key-value interface can meet the needs enough. What do you mean by "collection type"? Where do we keep them and what does it do? Can you share some code examples?

The reason is that the current lib is unaware of the upper layer, and it's somewhat intrusive when we can only define table names from the upper layer.

i know it's tempting, but IMO we can (and should) couple the storage implementation to the application schema (that means that the code interfacing with the db, filesystem is aware of the application objects). the database or the filesystem is a storage abstraction, we don't need an abstraction on top of those (a generic key/value storage, applicable to everything).

A database schema is an implementation detail of an application that a user shouldn't be concerned about. Making it a configuration surface will increase complexity (without a good use case, at least I'm not aware of one). Migration scripts (*.up.sql, etc) now have to be templated and process configuration to enforce constraints (you cannot use the same table for policy and reference_values, etc...)

Let me imagine a user configuration process following your line of thought. For an all-in-one KBS, the user only needs to configure a data-backend or similar option in the KBS configuration file that specifies the db (address/port/pswd/..) or fs(path). And the table names and directory names of underlying items will be hardcoded.

For microservice deployment models, RVPS, AS, and KBS will all have such an option. Is it reasonable for me to push things forward in this direction?

yes, however the configuration of the storage all-in-one vs microservices is a discrete issue.

in theory every (micro)service should have its own schema and maybe a user might want to use a different storage backend on KBS and on RVSP. but, that might also be a valid choice for all-in-one. I think it's fair to then prescribe a microservice deployment model to the user.

A user might also want to have their KBS secrets backed by a file storage (mounted from some secret vault), but the KBS policies should be in a database. so... now we have per-class storage configuration...

I don't think this all this has to be addressed initially; we can iterate from restricted to more flexible configuration options later, when concrete use cases are being requested by users. as long as we don't have a large api/config surface to support this is easy, because it's an internal refactoring. once an api or config surface is exposed it's harder to iterate.

fitzthum · 2025-12-30T19:51:52Z

deps/policy-engine/src/lib.rs

    /// Concrete policy engine backend may handle the policy in different ways.
    pub async fn set_policy(&self, policy_id: &str, policy: &str, overwrite: bool) -> Result<()> {
        let params = SetParameters { overwrite };
+        let policy_id = format!("{}{}", policy_id, T::policy_suffix());


Are we doing anything to make sure that there are no collisions between policies and other things in KV storage like resources?

Good question. Actually, for the database as the backend, each specific configuration point can define a table (with default values if not configured). We can store AS policy, KBS policy, resources, etc., on different tables within the same database to prevent cross-domain conflicts.

deps/policy-engine/src/policy/rego.rs

Xynnn007 · 2025-12-31T07:26:43Z

The change in the abstraction is a little confusing. I guess the properties of the Rego backend (setting extensions, getting data and input), etc) are too specific to implement in one generic policy trait.

Yes. Especially extensions.

mythi · 2025-12-31T07:37:56Z

note: this PR does not trigger CoCo-AS Rust Suites tests at all.

- Add SetResult enum with Inserted and AlreadyExists variants - Update KeyValueStorage::set() to return Result<SetResult> instead of Result<()> - Change behavior: return AlreadyExists instead of error when key exists and overwrite=false - Update all storage implementations (LocalFs, LocalJson, Memory, Postgres) - Update PolicyEngine::set_policy() to handle new return type This change allows callers to distinguish between successful insertion and existing key scenarios without treating 'already exists' as an error. Signed-off-by: Xynnn007 <[email protected]>

…pport This commit removes the Engine trait abstraction in favor of concrete, scenario-specific implementations. The rationale is: 1. Different policy engines may have different entry parameters and APIs, making a unified trait interface impractical and overly complex. 2. Policy engines are tightly coupled to their specific use cases. For example, the AS (Attestation Service) for EAR token issuance uses Rego policies, and there's no foreseeable need to support other policy languages in the near future. 3. By creating concrete engine instances at their usage sites and encapsulating the implementation details, we achieve better type safety and clearer code organization without unnecessary abstraction. To achieve these goals, this commit - Refactor PolicyEngine from trait-based to generic design (PolicyEngine<T>) - Remove Engine trait and PolicyType enum - Enhance Regorus engine with extension support - Add ExtensionWrapper struct for Regorus extensions - Make data parameter optional in Regorus::evaluate - Add AddRegorusExtensionFailed error variant - Update error comments from OPA to Regorus terminology - Move engine-specific methods to PolicyEngine<Regorus> implementation Signed-off-by: Xynnn007 <[email protected]>

mkulke · 2026-01-02T11:42:50Z

deps/policy-engine/src/lib.rs

+        let policies = policies
+            .into_iter()
+            .filter_map(|policy| {
+                policy
+                    .strip_suffix(T::policy_suffix())
+                    .map(|policy| policy.to_string())
+            })
+            .collect();


if we want to drop all suffixes regardless of whether they exist or not we could use:

policies = policies.iter().map(|p| p.trim_end_matches(T::policy_suffix()).into()).collect();

this is slightly different than discarding all policies that do not have a suffix. I'm not sure either approach is a good idea.

if we encounter something that we do not expect (there's is a policy that should have a suffix, but there's none) we should bail out with an error, in my opinion. being forgiving (silently dropping) on faulty input data is almost never a good idea, especially for security-sensitive applications.

Good point. Let me take your advice (or not) once we have a conclusion on #1131 (comment) - could you share some idea on that?

I like what Magnus suggested regarding a collection on the other thread. One interesting idea would be to use a URI (like the resource URI) to point to things in kv storage. We could use the first part to delineate the type of resource. Not sure if this is optimal, but just throwing it out there.

Xynnn007 marked this pull request as ready for review December 25, 2025 07:53

Xynnn007 requested a review from a team as a code owner December 25, 2025 07:53

fitzthum reviewed Dec 30, 2025

View reviewed changes

Xynnn007 force-pushed the kv/set branch from 018fd09 to d47449d Compare December 31, 2025 07:26

Xynnn007 added 2 commits December 31, 2025 16:14

Xynnn007 force-pushed the kv/set branch from 1181f81 to c282403 Compare December 31, 2025 08:15

mkulke reviewed Jan 2, 2026

View reviewed changes

Update Key-value-storage and policy-engine #1131

Are you sure you want to change the base?

Update Key-value-storage and policy-engine #1131

Uh oh!

Conversation

Xynnn007 commented Dec 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fitzthum left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fitzthum Jan 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Xynnn007 Jan 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Xynnn007 commented Dec 31, 2025

Uh oh!

mythi commented Dec 31, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Xynnn007 commented Dec 25, 2025 •

edited

Loading

fitzthum Jan 5, 2026 •

edited

Loading

Xynnn007 Jan 6, 2026 •

edited

Loading