Add support for Rego registration policies (rego-cpp 1.2.0)#338
Add support for Rego registration policies (rego-cpp 1.2.0)#338
Conversation
|
Improvements that can be applied post 1.1.0:
A default allow-list for built-ins would be very long, so I have left this out of this PR, to be discussed separately. |
| // Use Rego policy if defined, otherwise use JS policy if defined | ||
| // If neither is defined, do not apply any policy, but reject if | ||
| // CWT issuer is present. | ||
| if (cfg.policy.policy_rego.has_value()) |
There was a problem hiding this comment.
Would it be accurate to say that this if cleanly separates the Rego and JavaScript flows, in that a ledger that only has a JavaScript policy will never hit check_for_policy_violations_rego() and therefore never enter the Rego functions in policy_engine.h? I'm asking to understand if there's enough modularization of the Rego feature.
I think there is (I don't know much about C++ project architecture) and the next level of modularization I can think of would be making Rego a build time configure option.
There was a problem hiding this comment.
It's actually the other way around, if there is a rego policy set (checked on line 292), then that's what is applied. Only if there isn't one set do we fall through to line 318, which checks if there is a policy_script (a JS policy), which gets applied then.
If neither is set, we fall through to line 309, where if there is a CWT we fail systematically. I think this (separately) ought to always be tightened, so that we always reject entries if no policy is set at all, regardless of their shape (CWT is now the standard, and I believe nobody is sending the old format). But that's a separate issue, and I understand the current logic there comes from historical practice.
There was a problem hiding this comment.
There is effectively no way to block the usage of Rego when it ships unless the one who controls the "member/admin" key forbids setting this value. In prod we have the control of this behavior where the users will not be able to set Rego because it is not exposed as a setting in control plane.
But there is another related issue which we need a good and practical answer to, how do you know which configured policy applied to a specific registration. If you have a receipt or just a transaction id it is difficult to tell. Now we have this possibility to have 2 policies in config. It might make sense to bind the policy decision to the table submission table so that the receipt would be able to point to it.
There was a problem hiding this comment.
The presence of rego does not fundamentally change the process: you need to read the ledger, and find the latest transaction that updated the configuration value in the KV. The same is true for the consortium, the constitution, OIDC JWKS, allowed code etc.
Because the current fallback is that Rego is always used if it is set, you may need to look further than one in situations where you set a Rego policy and later set a JS policy. If we are worried that this is going to happen, we can add a constraint to prevent the JS policy from being updated if a Rego policy is present. That way all updates are linear again, and a lookup for the latest value is again sufficient in all cases. @ivarprudnikov do you want me to make this change?
|
@achamayou I've opened a new pull request, #354, to work on those changes. Once the pull request is ready, I'll request review from you. |
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: achamayou <4016369+achamayou@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR adds support for evaluating registration policies written in Rego (via rego-cpp 1.2.0), alongside the existing JavaScript policy mechanism, and wires this through configuration, runtime, tests, and documentation.
Changes:
- Integrates the rego-cpp engine into the service (
policy_engine.h,main.cpp,kv_types.h, CMake) with a newpolicyRegoconfiguration field and an optionalpolicyRegoStatementLimit. - Generalises and extends configuration and functional tests to run both JavaScript and Rego variants of existing policies, including a performance regression test that compares JS vs Rego execution.
- Updates documentation, development scripts, and Docker/dev configs to describe and support Rego-based policies and the new dependency.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| app/src/policy_engine.h | Adds Rego policy support, including mapping signed statements and attestation details into Rego input, compiling/caching bundles, and evaluating policy/allow and policy/errors. |
| app/src/kv_types.h | Extends the Configuration::Policy model with policyRego and policyRegoStatementLimit, including JSON (de)serialisation helpers and a default limit helper. |
| app/src/main.cpp | Wires Rego policy evaluation into statement registration: prefers policyRego over policyScript, applies the configured statement limit, and logs timing for both Rego and JS policy checks. |
| app/constitution/scitt.js | Governance validation now also type-checks configuration.policy.policyRego as an optional string for proposals. |
| app/CMakeLists.txt | Fetches and links the rego-cpp library (via FetchContent) into the SCITT application and its unit tests, and configures related build options. |
| test/test_configuration.py | Refactors policy-related functional tests to be parameterised over language (JS/Rego), adds tests for Rego-specific behaviours (invalid modules, statement limit, trivial pass/fail), and centralises policy snippets via test/policies.py. |
| test/policies.py | Introduces reusable JS and Rego policy definitions (sample policies, invalid cases, pass/fail variants, DID:x509 and SVN-specific policies) for use across configuration tests. |
| test/test_perf.py | Extends the performance benchmark to run both JS and Rego variants of the hashv and attested service policies and increases iteration count to better characterise latency. |
| test/infra/cchost.py | Reduces the enclave log level from debug to info for the test host process, likely to reduce log volume during tests. |
| docs/configuration.md | Documents the new policyRego configuration option, describes the Rego input mapping (phdr, payload, attestation), introduces policyRegoStatementLimit, and provides Rego-based configuration examples. |
| DEVELOPMENT.md | Updates developer guidance to include an example for configuring a Rego policy and references the existing configuration workflow. |
| docker/dev-config.tmpl.json | Adds a worker_threads setting (set to 2) to the development configuration template. |
| docker/Dockerfile | Ensures git is available in the build image so rego-cpp can be fetched via CMake’s FetchContent. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@achamayou I've opened a new pull request, #359, to work on those changes. Once the pull request is ready, I'll request review from you. |
Add support for a "policyRego" field in configuration, similar to "policyScript" but allowing the definition of Rego policies.
A sample policy may be:
See documentation for more detail.
All JS policy testcases have been templatised to run a rego equivalent, including performance regression test_perf, which feeds into the bencher dashboard. Performance tests show that equivalent Rego policies cause no performance regression compared to JS.
This replaces #260.
Compared to #325, the build and supply chain story is simpler. No additional build tooling is necessary, and this does not introduce dependencies on external crates or non-Microsoft repositories.