feat(jans-cedarling): make Cedar schema optional#14214
Conversation
Add CEDARLING_STRICT_SCHEMA_VALIDATION (default enabled) bootstrap property. When disabled, Cedarling starts and authorizes without a Cedar schema — entities built with None, is_authorized called without schema. Key changes: - LoadedPolicyStore.schema: String → Option<String> - PolicyStore.schema: CedarSchema → Option<CedarSchema> - LegacyPolicyStore.schema: LegacyCedarSchema → Option<LegacyCedarSchema> - build_context split into dispatcher + with/without_schema variants - All authz paths pass Option<&Schema> for entity building / context - Legacy & new format loaders all enforce strict flag consistently - WARN logged when disabled but schema present Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
…perty Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
fix clippy issues Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdd CEDARLING_STRICT_SCHEMA_VALIDATION feature toggle (default enabled); make policy-store schema optional and thread strict-schema flag through loaders, conversion, entity/context building, request construction, service wiring, examples, and tests to support both strict and schemaless modes. ChangesStrict Schema Validation Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
mo-auto
added
area-documentation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
jans-cedarling/cedarling/src/common/policy_store/manager.rs (1)
632-665: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winConsider adding test coverage for
strict_schema_validation=falsepath.The tests verify strict mode behavior, but there's no test for the schemaless path (
strict_schema_validation=falsewithschema: None). This is a new feature behavior that should be tested.♻️ Suggested test for schemaless mode
#[test] fn test_convert_to_legacy_schemaless_mode() { let loaded = LoadedPolicyStore { metadata: create_test_metadata(), schema: None, // No schema provided policies: vec![PolicyFile { name: "test.cedar".to_string(), content: "permit(principal, action, resource);".to_string(), }], templates: vec![], entities: vec![], trusted_issuers: vec![], }; // With strict=false, missing schema should succeed let result = PolicyStoreManager::convert_to_legacy(loaded.clone(), false); assert!(result.is_ok(), "Schemaless mode should succeed: {:?}", result.err()); let store = result.unwrap(); assert!(store.schema.is_none(), "Schema should be None in schemaless mode"); } #[test] fn test_convert_to_legacy_strict_requires_schema() { let loaded = LoadedPolicyStore { metadata: create_test_metadata(), schema: None, // No schema provided policies: vec![PolicyFile { name: "test.cedar".to_string(), content: "permit(principal, action, resource);".to_string(), }], templates: vec![], entities: vec![], trusted_issuers: vec![], }; // With strict=true, missing schema should fail let result = PolicyStoreManager::convert_to_legacy(loaded, true); let err = result.expect_err("Strict mode should fail without schema"); assert!( matches!(err, ConversionError::SchemaConversion(_)), "Expected SchemaConversion error, got: {err:?}" ); }Also applies to: 667-720
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@jans-cedarling/cedarling/src/common/policy_store/manager.rs` around lines 632 - 665, Add unit tests covering the schemaless path for PolicyStoreManager::convert_to_legacy: create a LoadedPolicyStore with schema: None and a minimal PolicyFile, then assert convert_to_legacy(loaded.clone(), false) returns Ok and resulting store.schema is None (test name e.g. test_convert_to_legacy_schemaless_mode); also add a test that convert_to_legacy(loaded, true) returns an Err of the SchemaConversion variant (test name e.g. test_convert_to_legacy_strict_requires_schema). Reference the LoadedPolicyStore, PolicyFile, and ConversionError enums and use PolicyStoreManager::convert_to_legacy in the new tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@jans-cedarling/cedarling/src/common/policy_store/loader_tests.rs`:
- Line 227: Replace bare assertions with ones that include explanatory failure
messages: for the assertion assert!(loaded_directory.schema.is_some()) in
loader_tests.rs update it to include a descriptive message like "expected
loaded_directory.schema to be Some but was None" and do the same for the other
bare asserts referenced (the ones around lines 1075, 1195, 1237, 1360) so each
assertion reports what was being tested and the unexpected state; locate these
by the symbol loaded_directory and the test functions in loader_tests.rs and
append an informative string as the second argument to assert! (or use
assert_eq!/assert_ne! with messages where appropriate).
In `@jans-cedarling/cedarling/src/common/policy_store/loader.rs`:
- Around line 278-286: The match on self.vfs.read_file(&schema_path) swallows
all errors; change the Err(_) arm to inspect the underlying I/O error and only
treat NotFound as Ok(None), while propagating other errors as
PolicyStoreError::FileReadError (including path and source). Update the match to
use Err(e) and check e.kind() == std::io::ErrorKind::NotFound (or the equivalent
from your VFS error type), returning Ok(None) for NotFound and mapping other
errors to PolicyStoreError::FileReadError with schema_path and the original
error as source so permission/disk errors are surfaced.
In `@jans-cedarling/cedarling/src/init/service_factory.rs`:
- Around line 130-154: The code warns when
bootstrap_config.authorization_config.strict_schema_validation is false but
still passes policy_store.schema into EntityBuilder::new, so update the schema
argument supplied to EntityBuilder::new to be None when strict_schema_validation
is disabled (instead of schema.as_deref()); locate the schema local and the call
to EntityBuilder::new and replace the passed schema with a conditional that uses
Some(schema) or schema.as_deref() only if
self.bootstrap_config.authorization_config.strict_schema_validation is true,
otherwise pass None so entity-building skips schema-based validation.
---
Outside diff comments:
In `@jans-cedarling/cedarling/src/common/policy_store/manager.rs`:
- Around line 632-665: Add unit tests covering the schemaless path for
PolicyStoreManager::convert_to_legacy: create a LoadedPolicyStore with schema:
None and a minimal PolicyFile, then assert convert_to_legacy(loaded.clone(),
false) returns Ok and resulting store.schema is None (test name e.g.
test_convert_to_legacy_schemaless_mode); also add a test that
convert_to_legacy(loaded, true) returns an Err of the SchemaConversion variant
(test name e.g. test_convert_to_legacy_strict_requires_schema). Reference the
LoadedPolicyStore, PolicyFile, and ConversionError enums and use
PolicyStoreManager::convert_to_legacy in the new tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 078f8007-75fb-4376-863b-3a110f452275
📒 Files selected for processing (21)
docs/cedarling/reference/cedarling-properties.mdjans-cedarling/cedarling/config/default_config.yamljans-cedarling/cedarling/examples/authorize_unsigned.rsjans-cedarling/cedarling/examples/lock_integration.rsjans-cedarling/cedarling/src/authz/build_ctx.rsjans-cedarling/cedarling/src/authz/mod.rsjans-cedarling/cedarling/src/bootstrap_config/authorization_config.rsjans-cedarling/cedarling/src/bootstrap_config/decode.rsjans-cedarling/cedarling/src/bootstrap_config/raw_config/config.rsjans-cedarling/cedarling/src/common/policy_store.rsjans-cedarling/cedarling/src/common/policy_store/legacy_store/mod.rsjans-cedarling/cedarling/src/common/policy_store/legacy_store/test.rsjans-cedarling/cedarling/src/common/policy_store/loader.rsjans-cedarling/cedarling/src/common/policy_store/loader_tests.rsjans-cedarling/cedarling/src/common/policy_store/log_entry.rsjans-cedarling/cedarling/src/common/policy_store/manager.rsjans-cedarling/cedarling/src/init/policy_store.rsjans-cedarling/cedarling/src/init/service_config.rsjans-cedarling/cedarling/src/init/service_factory.rsjans-cedarling/cedarling/src/tests/policy_store_loader.rsjans-cedarling/cedarling/src/tests/ssa_validation_integration.rs
haileyesus2433
left a comment
haileyesus2433
left a comment
There was a problem hiding this comment.
No end-to-end tests for schemaless authorize / authorize_unsigned
it will be nice to add this test scenarios:
- Load with
strict=false, no schema, callauthorize_unsigned, verifyPermit - Load with
strict=true, no schema, verifyPolicyStoreLoadError - Load with
strict=false, schema present, verify WARN is logged - Load with
strict=false, no schema, verify WARN is logged
It is not documented in any binding API it will be nice to update the bindings as well
| } | ||
|
|
||
| // Warn when strict schema validation is disabled but schema is present | ||
| if !self.bootstrap_config.authorization_config.strict_schema_validation |
There was a problem hiding this comment.
The implementation warns when disabled AND schema-present but is completely silent when disabled AND schema-absent which i think is the more dangerous state, because schema-present-but-not-enforced is obvious from logs while schema-absent is invisible.
There was a problem hiding this comment.
We notify the user about a potential misconfiguration (because the schema is present).
But if schema validation is disabled (intentionally; it is not the default behaviour) and the schema is not present, it looks like it what user wants.
There was a problem hiding this comment.
One more angle: we already emit warnings when CEDARLING_JWT_SIG_VALIDATION or CEDARLING_JWT_STATUS_VALIDATION are disabled (#14141) even though those are intentional opt-outs. It seems to me that schema-off is the same kind of situation. it’s a deliberate choice but it changes evaluation since we no longer do attribute-shape validation.
So instead of only warning when a schema is present but not enforced, we could just always WARN when strict mode is off and vary the message depending on the case (“schema present but not enforced” vs “no schema loaded. policies run without attribute validation”). I think that would make it consistent with how we handle the JWT validation toggles
| action: &cedar_policy::EntityUid, | ||
| pushed_data: HashMap<String, Value>, | ||
| ) -> Result<cedar_policy::Context, BuildContextError> { | ||
| let store_schema = config.policy_store.schema.as_ref() |
There was a problem hiding this comment.
The function accepts schema: &cedar_policy::Schema but fetches the JSON schema representation from config rather than deriving it from the passed parameter. The ok_or_else branch is unreachable in all current call sites because the dispatcher build_context only invokes this function when schema.is_some(), and schema_ref and config.policy_store.schema are always the same object.
Either derive json_schema from the passed schema parameter, or remove the schema parameter and derive it from config exclusively, making the contract explicit.
| // 5. Parse cedar version | ||
| let cedar_version = Self::parse_cedar_version(&loaded.metadata.cedar_version)?; | ||
|
|
||
| logger.log_any(PolicyStoreLogEntry::info(format!( |
There was a problem hiding this comment.
is there a reason why this might be removed because i think this was a useful observability signal (confirming how many policies and issuers were loaded at startup)
There was a problem hiding this comment.
Thanks, I returned the log message
Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
load_schema swallowed all read errors as Ok(None), hiding permission and disk failures. Now only NotFound maps to Ok(None); other I/O errors propagate as PolicyStoreError::FileReadError with path and source. Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
…ation is off Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
…h_schema Both `schema` (cedar_policy::Schema) and `json` (CedarSchemaJson) were derived from `config.policy_store.schema` separately, making the `schema` parameter redundant and the `ok_or_else` branch unreachable. Now everything is derived from `config` exclusively, making the contract explicit. Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
- build_ctx: no-schema context building, multi-issuer without schema - manager: convert_to_legacy with/without schema × strict true/false - policy_store: extract_first_policy_store strict validation paths - loader_tests: non-NotFound IO error propagation on schema read - legacy_store: null/missing schema field deserialization Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
- authorize_unsigned: allow, deny, no-principal, strict-fail, schema-present-flag-false - authorize_multi_issuer: single-token, strict-fail - context_data_api: pushed data without schema - test fixtures: policy-store_no_schema.yaml, policy-store-multi-issuer-no-schema.yaml Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
jans-cedarling/cedarling/src/authz/build_ctx.rs (1)
23-34:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGate schema-based context building on
strict_schema_validation.With
strict_schema_validation = falseand a loaded schema, this still routes intobuild_context_with_schema. That path callsContext::from_json_value(..., Some((schema, action))), so request context is still schema-validated and unknown actions can still fail. That breaks the PR contract for the “schema present but not enforced” mode.Proposed fix
pub(super) fn build_context( config: &AuthzConfig, request_context: Value, build_entities: &BuiltEntities, action: &cedar_policy::EntityUid, pushed_data: HashMap<String, Value>, ) -> Result<cedar_policy::Context, BuildContextError> { - if config.policy_store.schema.is_some() { + if config.authorization.strict_schema_validation && config.policy_store.schema.is_some() { build_context_with_schema(config, request_context, build_entities, action, pushed_data) } else { build_context_no_schema(request_context, pushed_data) } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@jans-cedarling/cedarling/src/authz/build_ctx.rs` around lines 23 - 34, The current build_context always chooses build_context_with_schema when a schema exists, causing validation even when strict_schema_validation is disabled; change build_context to check both that a schema is present and that strict_schema_validation is true (e.g. if config.policy_store.schema.is_some() && config.strict_schema_validation) and only then call build_context_with_schema, otherwise call build_context_no_schema; update the conditional in build_context so it references the strict_schema_validation flag on AuthzConfig and preserves existing behavior when true.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@jans-cedarling/cedarling/src/tests/authorize_without_schema.rs`:
- Around line 131-144: Replace the assert!(result.is_err(), ...) in the
test_unsigned_strict_true_missing_schema_fails_init test with
result.expect_err("init should fail: strict=true but no schema in policy store")
so the test uses expect_err on the Cedarling::new(&config).await result for
clearer failure messages; update the assertion on the local result variable
returned by Cedarling::new to call expect_err with the same descriptive message.
- Around line 234-247: Replace the generic assert!(result.is_err()) check in the
test_multi_issuer_without_schema_strict_true_fails test with an explicit
expect_err call on the result (e.g., result.expect_err("multi-issuer init should
fail: strict=true but policy store has no schema")) so the test uses expect_err
and provides the descriptive failure message; update the assertion in the
test_multi_issuer_without_schema_strict_true_fails function accordingly.
---
Outside diff comments:
In `@jans-cedarling/cedarling/src/authz/build_ctx.rs`:
- Around line 23-34: The current build_context always chooses
build_context_with_schema when a schema exists, causing validation even when
strict_schema_validation is disabled; change build_context to check both that a
schema is present and that strict_schema_validation is true (e.g. if
config.policy_store.schema.is_some() && config.strict_schema_validation) and
only then call build_context_with_schema, otherwise call
build_context_no_schema; update the conditional in build_context so it
references the strict_schema_validation flag on AuthzConfig and preserves
existing behavior when true.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8fd93212-42a4-4366-acc2-ac60303daa08
📒 Files selected for processing (14)
jans-cedarling/cedarling/src/authz/build_ctx.rsjans-cedarling/cedarling/src/authz/mod.rsjans-cedarling/cedarling/src/common/policy_store.rsjans-cedarling/cedarling/src/common/policy_store/legacy_store/test.rsjans-cedarling/cedarling/src/common/policy_store/loader.rsjans-cedarling/cedarling/src/common/policy_store/loader_tests.rsjans-cedarling/cedarling/src/common/policy_store/manager.rsjans-cedarling/cedarling/src/init/policy_store.rsjans-cedarling/cedarling/src/init/service_factory.rsjans-cedarling/cedarling/src/tests/authorize_without_schema.rsjans-cedarling/cedarling/src/tests/context_data_api.rsjans-cedarling/cedarling/src/tests/mod.rsjans-cedarling/test_files/policy-store-multi-issuer-no-schema.yamljans-cedarling/test_files/policy-store_no_schema.yaml
💤 Files with no reviewable changes (1)
- jans-cedarling/cedarling/src/authz/mod.rs
tareknaser
left a comment
tareknaser
left a comment
There was a problem hiding this comment.
Minor comments. I think we are good to go
|
|
||
| - **`CEDARLING_JWT_SIG_VALIDATION`** : `enabled` | `disabled` -- Whether to check the signature of all JWT tokens. When enabled, this requires the `iss` claim to be present in all tokens and the issuer URL must use the `https` scheme. Loopback hosts (`localhost`, `127.0.0.1`, `::1`) are allowed over plain HTTP so local development is not blocked. Default is `enabled`. **Disabling this is strongly discouraged outside of testing**: a JWT without signature validation is just plain JSON and is trivially spoofable. | ||
| - **`CEDARLING_JWT_STATUS_VALIDATION`** : `enabled` | `disabled` -- Whether to check the status of the JWT. On startup, the Cedarling should fetch and retrieve the latest Status List JWT from the `.well-known/openid-configuration` via the `status_list_endpoint` claim and cache it. See the [IETF Draft](https://datatracker.ietf.org/doc/draft-ietf-oauth-status-list/) for more info. Default is `enabled`. If the issuer does not publish a `status_list_endpoint`, status checks are skipped gracefully for that issuer. | ||
| - **`CEDARLING_STRICT_SCHEMA_VALIDATION`** : `enabled` | `disabled` -- When `enabled` (default), a Cedar schema is required and policies/entities are validated against it (current behavior). When `disabled`, Cedarling runs without schema-based validation, allowing quick-start and prototyping without maintaining a schema. If disabled and a schema is present, a warning is logged that the schema is loaded but not enforced. Default is `enabled`. |
There was a problem hiding this comment.
This explains how to turn schema off but not what you lose which is the more important part.
AFAIK, the schema is the only thing that enforces shape constraints on entities and context and action arguments. Without it, things like a typo in a policy won’t fail at load time. the policy just silently never matches. You also lose type checking on claims/attributes so mismatches can reach the engine unchecked.
It might be worth adding a line like: “Disabling this removes the load-time guard on attribute names and types. Typos in policies will silently fail to match and claim/attribute types are not validated. Keep enabled outside of prototyping”
That should be consistent with the existing “strongly discouraged outside of testing” wording used for CEDARLING_JWT_SIG_VALIDATION
| } | ||
|
|
||
| // Warn when strict schema validation is disabled but schema is present | ||
| if !self.bootstrap_config.authorization_config.strict_schema_validation |
There was a problem hiding this comment.
One more angle: we already emit warnings when CEDARLING_JWT_SIG_VALIDATION or CEDARLING_JWT_STATUS_VALIDATION are disabled (#14141) even though those are intentional opt-outs. It seems to me that schema-off is the same kind of situation. it’s a deliberate choice but it changes evaluation since we no longer do attribute-shape validation.
So instead of only warning when a schema is present but not enforced, we could just always WARN when strict mode is off and vary the message depending on the case (“schema present but not enforced” vs “no schema loaded. policies run without attribute validation”). I think that would make it consistent with how we handle the JWT validation toggles
Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
…abled Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
olehbozhok
dismissed stale reviews from coderabbitai[bot], dagregi, and tareknaser
via
9b6a9c0
5 participants
Prepare
Description
Target issue
closes #14204
Implementation Details
The feature makes Cedar schema validation optional via the new
CEDARLING_STRICT_SCHEMA_VALIDATIONbootstrap property (default:enabled).Key changes:
PolicyStore.schemais nowOption<CedarSchema>— all schema-dependent code paths dispatch onSome/Noneinstead of assuming a schema is always present.Two code paths in authorization —
build_context_with_schema(injects entity refs from the action's schema-defined context shape) andbuild_context_no_schema(merges only request context + pushed data; passesNonetoContext::from_json_value). TheRequest::builder()call also skips.schema()when no schema is available.Schema loading is lenient —
schema.cedarschemais no longer a required file in directory/cjar loaders. If absent andstrict_schema_validationisfalse, the store loads withschema: None. If absent andstrict_schema_validationistrue, loading fails with a clear error.convert_to_legacyaccepts astrict_schema_validationflag — propagated through all loader entry points (JSON, YAML, Lock, cjar, directory, archive bytes). The legacy store deserializer also allows a missing or nullschemafield.Startup warning — when
strict_schema_validationis disabled but a schema is present, a warning is logged that the schema will not be enforced.Test and Document the changes
Please check the below before submitting your PR. The PR will not be merged if there are no commits that start with
docs:to indicate documentation changes or if the below checklist is not selected.Summary by CodeRabbit
New Features
Documentation
Refactor
Tests