Repository navigation
feat: add key-value database catalog - #1286
aruunkumar wants to merge 2 commits into
Conversation
✅ Deploy Preview for common-cloud-controls canceled.
|
|
Thanks for this @aruunkumar! Pinging a few folks for review. |
julianvance242
left a comment
There was a problem hiding this comment.
🤖 Multi-model review (haiku, sonnet, opus over a local worktree of the PR head 502b14c; every finding below was re-checked against source). Compile and cue vet against Gemara v1.2.0 pass for all three catalogs, imports and counts match the description, and there are no inline external-mappings.
Needs attention: two inline majors (CN06 claims TH06 without addressing it; CP15 duplicates Core CP35). The rest are inline minor items.
| - reference-id: CCC | ||
| entries: | ||
| - reference-id: CCC.KVDB.TH06 | ||
| strength: 8 |
There was a problem hiding this comment.
🤖 Major — CN06 claims CCC.KVDB.TH06 (backups disabled, delayed or failing) at strength 8, but none of its three ARs (lines 257-290) requires backups to be enabled or to succeed; they cover caller authorization, snapshot confirmation and sharing. Imported Core CN14 maps only CCC.Core.TH06. As written, TH06 is mitigated in name only. Add an AR (PITR/backups enabled, alert on failure) here or in a new control, or drop this mapping and lower the claim.
There was a problem hiding this comment.
Note that the suggestion here is just to lower the strength to clarify that this is only partial satisfaction of CCC.KVDB.TH06.
There was a problem hiding this comment.
My perspective when I wrote this was that the threat of disabling backups could be caused by unauthorized actors and hence the authorization control would address it. But I get your viewpoint, so I've downgraded the strength now.
| strength: 9 | ||
| remarks: Data Exposure Through Insecure Reads or Key Leakage | ||
|
|
||
| - id: CCC.KVDB.CN03 |
There was a problem hiding this comment.
🤖 Minor — CN03 overlaps Core CN15 (Enforce Transaction Rate Limits), which also targets CCC.Core.TH12 and is not imported here. Suggest importing CN15 and keeping CN03 only for the hot-partition ARs (AR02), so the rate-limit requirement lives in one place.
| recommendation: | | ||
| Define lag thresholds from application tolerance for stale reads and | ||
| test failover while lag is elevated to confirm recovery behavior. | ||
| - id: CCC.KVDB.CN04.AR02 |
There was a problem hiding this comment.
🤖 Minor — CN04.AR02 constrains how the application reads ("When an application requires read-after-write freshness…"), so there is no service-side state a behavioural test can evaluate. CN06.AR02 (line 269) and CN07.AR01 (line 318) have the same shape (operator confirmation / validation steps). Restate as a condition observable on the service or move to the recommendation text.
There was a problem hiding this comment.
Description rewritten.
| Ensure that concurrent updates to the same item cannot silently overwrite | ||
| each other or leave partial state. | ||
| assessment-requirements: | ||
| - id: CCC.KVDB.CN05.AR01 |
There was a problem hiding this comment.
🤖 Minor — Same root cause as CN04.AR02 (line 193): "When an application performs updates…" is application behaviour, not a service property the catalog can assess.
There was a problem hiding this comment.
Not sure how valid this point is, but the validity of this feedback will be clearer if we reformat the statement based on the requested changes I already posted.
There was a problem hiding this comment.
Description rewritten.
| recommendation: | | ||
| Use the provider's identity federation or token service to mint | ||
| short-lived data-plane tokens at workload startup. | ||
| - id: CCC.KVDB.CN08.AR03 |
There was a problem hiding this comment.
🤖 Minor — AR02 (amber/red) already requires short-lived or identity-based credentials instead of static keys, so "when static data-plane keys are used" can never be true at those tiers, and tlp-green gets no rotation requirement for static keys at all. Suggest applying this AR at tlp-green (or all tiers) so it covers the tiers where static keys are still allowed.
There was a problem hiding this comment.
Good point. I have corrected the applicability to only tlp-green.
eddie-knight
left a comment
There was a problem hiding this comment.
Hey @aruunkumar there are a couple of points that would be helpful to adjust. If they're not fixed now, it'll come up again when we do a review for idiomatic Gemara later.
- Avoid using
ANDwithin an assessment requirement; this is almost always a sign that it should be split into two reqs - When writing an assessment requirement, the noun immediately prior to
MUSTshould be the entity that is being evaluated. For example:The quick brown fox MUST jump over the lazy dog. - On threat-to-control mappings,
strengthvalues should either be unset or, if set, reflect an approximation of how thoroughly that threat is mitigated by the control. Above 5 means that it is reasonably mitigated, above 7 means it's mostly or probably mitigated, and a literal 10 means it's definitely completely mitigated. If you don't want to provide an estimate, just omit that field.
502b14c to
dd50716
Compare
|
Addressed all the comments. Please review. |
Summary
Adds a new Key-Value Database service catalog under
catalogs/database/key-value-db/, covering the key-value store category (such as AWS DynamoDB, Azure Cosmos DB, GCP Firestore, etc.) which currently does not exist. Built following the repo's capability → threat → control skill flow:The following are deferred for a later PR: Behavioral tests (
tests/) and threat→framework mapping documents (mappings/).Related issues
Checklist