Skip to content

feat(dir): add ownership claim referrer, search, and CLI (v1)#1478

Open
vivekkrishna wants to merge 2 commits into
agntcy:mainfrom
vivekkrishna:feat/ownership-v1
Open

feat(dir): add ownership claim referrer, search, and CLI (v1)#1478
vivekkrishna wants to merge 2 commits into
agntcy:mainfrom
vivekkrishna:feat/ownership-v1

Conversation

@vivekkrishna
Copy link
Copy Markdown
Contributor

Summary

  • Introduces single-source-of-truth record ownership via OCI referrers (agntcy.dir.ownership.v1.Claim)
  • Adds owners DB table as a derived search index synced from OCI referrers
  • Enforces caller SPIFFE ID == owner_id when authentication is enabled
  • Exposes dirctl ownership claim command and --owner search filter

Changes

Proto / API

  • proto/agntcy/dir/ownership/v1/claim.proto: new Claim message (owner_id, claimed_at)
  • api/ownership/v1/claim.go: MarshalReferrer / UnmarshalReferrer + ReferrerType()
  • api/core/v1/referrer_types.go: OwnershipClaimReferrerType = "agntcy.dir.ownership.v1.Claim"
  • proto/agntcy/dir/core/v1/rules.proto: CEL whitelist extended with ownership type
  • proto/agntcy/dir/search/v1/record_query.proto: RECORD_QUERY_TYPE_OWNER = 17

Server

  • server/store/oci/types.go: OwnershipClaimArtifactMediaType + OCI type mapping
  • server/database/gorm/record.go: Owner model, AddOwner, RemoveOwners, ownership JOIN filter
  • server/database/gorm/gorm.go: Owner table migration
  • server/types/database.go: OwnershipDatabaseAPI interface
  • server/types/search.go: Owners []string field + WithOwners() filter option
  • server/database/utils/utils.go: RECORD_QUERY_TYPE_OWNERWithOwners()
  • server/controller/store.go: ownership claim enforcement + immediate DB indexing on push

Reconciler

  • reconciler/tasks/ownership/: new task that walks OCI referrers and syncs owners table
  • reconciler/config/config.go: Ownership config field + env var bindings
  • reconciler/service/service.go: ownership task registration

CLI

  • cli/cmd/ownership/ownership.go: dirctl ownership claim --record <CID> --owner <id>
  • cli/cmd/root.go: ownership command registered
  • cli/cmd/search/filters.go: --owner flag + RECORD_QUERY_TYPE_OWNER query
  • cli/cmd/daemon/config.go: ownership reconciler defaults (enabled=true, interval=5m)

Tests

  • tests/e2e/local/13_ownership_search_test.go: 7 e2e tests (all passing) covering exact match, wildcards, combined filters, and negative cases

Test plan

  • All unit tests pass (go test ./... in server, reconciler, api, cli modules)
  • Lint clean (task lint)
  • 7 ownership e2e tests pass locally with running daemon
  • Full e2e suite in CI (requires Docker for DNS validation container)

🤖 Generated with Claude Code

@vivekkrishna vivekkrishna requested a review from a team as a code owner May 9, 2026 12:27
@github-actions github-actions Bot added the size/M Denotes a PR that changes 200-999 lines label May 9, 2026
@vivekkrishna
Copy link
Copy Markdown
Contributor Author

@ramizpolic Any comments on this?

@ramizpolic
Copy link
Copy Markdown
Member

ramizpolic commented May 20, 2026

Hey @vivekkrishna, thanks for the PR. It looks good from my end, the only issue is that we are working on simplifying the interfaces and API surface in this PR #1500, and I am not sure which order to take (merge that PR, adjust this, or something else). I think that work will be done soon (end of next week). Can we hold this PR open until then and do a rebase together to merge this PR?

Alternatively, I think you don't even need any code changes for your functionality since you can already push whichever referrer you want and it should work out of box. Code snippet below

// Route based on referrer type
switch referrer.GetType() {
case corev1.SignatureReferrerType:
// TODO: validate signature
return s.pushReferrer(ctx, recordCID, referrer)
case corev1.PublicKeyReferrerType:
// TODO: validate public key
return s.pushReferrer(ctx, recordCID, referrer)
default:
// Store as generic OCI referrer
return s.pushReferrer(ctx, recordCID, referrer)
}

