Skip to content

Validate admin API provisioner payloads; minor status-code fixes - #2755

Open
coffee4tw wants to merge 9 commits into
masterfrom
thor/admin-api-input-validation
Open

Validate admin API provisioner payloads; minor status-code fixes#2755
coffee4tw wants to merge 9 commits into
masterfrom
thor/admin-api-input-validation

Conversation

@coffee4tw

@coffee4tw coffee4tw commented Jul 29, 2026

Copy link
Copy Markdown

What

Hardens admin provisioner create/update handling and fixes the provisioner
webhook update response:

  • Centralized structural validation for POST/PUT /admin/provisioners:
    new writes reject missing or nil details and require the linkedca details
    oneof to match the declared provisioner type.
  • No duplicated per-type API rules: provisioner-specific semantic
    validation remains in provisioner.Interface.Init.
  • Bad-request classification: request-controlled conversion and initial
    Init failures return structured 400 errors instead of 500s.
  • Upgrade compatibility: legacy persisted records whose type and details
    do not match can still load during admin-resource reload; only new
    Store/Update writes enforce the invariant.
  • Webhook update PUT returns 200 OK instead of 201 Created (create
    still returns 201).
  • Adds a concise CHANGELOG entry under [Unreleased].

Compatibility

Successful, valid provisioner requests are unchanged. Malformed new writes
now fail with 400s. Read/reload conversion deliberately preserves historical
behavior for legacy mismatched records so an upgrade cannot turn one such
record into a CA startup failure.

The shipped ca.AdminClient treats any status below 400 as success on webhook
update, so the 201β†’200 correction is compatible with existing clients.

Test plan

  • Table-driven coverage for all supported linkedca provisioner details arms,
    missing details, missing oneofs, and typed-nil inner messages.
  • Regression coverage for type/details mismatches on both Store and Update
    returning 400.
  • Regression coverage proving legacy mismatched records still load through
    the admin-resource conversion path.
  • Coverage for invalid Update Init configuration returning 400.
  • Webhook responder coverage pins update at 200 and leaves create at 201.
  • go test ./... passes on the final pushed checkout.
  • go vet ./authority/... passes.
  • Every commit in the PR has a good SSH signature.

πŸ€– Generated with Claude Code

https://claude.ai/code/session_01LBsTWD57oxThZz44wVy3EL

coffee4tw and others added 5 commits July 29, 2026 15:27
Authority.UpdateProvisioner wrapped Init() failures as an internal
server error (500), unlike StoreProvisioner which correctly treats
them as a client-side configuration problem (400). Bad or incomplete
provisioner details submitted via UpdateProvisioner now surface as a
400, matching StoreProvisioner's behavior.
PUT (update) is not a resource-creation operation, so it should not
return 201 Created. Verified safe against the shipped ca.AdminClient,
which only checks for status >= 400.
@github-actions github-actions Bot added the needs triage Waiting for discussion / prioritization by team label Jul 29, 2026
@CLAassistant

CLAassistant commented Jul 29, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@coffee4tw
coffee4tw requested a review from a team July 30, 2026 11:12

@xhon-pelushi xhon-pelushi left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Read this against master. The direction looks right to me, and the status-code half is a clear improvement β€” UpdateProvisioner was returning 500 for Init failures while StoreProvisioner already returned ErrorBadRequestType for the same call, so this removes a real create/update asymmetry.

The type/details invariant is a bigger change than the description suggests. On master, ProvisionerToCertificates dispatches purely on the details oneof and never consults p.Type, so on writes the declared type was effectively decorative: a payload with type: ACME and JWK details produced a JWK provisioner recorded as ACME. Enforcing the invariant on Store/Update is the substantive part of this PR, and I think the CHANGELOG entry should say so directly β€” operators want to know that a payload shape which used to be accepted is now a 400.

One compatibility gap. The description says legacy persisted records stay loadable, and that is true β€” ProvisionerToCertificates still dispatches on details. But UpdateProvisioner now calls validateProvisionerTypeAndDetails(nu) before anything else, so a legacy record with a mismatched type/details pair can be loaded and used, yet any update to it now fails with a 400 β€” including edits that have nothing to do with the mismatch, like renaming it or changing its claims. As far as I can see that leaves an operator with no API path to correct the record either, since the corrected payload is the one being rejected. Either allowing an update whose details match the new type (i.e. validating the outcome rather than refusing the request), or documenting the manual fix, would close that. A short note in the CHANGELOG about how to detect affected records would help too.

Error text conflates three cases. provisionerTypeFromDetails returns errors.New("provisioner details are required") when details == nil, when the oneof is unset, and when the oneof is set but the inner message is nil β€” the third falls out of the switch because each arm only returns inside if d.X != nil. On an admin API that surfaces these as 400 bodies, "details are required" is actively misleading for the last two. Something like "provisioner details for type %s are empty" for the nil-inner case would save someone a debugging session.

A note on the sentinel. linkedca.Provisioner_NOOP is the enum zero value and a real member, so provisionerTypeFromDetails returning it on error means a caller that ever forgets to check err gets a plausible-looking type rather than something obviously invalid. It is not used anywhere else in this repo today, so this is only a robustness point, but returning (Provisioner_Type, bool, error) or naming a dedicated invalid constant would make misuse impossible.

Worth a test: the nil-inner-oneof case is the one that previously reached the type switch β€” for example ProvisionerDetails_JWK with JWK == nil would have hit json.Unmarshal(d.JWK.PublicKey, ...) and nil-dereferenced. The new guard closes that, which is a nice side effect worth pinning down. I could not convince myself whether protojson can actually construct that shape from an admin API request body (a "jwk": null may just leave the oneof unset), so it may be unreachable from outside β€” but a unit test asserting the 400 rather than a panic would keep it that way regardless of how the decoding path changes later.

The new tests in provisioners_test.go cover the matching and mismatching cases well; the above is really about the two edges around them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs triage Waiting for discussion / prioritization by team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants