Repository navigation
Conversation
The Datadog connector vended its org API key as the raw key bytes with no schema, so the credential carried no declared type. Multipass needs both: a type announced before the credential is minted, and a payload in that type's shape afterwards. The organization API key now returns an api_key payload whose value is Multipass api_key_v2 JSON -- key_value, plus key_id, provider and base_url where they apply -- and declares it on the issue descriptor through the new CredentialIssueOptionDescriptor.output field. The MIME type, its tags and the field name live in one place so the announcement and the payload cannot disagree. Service-account application keys are unchanged: they still return raw key bytes with no schema. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Pins github.com/conductorone/baton-sdk to the IGA-5308 branch pseudo-version, which adds CredentialIssueOptionDescriptor.output, and re-vendors it. The dependency bump is required before the connector can declare what an issued credential is, so it lands on its own. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Superseded — see the current review report for commit
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| require ( | ||
| github.com/DataDog/datadog-api-client-go/v2 v2.43.0 | ||
| github.com/conductorone/baton-sdk v0.43.0 | ||
| github.com/conductorone/baton-sdk v0.43.1-0.20261103192834-4c7962f2d9e5 |
There was a problem hiding this comment.
🟠 Bug: This pin isn't just v0.43.0 plus the output field. It's v0.43.0 plus 12 unreleased commits, including baton-sdk #1203 ("sync: require the page ledger and delete the token-checkpoint sync path") and #1202 (deletes the expand paths). That's why the vendored diff removes about 3.5k lines from pkg/sync and pkg/dotc1z/engine/pebble. The PR body doesn't mention any of this sync-engine change, and it points at an unmerged feature-branch commit. Repin to a tagged baton-sdk release that contains #1218, and call out any sync-path changes that release brings in. (Confidence: high)
| PlaintextData: []*v2.PlaintextData{ | ||
| v2.PlaintextData_builder{Name: "api_key", Bytes: []byte(key.Secret)}.Build(), | ||
| v2.PlaintextData_builder{ | ||
| Name: datadogAPIKeyFieldName, |
There was a problem hiding this comment.
🟡 Suggestion: The org API key plaintext used to be the bare key. Now it's JSON, and nothing makes that opt-in. Anything that reads the issued bytes as the key itself (reveal flows, automations, older C1 consumers that ignore Schema) will get {"key_value":...} instead. If every consumer already honors the declared output, say so in the PR. Otherwise, gate the change. (Confidence: medium)
Superseded — see the current review report for commit
|
There was a problem hiding this comment.
Blocking issues found — see the full review report
baton-sdk now ships pkg/secret, so the connector does not need its own api_key_v2 struct and constants. The organization API key builds secret.APIKeyV2, encodes it with secret.Plaintext -- which sets the schema from the same type id -- and declares the descriptor output with secret.OutputField. The descriptor and the payload can no longer name different types, because both are derived from secret.TypeAPIKeyV2. The emitted JSON is unchanged: secret.APIKeyV2 carries the same field names and omitempty tags as the struct this replaces. DisplayName stays on the field. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Moves the pin to d483c04a, which carries the standard Multipass types in pkg/secret that the previous commit uses, and re-vendors it. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Superseded — see the current review report for commit
|
| require ( | ||
| github.com/DataDog/datadog-api-client-go/v2 v2.43.0 | ||
| github.com/conductorone/baton-sdk v0.43.0 | ||
| github.com/conductorone/baton-sdk v0.43.1-0.20261103193733-d483c04a2873 |
There was a problem hiding this comment.
🟠 Bug: The new pin v0.43.1-0.20261103193733-d483c04a2873 is still an unreleased feature-branch commit. The vendor diff still carries the unrelated sync-engine changes: pkg/sync/*, pkg/sync/expand/*, pkg/dotc1z/engine/pebble/* and pkg/synccompactor/* (the ledger-only sync path from SDK #1203, plus the expand-path deletions). Before merging, repin to a tagged baton-sdk release that includes #1218, then run go mod tidy && go mod vendor. (Confidence: high)
| // base_url is the endpoint the key authenticates against, built from the | ||
| // connector's configured site the way the REST client builds its own base | ||
| // URL -- not from whatever host a test server happens to use. | ||
| plaintext, err := organizationAPIKeyPlaintext(organizationAPIKeyPayload( |
There was a problem hiding this comment.
🟡 Suggestion: The org API key plaintext used to be the bare key. Now it's api_key_v2 JSON, and that is not opt-in (BP1/BP2). A consumer that ignores PlaintextData.Schema and uses the bytes as the key will break. Either confirm that every reveal/consumer path honors the declared schema, or gate the format change. (Confidence: medium)
Superseded — see the current review report for commit
|
There was a problem hiding this comment.
Blocking issues found — see the full review report
Moves the pin to the SDK head that adds one test-only lint suppression after d483c04. No API change, so nothing but go.mod, go.sum and the vendored version line moves. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Superseded — see the current review report for commit
|
| require ( | ||
| github.com/DataDog/datadog-api-client-go/v2 v2.43.0 | ||
| github.com/conductorone/baton-sdk v0.43.0 | ||
| github.com/conductorone/baton-sdk v0.43.1-0.20261103194337-101e5eed424c |
There was a problem hiding this comment.
🟠 Bug: The repin to v0.43.1-0.20261103194337-101e5eed424c is still an unreleased pseudo-version. The vendor tree still carries the unrelated sync-engine changes (pkg/sync/*, pkg/sync/expand, pkg/dotc1z/engine/pebble, pkg/synccompactor), so merging now would ship unreleased SDK sync behavior. Repin to the tagged baton-sdk release that includes #1218, then run go mod tidy && go mod vendor. (Confidence: high)
Superseded — see the current review report for commit
|
There was a problem hiding this comment.
Blocking issues found — see the full review report
Superseded — see the current review report for commit
|
Superseded — see the current review report for commit
|
There was a problem hiding this comment.
Blocking issues found — see the full review report
gosec reads a string containing "API key" as a possible credential. This one is the label C1 shows for the field, not a secret, so the finding is suppressed on that line alone. CI pointed at line 24; gosec flags the display name there, not the field name, so the suppression sits on the line the linter actually reports -- an unused nolint is itself a failure. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
The new credential descriptor Output changes the connector's generated capabilities, so baton_capabilities.json is regenerated from the binary. config_schema.json is unchanged but regenerated alongside it, as the metadata workflow does. docs/connector.mdx describes what the issued organization API key now looks like, since the delivery format is what a caller handling the key needs to know. Generated with: go build -o connector ./cmd/baton-datadog ./connector capabilities > baton_capabilities.json ./connector config > config_schema.json Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
0e29ec6 to
c47fb05
Compare
Superseded — see the current review report for commit
|
| plaintext, err := organizationAPIKeyPlaintext(organizationAPIKeyPayload( | ||
| key.Secret, | ||
| key.ID, | ||
| fmt.Sprintf(client.DefaultBaseURL, u.wrapper.GetSite()), |
There was a problem hiding this comment.
🟡 Suggestion: base_url is always https://api.<site>, but when base-url is set, connector.go:192-198 points the official client (the one that mints this key) at that override instead. In that case the payload names a different endpoint from the one that issued the key. The override is hidden and CLI-only, so this only affects test setups. Pass the effective base URL through the wrapper rather than re-deriving it from the site. (Confidence: medium)
Superseded — see the current review report for commit
|
There was a problem hiding this comment.
Blocking issues found — see the full review report
Superseded — see the current review report for commit
|
There was a problem hiding this comment.
Blocking issues found — see the full review report
baton-multipass-sdk cannot be published until it is public, so the shared Multipass types are vendored by copying them. Every file is byte-identical to the source at b9f4b677 apart from the provenance header, and the package keeps its name and its exported API, so replacing this directory with a module dependency later is an import-path change only. Copied from github.com/conductorone/baton-multipass-sdk/pkg/multipass at b9f4b6776da475759bbcdc18f8209fedab389a6c, tests included. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
The four connector files import baton-datadog/pkg/multipass instead of baton-sdk/pkg/secret, which the shrunken SDK no longer carries. 61f186d3 was the only baton-sdk commit carrying pkg/secret, so it is orphaned; this pins the shrunken #1218 (dc065ae5) instead. The pinned pseudo-version is v0.44.1-0.20261103220501-dc065ae56b48: v0.44.0 is the commit those changes sit on, so v0.44.1 is the correct base. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Superseded — see the current review report for commit
|
| require ( | ||
| github.com/DataDog/datadog-api-client-go/v2 v2.43.0 | ||
| github.com/conductorone/baton-sdk v0.43.0 | ||
| github.com/conductorone/baton-sdk v0.44.1-0.20261103220501-dc065ae56b48 |
There was a problem hiding this comment.
🟠 Bug: baton-sdk is still pinned to a pseudo-version, v0.44.1-0.20261103220501-dc065ae56b48 (vendor/modules.txt:281), from the unmerged #1218 branch. Compared with main (v0.43.0), the vendor tree also brings in upstream sync-engine changes (pkg/sync, pkg/sync/expand, pkg/dotc1z/engine/pebble, pkg/synccompactor) that this PR doesn't use. The PR body itself says not to merge until #1218 is released, so repin to that release and re-vendor. (Confidence: high)
| return nil, fmt.Errorf("baton-datadog: build organization API key secret resource: %w", err) | ||
| } | ||
| // Multipass rejects nulls, so the optional api_key_v2 fields use omitempty. | ||
| plaintext, err := multipass.Plaintext(datadogAPIKeyFieldName, multipass.APIKeyV2{ |
There was a problem hiding this comment.
🟡 Suggestion: The org API key plaintext used to be the bare key. It is now api_key_v2 JSON, and this is on by default with no opt-in (BP1). The PR body and docs describe the change (BP2/BP3), but anything that ignores PlaintextData.Schema, including paper-vault deliveries until IGA-5311, will get JSON where it expected the key. Consider a config flag, or get explicit sign-off that defaulting this on is intended. (Confidence: medium)
Superseded — see the current review report for commit
|
There was a problem hiding this comment.
Blocking issues found — see the full review report
baton-multipass-sdk tagged v0.0.1 at b9f4b677, the revision copied here, so the header names the tag as well as the commit, and names IGA-5320 as the ticket that swaps the copy for the real module. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Superseded — see the current review report for commit
|
Superseded — see the current review report for commit
|
There was a problem hiding this comment.
Blocking issues found — see the full review report
The two header lines are now exactly the specified text, with the source, tag and commit on one line rather than wrapped. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Superseded — see the current review report for commit
|
Superseded — see the current review report for commit
|
There was a problem hiding this comment.
Blocking issues found — see the full review report
The application key now comes back as Multipass api_key_v2 JSON, the same shape as the organization key, so both issuance kinds deliver typed credentials instead of one typed and one bare. It carries the header the key is sent in and the scopes Datadog reported; nil scopes (the provider said nothing) are omitted rather than emitted as an empty list, which would assert the key is unscoped. Encoding failures revoke the key through the new discardServiceAccountApplicationKey, beside the organization key's equivalent. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
The application key descriptor now declares its output, so the generated capabilities gain that block. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Superseded — see the current review report for commit
|
| </Warning> | ||
|
|
||
| ‡Organization API key issuance and revocation both require **Allow organization API key deletion**, which is off by default and is separate from **Sync secrets**. An organization API key belongs to the whole organization rather than to the person it was issued to, and it cannot be scoped, so C1 will not mint one it has no permission to revoke. With the setting off, organization API keys still sync; they simply cannot be issued or deleted. | ||
| ‡Organization API key issuance and revocation both require **Allow organization API key deletion**, which is off by default and is separate from **Sync secrets**. An organization API key belongs to the whole organization rather than to the person it was issued to, and it cannot be scoped, so C1 will not mint one it has no permission to revoke. An issued organization API key is delivered as a Multipass API key (`application/vnd.multipass.api_key_v2+json`): a JSON object carrying the key itself, the key's Datadog ID, the provider, and the Datadog API base URL. A vended service account application key is still delivered as the bare key. With the setting off, organization API keys still sync; they simply cannot be issued or deleted. |
There was a problem hiding this comment.
🟡 Suggestion: This line still says "A vended service account application key is still delivered as the bare key". As of users.go:215 the application key is also api_key_v2 JSON, and it also carries header_name and scopes. The app key is the default (preferred) issuance kind, so the BP1/BP2 format change now reaches every vend, not just the opt-in org key. Update the docs (BP3) and the PR title, which still says "for org API keys". (Confidence: high)
| // discardOrganizationAPIKey revokes an organization API key whose plaintext | ||
| // cannot be returned. Issuance is not retried automatically, so a mint whose | ||
| // result never reaches the caller would otherwise be an orphaned live | ||
| // credential nobody holds. A failed delete is logged rather than returned: the | ||
| // original error is what the caller needs to see, and the key id is what lets | ||
| // an operator clean up by hand. | ||
| // discardServiceAccountApplicationKey revokes a service account application key | ||
| // whose plaintext cannot be returned. Same reasoning as | ||
| // discardOrganizationAPIKey: issuance is not retried, so a mint that never | ||
| // reaches the caller would otherwise be an orphaned live credential. |
There was a problem hiding this comment.
🟡 Suggestion: discardServiceAccountApplicationKey was inserted between discardOrganizationAPIKey's doc comment and its func. godoc now merges both comments onto the new function, and discardOrganizationAPIKey is left undocumented. Move the new function, with its comment, above the // discardOrganizationAPIKey ... block. (Confidence: high)
| require.True(t, ok) | ||
|
|
||
| issuer := newCredentialUserBuilder(datadogConnector.wrapper, false, true) | ||
| issuer := newCredentialUserBuilder(datadogConnector.wrapper, false, true, testAPIBaseURL) |
There was a problem hiding this comment.
🟡 Suggestion: The application key plaintext is now api_key_v2 JSON. This live smoke test still passes string(issued.PlaintextData[0].GetBytes()) to canAuthenticate as the app key, at lines 97 and 111. With DATADOG_CREDENTIAL_SMOKE=1 the usability check will time out, and the revocation check passes vacuously because the JSON is always rejected. Use appKeyPlaintextKeyValue(t, issued) there, and datadogConnector.apiBaseURL() instead of the fixture testAPIBaseURL here. (Confidence: high)
Superseded — see the current review report for commit
|
There was a problem hiding this comment.
Blocking issues found — see the full review report
Moving the application-key helper above the organization-key one left the organization-key comment glued to the application-key func. Each comment now sits directly above the function it describes. Also replace the scopesOrNil comment, which argued both that omitempty drops nil and empty lists and that the two are distinct. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Superseded — see the current review report for commit
|
Connector PR Review: IGA-5308: Emit Multipass api_key_v2 JSON for org API keysBlocking Issues: 1 | Suggestions: 4 | Threads Resolved: 0 Review SummaryThe new commit changes only comments and declaration order in
There is no behavior change. I scanned the full PR diff again for security and correctness: the non-vendor connector, client, I applied the repo-local criteria as follows:
Security IssuesNone found. Correctness Issues
Suggestions
Resolved prior findings
Prompt for AI agentsReviewed commit: |
There was a problem hiding this comment.
Blocking issues found — see the full review report
Squire (Together/DeepSeek-V4.1-Flash-1017):
What
Both Datadog credential kinds now issue as Multipass
api_key_v2credentials:key_value,key_id(Datadog's id, from thecreate response),
provider(datadog),base_url(the endpoint the keyauthenticates against: the
base-urloverride when set, otherwise what theconfigured site implies), and
header_name(DD-API-KEY).DD-APPLICATION-KEYasheader_nameand thescopesDatadog reported. Scopesthe provider did not report are omitted rather than emitted as an empty list,
which would assert the key is unscoped.
For both, the plaintext is
application/vnd.multipass.api_key_v2+jsonJSON,PlaintextData.Schemacarries that MIME type, and the descriptor declares itsoutput through
CredentialIssueOptionDescriptor.output. Multipass rejects nulls,so the optional fields use
omitempty.The payload and the declaration both come from the Multipass types in
pkg/multipass:multipass.APIKeyV2is the payload,multipass.Plaintextsetsthe schema from the type id, and
multipass.OutputField(name, multipass.TypeAPIKeyV2)builds the descriptor field. The declared type and theemitted payload therefore cannot disagree — both derive from
multipass.TypeAPIKeyV2. Tags are inherited frommultipass.Tags, nothardcoded, and currently come out as
[credential, api_key, multipass, api_key_v2].Behavior change
Paper-vault delivery of either Datadog credential kind now stores
api_key_v2JSON rather than the bare key: organization API keys and service account
application keys alike, because the connector cannot tell which delivery target
it is serving.
Until IGA-5311 ships, paper-vault
deliveries also lack the content type: the stored value is the right JSON, but
nothing records that it is
api_key_v2.Viewer rendering for the structured value is
IGA-5309, which is blocked by
IGA-5311.
Dependency
The Multipass types are a temporary copy, not a dependency.
ConductorOne/baton-multipass-sdkcannot be made public yet, sopkg/multipass/is copied from it verbatim — tests included — atv0.0.1, commit
b9f4b6776da475759bbcdc18f8209fedab389a6c(privaterepository, so the tag and SHA are given rather than a link). Each file carries
a header saying so, and naming
IGA-5320 as the ticket that
replaces the copy with the real module. The package keeps its name and exported
API, so that replacement is an import-path change only.
The descriptor
outputfield comes from baton-sdk #1218.go.modpinsv0.44.1-0.20261103220501-dc065ae56b48(the shrunken #1218, which no longercarries
pkg/secret) and the SDK is re-vendored.Do not merge before baton-sdk#1218 merges. The pin is a pseudo-version, so
the vendor diff also carries upstream baton-sdk commits landed since the tag it
builds on; those are not part of this change, and this PR will be repinned to the
release that includes #1218 before merge.
Tests
TestOrgAPIKeyIssueEmitsMultipassAPIKeyV2— drivesIssueand asserts thewire JSON has exactly
key_value,key_id,provider,base_url,header_name, that no value is null, and thatSchemaismultipass.ContentType(multipass.TypeAPIKeyV2).TestAppKeyIssueEmitsMultipassAPIKeyV2— same for the application key,including
scopes.TestAppKeyIssueOmitsScopesWhenProviderDidNotReportandTestAppKeyIssueCarriesReportedScopes— nil scopes are absent from thepayload; reported scopes are carried through.
TestOrgAPIKeyBaseURLFollowsOverride— an issued key advertises thebase-urloverride when one is configured.TestOrgAPIKeyDescriptorDeclaresMultipassOutput— both descriptors declarethe content type, display name,
IsSecretandmultipass.Tags(multipass.TypeAPIKeyV2).pkg/multipassruns the copied package's own tests.go build ./...,go vet ./...,go test ./...andgolangci-lint run(CI'sv2.14.0) pass. There is no live-provider run here: the org key smoke test stays
opt-in (
DATADOG_CREDENTIAL_SMOKE=1) and readskey_valuerather than the rawbytes.
Known CI
verify-versions-matchfails because.versions.yamlonmainpins baton-sdkv0.43.0, and this branch needs the SDK commit that adds the credential-issueoutput field.
.versions.yamlis managed by baton-admin and edits to it arerejected in PRs, so this branch cannot fix it. It clears once baton-sdk #1218
is released and baton-admin bumps the pinned SDK version.