Add Datadog credential issuance - #40
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Connector PR Review: Add Datadog credential issuanceBlocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0 Review SummaryThe only new commit is documentation: a doc-comment paragraph on Security IssuesNone found. Correctness IssuesNone found. SuggestionsNone. |
The organization API key deleter was registered whenever sync-secrets was on. Any install already syncing secrets would therefore acquire org-wide Datadog API key deletion the moment it upgraded the connector, without anyone choosing it. Reading a credential inventory is not consent to destroy what is in it. Deletion now needs allow-org-api-key-deletion, a separate flag that defaults to off. Sync behaviour is unchanged either way. The capability has to be absent, not merely refused: C1 resolves what it may do from the advertisement, and the SDK derives CAPABILITY_RESOURCE_DELETE from a type assertion on the registered syncer. So Delete moves off apiTokenBuilder onto deletableAPITokenBuilder and only that variant is registered when the grant is set. The advertised Datadog permissions follow the same split: api_keys_delete and api_keys_write are advertised only by the variant that can reach them. Also updates credential_lifecycle_test.go for the moved constructor. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Datadog has two kinds of API key and they are not interchangeable. A service account application key is scoped to and owned by one identity; an organization API key is owned by the whole organization, has no owner inside it, and cannot be scoped at all. Both are the API_KEY shape, so the shape enum alone cannot tell a caller which one it is getting. IssueCapabilityDetails now advertises one descriptor per kind, separated by secret_resource_type_id, with the service account application key marked preferred. Issue dispatches on the requested type rather than always minting an application key, and refuses an unrecognised type instead of falling back to a default arm. Organization API key issuance follows allow-org-api-key-deletion. The SDK will not register an issuance descriptor whose secret resource type has no ResourceDeleterV2, and that is the right constraint: a credential this connector cannot revoke is one it should not mint. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Regenerated from the built connector: ./connector config > config_schema.json ./connector capabilities > baton_capabilities.json README and docs/connector.mdx describe allow-org-api-key-deletion and the two issuance kinds. The capabilities document is static and has no conditional form, so main.go forces every optional surface on when generating it, as it already did for sync-secrets and sync-schedules. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
TestCredentialIssueLifecycle covers the service account application key arm only. The organization API key arm is the new one and had no live coverage, so a caller could not tell from the suite whether the dispatch actually reaches a different Datadog API. The new test mirrors the existing opt-in guard and always revokes what it mints. It asserts the issued resource comes back as the kind that was requested rather than the preferred one, that it carries no parent resource id, and that the key authenticates before revocation and stops afterwards. The probe is GET /api/v1/validate, which authenticates on the API key alone. An organization API key has no application key to pair with, so the connector's own ValidateCredentials path would not isolate the credential under test. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Datadog answers 404, not an empty list, when asked for a disabled service account's application keys. listApplicationKeyPage treats 404 as fatal, so one disabled service account failed the entire application-key walk — and Datadog never deletes users, it only disables them. Any organization that has ever disabled a service account could therefore never sync a single application key, and the resource type never appeared in C1 at all. The walk now skips service accounts Datadog reports as disabled. They cannot authenticate, so no live credential is dropped. A 404 on an account this walk did choose to visit still fails closed: that one was enabled when the users page was read, so not being readable now is a real anomaly. Found against a live Datadog organization, where it kept the service-account application key from ever reaching C1. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
…lient The organization API key smoke test hand-rolled an http.Client call to /api/v1/validate. gosec's taint analysis flagged it (G704), and it was the wrong shape anyway: every other probe in this package goes through client.DatadogClient. ValidateCredentials calls the same endpoint. It authenticates on the API key alone, which is what makes it the right probe for an organization key — that kind has no application key to pair with, so a probe requiring one would not isolate the credential under test. The empty application key is deliberate and now documented. Datadog refusing the credential is now returned as "not valid" rather than as a probe error, which is what the revocation check was already treating it as. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
gosec flags G101 on a struct literal that assigns string constants to fields named apiKey and appKey, even in a test against a fake provider. These tests read advertised capabilities, which never touch the connector's own credentials, so the fields were doing nothing. Leaving them unset removes the finding rather than suppressing it. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Delete required parentResourceID to name the owning service account, and no C1 caller populates it, so every revoke of an issued service-account application key failed InvalidArgument and left the key live at the provider -- while the connector advertised CAPABILITY_RESOURCE_DELETE for that type. That is the failure mode api_token.go argues against. The owner is recoverable from the key: Datadog carries it as the owned_by relationship on GetApplicationKey. Delete now reads it when the parent is absent and keeps failing closed only when the lookup cannot name an owner, so the fallback widens what can be revoked without letting Delete guess. Also bounds the users level of the application-key walk. The application-key level already had maxApplicationKeyPages; the users level terminates on an empty page rather than a short one, so a provider that ignores page[number] would re-push child states forever. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
The warning and footnote both said a revoke that omits the owning service account fails, and that revocation is advertised but cannot complete until the requesting workflow threads the owner through. The connector now reads the owner from the key, so both claims are stale. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Listing a service account's application keys needs Datadog's service_account_write, which api_keys_read does not imply, and a 403 there fails the whole sync rather than skipping the account. Registering that syncer under sync-secrets alone therefore broke every existing sync-secrets install on upgrade, on a permission the operator was never asked for. sync-service-account-application-keys is that ask, off by default. It follows the shape allow-org-api-key-deletion already set in this connector: the syncer is not registered without the grant, so the capability is absent from what C1 is advertised rather than advertised and refused. Once granted, the fail-hard behaviour is unchanged -- a credential absent from a completed sync reads as deleted, so skipping would retire live credentials from the inventory. The grant also gates issuance of that kind, because the SDK refuses an issuance descriptor whose secret resource type has no registered deleter. With secrets synced and neither kind granted there is nothing to issue, so CAPABILITY_CREDENTIAL_ISSUE is absent rather than advertised with an empty option list. baton_capabilities.json is unchanged: capability generation forces every optional surface on, so the document still describes the connector's full capability set. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Two review findings on the owner-lookup fallback. Delete collapsed every non-NotFound failure from FindApplicationKeyOwner into InvalidArgument, discarding the classification wrapOfficialClientError exists to produce: a 500 is retryable, a 403 names a missing permission, and %v dropped the wrapped error with its rate-limit annotations. A transient blip therefore read as a terminal revoke failure and stranded the key -- the outcome the fallback was added to remove. Only the provider answering with no owner is unresolvable, and that case arrives as a plain error, so the code distinguishes them. The lookup also reaches an org-scoped endpoint the advertised permissions did not cover. Datadog governs GetApplicationKey under org_app_keys_read, not service_account_write, so a role holding exactly what this connector asked for would have 403'd on every revoke. Both permissions are now advertised and documented. Verified against the same OpenAPI spec the rest of these permissions came from, which also confirms the service-account endpoints need only service_account_write. Both name lookups now make one request instead of walking pages. The name searched for is always "c1-<request id>", so only a key whose own name contains that whole string can come back and a page of 100 cannot fill with them. Paging was answering a question the filter had already settled, and it was the source of the rate-limit concern raised in review. A full page now fails rather than reporting no match, because reporting no match would mint a duplicate of a key whose plaintext Datadog will not reissue. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
revive rejected the description line at 213 characters. Shortened rather than wrapped, since the same string is the flag's help text and appears in README.md and config_schema.json. Two review points on the gate's documentation, both correct. The comment on userResourceType still said sync-secrets alone registers applicationKeyBuilder, which was the reasoning for scoping service_account_write to the application-key resource type -- that reasoning holds but now runs through two flags. And the capability table's application-key row carried no marker, so a reader scanning only the table would conclude those keys sync out of the box with Sync secrets. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
The self-hosted env-var block is the only place the connector's variable names are written out, and it named BATON_ALLOW_ORG_API_KEY_DELETION but not BATON_SYNC_SERVICE_ACCOUNT_APPLICATION_KEYS. An operator copying it got organization-key deletion and no application keys at all. 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>
6b5f212 to
b4cb4f1
Compare
|
|
||
| // warnMalformedTimestamp reports an unparseable provider timestamp. The field | ||
| // is dropped and the key still syncs. | ||
| func (o *apiTokenBuilder) warnMalformedTimestamp(ctx context.Context, apiKeyID, field, raw string, err error) { |
There was a problem hiding this comment.
🟡 Suggestion: this fires at Warn once per malformed field per key, and it is called twice per key (created_at + modified_at), as TestApiTokenListSurvivesMalformedTimestamps asserts. A provider-side timestamp format change would emit two warnings for every API key on every sync; the same shape exists for the created_at callback in application_key.go:305. Repo criteria L7 asks for logarithmic sampling (1, 10, 100, every 1000) with a total_occurrences field on warnings that can fire per-resource.
|
|
||
| nextPageToken := "" | ||
| if len(apiTokens) != 0 { | ||
| if hasMoreAPIKeyPages(res, page, int64(len(apiTokens)), defaultV2PageSize) { |
There was a problem hiding this comment.
🟡 Suggestion: this walk has no page ceiling. When meta.page.total_filtered_count is absent (helpers.go:117/121/131), hasMoreAPIKeyPages falls back to count != 0, so a provider that ignores page[number] and keeps returning full pages would page forever. The new application-key walk added maxUserPages/maxApplicationKeyPages for exactly this failure mode; consider the same bound here so both walks fail closed consistently.
| switch status.Code(err) { | ||
| case codes.NotFound: | ||
| return nil, nil | ||
| case codes.Unknown: |
There was a problem hiding this comment.
🟡 Suggestion: codes.Unknown is broader than "the provider answered and its answer names no owner". wrapOfficialClientError runs uhttp.GrpcCodeFromHTTPStatus, whose default arm returns Unknown for any status outside 4xx/5xx — so a 2xx or 3xx whose body the generated client cannot unmarshal (an intercepting proxy returning HTML with 200, for example) also lands here and gets converted into a permanent InvalidArgument refusal instead of a retryable error. Consider distinguishing the connector's own "no owner" sentinels in FindApplicationKeyOwner (a typed error / errors.Is) from an unmapped transport code. Same at line 120.
| # The auth-error check intentionally invalidates BATON_* secrets. | ||
| # Keep this feature flag parseable so it reaches authentication. | ||
| bad-credentials: | | ||
| BATON_SYNC_SECRETS=true |
There was a problem hiding this comment.
🟡 Suggestion: the live sync test still runs with only BATON_SYNC_SECRETS, so the new two-level service-account application-key walk (users page → per-service-account key pages, bag push/pop, disabled-account skip) is never exercised against a real Datadog org — and the PR notes multi-page application-key pagination was not exercised in the live smoke run either. If the CI role can be granted service_account_write, adding BATON_SYNC_SERVICE_ACCOUNT_APPLICATION_KEYS: true to the job env would cover the highest-risk new code path; if it cannot, the fail-hard-on-403 design means it must stay off, which is worth noting in the PR.
apiTokenBuilder.List had no page ceiling. hasMoreAPIKeyPages compares against meta.page.total_filtered_count only when the response carries one (pkg/connector/helpers.go:119-131); with the field absent it falls back to "the page was not empty", so a provider that ignores page[number] and keeps answering with full pages is paged forever. Nothing else in the walk terminates it. Add maxOrgAPIKeyPages, mirroring maxApplicationKeyPages and maxUserPages in application_key.go: same 10_000 value, same fail-closed check before the request goes out, same error shape naming the resource type. 10_000 pages of 100 is 1M organization API keys, far past any real Datadog organization. TestApiTokenListBoundsPagination drives the exact response shape hasMoreAPIKeyPages cannot terminate on -- a full page with no meta -- and asserts the page below the bound is still served and still offers another, that the page at the bound errors, and that no request is issued for it. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
…PC code Delete's two owner-lookup switches treated codes.Unknown as "the provider answered and named no owner" and converted it to a permanent codes.InvalidArgument refusal. Unknown is too broad for that. FindApplicationKeyOwner's failures reach Delete through wrapOfficialClientError, which classifies with uhttp.GrpcCodeFromHTTPStatus; that function's default arm returns Unknown for every status outside 4xx/5xx, including a 2xx or 3xx whose body the generated Datadog client cannot unmarshal. A response the transport could not decode is retryable, and Delete was making it permanent -- stranding a live application key that a retry would have revoked. Fix it where the distinction is known. FindApplicationKeyOwner now joins a package-level client.ErrApplicationKeyOwnerUnknown onto its two hand-rolled cases -- a response with no owned_by relationship, and an owned_by naming no user -- which are the only failures where the provider succeeded and still named no owner. Delete branches on errors.Is against that sentinel, so every other error, whatever its code, falls through to the wrapped-error default arm with its classification intact. The sentinel matches how this package already distinguishes an unretryable condition the gRPC code cannot express (client.ErrNotFound, joined the same way). Two tests pin the split. The ownerless case still yields InvalidArgument and now asserts the sentinel where it is produced, since status.Errorf formats its cause with %v rather than chaining it. TestApplicationKeyBuilderDeleteKeepsUnmappedTransportFailureRetryable drives a 200 with an undecodable body and requires the error to keep codes.Unknown, to not be InvalidArgument, and to not carry the sentinel; it fails on the InvalidArgument assertion against the old codes.Unknown branch. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
| // Both of these are the provider answering and naming no owner, which no | ||
| // retry changes; ErrApplicationKeyOwnerUnknown is how a caller tells them | ||
| // apart from a transport failure that merely mapped to the same gRPC code. | ||
| if response.Data == nil || response.Data.Relationships == nil || response.Data.Relationships.OwnedBy == nil { |
There was a problem hiding this comment.
🟡 Suggestion: response.Data == nil is folded into ErrApplicationKeyOwnerUnknown, but it is not only reachable when the provider named no owner — it is also what an undecodable 200 looks like. ApplicationKeyResponse.UnmarshalJSON (vendor/.../model_application_key_response.go:120) falls back to datadog.Unmarshal(bytes, &o.UnparsedObject) and returns a nil error whenever the outer decode fails, leaving Data == nil. So a 200 body of {}, {"data": 5}, or a proxy envelope like {"error":"upstream"} reaches FindApplicationKeyOwner with no error and nil Data, gets the sentinel, and application_key.go:106 converts it into a permanent codes.InvalidArgument refusal — the exact "retryable transport failure made permanent" outcome this commit set out to avoid, and it leaves the issued key live at the provider. The new TestApplicationKeyBuilderDeleteKeepsUnmappedTransportFailureRetryable misses it because {"data": not-json is syntactically invalid, so the UnparsedObject fallback also fails and the call errors out on the codes.Unknown path instead.
Consider narrowing the sentinel to the cases where the provider demonstrably answered about this key — Relationships == nil, OwnedBy == nil, and ownerID == "" — and returning a plain retryable error for Data == nil, since a response carrying no data object at all is indistinguishable from a body the client could not decode.
A failed issuance is not re-driven by the platform, so every failure in Issue is terminal for that request. The fact only lived in a resolved review thread; it explains two existing design choices -- the single-request name lookups and the best-effort cleanup of a mint whose result cannot be returned -- so it belongs on the function. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
5 participants
Datadog credential issuance
What this enables
Dependencies
main.Summary
API_KEYshape, selected by the caller.Two kinds of API key
Datadog organization API keys and service-account application keys are two kinds of the same shape. Both are
CAPABILITY_DETAIL_CREDENTIAL_OPTION_API_KEY; they differ in what they are, who owns them, and what they can do. Before baton-sdk#1109 a connector could advertise only one descriptor per shape, so a connector offering both had no way to say which one a request meant.That SDK release makes descriptor identity the pair
(option, secret_resource_type_id)and requiresCredentialIssueOptions.secret_resource_type_idon every issue request. This connector advertises both:secret_resource_type_idservice-account-application-keyapi-keyIssuereads the requested type off the request and dispatches. An unrecognized type is refused rather than silently minting the other kind.Mapping
Application-key issuance targets a service account, not a human. 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 non-duplicating on both kinds: it looks the deterministic request name up first and refuses rather than minting a second key, because Datadog cannot re-return plaintext material.Revocation of an application key needs the owning service account as well as the key — Datadog has no delete-by-key-id-alone form. It travels in the resource deleter's existing
parentResourceIDrather than a packed composite handle, so the handle stays a bare provider id like every other secret this connector syncs. No C1 caller populates it today, so when it is absent the connector reads the owner from the key'sowned_byrelationship and proceeds; a key whose owner cannot be identified is refused rather than guessed at.Gating organization API key deletion
Organization API keys belong to the organization, not to the person they were issued to, and they cannot be scoped. Deleting one is destructive in a way reading one is not, so read capability is not consent to destroy: the deleter is registered only when
allow-org-api-key-deletionis set, and that grant is off by default.The gate is structural, not a guard inside a method body. The SDK derives
CAPABILITY_RESOURCE_DELETEfrom a type assertion on the registered syncer, so the deleter lives on its own type and is simply not registered without the grant — the capability is absent from what the connector advertises rather than advertised and refused at call time.The same grant gates organization-API-key issuance, because baton-sdk#1109 requires every advertised issuance descriptor to have a registered deleter. C1 will not mint a credential it has no permission to revoke.
Permissions
Advertised permissions come from the
x-permissionblock Datadog publishes for each operation in its own OpenAPI spec, not from inference:ListAPIKeysapi_keys_readCreateAPIKeyapi_keys_writeDeleteAPIKeyapi_keys_deleteList/Create/DeleteServiceAccountApplicationKeyservice_account_write‡ Only reachable with Allow organization API key deletion on. With it off, organization API keys still sync; they simply cannot be issued or deleted, and the extra permissions are not exercised.
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 ./...— cleango vet ./...— cleango test ./... -count=1— pass, 79 assertions inpkg/connectormake lint—0 issues.baton_capabilities.jsonregenerated from the built binary rather than hand-edited.Unit coverage for the new paths: both issuance kinds advertised with distinct output types; dispatch on the requested type; refusal of an unrecognized type; refusal of the organization kind without the grant; the delete capability absent from advertised capabilities without the grant and present with it; application-key create and delete plus provider 404 mapping; create with no returned key material; exact-match-after-filter lookups including a cross-page match; duplicate-request refusal; handle-is-not-the-secret assertions on both delete paths; owner resolution from
owned_bywhenparentResourceIDis absent, and refusal when the owner cannot be identified; one-provider-page-per-List-call pagination; bounded paging at both levels of the application-key walk; disabled service accounts skipped rather than failing the sync; and hard failure of the sync on both a 403 and a 404 from the service-account application-key list.CI coverage gap. The
sync-testjob does not setBATON_SYNC_SERVICE_ACCOUNT_APPLICATION_KEYS, so the two-level service-account application-key walk is covered by httptest fakes only; enabling it in CI requires the CI Datadog role to holdservice_account_write, without which the walk fails the sync by design.Live provider
The live smoke tests are opt-in behind
DATADOG_CREDENTIAL_SMOKE=1and are skipped in CI. Both kinds were exercised against a disposable Datadog trial organization: minted, found through the provider's own list API, used to authenticate a real request, revoked, confirmed to stop authenticating, and confirmed delisted.Application-key ownership was confirmed independently of the connector:
owned_byas the named service account, and that owner's user record hasservice_account: true;Multi-account attachment was verified across several service accounts: each vended key was ATTACHED to its own account and absent from the others.
Not covered, 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
Datadog application keys do not support create-time expiry.
Datadog may retain key metadata after revocation, so authentication failure is the revoke assertion — and only a 401/403 counts as evidence, not any error.
Datadog answers 404, not an empty list, for a disabled service account's application keys, and it never deletes users, only disables them. The walk skips disabled accounts; without that, one disabled account made every application key in the organization unsyncable.
A
Listcall returns at most one provider page so the SDK keeps control of checkpointing, rate limits and cancellation. Both levels of the walk are page-bounded so a provider that ignorespage[number]fails closed instead of paging forever.A service account whose keys the role cannot read fails the whole sync rather than being skipped, because C1 reads a resource absent from a completed sync as deleted, so skipping would retire live credentials from the inventory instead of reporting that they could not be read. A 403 and a 404 are both terminal but carry different messages: the 403 names
service_account_write, and the 404 says the service account was not found and may have been deleted mid-sync.Compatibility note. Service-account application-key sync is off by default. Existing installs using Sync secrets continue syncing organization API keys unchanged. After enabling Sync service account application keys, the role also needs
service_account_write; without it the connector fails the sync rather than treating an unreadable credential inventory as empty.CLI compatibility note. baton-sdk v0.26.0 removes
--diff-syncs,--base-sync-id, and--applied-sync-id. Update automation that invokes those flags before upgrading this connector.