feat(accesscontrol): add access control API for secret resolution#588
Conversation
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🔴 CRITICAL
2 findings in the new access-control proto — one fail-open security bug and one enum design issue that conflates distinct security states.
| } | ||
|
|
||
| enum Decision { | ||
| DECISION_UNSPECIFIED = 0; |
There was a problem hiding this comment.
[HIGH — CONFIRMED] DECISION_UNSPECIFIED = 0 causes fail-open: default/unset response grants access
The PR description states "(UNSPECIFIED=0 fail-closed)" but the enum layout contradicts this. In proto3, any field that is not explicitly set deserializes to its zero value. Since DECISION_UNSPECIFIED = 0 is the zero value of Decision, any CheckAccessResponse where decision is not populated will produce DECISION_UNSPECIFIED at the receiver — not DECISION_DENY.
Trigger paths:
- A future implementation that forgets to set
decisionbefore returning - A partial message (e.g. a stub or mock that returns an empty
CheckAccessResponse{}) - Deserialization of a truncated or malformed response (proto silently zero-fills missing fields)
Unless every caller explicitly treats DECISION_UNSPECIFIED as a denial, the access-control gate fails open.
Fix: Swap the zero value so the enum enforces fail-closed by default:
enum Decision {
DECISION_DENY = 0; // zero/default → deny (fail-closed)
DECISION_ALLOW = 1;
DECISION_UNSPECIFIED = 2; // or remove entirely
}Alternatively, rename DECISION_DENY = 0 directly and remove the _UNSPECIFIED variant — an access-control enum has no safe "unspecified" state.
| enum IntegrityLevel { | ||
| // SECURITY_MANDATORY_UNTRUSTED_RID (0x0000). Also the value used when the | ||
| // integrity level could not be determined. | ||
| INTEGRITY_LEVEL_UNSPECIFIED = 0; |
There was a problem hiding this comment.
[MEDIUM — LIKELY] INTEGRITY_LEVEL_UNSPECIFIED = 0 conflates "untrusted (0x0000)" with "could not be determined"
The comment on this enum value acknowledges two distinct semantic states are merged into one:
- SECURITY_MANDATORY_UNTRUSTED_RID (0x0000) — a real Windows integrity level explicitly assigned to some processes (e.g. processes that have been explicitly downgraded below the standard user level)
- "integrity level could not be determined" — an error/unknown state where the query itself failed
These are semantically different: in case 1, the integrity level is known and equals untrusted; in case 2, no information is available. Any access-control policy that needs to distinguish "positively identified as untrusted" from "unknown/query-failed" cannot do so with this enum.
Impact: In a deny-by-default policy both states are safely rejected. But if a policy ever has special handling (e.g. logging, alerting, or a separate code path) for an unresolvable integrity level vs. a confirmed-untrusted process, it will silently misclassify failed lookups as confirmed-untrusted processes.
Suggested fix: Separate the two states:
enum IntegrityLevel {
INTEGRITY_LEVEL_UNSPECIFIED = 0; // could not be determined (error/unknown)
INTEGRITY_LEVEL_UNTRUSTED = 1; // SECURITY_MANDATORY_UNTRUSTED_RID (0x0000)
INTEGRITY_LEVEL_LOW = 2; // SECURITY_MANDATORY_LOW_RID (0x1000)
INTEGRITY_LEVEL_MEDIUM = 3; // SECURITY_MANDATORY_MEDIUM_RID (0x2000)
INTEGRITY_LEVEL_HIGH = 4; // SECURITY_MANDATORY_HIGH_RID (0x3000)
INTEGRITY_LEVEL_SYSTEM = 5; // SECURITY_MANDATORY_SYSTEM_RID (0x4000)
}Using sequential values also avoids the sparse 0x1000-stepped numbering, which offers no proto-level benefit (the actual Windows RID can be documented in comments).
Add accesscontrol.v1.AccessControlService with a CheckAccess RPC to be called for every GetSecrets request. The request carries the resolver pattern plus the requesting process context (Requester), including platform-specific code-signing info via a oneof: - DarwinSigningInfo: macOS SecCode* identity (team ID, identifier, CD hash) and the raw dynamic status bitmask. - WindowsSigningInfo: Authenticode signature identity, EV flag, mandatory integrity level, and (untrusted) PE version metadata. - LinuxSigningInfo: sigstore Fulcio identity/provenance and Rekor transparency-log inclusion, one entry per signer. The response is a DENY/ALLOW Decision with DENY as the zero value, so an unset, empty, or truncated response denies access — the gate is fail-closed by construction rather than by caller discipline. This requires a buf:lint:ignore for ENUM_ZERO_VALUE_SUFFIX, since the safe default takes precedence over the naming convention.
Add accesscontrol.v1.AccessControlService with a CheckAccess RPC to be called for every GetSecrets request. The request carries the resolver pattern plus the requesting process context (Requester), including platform-specific code-signing info via a oneof:
The response is a simple ALLOW/DENY Decision (UNSPECIFIED=0 fail-closed).