Add Datadog credential issuance - #40
Open
santhosh-c1 wants to merge 29 commits into
Open
Conversation
Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
Connector PR Review: Add Datadog credential issuanceBlocking Issues: 0 | Suggestions: 3 | Threads Resolved: 0 Review SummaryScanned the full PR diff for security and correctness: the new Security IssuesNone found. Correctness IssuesNone found. Suggestions
Prompt for AI agents |
Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Fixes formatting on api_token.go (tab indent) and resource_types.go (trailing newline). Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Comment on lines
+79
to
+85
| if parentResourceID == nil || parentResourceID.GetResourceType() != userResourceType.Id { | ||
| return nil, status.Error(codes.InvalidArgument, "baton-datadog: the owning service account id is required to delete a service account application key") | ||
| } | ||
| serviceAccountID := parentResourceID.GetResource() | ||
| if isMalformedAPIKeyHandle(serviceAccountID) { | ||
| return nil, status.Errorf(codes.InvalidArgument, "baton-datadog: owning service account id %q is malformed", serviceAccountID) | ||
| } |
Contributor
There was a problem hiding this comment.
🟡 Suggestion: Delete hard-fails with InvalidArgument whenever parentResourceID is absent, and the doc comment above states that no C1 caller populates it today. Meanwhile baton_capabilities.json advertises CAPABILITY_RESOURCE_DELETE for this type (the SDK requires a deleter for any credential-issue secret type) and docs/connector.mdx:151 shows Revoke ✓ for service account application keys — so an issued key that C1 believes is revocable may in practice never be revocable. A fallback would close this without depending on a platform change: when parentResourceID is missing, resolve the owner via the org-level GET /api/v2/application_keys/{app_key_id} (relationships.owned_by) before deleting. If you'd rather keep the fail-closed behavior, the docs capability table should say revoke is not yet wired for this type rather than ✓.
…er call
The advertised Datadog permissions did not match what the endpoints this
connector actually calls require, per the per-operation "x-permission" blocks
in Datadog's own API spec and the published role-permission table:
- DeleteAPIKey requires api_keys_delete. api_keys_write covers only
CreateAPIKey/UpdateAPIKey ("Create and rename API Keys"), which no
advertised capability on the api-key type calls, so it is dropped rather
than making operators grant org-wide key-creation rights the connector
never exercises.
- Every service-account application-key endpoint the connector calls --
list (sync), create (issue) and delete (revoke) -- requires
service_account_write. user_access_manage covers user disable, role
management, SAML-to-role mappings and logs restriction queries, and grants
none of the three, so sync/issue/revoke were advertised against a role
Datadog answers with a 403. service_account_write is added to the
service-account-application-key type and to the user type, where
CAPABILITY_CREDENTIAL_ISSUE is registered.
baton_capabilities.json is regenerated from the built binary rather than
hand-edited, and the docs' custom-role guidance now names the same set.
applicationKeyBuilder.List drained every application-key page for every
service account inside a single call, buffering the whole org's keys before
the SDK could checkpoint, respect rate limits or cancel. It now returns one
provider page per call, keeping the users page and one child state per
discovered service account in the pagination bag, and a 403 or 404 for a
single service account is warned and skipped instead of failing the whole
sync. Both behaviours are covered by new tests.
Also drop GetAPIKey and ValidateAPIKey, which no production code and no test
referenced after the smoke-test rework, and make the smoke test's
canAuthenticate probe separate a credential rejection from a transient
failure so the revocation assertion cannot pass on a 429 or a 5xx.
Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
highb
dismissed
github-actions[bot]’s stale review
August 22, 2026 03:45
judge is not clearing reviews
…p warning Three follow-ups on the permission metadata, all in the surfaces the previous commit touched. Drop service_account_write from userResourceType. That type is registered on every install, but issuance only exists when sync-secrets is on -- the same flag that swaps credentialUserBuilder in for userBuilder and registers applicationKeyBuilder at all -- so a sync-secrets-off install was being told to grant a Datadog Admin permission no code path in that configuration can reach. baton_capabilities.json cannot express the condition directly: it is one static document generated from a connector built with SyncSecrets and SyncSchedules forced true (cmd/baton-datadog/main.go), and CapabilityPermissions has no conditional form. Scoping the permission to serviceAccountApplicationKeyResourceType, which is only registered under the flag, is how the conditionality is carried; the user type's credential_issue block already points at that type via secretResourceTypeId, so the requirement stays discoverable. This also makes the metadata agree with what docs/connector.mdx already said. Confirm service_account_write as the permission for the service-account application-key endpoints, and do not also advertise user_access_manage. Datadog's published OpenAPI spec gives each operation an "x-permission" OR list, and for ListServiceAccountApplicationKeys, CreateServiceAccountApplicationKey and DeleteServiceAccountApplicationKey that list has exactly one entry. The same spec does name several accepted permissions where several are accepted -- DisableUser and UpdateUser accept user_access_manage OR service_account_write -- so the single-entry list is meaningful rather than an omission. user_access_manage is an AuthZ oauth2 scope, a different axis from RBAC that does not apply to a connector authenticating with apiKeyAuth and appKeyAuth. Recorded in the type's doc comment so the next reader does not have to re-derive it. Sample the per-service-account skip warning. The case it exists for is a role missing service_account_write org-wide, which made it fire once per service account on every sync. It now logs the 1st, 10th and 100th occurrence and every 1000th after that, with a total_occurrences field, per criteria L7. Neither this repo nor the vendored baton-sdk ships a sampling helper, so shouldLogSampled is the smallest thing that satisfies the criteria: a pure function of the count, with the counter owned by the builder. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
applicationKeyBuilder is constructed once per connector process -- Datadog.ResourceSyncers runs inside connectorbuilder.NewConnector, which the SDK calls once at startup and whose registered syncers are reused for every sync -- so skippedServiceAccounts never reset. The first sync consumed the 1st, 10th and 100th log slots, and a second sync against the same org-wide missing service_account_write incremented the total while emitting nothing at all until it reached 1000. That is worse than the per-resource noise the sampling was added to prevent. List now zeroes the counter when the page token is empty, giving each walk its own schedule. An empty token is a safe first-walk signal here: List only returns an empty NextPageToken from bag.Marshal() with an empty bag, and the bag can only be emptied by popping the users-level state, which happens solely on an empty users page -- the end of the walk -- so no mid-walk call can carry one. A retried first page resets again, which is what a restarted walk wants. TestApplicationKeyBuilderListSamplesSkipWarning now drains twice against the same builder and asserts each walk logs its own occurrence 1 and 10. Without the reset the second walk logs nothing, so the assertion discriminates. The drain helper also asserts no mid-walk token is empty, pinning the invariant the reset relies on. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Two assertions added with the sampling tests could not fail, which is worse
than no assertion because it reads as coverage.
require.NotEmpty on results.NextPageToken sat three lines below an
`if results.NextPageToken == "" { break }`, so the only input that could have
failed it had already left the loop. The invariant it claimed to pin -- that an
empty token means first-call-of-this-walk, which is what List's counter reset
depends on -- was asserted nowhere. The check now lives in the break branch and
asserts the walk did not terminate on call zero, which is the property that
makes an empty token unambiguous between start and end. Early termination
partway through a walk is caught separately by the attempted-request count.
The occurrence assertion compared the log line against the substring
`"total_occurrences":1`, which is a prefix of :10, :100 and :1000, so a walk
whose first sampled line was a later occurrence would still have passed -- and
that is precisely the property the per-walk reset exists to prove. Skip warnings
are now decoded from the JSON records and total_occurrences is compared
numerically, so 1 cannot be satisfied by 10. Decoding also tightened the rest:
the message is matched exactly instead of by substring, total_occurrences must
be present rather than merely mentioned, the gRPC code must be PermissionDenied,
and the record must name a service account.
Every one of these was checked by removing the behaviour it describes and
confirming the test fails: an empty users page, a sampling schedule that skips
occurrence 1, a 404 in place of the 403, each log field dropped in turn, a
reworded message, a single walk instead of two, and the counter reset removed.
Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
highb
approved these changes
Aug 22, 2026
SecretTrait has no scopes field, so a synced application key gave C1 no way to
see what the credential is actually allowed to do. Resource.Profile is free-form
and has a first-class setter, so the scopes ride there, on both the sync path
and the issuance path -- a freshly vended key reports its scopes immediately
rather than only after the next sync rebuilds it.
Datadog's scopes field is a three-state nullable list: absent, explicit null, or
a list. The profile collapses that to two states, chosen so no state is
misreported:
- Datadog did not report scopes: the key is absent from the profile. Emitting
an empty list would assert the key is unscoped, which the response never
said, and that is the one error a consumer cannot detect.
- Datadog reported an unscoped key, as explicit null or an empty list: the key
is present and empty, positively stating that the key carries its owner's
full permissions.
- Datadog reported scopes: the key holds them.
The value is therefore always a list when present and never null, so a consumer
never needs a type switch, and absence is reserved for the one distinction that
matters. The field is named "scopes" rather than a provider-prefixed name: a
consumer should be able to learn what a synced credential can do without
knowing which provider minted it, and this repo's existing profiles are
unprefixed too.
Issuance records what the provider echoed back rather than what was requested,
since Datadog may normalize the list, so an issued key agrees with what the
syncer will later report for it instead of drifting from it. That required the
client to carry the created key's scopes, which it previously discarded.
The scopes pass-through into CreateServiceAccountApplicationKey had never been
exercised by any test. It is now, along with a scoped key, an unscoped key, a
key whose scopes change between syncs, a provider that reports nothing, and the
guarantee that setting a resource profile does not disturb the secret trait.
Each new assertion was checked by removing the behaviour it describes and
confirming the test fails.
baton_capabilities.json is unchanged: profile data is per-resource sync output,
not capability metadata. Verified by regenerating and diffing.
Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
A fake provider's handler runs on the httptest server's goroutine while the test body reads and writes the same variables. net/http promises no happens-before edge between the two, so three sites in the tests added for this PR relied on runtime implementation detail rather than on the memory model: - TestApplicationKeyListCarriesScopes rewrote the upstream key set between two drains while the handler was reading it. - TestIssuePassesScopesToProviderAndProfile captured the create request body in the handler and read it from the test body. - recordRequest appended to a shared slice from the handler, which every test using a recording fake then iterated. Guarding the helper is the minimal fix for the call sites this PR added, and it covers the pre-existing ones in the same file as a side effect. Each is now behind a mutex, with accessors at both ends. The rescope test keeps a single server across both drains on purpose: giving each drain its own server would remove the race by no longer exercising the rescope-mid-life path the test exists for. All three guards were checked by neutering them and confirming the assertions they protect fail. Worth recording for anyone auditing this later: `go test -race` does NOT report any of these, at this commit or in a minimal isolated reproduction run twenty times. The socket path between an httptest client and its handler appears to carry enough runtime synchronization for the detector to see an edge. So this is a robustness fix argued from the memory model, not a detector-confirmed failure, and -race is added to the gates because it is cheap and catches other classes -- not because it would have caught this. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Comment on lines
+159
to
+171
| var body string | ||
| server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| buf, _ := io.ReadAll(r.Body) | ||
| body = string(buf) | ||
| w.Header().Set("Content-Type", "application/json") | ||
| _, _ = w.Write([]byte(`{"data":{"id":"appkey-id","type":"application_keys","attributes":{"key":"plaintext-app-key","name":"c1-request"}}}`)) | ||
| })) | ||
| defer server.Close() | ||
|
|
||
| _, err := newOfficialTestClient(server.URL).CreateServiceAccountApplicationKey(context.Background(), serviceAccountID, "c1-request", []string{"dashboards_read", "metrics_read"}) | ||
| assertNoError(t, err, "create service account application key should succeed") | ||
| assertContains(t, body, "dashboards_read", "request body should include the requested scopes") | ||
| assertContains(t, body, "metrics_read", "request body should include the requested scopes") |
Contributor
There was a problem hiding this comment.
🟡 Suggestion: body is written on the httptest handler goroutine (line 162) and read by the test body (lines 170-171) with no synchronization. This is exactly the shape the last commit guarded with bodyMu/keysMu in credential_lifecycle_test.go — net/http gives no happens-before edge between a handler returning and the client's Do returning, so the same reasoning applies here. Wrapping the write/read in a mutex (or using sync/atomic.Pointer) would make this file consistent with the fix already applied in the connector tests.
| bag *pagination.Bag, | ||
| page int64, | ||
| ) ([]*v2.Resource, *resource.SyncOpResults, error) { | ||
| users, err := o.wrapper.ListUsers(ctx, datadogV2.NewListUsersOptionalParameters().WithPageNumber(page)) |
Contributor
There was a problem hiding this comment.
🟡 Suggestion: this ListUsers call sets only WithPageNumber, so Datadog applies its default page[size] of 10 (the vendored client's own paginator hardcodes pageSize_ := int64(10) for ListUsers). Since each List call consumes exactly one users page, an org with N users costs N/10 provider round-trips before any application key is fetched. apiTokenBuilder.List gained .WithPageSize(defaultV2PageSize) in this same PR; adding it here too would cut that by 10× without changing the len(data) == 0 termination condition.
Comment on lines
+374
to
+380
| if attrs != nil && attrs.CreatedAt != nil { | ||
| createdAt, err := time.Parse(time.RFC3339Nano, *attrs.CreatedAt) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("baton-datadog: parse application key created_at: %w", err) | ||
| } | ||
| resourceOptions = append(resourceOptions, resource.WithResourceCreatedAt(createdAt)) | ||
| } |
Contributor
There was a problem hiding this comment.
🟡 Suggestion: a created_at value that does not parse as RFC3339Nano makes applicationKeyResource return an error, which listApplicationKeyPage propagates as a hard failure — one malformed timestamp on one key aborts the whole sync, including every service account not yet walked. That matches the existing apiTokenBuilder behaviour, but the app-key path is new and untested here: none of the TestApplicationKeyResource* fixtures include created_at, and the live smoke test never exercises applicationKeyResource (it calls ListServiceAccountApplicationKeys directly). Worth either adding a created_at case to the table test, or degrading to a Debug/Warn + omit-the-timestamp so an unexpected provider format cannot take down a whole sync.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Mapping
Issuance targets a service account, not an organization API key. An org-scoped key issued "on behalf of" a selected user is not an honest record of who holds it, so
Issuere-checks that the selected Datadog user is still a service account, refuses a human user, and mints a key owned by and scoped to that service account.Issueis also non-duplicating: it looks the deterministic request name up first and refuses rather than minting a second key, because Datadog cannot re-return plaintext material.Revocation needs the owning service account as well as the key — Datadog has no delete-by-key-id-alone form for these keys. It travels in the resource deleter's existing
parentResourceIDparameter rather than a packed composite handle, so the handle stays a bare provider id like every other secret this connector syncs. Until a caller populates it, revoke fails closed instead of guessing; the capability table in the docs carries a footnote saying so.Permissions
Advertised permissions come from the
x-permissionblock Datadog publishes for each operation in its own OpenAPI spec, not from inference:ListAPIKeysapi_keys_readDeleteAPIKeyapi_keys_deleteList/Create/DeleteServiceAccountApplicationKeyservice_account_writeapi_keys_writeis deliberately not advertised: Datadog scopes it to create and rename, which no advertised capability calls.service_account_writesits only on the secret resource types, which are registered only when secret sync is on, so a secrets-off install is not asked to grant it.Verification
go build ./cmd/baton-datadog— cleango test ./... -count=1— passmake lint—0 issues.baton_capabilities.jsonregenerated from the built binary rather than hand-edited.Unit coverage for the new paths: application-key create and delete plus provider 404 mapping, create with no returned key material,
FindAPIKeyByNameexact-match-after-filter including a cross-page match, duplicate-request refusal, handle-is-not-the-secret assertions on both delete paths, one-provider-page-per-List-call pagination, the permission-denied skip, and warning sampling.Live provider
The live smoke test is opt-in behind
DATADOG_CREDENTIAL_SMOKE=1and is skipped in CI. It was run against a disposable Datadog trial organization at0228d6eband passed: the key was minted, found throughListServiceAccountApplicationKeys, used to authenticate a real request, revoked, confirmed to stop authenticating, and confirmed delisted from its service account.Ownership was confirmed independently of the connector, which is the claim the retarget rests on:
owned_byas the named service account, and that owner's user record hasservice_account: true;Delete scoping was exercised live as well: revocation goes through the service-account-scoped endpoint.
The two commits since
0228d6ebare test-only, so this evidence still describes the current code.Not covered by that run, and stated so this is not read as broader than it is: multi-page application-key pagination was not exercised, and neither was reduced-permission behavior — the trial credential holds Datadog Admin-role permissions, so the run shows that
service_account_writeis sufficient but not that it is necessary.Notes
Listcall returns at most one provider page so the SDK keeps control of checkpointing, rate limits and cancellation; a service account whose keys the role cannot read is warned about and skipped rather than failing the whole sync.