The only issue would be that the CLI is not available, but this can be handled through a separate CLI for your needs built with DIR Go SDK. Lets sync tomorrow/when it works for you on Slack and we can decide next steps together.

@ramizpolic ramizpolic self-requested a review May 20, 2026 21:00
@github-project-automation github-project-automation Bot moved this to Backlog in Discovery May 20, 2026
@ramizpolic ramizpolic moved this from Backlog to In Progress in Discovery May 20, 2026
@vivekkrishna
Copy link
Copy Markdown
Contributor Author

Sure @ramizpolic , I will sync up over slack.

@vivekkrishna
Copy link
Copy Markdown
Contributor Author

vivekkrishna commented May 23, 2026

Hi @ramizpolic . After analysis, merging as-is makes sense for these reasons.

The generic PushReferrer path (referrers.go:72-74) just stores a blob in OCI with no DB indexing — no owners table, no --owner search, and no foundation for manager subtree search. #1478's owners table + eager indexing + reconciler is exactly what makes ownership searchable and is the prerequisite for manager search (#1468).

On the #1500 question: planning to land #1478 on v1 PushReferrer as-is, then rebuild ownership onto the v2 ObjectStore + ObjectReferrer path once #1500 merges — does that sequencing work? Only immediate blocker is Target() added to StoreAPI in #1500 will break #1478's compile, so I'll add that method to the v1 OCI store when rebasing. Happy to hold if you'd rather do it right in v2 from the start — just didn't want to block manager search on #1500's timeline.

Comment on lines +258 to +264
var ownershipClaim *ownershipv1.Claim

if request.GetType() == corev1.OwnershipClaimReferrerType {
claim := &ownershipv1.Claim{}
if err := claim.UnmarshalReferrer(&corev1.RecordReferrer{Data: request.GetData()}); err != nil {
errMsg := fmt.Sprintf("failed to decode ownership claim: %v", err)

Copy link
Copy Markdown
Member

@ramizpolic ramizpolic May 25, 2026

Choose a reason for hiding this comment

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

this ownership claim is currently unverifiable since the following things can happen:

  • user has write access to the OCI store (either directly or via sync)
  • user writes an ownership claim to a non-owned resource through OCI referrers (either directly or through its own OCI that gets synced to a targeted node)
  • target node indexes ownership through the reconciler
  • the resource is reported as owned by someone who actually doesnt own it

this behavior enables targeted attacks across the network since the data has 2 access points:

  • unsafe route through OCI that doesnt get verified in the reconciler
  • secure route through the controller that does get verified

my concern here is that the ownership claims are not verifiable. we already support verifiable claims through a public key and keyless OIDC signing (dirctl sign) via COSIGN which gets verified and indexed into the database. I am not sure if we currently have a mechanism to search over this data, but it is present and may only need query params to be exposed on the client-side. perhaps @paralta can assist on this question

i think a secure/verifiable way to enable this functionality is to add support for SPIFFE-based signing. this would provide all the things you need, and i would happily merge it. it would also be a super important feature for us, not just for local use-case, but across the (insecure) network.

could you please adjust the implementation according to the notes above and we can merge it right away

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the input @ramizpolic , addressed with new commit.

vivekkrishna pushed a commit to vivekkrishna/dir that referenced this pull request May 26, 2026
Addresses ramizpolic's security comment on PR agntcy#1478. Without signing,
any caller with OCI write access could forge ownership claims; the
reconciler would index them blindly.

Changes:
- proto: add signature (bytes) and certificate (bytes) fields to Claim
- api/ownership/v1/sign.go: SignClaim, VerifyClaim, IsSigned; supports
  ECDSA (P256/P384/P521) and RSA; canonical payload SHA-256(owner_id:claimed_at)
- cli: --key / --cert flags on ownership claim; identity mismatch caught
  client-side before any network call
- reconciler: TrustedCACertFile config; verifyClaim() gate before db.AddOwner();
  unsigned claims accepted with warning in dev mode (no SPIFFE configured)
- server/controller: indexOwnershipClaim() helper verifies signed claims
  before eager db.AddOwner() so tampered claims are rejected at push time
- tests: GenerateSpiffeTestCert helper (pure crypto/x509, no SPIRE needed);
  14_ownership_signing_test.go (happy path, identity mismatch, idempotent re-push);
  5 unit tests covering round-trip, tamper detection, and unsigned-claim rejection
@github-actions github-actions Bot added size/L Denotes a PR that changes 1000-1999 lines and removed size/M Denotes a PR that changes 200-999 lines labels May 26, 2026
vivekkrishna pushed a commit to vivekkrishna/dir that referenced this pull request May 26, 2026
Addresses ramizpolic's security comment on PR agntcy#1478. Without signing,
any caller with OCI write access could forge ownership claims; the
reconciler would index them blindly.

Changes:
- proto: add signature (bytes) and certificate (bytes) fields to Claim
- api/ownership/v1/sign.go: SignClaim, VerifyClaim, IsSigned; supports
  ECDSA (P256/P384/P521) and RSA; canonical payload SHA-256(owner_id:claimed_at)
- cli: --key / --cert flags on ownership claim; identity mismatch caught
  client-side before any network call
- reconciler: TrustedCACertFile config; verifyClaim() gate before db.AddOwner();
  unsigned claims accepted with warning in dev mode (no SPIFFE configured)
- server/controller: indexOwnershipClaim() helper verifies signed claims
  before eager db.AddOwner() so tampered claims are rejected at push time
- tests: GenerateSpiffeTestCert helper (pure crypto/x509, no SPIRE needed);
  14_ownership_signing_test.go (happy path, identity mismatch, idempotent re-push);
  5 unit tests covering round-trip, tamper detection, and unsigned-claim rejection

Signed-off-by: Vivek Krishna Choppa <vivekkrishnachoppa@gmail.com>
Vivek Krishna Choppa added 2 commits May 27, 2026 12:38
Introduces a single-source-of-truth ownership model using OCI referrers:

- api: add OwnershipClaim proto + MarshalReferrer/UnmarshalReferrer
- api/core: add OwnershipClaimReferrerType constant and CEL whitelist entry
- server/store/oci: add OwnershipClaimArtifactMediaType mapping
- server/database: add owners table (Owner model, AddOwner, RemoveOwners, WithOwners filter)
- server/controller: enforce caller SPIFFE ID == owner_id; index claim on push
- reconciler: add ownership task to walk referrers and sync owners table
- cli: add dirctl ownership claim command and --owner search filter
- cli/daemon: register ownership reconciler defaults
- tests/e2e: add ownership-based search test suite (7 cases, all passing)

Signed-off-by: Vivek Krishna Choppa <vivekkrishnachoppa@gmail.com>
Addresses ramizpolic's security comment on PR agntcy#1478. Without signing,
any caller with OCI write access could forge ownership claims; the
reconciler would index them blindly.

Changes:
- proto: add signature (bytes) and certificate (bytes) fields to Claim
- api/ownership/v1/sign.go: SignClaim, VerifyClaim, IsSigned; supports
  ECDSA (P256/P384/P521) and RSA; canonical payload SHA-256(owner_id:claimed_at)
- cli: --key / --cert flags on ownership claim; identity mismatch caught
  client-side before any network call
- reconciler: TrustedCACertFile config; verifyClaim() gate before db.AddOwner();
  unsigned claims accepted with warning in dev mode (no SPIFFE configured)
- server/controller: indexOwnershipClaim() helper verifies signed claims
  before eager db.AddOwner() so tampered claims are rejected at push time
- tests: GenerateSpiffeTestCert helper (pure crypto/x509, no SPIRE needed);
  14_ownership_signing_test.go (happy path, identity mismatch, idempotent re-push);
  5 unit tests covering round-trip, tamper detection, and unsigned-claim rejection

Signed-off-by: Vivek Krishna Choppa <vivekkrishnachoppa@gmail.com>
@vivekkrishna
Copy link
Copy Markdown
Contributor Author

should we add this 1.5 milestone?

@ramizpolic ramizpolic added this to the [WIP] DIR v1.5 milestone May 29, 2026
@ramizpolic ramizpolic linked an issue May 29, 2026 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/cli area/dir/server size/L Denotes a PR that changes 1000-1999 lines

Projects

Status: In Progress

Development

Successfully merging this pull request may close these issues.

[WIP] Support SPIFFE-based record identifiers

2 participants