[ADR] Policy management#2377
Conversation
Reviewer's GuideAdds a new Architecture Decision Record (ADR 00018) specifying the initial design for policy management in Trustify, including the data model, API surface, permissions, and file/module layout for a new policy module that stores external policy references for later SBOM validation (initially via Conforma). File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- Consider using typed enums for
ValidatorKindandPolicyAuth.auth_type(instead of genericNullorString) to make invalid policy types/auth modes unrepresentable and reduce runtime validation complexity. - The
Policy.configurationfield is defined asserde_json::Valuewhile a strongly typedPolicyConfigurationstruct is specified; it would be clearer and safer to store/return the typed struct directly and only fall back to raw JSON if you explicitly plan for backend-specific extensions. - The DELETE semantics specify
204even when the policy is already deleted; if you want optimistic concurrency to matter here, you may want to clarify whether a missing resource with a matchingIfMatchshould still return204or a404to avoid surprising clients.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider using typed enums for `ValidatorKind` and `PolicyAuth.auth_type` (instead of generic `Null` or `String`) to make invalid policy types/auth modes unrepresentable and reduce runtime validation complexity.
- The `Policy.configuration` field is defined as `serde_json::Value` while a strongly typed `PolicyConfiguration` struct is specified; it would be clearer and safer to store/return the typed struct directly and only fall back to raw JSON if you explicitly plan for backend-specific extensions.
- The DELETE semantics specify `204` even when the policy is already deleted; if you want optimistic concurrency to matter here, you may want to clarify whether a missing resource with a matching `IfMatch` should still return `204` or a `404` to avoid surprising clients.
## Individual Comments
### Comment 1
<location path="docs/adrs/00018-policy-management.md" line_range="145" />
<code_context>
+The trade-off:
+
+- Validation always uses the latest policy content from the referenced branch or tag, but network failures or policy repo outages will cause execution errors.
+- For private policy repositories, authentication credentials are stored in the `configuration` JSONB column and encrypted at rest using `ring::aead` (AES-256-GCM authenticated encryption); they are never logged The `ring` crate is already a direct dependency of the project (used for digest hashing), so no new dependency is required.
+
+## Trustify API Endpoints
</code_context>
<issue_to_address>
**issue (typo):** Add missing punctuation between "logged" and "The` ring crate" to fix the run-on sentence.
The sentence should read "...they are never logged. The `ring` crate is already a direct dependency..." to avoid the run-on.
```suggestion
- For private policy repositories, authentication credentials are stored in the `configuration` JSONB column and encrypted at rest using `ring::aead` (AES-256-GCM authenticated encryption); they are never logged. The `ring` crate is already a direct dependency of the project (used for digest hashing), so no new dependency is required.
```
</issue_to_address>
### Comment 2
<location path="docs/adrs/00018-policy-management.md" line_range="280-289" />
<code_context>
+| header | `IfMatch` | `Option<String>` | ETag value, revision to update |
</code_context>
<issue_to_address>
**issue (typo):** Use the standard HTTP header spelling `If-Match` instead of `IfMatch` in the documentation tables.
In the PUT and DELETE request tables, the header is listed as `IfMatch`; please update it to `If-Match` to match the actual HTTP header name and keep the docs accurate.
Suggested implementation:
```
| header | `If-Match` | `Option<String>` | ETag value, revision to update |
```
There is also a DELETE request table mentioned in your review comment where the header is listed as `IfMatch`. Apply the same textual change in that table: replace the header name `IfMatch` with `If-Match` so both PUT and DELETE documentation consistently use the correct HTTP header spelling.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@sourcery-ai resolve |
|
@sourcery-ai dismiss |
ctron
left a comment
There was a problem hiding this comment.
Looks good. Some changes and clarifications needed.
bab44fe to
4b88e77
Compare
|
|
||
| ```rust | ||
| /// extending modules/importer/src/model/mod.rs | ||
| pub enum ImporterConfiguration { |
There was a problem hiding this comment.
Looks like a copy and paste error
There was a problem hiding this comment.
@ctron, it isn't, don't we need this in order to wrap the PolicyConfiguration under the patter of ImporterConfiguration, or may we don't need to show that part ?
| schemars::JsonSchema, | ||
| )] | ||
| #[serde(rename_all = "camelCase")] | ||
| pub struct PolicyImporter { |
There was a problem hiding this comment.
Why is this called "Importer"?
There was a problem hiding this comment.
Right, I suppose it's because it's wrapped under the ImporterConfiguration but it makes more sense to dub it PolicyConfiguration
| #[serde(rename_all = "camelCase")] | ||
| pub struct PolicyImporter { | ||
| #[serde(flatten)] | ||
| pub common: CommonImporter, |
There was a problem hiding this comment.
Looks like a copy and paste error.
There was a problem hiding this comment.
@ctron, isn't that needed to wrap the PolicyConfiguration with the ImporterConfiguration ?
| name: String, | ||
| description: String, | ||
| policy_type: PolicyType, | ||
| configuration: PolicyImporter, |
There was a problem hiding this comment.
Looks like a copy and paste error
There was a problem hiding this comment.
PolicyRequest is just the API input shape, not the stored entity.
| struct PolicyRequest { | ||
| name: String, | ||
| description: String, | ||
| policy_type: PolicyType, |
There was a problem hiding this comment.
This is splitting now the discriminator and the data into two fields. It looks like with no validation between them. Why not fully adopt the existing pattern:
trustify/modules/importer/src/model/mod.rs
Lines 177 to 186 in 1845ce8
There was a problem hiding this comment.
Right, I removed the PolicyRequest
| #[derive(Serialize, Deserialize)] | ||
| struct PolicyAuth { | ||
| #[serde(rename = "type")] | ||
| auth_type: AuthType, |
There was a problem hiding this comment.
We could just name it type (or r#type).
| /// The policy reference information | ||
| #[derive(Serialize, Deserialize)] | ||
| struct Policy { | ||
| id: Uuid, |
88fac5c to
ecbdb9a
Compare
ecbdb9a to
571bc74
Compare
This ADR provides the description of the policy management.
The policies are defined outside of Trustify, initially in Git repositories (Github, Gitlab, etc) meanwhile we need to store some "meta-data" about a given policy, such as the link to the repo, what type of policy is used for (Conforma to start with), and some extra configuration when needed such as authentication details when relevant.
The policy management is the initial building block on the road towards SBOMs validation which will come when the first policy validation solution integration (Conforma API) with Trustify will be available which will permit to validate a SBOM against a specific policy and keep track of such result.
Summary by Sourcery
Documentation: