feat: extract openshell-policy-schema as a thin crate with no proto dependency#1775
Open
feloy wants to merge 17 commits into
Open
feat: extract openshell-policy-schema as a thin crate with no proto dependency#1775feloy wants to merge 17 commits into
openshell-policy-schema as a thin crate with no proto dependency#1775feloy wants to merge 17 commits into
Conversation
feloy
requested review from
a team,
derekwaynecarr,
maxamillion and
mrunalp
as code owners
feloy
force-pushed
the
crate-policy-schema-2
branch
from
bb47e36 to
5c67177
Compare
Collaborator
|
/ok to test 5c67177 |
|
Label |
pimlock
reviewed
Jun 5, 2026
Comment on lines
+471
to
+563
| pub fn validate_policy(policy: &PolicyFile) -> std::result::Result<(), Vec<PolicyViolation>> { | ||
| let mut violations = Vec::new(); | ||
|
|
||
| // Check process identity — must be "sandbox". | ||
| // `ensure_sandbox_process_identity` should be called before this to | ||
| // fill in defaults; anything other than "sandbox" is rejected. | ||
| if let Some(ref process) = policy.process { | ||
| if process.run_as_user != "sandbox" { | ||
| violations.push(PolicyViolation::InvalidProcessIdentity { | ||
| field: "run_as_user", | ||
| value: process.run_as_user.clone(), | ||
| }); | ||
| } | ||
| if process.run_as_group != "sandbox" { | ||
| violations.push(PolicyViolation::InvalidProcessIdentity { | ||
| field: "run_as_group", | ||
| value: process.run_as_group.clone(), | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| // Check filesystem paths | ||
| if let Some(ref fs) = policy.filesystem_policy { | ||
| let total_paths = fs.read_only.len() + fs.read_write.len(); | ||
| if total_paths > MAX_FILESYSTEM_PATHS { | ||
| violations.push(PolicyViolation::TooManyPaths { count: total_paths }); | ||
| } | ||
|
|
||
| for path_str in fs.read_only.iter().chain(fs.read_write.iter()) { | ||
| if path_str.len() > MAX_PATH_LENGTH { | ||
| violations.push(PolicyViolation::FieldTooLong { | ||
| path: truncate_for_display(path_str), | ||
| length: path_str.len(), | ||
| }); | ||
| continue; | ||
| } | ||
|
|
||
| let path = Path::new(path_str); | ||
|
|
||
| if !path.has_root() { | ||
| violations.push(PolicyViolation::RelativePath { | ||
| path: path_str.clone(), | ||
| }); | ||
| } | ||
|
|
||
| if path | ||
| .components() | ||
| .any(|c| matches!(c, std::path::Component::ParentDir)) | ||
| { | ||
| violations.push(PolicyViolation::PathTraversal { | ||
| path: path_str.clone(), | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| // Only reject "/" as read-write (overly broad) | ||
| for path_str in &fs.read_write { | ||
| let normalized = path_str.trim_end_matches('/'); | ||
| if normalized.is_empty() { | ||
| // Path is "/" or "///" etc. | ||
| violations.push(PolicyViolation::OverlyBroadPath { | ||
| path: path_str.clone(), | ||
| }); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Check network policy endpoint hosts for TLD wildcards. | ||
| for (key, rule) in &policy.network_policies { | ||
| let name = if rule.name.is_empty() { | ||
| key.clone() | ||
| } else { | ||
| rule.name.clone() | ||
| }; | ||
| for ep in &rule.endpoints { | ||
| if ep.host.contains('*') && (ep.host.starts_with("*.") || ep.host.starts_with("**.")) { | ||
| let label_count = ep.host.split('.').count(); | ||
| if label_count <= 2 { | ||
| violations.push(PolicyViolation::TldWildcard { | ||
| policy_name: name.clone(), | ||
| host: ep.host.clone(), | ||
| }); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if violations.is_empty() { | ||
| Ok(()) | ||
| } else { | ||
| Err(violations) | ||
| } | ||
| } |
Collaborator
There was a problem hiding this comment.
This validation logic is now duplicated between here and what remained in openshell-policy: https://github.com/feloy/OpenShell/blob/5c6717756dc344691f820d2a70df961d25624dea/crates/openshell-policy/src/lib.rs#L439-L533
This could lead to drift when one function is updated and not the other one.
Contributor
Author
There was a problem hiding this comment.
I have added a commit to remove the duplicated logic
Collaborator
PR Review StatusValidation: This PR is project-valid because it implements linked issue #1608 by splitting dependency-light policy YAML schema handling out of the proto-backed Review findings:
Docs: Fern docs are not required for a direct user-facing workflow change in this PR, but the architecture crate-boundary note above is still needed. Next state: |
2 tasks
johntmyers
added
the
gator:in-review
Signed-off-by: Philippe Martin <phmartin@redhat.com>
Signed-off-by: Philippe Martin <phmartin@redhat.com>
Verbatim copy from crates/openshell-policy/src/lib.rs. Only visibility changes: MAX_FILESYSTEM_PATHS, MAX_PATH_LENGTH, and truncate_for_display promoted to pub(crate). Signed-off-by: Philippe Martin <phmartin@redhat.com>
Verbatim copy from crates/openshell-policy/src/lib.rs lines 681-735. Signed-off-by: Philippe Martin <phmartin@redhat.com>
Verbatim copy of function bodies from crates/openshell-policy/src/lib.rs, adapted to operate on schema types (PolicyFile, FilesystemDef, ProcessDef, etc.) instead of proto types, and using policy.filesystem_policy instead of policy.filesystem. Added Default derive to ProcessDef: the original ensure_sandbox_process_identity uses ProcessPolicy::default(), which proto-generated types provide automatically. ProcessDef needs an explicit derive to achieve the same. Signed-off-by: Philippe Martin <phmartin@redhat.com>
Ported from crates/openshell-policy/src/lib.rs tests section.
Adaptations required vs the original:
- parse_sandbox_policy/serialize_sandbox_policy -> parse_policy/serialize_policy
- policy.filesystem -> policy.filesystem_policy
- FilesystemPolicy/ProcessPolicy -> FilesystemDef/ProcessDef
- SandboxPolicy{..., network_policies: HashMap::new()} -> PolicyFile{..., BTreeMap::new()}
- rule.allow.as_ref().unwrap() -> rule.allow (L7RuleDef.allow is not an Option)
- .query[k].glob/.any proto fields -> QueryMatcherDef::Glob/Any enum matching
- NetworkPolicyRule/NetworkEndpoint ..Default::default() construction ->
parse_policy() helper (those types have no Default derive)
- parse_ports_array/parse_single_port: no port normalization in schema crate
(normalization happens in to_proto in openshell-policy)
Signed-off-by: Philippe Martin <phmartin@redhat.com>
Signed-off-by: Philippe Martin <phmartin@redhat.com>
Signed-off-by: Philippe Martin <phmartin@redhat.com>
Remove the private serde type definitions (PolicyFile, FilesystemDef, LandlockDef, ProcessDef, NetworkPolicyRuleDef, NetworkEndpointDef, GraphqlOperationDef, L7RuleDef, L7AllowDef, QueryMatcherDef, QueryAnyDef, L7DenyRuleDef, NetworkBinaryDef, is_zero, is_zero_u32) and import them from openshell-policy-schema instead. to_proto, from_proto, and all public functions are unchanged — they reference the same type names, now resolved from the schema crate. Signed-off-by: Philippe Martin <phmartin@redhat.com>
parse_sandbox_policy, serialize_sandbox_policy, sandbox_policy_to_json_value, serialize_sandbox_policy_json, load_sandbox_policy, and restrictive_default_policy now delegate to the schema crate and apply to_proto / from_proto for the proto conversion layer. No behaviour change. Signed-off-by: Philippe Martin <phmartin@redhat.com>
truncate_for_display stays in openshell-policy as a private helper for validate_sandbox_policy, since the schema crate exposes it as pub(crate) only. Signed-off-by: Philippe Martin <phmartin@redhat.com>
…erde_yml deps Remove the local PolicyViolation definition and re-export it from openshell-policy-schema. Callers importing openshell_policy::PolicyViolation are unaffected — the re-export preserves the public path. serde and serde_yml are no longer direct dependencies of openshell-policy since all YAML logic is now handled by openshell-policy-schema. Signed-off-by: Philippe Martin <phmartin@redhat.com>
Signed-off-by: Philippe Martin <phmartin@redhat.com>
Signed-off-by: Philippe Martin <phmartin@redhat.com>
Signed-off-by: Philippe Martin <phmartin@redhat.com>
Signed-off-by: Philippe Martin <phmartin@redhat.com>
Replace the duplicated validation logic in validate_sandbox_policy with a one-liner that delegates to openshell_policy_schema::validate_policy via from_proto. Remove the dead constants, truncate_for_display helper, and Path import that were only used by the now-removed logic. Replace 15 duplicate validation tests with 3 smoke tests (one per field category: process, filesystem, network) that confirm violations survive the from_proto roundtrip.
feloy
force-pushed
the
crate-policy-schema-2
branch
from
5c67177 to
a02621d
Compare
Collaborator
Re-check After Author UpdateI re-evaluated latest head Disposition: partially resolved. The duplicate validation concern is structurally addressed, but the update introduced a validation regression and two prior gator findings remain unresolved. Remaining items:
Docs: Fern docs are not required for this PR, but the architecture boundary note above is still needed. Next state: |
Collaborator
|
/ok to test a02621d |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
openshell-policyis consumed by external projects that only need YAML parsing and serialization, but it transitively pulls intonic,tonic-build, andprotobuf-src(which compiles protobuf from C++ source). This makes it unusable on Windows without MSYS2.This PR extracts the YAML serde types and all pure-Rust logic into a new
crates/openshell-policy-schemacrate whose only dependencies areserde,serde_yml,serde_json, andmiette— no proto, no gRPC.openshell-policyis updated to delegate all YAML work to the new crate and add only the proto conversion layer on top. Its public API and all existing behavior are unchanged.Related Issue
Fixes #1608
Changes
Each commit is intentionally minimal and independently reviewable.
New crate — additive only, no changes to existing code:
feat(policy-schema): create openshell-policy-schema crate skeletonCargo.toml+ emptysrc/lib.rs. Verify deps:serde,serde_yml,serde_json,miette— nothing else.feat(policy-schema): add YAML serde types (verbatim, names unchanged)openshell-policy/src/lib.rslines 37–229. Only change:struct→pub struct,enum→pub enum. No logic.feat(policy-schema): add constants and utility functionsopenshell-policy/src/lib.rs.MAX_FILESYSTEM_PATHS,MAX_PATH_LENGTH, andtruncate_for_displaypromoted from private topub(crate).feat(policy-schema): add PolicyViolation enum and Display implopenshell-policy/src/lib.rslines 681–735. No changes.feat(policy-schema): add public API functionsopenshell-policy/src/lib.rs: same logic, operating on schema types instead of proto types (policy.filesystem_policyinstead ofpolicy.filesystem,ProcessDefinstead ofProcessPolicy, etc.).Defaultadded toProcessDef— necessary because proto-generated types auto-derive it, but our serde struct does not.feat(policy-schema): add testsopenshell-policy/src/lib.rstest suite. See commit message for the full list of adaptations (enum matching instead of proto field access, no port normalization, etc.).style(policy-schema): apply rustfmt formattingWiring
openshell-policyto the schema crate:feat(policy): add openshell-policy-schema dependencyopenshell-policy/Cargo.toml.refactor(policy): import YAML serde types from openshell-policy-schemaopenshell-policy/src/lib.rs(the private type definitions) and replaces them with a singleuse openshell_policy_schema::{...}import.to_proto,from_proto, and all public functions are unchanged.refactor(policy): delegate YAML functions to openshell-policy-schemaparse_sandbox_policy,serialize_sandbox_policy,sandbox_policy_to_json_value,serialize_sandbox_policy_json,load_sandbox_policy, andrestrictive_default_policynow call the schema crate and applyto_proto/from_proto. No behaviour change.refactor(policy): delegate normalize_path to openshell-policy-schemavalidate_sandbox_policyand its helpers (truncate_for_display,MAX_FILESYSTEM_PATHS,MAX_PATH_LENGTH) intentionally stay:from_protoelides the process section when bothrun_as_userandrun_as_groupare empty strings (serialization optimization), so delegating viafrom_protowould silently pass policies that should be rejected.refactor(policy): re-export PolicyViolation from schema, drop serde/serde_yml depsPolicyViolationdefinition and re-exports it fromopenshell-policy-schema. Callers importingopenshell_policy::PolicyViolationare unaffected.serdeandserde_ymlare removed fromopenshell-policy's direct dependencies.refactor(policy): re-export path constants from openshell-policy-schemaCONTAINER_POLICY_PATHandLEGACY_CONTAINER_POLICY_PATHare re-exported from the schema crate instead of being defined locally.fix(policy): move HashMap import into test moduleHashMapis only used in tests; the module-level import was unused in non-test builds.docs(policy): update module doc comment to reflect schema crate splitdocs(agents): add openshell-policy-schema to architecture tableTesting
mise run pre-commitpassesChecklist