fix(kas): accept spec-compliant policy bindings

[pflynn-virtru]

Fixes the KAS half of #3578. Split out of #3597, which depends on this.

The bug

The spec defines the key access object's policy binding as Base64(HMAC(KEY, POLICY)) (spec). #2095 added a branch so KAS would accept that alongside the legacy Base64(hex(HMAC)). It has never worked:

policyBinding := make([]byte, base64.StdEncoding.DecodedLen(len(b64)))  // 44 chars -> 33 bytes
n, err := base64.StdEncoding.Decode(policyBinding, []byte(b64))          // n == 32
if n == 64 {
    dehexed := make([]byte, hex.DecodedLen(n))
    _, err = hex.Decode(dehexed, policyBinding[:n])
    if err == nil { policyBinding = dehexed }   // hex path: replaced, right-sized
}
// raw path: policyBinding is still the untrimmed 33-byte buffer
dek.VerifyBinding(ctx, policyBody, policyBinding)

DecodedLen over-allocates. Only the hex branch swaps in a correctly-sized slice, so the raw branch hands 33 bytes to VerifyBinding, which compares with hmac.Equal against a 32-byte digest. hmac.Equal is length-sensitive, so it is always false. Confirmed present in released service/v0.9.0.

Net effect: no released KAS can accept a spec-compliant policy binding, and any SDK that starts emitting one becomes undecryptable against it.

The fix

Extract decodePolicyBinding, trimmed to the bytes actually decoded, and use it on the rewrap path. Both encodings are accepted; length disambiguates them (32 vs 64), so no version signal is needed. Legacy hex stays accepted indefinitely — TDFs are archival.

The extraction is there to make the fix testable; the inline block it replaces was 17 lines and is now 6.

Checklist

• I have added or updated unit tests
• I have added or updated integration tests (if appropriate)
• I have added or updated documentation

Testing

TestDecodePolicyBindingIsTrimmed is the regression test, and it asserts the decoded length, with the over-allocation written out as a precondition. That matters: the buggy code produced the right bytes and only the wrong length, so a content comparison alone passes against it. Verified by reverting the one-line trim — the test fails with a trailing 0x0.

TestDecodePolicyBinding covers raw, legacy hex, and invalid base64.

go test ./service/kas/... -race green; golangci-lint clean on both files (the one sloglint hit in rewrap_test.go is pre-existing, just line-shifted by two added imports); make fmt clean.

Scope

Deliberately KAS-only: it widens what the server accepts and changes no output, so it can merge and deploy independently.

Left out on purpose, as separate concerns:

• The SDK writer fix (fix(sdk): align policy binding encoding with spec, keep legacy compat #3597). It must not land until a KAS carrying this fix is deployed, and it is a breaking change when it does — worth a ! marker. Note release-please-config.sdk.json uses always-bump-patch, so the version number won't signal it.
• Deleting verifyPolicyBinding. It has no production callers and is likely why the broken dual-accept looked correct — it reads like the verification path and its own hex handling is fine. Worth removing, but it is cleanup, not this fix.
• Java and JS need the same writer change; KAS is Go-only, so nothing server-side there.

Worth checking any third-party KAS for the same untrimmed-buffer bug before relying on raw bindings against it.

🤖 Generated with Claude Code

[coderabbitai]

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

• Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

Benchmark results, click to expand

Benchmark authorization.GetDecisions Results:

Metric	Value
Approved Decision Requests	1000
Denied Decision Requests	0
Total Time	163.08264ms

Benchmark authorization.v2.GetMultiResourceDecision Results:

Metric	Value
Approved Decision Requests	1000
Denied Decision Requests	0
Total Time	85.430059ms

Benchmark Statistics

Name	№ Requests	Avg Duration	Min Duration	Max Duration

Bulk Benchmark Results

Metric	Value
Total Decrypts	100
Successful Decrypts	100
Failed Decrypts	0
Total Time	328.864214ms
Throughput	304.08 requests/second

TDF3 Benchmark Results:

Metric	Value
Total Requests	5000
Successful Requests	5000
Failed Requests	0
Concurrent Requests	50
Total Time	42.096638428s
Average Latency	419.959582ms
Throughput	118.77 requests/second

[github-actions]

X-Test Failure Report

govulncheck-failure-3
govulncheck-failure-8
govulncheck-failure-1
opentdfplatformFOKSPB.dockerbuild

[github-actions]

Benchmark results, click to expand

Benchmark authorization.GetDecisions Results:

Metric	Value
Approved Decision Requests	1000
Denied Decision Requests	0
Total Time	248.372925ms

Benchmark authorization.v2.GetMultiResourceDecision Results:

Metric	Value
Approved Decision Requests	1000
Denied Decision Requests	0
Total Time	141.874384ms

Benchmark Statistics

Name	№ Requests	Avg Duration	Min Duration	Max Duration

Bulk Benchmark Results

Metric	Value
Total Decrypts	100
Successful Decrypts	100
Failed Decrypts	0
Total Time	426.282406ms
Throughput	234.59 requests/second

TDF3 Benchmark Results:

Metric	Value
Total Requests	5000
Successful Requests	5000
Failed Requests	0
Concurrent Requests	50
Total Time	1m1.632943326s
Average Latency	614.895236ms
Throughput	81.13 requests/second

[github-actions]

⚠️ Govulncheck found vulnerabilities ⚠️

The following modules have known vulnerabilities:

• otdfctl
• service
• tests-bdd

See the workflow run for details.

[github-actions]

X-Test Results

✅ js@main-main
bats-test-results
✅ js@main-pull-4081
govulncheck-failure-3
✅ js@main-v0.9.0
✅ go@main-main
✅ java@main-main
✅ java@main-pull-4081
✅ go@pull-4081-v0.9.0
opentdfplatformCRGXMA.dockerbuild
govulncheck-failure-8
✅ go@pull-4081-main
✅ go@pull-4081-pull-4081
✅ go@main-pull-4081
✅ java@main-v0.9.0
govulncheck-failure-1
cukes-report
✅ go@main-v0.9.0

[pflynn-virtru]

Security review: no findings

Reviewed this change specifically against the concern it invites — that relaxing policy-binding decoding opens a fail-open in an integrity control. It doesn't:

• The comparison stays secret-keyed and length-sensitive. The only production VerifyBinding is HMAC-SHA256 + hmac.Equal, so anything decodePolicyBinding returns must be exactly 32 bytes and equal to the HMAC computed under the unwrapped DEK.
• Both accepted encodings require the secret. Raw and hex are two spellings of the same HMAC; neither is producible without the DEK. The accepted set widens to exactly "base64 of the correct HMAC" and nothing else.
• Edge cases stay fail-closed: empty hash, invalid base64, 64 bytes of invalid hex, and any other length all fail the compare or short-circuit to err400.
• The old behaviour was a false negative, not a control. Trimming turns always-reject into correct-accept; it can't turn a false negative into a false positive.

Entitlement is unaffected — canAccess still runs after verification, over the same policy bytes that were HMAC'd.

One informational note, not a finding and not introduced here: the dead verifyPolicyBinding still logs the raw policyBinding value. Out of scope for this PR, but another small reason to delete it in the follow-up.

Worth saying plainly: this was an AI-assisted review, not a substitute for a human security sign-off on a change to an integrity check.

🤖 Generated with Claude Code