feat(connectorbuilder): resolve credential issuers on shape + output type - #1109
Conversation
…t type A connector could advertise at most one credential issue option per credential shape, so a provider minting two API-key-shaped credentials -- Datadog's organization API key and its service-account application key -- could only ever offer one of them. Identify an issuance option by the pair the connector already declares: its shape and the resource type the minted credential comes back as. - Registration dedupes on (option, secret_resource_type_id) rather than option alone, so two same-shape options with distinct output types both register. - IssueCredentialRequest and the issue-credential task carry secret_resource_type_id, and CredentialIssueInput surfaces it so a connector can branch on which kind was requested. - Issue-time resolution matches on the same pair. An absent output type resolves the sole descriptor for the shape, exactly as before, and is rejected as ambiguous only where a connector advertises more than one descriptor for that shape -- a declaration registration used to reject outright. Every existing connector and caller is unaffected. preferred_option stays a bare shape. It now expresses no preference among descriptors sharing that shape; naming one needs a companion field, left out until a caller consumes it. 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>
| CredentialOptions: request.GetCredentialOptions(), | ||
| ExpiresAt: request.GetExpiresAt(), | ||
| RequestID: request.GetRequestId(), | ||
| SecretResourceTypeID: request.GetSecretResourceTypeId(), |
There was a problem hiding this comment.
🟡 Suggestion: input.SecretResourceTypeID stays the caller's raw value, so a connector that branches on it still receives "" on the legacy single-descriptor path and must re-derive which descriptor the SDK actually selected. Since validateCredentialIssueInput already resolved a unique descriptor, consider normalizing after validation (input.SecretResourceTypeID = descriptor.GetSecretResourceTypeId()) so Issue always sees the resolved type — a connector that guesses wrong only finds out via validateCredentialIssueOutput, i.e. after the provider was already mutated and with no automatic retry, leaving an orphaned credential. The field is new in this PR, so nothing depends on it being empty today.
There was a problem hiding this comment.
Squire (claude-opus-5): This one is moot at the current head rather than fixed — flagging that rather than inventing a change.
CredentialIssueInput.SecretResourceTypeID was never added. The revision that moved the discriminator onto CredentialIssueOptions (0873c7c) dropped it, for the same reason the comment identifies: a flat copy beside CredentialOptions is a second place the selection can live and disagree with the first. pkg/connectorbuilder/credentials.go is byte-identical to main.
The normalisation the comment asks for is structural now: connectors read input.CredentialOptions.GetSecretResourceTypeId(), which is the value resolveCredentialIssueDescriptor matched the descriptor on, so there is no raw-versus-resolved pair to reconcile. Leaving unresolved for a human to close.
| // Resource type returned by IssueCredential. It must be registered with a | ||
| // ResourceDeleterV2 so every issued credential has a provider revoke path, | ||
| // including virtual credentials that cannot be listed later. | ||
| // including virtual credentials that cannot be listed later. Together with | ||
| // option it identifies this descriptor, so two credential kinds sharing one | ||
| // shape must return distinct resource types. |
There was a problem hiding this comment.
🟡 Suggestion: worth stating the rollout ordering here. Once a connector declares a second descriptor for a shape, every request that omits secret_resource_type_id for that shape starts failing as ambiguous — including the one that works today. Registration accepts the new declaration silently, so the break only surfaces at issue time, and the PR notes C1 does not populate the field yet. A sentence saying "declare a second same-shape descriptor only once callers populate IssueCredentialRequest.secret_resource_type_id" would put the constraint where connector authors will read it.
There was a problem hiding this comment.
Squire (claude-opus-5): Declined for the proto comment; the constraint is now recorded in the PR body under "Rollout ordering".
Two reasons. The narrower phrasing this asks for understates the requirement at the current head: secret_resource_type_id is required on every issuance request, not only against a connector that declares a second same-shape descriptor, so a single-descriptor connector is affected too. And the actual constraint is about vendoring order between two specific pull requests — do not vendor this SDK version into c1 before the caller-side PR populates the field — which goes stale in source the moment both land.
The proto comment already says the field is required, which is the whole of what a connector author reading it needs. Leaving unresolved for a human to disagree with.
| google.protobuf.Timestamp expires_at = 4; | ||
| // Forwarded verbatim to IssueCredentialRequest.secret_resource_type_id. | ||
| // Empty keeps the pre-existing shape-only selection. | ||
| string secret_resource_type_id = 5; |
There was a problem hiding this comment.
🟡 Suggestion: this field carries no validate.rules, while the IssueCredentialRequest.secret_resource_type_id it forwards into is bounded at max_bytes: 1024. Neither bound is actually enforced on the serving path — IssueCredentialRequest.Validate() is never called — so unlike request_id, whose proto rule is mirrored by hand in validateCredentialIssueInput, this one is decorative. Low impact (an oversized value just fails to match a descriptor), but mirroring the 1024-byte cap here and/or in validateCredentialIssueInput would keep the two hops consistent.
There was a problem hiding this comment.
Squire (claude-opus-5): Moot at the current head, not fixed by this pass.
The field this targeted — Task.IssueCredentialTask.secret_resource_type_id = 5 — no longer exists; 0873c7c removed it along with its "Forwarded verbatim" comment when the discriminator moved onto CredentialIssueOptions, which the task already carries as credential_options = 2. proto/c1/connectorapi/baton/v1/baton.proto is byte-identical to main.
The consistency point survives the move and was taken on the field that replaced it — see the resource.proto:234 thread. Leaving unresolved for a human to close.
General PR Review: feat(connectorbuilder): resolve credential issuers on shape + output typeBlocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0 Review SummaryThe new commits change only the Risk triage: durable and cross-consumer (proto wire types read by c1 and by future SDK versions), but both proto changes are new optional fields on released messages occupying previously-unused numbers, and widening the registration key from Security IssuesNone found. Correctness IssuesNone found. SuggestionsNone. Reviewed head |
…lIssueOptions The discriminator sat on IssueCredentialRequest as a sibling of credential_options, splitting the descriptor selection across two fields: the oneof arm gave the shape, the request field gave the kind within it. Move it onto CredentialIssueOptions beside the oneof so the selection is one value that travels wherever the options do. Task.IssueCredentialTask.secret_resource_type_id and its forwarding in the c1api handler go away with it -- the task already carries credential_options, so the discriminator rides along for free. Make it required and drop the empty-value resolution fallback. No caller predates the field, and the only real caller already rejects an empty value, so resolution collapses to a lookup on (option, secret_resource_type_id). The check is in Go, not a proto rule: this repo has no protovalidate dependency and the connector interceptor chain is recovery-only, so a min_bytes rule would read as enforcement and do nothing. 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>
| secretResourceTypeID := options.GetSecretResourceTypeId() | ||
| if secretResourceTypeID == "" { | ||
| return nil, fmt.Errorf("credential_options.secret_resource_type_id is required") | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion (rollout ordering): this makes secret_resource_type_id unconditionally required on a request contract that shipped in v0.25.1, so the requirement is global, not scoped to connectors that declare a second same-shape descriptor. Any caller that omits it fails with InvalidArgument — including single-descriptor connectors that work fine on main today. I can't verify the "no live producer" claim from this repo (the SDK's own producer at pkg/tasks/c1api/issue_credential.go:43 forwards credential_options wholesale, so it's clean), and the failure is loud and lands before issuer.Issue mutates the provider, which is the right failure shape. The residual is ordering: this SDK version must not be vendored into c1 before the caller-side PR that populates the field. Worth stating that constraint in the PR/release note so the two land in order.
There was a problem hiding this comment.
Squire (claude-opus-5): Agreed, and the constraint is now stated in the PR body under "Rollout ordering" rather than in a comment: do not vendor this SDK version into c1 until the caller-side PR that populates the field lands. That PR is ductone/c1#24250, held in draft for exactly this reason.
This and the connector.proto:131 thread are the same constraint stated at two widths; folded into the one note, at the wider width you have here, since the requirement is global rather than scoped to same-shape declarations. Leaving unresolved — nothing in the code changed for it.
|
Worth fixing (both trivial):
|
The zero-match error reported "credential option X producing secret resource type Y is not advertised" whether the shape was unadvertised or only the output type was, so an unadvertised shape read as though the shape existed. Branch on the lookup failure instead: an unadvertised shape says so on its own, and an unadvertised output type names the ones that shape does produce. Drop the decorative max_bytes rule on CredentialIssueOptions.secret_resource_type_id and bound it in Go beside the presence check. Nothing on the issuance path calls the generated Validate(), which is the same reason min_bytes was never added. Cover the case the tests could not distinguish: two API-key descriptors that differ in advertised scopes, asserting the selected descriptor's own constraints gate the request. 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>
|
Squire (claude-opus-5): Both addressed — one fixed, one already gone. 1 ·
Both cases are pinned by tests, including a negative assertion that the unadvertised-shape error does not name an output type. 2 · Head is now |
… registration The request-side length check had no registration-side counterpart, so a connector advertising an over-long output type registered cleanly and then failed every request naming it. Mirror the bound beside the existing presence check, where the rest of the descriptor's validation fails fast. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
The bound is mirrored in the proto on the descriptor and absent on the request, so "rather than in the proto" described only half of it. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
preferred_option picks a credential shape, which stopped naming a single descriptor once two can share one. Add bool preferred on the descriptor so the two together always resolve to one default for a caller presenting a choice. Registration requires exactly one preferred descriptor per shape that has several, and at most one per shape otherwise. A declaration that needs the flag is one that could not register before this branch, so nothing that registers today has to change. The flag selects nothing at issue time -- credential_options.secret_resource_type_id is still required. 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>
|
Squire (claude-opus-5): Selection is two-level and both levels ship here: Registration enforces that rather than documenting it — exactly one preferred descriptor per shape that has several, at most one per shape otherwise. The two new rejections cost no existing connector anything: a declaration with two descriptors sharing a shape is one
Not deprecating Body section rewritten from "open design question" to the resolved contract, with the registration table. |
preferred is a proto3 scalar with no presence and is only required where several descriptors share a shape, so on every connector shipping today the default descriptor has it unset. A consumer looking only for the flag finds nothing in the common case; one scanning the whole list can land outside the preferred shape. 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>
| // must whenever several descriptors share that option: a default taken from | ||
| // declaration order would not be stable. It selects nothing at issue time -- | ||
| // CredentialIssueOptions.secret_resource_type_id is still required. | ||
| bool preferred = 10; |
There was a problem hiding this comment.
Why not use the order of the CredentialIssueOptionDescriptor list in CredentialDetailsCredentialIssue as a priority list? Then there's no chance of violating the contract.
IGA-4107
What
A connector can advertise at most one credential issue option per credential shape (
API_KEY/KEYPAIR/TOKEN/CLIENT_SECRET). Registration dedupes on that shape alone, so a provider that mints two credentials sharing a shape can only ever offer one of them.Datadog is the live case: an organization API key and a service-account application key are both API-key-shaped.
baton-datadoghit this and shipped only the service-account half.The discriminator already exists and is already mandatory. Registration rejects a descriptor with an empty
secret_resource_type_id, so every descriptor already declares the resource type its credential comes back as. Dedup and resolution simply never looked at it.This identifies an issuance option by the pair the connector already declares: its shape and its output type — and names which of them is the default, so a caller presenting a choice has one selected.
The four edits
1 · Dedupe on
(option, secret_resource_type_id)—validateCredentialIssueCapabilityDetails,pkg/connectorbuilder/connectorbuilder.go. Two descriptors sharing a shape with distinct output types both register; the same output type twice is still a duplicate.2 · Resolve on the same pair, and require it —
resolveCredentialIssueDescriptorinpkg/connectorbuilder/credential_issue_validation.goreplaces the first-shape-match loop. Both halves of the selection are required, so the pair names at most one advertised descriptor and resolution is a lookup, not a search with fallbacks.An earlier revision of this PR treated an empty output type as "the caller predates this field" and resolved the sole descriptor for the shape. That branch serves nobody:
Task.IssueCredentialTaskhas no producer anywhere, and c1's only issuance call site already hard-fails on an empty value before it builds a request. Meanwhile the fallback's ambiguity error was reachable only once a connector used the new capability — i.e. it existed to soften exactly the case this PR exists to enable. Requiring the field now is the reversible direction; optional → required later would be a breaking tightening.The requirement is enforced in Go, not in proto. This repo has no
protovalidatedependency and the connector server's interceptor chain is recovery-only, so amin_bytes: 1rule would read as enforcement and do nothing —pkg/dotc1z/grants.go:448already says so aboutprincipal_id. The same reasoning applies to a length bound, soCredentialIssueOptions.secret_resource_type_idcarries novalidate.rulesat all: presence and the 1024-byte cap are both checked inresolveCredentialIssueDescriptor, and the proto comment names that as the enforcement point. Keeping a decorativemax_bytesbeside a deliberately-omittedmin_byteswould have been the inconsistency.3 · Carry the output type inside
CredentialIssueOptions—string secret_resource_type_id = 1, beside the oneof (arms start at 100, so field 1 was free for exactly this).CredentialIssueOptionsis already a selector: its oneof arm is what determines the shape, which is whycredentialIssueOptionKindhas to switch onWhichOptions(). Putting the output type onIssueCredentialRequestas a sibling ofcredential_optionssplit one selection across two fields at two levels, which could be read from different places and forwarded independently. On the options message the selection is a single value that travels wherever the options do.That is a strict reduction, not a relocation. It deleted the second wire hop this PR previously needed:
Task.IssueCredentialTask.secret_resource_type_id— gone. The task already carriescredential_options = 2, so the discriminator rides inside it for free.pkg/tasks/c1api/issue_credential.go— gone; that file is now byte-identical tomain.TestValidateCredentialIssueInputWithoutOutputTypeIsUnchanged, whose premise was false.CredentialIssueInput.SecretResourceTypeID— not added. A flat copy besideCredentialOptionswould reintroduce the same split in Go, where the two could disagree. Connectors readinput.CredentialOptions.GetSecretResourceTypeId().pkg/connectorbuilder/credentials.gois also back tomain.Net: the hand-written diff against
mainis now three files plus the new test file.4 · Name the default descriptor within a shape —
bool preferred = 10onCredentialIssueOptionDescriptor.preferred_optionalone stopped naming one descriptor the moment two could share a shape; the flag on the descriptor completes the selection, and registration enforces that it always resolves. Detail in Whatpreferred_optionmeans now below.The candidates that turned out to already be done
The brief flagged three things worth checking rather than assuming. All three already hold, so none of them is a separate edit:
secret_resource_type_idvalidateCredentialIssueOutput,credentials.go. Now stronger: the descriptor it checks against is the one the caller selected, so "you got what you asked for" is enforced per kind.ResourceDeleterV2registered for the declared output type ("revocability by construction")GetCapabilities. It iterates every descriptor, so it already covers the new multi-descriptor case. Test added to pin that.connector_min_expirycarriedIssuanceExpiryCapabilityhas bothminandmax, andvalidateCredentialIssueInputenforces both. The design's open question is about the C1-side model not capturingmin, not the SDK.Backward compatibility
Registration is fully compatible. A one-descriptor-per-shape declaration has a unique
(option, secret_resource_type_id)pair by construction, so widening the key can only accept declarations that used to be rejected. Nothing that registered before stops registering, and no connector needs a change.buf breaking --against mainpasses:CredentialIssueOptions.secret_resource_type_idis a new optional field, and the two fields the earlier revision added never shipped.Callers must now send the output type. This is a deliberate break of the earlier revision's fallback, not of anything released, and it is safe because there is no production caller to break:
IssueCredentialRequest. Every existing connector keeps working untouched.Task.IssueCredentialTaskhas no producer in c1 at any ref —taskToAPIhas noTask_IssueCredentialcase — so the queued-task path is not live.connector_actions_v2.go) already resolves the output type and already rejects an empty one. It just does not send it yet. That is the caller-side half, and it is a separate c1 PR.What
preferred_optionmeans now, and the flag that completes itCredentialDetailsCredentialIssue.preferred_optionis a bareCapabilityDetailCredentialOption. Once two descriptors can share a shape, a preferred option alone can no longer name which descriptor is preferred.preferred_option = API_KEYagainst a connector advertising both an org API key and a service-account application key does not say which one.A UI is being built against this, and it needs a default selected in the picker. So selection is now explicitly two-level, and both levels ship here:
preferred_optionpicks the shape. Unchanged meaning, unchanged validation: it must match the shape of at least one declared descriptor.CredentialIssueOptionDescriptor.preferred(bool, field 10, new) picks the descriptor within that shape.Together they always resolve to exactly one default. The flag lives on the thing preferred, so it cannot dangle the way a companion
preferred_secret_resource_type_idon the parent could — that alternative is a two-part pointer into a repeated field that must jointly resolve to one element, replacing a validation that can dangle one way with one that can dangle three.Registration enforces the guarantee rather than documenting it (
validateCredentialIssuePreference):preferredunsetpreferredsetpreferredpreferredpreferredThe two rejections cost no existing connector anything: a declaration with two descriptors sharing a shape is one that
mainrejects outright, so every declaration that registers today is in the first two rows. Requiring the flag exactly where ambiguity is possible is what lets a consumer assume a default always exists — a default the SDK picked out of declaration order would not be stable across a connector's own re-declaration.The rule a picker implements.
preferredis required only where a shape has several descriptors, so a single-descriptor shape is the default with the flag left unset — not as a legacy state, but as the permanent steady state for most connectors, since nothing will ever oblige them to set it:Scanning the whole
optionslist forpreferredis the wrong implementation: it can land on a descriptor outside the preferred shape. The rule is stated onpreferred_optionin the proto, which is where a consumer starts.The alternative — require exactly one
preferreddescriptor per shape unconditionally — gives a consumer a uniform rule with no fallback, and was rejected: it rejects every connector registering today, since none of them can set a field that does not exist yet. That is a fleet-wide registration break, an order of magnitude wider than the caller-side ordering constraint below.Preference is scoped per shape, not per declaration. A connector advertising two API keys and two tokens marks one of each, so a picker that lets the user switch shape still has a default in the new shape.
preferredselects nothing at issue time. It is advisory for a caller presenting a choice;CredentialIssueOptions.secret_resource_type_idis still required on every request, and resolution is still an exact lookup with no fallback. Making the flag a resolution fallback would reintroduce exactly the implicit selection edit 2 removed. There is a test pinning that an empty output type still fails even when a preferred descriptor exists.Not deprecating
preferred_option. It shipped inv0.25.1and is required at registration; it now has a well-defined narrower job (pick the shape) that composes with the new flag. Removing it is a wider decision on released public API and is not carried here.No Go helper resolving the pair. The rule is in the proto comment and enforced at registration, but there is no exported
PreferredDescriptor(details)in this PR: the consumer is c1's capability-to-offering mapping, so an SDK helper would ship with no in-repo caller. Unlike the field placement, a Go function is additive whenever it is added — the same asymmetry that put the proto field in now keeps the helper out.Consumers — checked, not changed
baton-datadog (
santhosh.kumar/credential-issuance) — verified against a localreplace(not committed; the working tree is back to clean). Built the real connector with a second descriptor forapi-keyalongsideservice-account-application-key, then ran the SDK's registration path viaGetMetadata→GetCapabilities:main:duplicate credential issue option CAPABILITY_DETAIL_CREDENTIAL_OPTION_API_KEY[service-account-application-key api-key]Both output types already have registered deleters (
apiTokenBuilderforapi-key,applicationKeyBuilderforservice-account-application-key), so the revocability invariant holds for both without further work. The connector also compiles unmodified against this branch. Restoring its org-API-key half is a separate PR.c1 — consumes this through its vendored copy (pinned
v0.25.1, identical tomainhere). Nothing in c1 was modified by this PR, and nothing in c1 breaks by merging it, because the issuance dispatch path is not live.The caller-side fix is a separate c1 PR, and it is a real defect rather than new plumbing:
resolveCredentialIssueResourceTypeFromCapabilities(pkg/temporal/activity/.../connector_actions_v2.go) already resolves the expected output type and hard-fails on empty, the request it then builds omits it and hardcodes theApiKeyarm, and the response is validated against the value that was never sent. With two same-shape descriptors that mints the wrong credential at the provider and then rejects it — a real key created, then discarded. That PR sets the field onCredentialIssueOptionsand builds the arm from the offering'scredential_option.Rollout ordering
secret_resource_type_idis required on every issuance request once this merges, not only against connectors that declare two same-shape descriptors. So the constraint is a single one, and it is about vendoring order, not about connector declarations:Do not vendor this SDK version into c1 until the caller-side PR that populates the field lands. That PR is ductone/c1#24250, held in draft for exactly this reason.
Cut this as a minor bump,
v0.26.0, not a patch. It changes the default behaviour of an RPC shipped inv0.25.1— a request that validated before now failsInvalidArgument. A downstream~v0.25constraint must not pick this up silently.pkg/sdk/version.gois generated from the tag, so this is an ask on the release tag rather than a file in this diff.Stating this here and in the release note rather than in the proto: the proto comment already says the field is required, which is the whole of what a connector author needs, and a release-coordination fact between two specific pull requests goes stale in source the moment they land.
The narrower phrasing — "declare a second same-shape descriptor only once callers populate the field" — is not the right note, because it understates the requirement. A single-descriptor connector is affected too.
Verification
go test ./...(-tags=baton_lambda_support): all packages pass.golangci-lint run: 0 issues.go vet: clean.buf lint,buf format: clean.buf breaking --against main: passes.main.preferredselecting nothing at issue time.scopes: ["read"]andscopes: ["write"], asserting each scope is accepted by the descriptor advertising it and rejected by the other, both throughvalidateCredentialIssueInputand end-to-end throughIssueCredential. A shape-only regression passes the selection tests and fails these.secret_resource_type_idis checked on both sides in Go — at registration invalidateCredentialIssueCapabilityDetailsand per-request inresolveCredentialIssueDescriptor— so an over-long output type fails fast at registration rather than on every issuance.Local toolchain note:
go1.27cannot build the vendoredcockroachdb/swiss, which build-tags[go1.20, go1.27). Pre-existing onmainand unrelated to this change; everything above ran underGOTOOLCHAIN=go1.26.0.CI: 12/12 green as of
85ca3770; re-running against1ff7ce2d.