Skip to content

feat(accesscontrol): add access control API for secret resolution#588

Merged
joe0BAB merged 1 commit into
mainfrom
feat/ae-api
Jul 16, 2026
Merged

feat(accesscontrol): add access control API for secret resolution#588
joe0BAB merged 1 commit into
mainfrom
feat/ae-api

Conversation

@joe0BAB

@joe0BAB joe0BAB commented Jul 16, 2026

Copy link
Copy Markdown
Collaborator

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 simple ALLOW/DENY Decision (UNSPECIFIED=0 fail-closed).

@joe0BAB
joe0BAB marked this pull request as ready for review July 16, 2026 14:36

@docker-agent docker-agent left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread x/api/accesscontrol/v1/api.proto Outdated
}

enum Decision {
DECISION_UNSPECIFIED = 0;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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 decision before 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;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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:

  1. 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)
  2. "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.
@joe0BAB
joe0BAB merged commit b1b4210 into main Jul 16, 2026
11 checks passed
@joe0BAB
joe0BAB deleted the feat/ae-api branch July 16, 2026 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants