fix: read TPP's PkixParameterSet key-algorithm policy (25.1+) - #682
fix: read TPP's PkixParameterSet key-algorithm policy (25.1+)#682wallrj-cyberark wants to merge 1 commit into
Conversation
wallrj
left a comment
There was a problem hiding this comment.
Review of the PKIX parameter set handling. The inline comments cover a panic on server-controlled input, four ways the getpolicy → setpolicy round trip breaks, a fail-open fallback, and some duplication.
One more finding that's outside the diff: toZoneConfig (tpp.go#L906) wasn't updated for PkixParameterSet, so the zone's default KeyConfiguration is still derived solely from the deprecated fields. On a PKIX-locked folder where the deprecated KeyAlgorithm is empty/unlocked, zc.KeyConfiguration stays nil (the KeyType.Set("") error is swallowed by the bare return), UpdateCertificateRequest falls back to RSA-2048 (endpoint.go#L522-L548), and the generated key is then rejected by the same zone's new AllowedKeyConfigurations — so enrolment that relies on zone defaults fails.
with claude fable-5
| if sp.KeyPair.PkixParameterSet.Locked && len(sp.KeyPair.PkixParameterSet.Values) > 0 { | ||
| configs, err := allowedKeyConfigurationsFromPkixParameterSet(sp.KeyPair.PkixParameterSet.Values) | ||
| if err != nil { | ||
| panic(err) |
There was a problem hiding this comment.
This panic fires on server-controlled input: any OID missing from the 8-entry table (a future PQC algorithm, Ed25519, a new RSA size) crashes the whole process. Both callers of toPolicy have error returns (connector.go#L1649, #L1686) and there's no recover() in pkg/, so SDK consumers (cert-manager, for one) crash outright. BuildPolicySpecificationForTPP returns a clean error for the identical condition, so the same folder state gives a tidy error from getpolicy but a crash from enroll.
I know the surrounding code already panics on KeyType.Set, but those inputs are closed-world; this one is open-ended. Suggest giving toPolicy an error return rather than extending the pattern.
There was a problem hiding this comment.
Agreed, and fixed in c5564da. toPolicy now returns (endpoint.Policy, error); the four panics inside it (including the pre-existing KeyType.Set ones, since the error return made them free to convert) are now returned errors, and both callers in connector.go propagate them. The panic("unreachable") in the domainRegex closure is left alone — that one really is unreachable.
I also took the point that a single unknown OID shouldn't be fatal at all; see the reply on policyUtils.go:630.
| p.UpnSanRegExs = []string{} | ||
| } | ||
| if sp.KeyPair.KeyAlgorithm.Locked { | ||
| if sp.KeyPair.PkixParameterSet.Locked && len(sp.KeyPair.PkixParameterSet.Values) > 0 { |
There was a problem hiding this comment.
If TPP ever returns PkixParameterSet{Locked: true, Values: []} (an empty allow-list, or a serialisation quirk), this condition is false and we fall through to the deprecated KeyAlgorithm.Locked branch — which TPP 25.1+ no longer locks — and tpp.go#L1061-L1083 then emits every RSA size plus all supported curves: an unrestricted policy, no error, no log. That's the fail-open behaviour this PR is fixing. Suggest treating Locked && len(Values) == 0 as an error, like the unknown-OID case. Same pattern in BuildPolicySpecificationForTPP.
There was a problem hiding this comment.
Fixed in c5564da. The condition is now just sp.KeyPair.PkixParameterSet.Locked, and the shared decoder errors on an empty list, so Locked && len(Values) == 0 fails closed instead of falling through to the deprecated (and, on 25.1+, unlocked) fields. BuildPolicySpecificationForTPP gets the same treatment via the same decoder.
Guarded by TestConvertServerPolicyToInternalPolicy_PkixParameterSet/locked_with_an_empty_list_is_an_error,_not_an_unrestricted_policy and TestBuildPolicySpecificationForTPPPkixParameterSetLockedEmpty.
| // TPP 25.1+'s Certificates/CheckPolicy KeyPair.PkixParameterSet.Values, into AllowedKeyConfigurations. | ||
| // The OID table (policy.PkixToKeyAlgorithms) is shared with pkg/policy's getpolicy/setpolicy CLI path, | ||
| // which decodes the same OIDs from the same TPP API family. | ||
| func allowedKeyConfigurationsFromPkixParameterSet(oids []string) ([]endpoint.AllowedKeyConfiguration, error) { |
There was a problem hiding this comment.
This duplicates the decode loop in BuildPolicySpecificationForTPP (policyUtils.go#L627-L645) — same map lookup, same RSA/ECDSA switch, error strings already diverged by the tpp: prefix — and PkixToKeyAlgorithms is a second hand-maintained inverse of KeyAlgorithmsToPKIX. The missed TppRsaKeySize update (see comment there) shows what drift across sibling tables costs. One shared decoder in pkg/policy returning (rsaSizes []int, curves []string, err error) would serve both paths, and the inverse map could be derived from the forward map at init so future OID additions are single-site. The anonymous PkixParameterSet struct above also re-declares the wire shape of policy.LockedArrayAttribute (policyStructures.go#L202).
There was a problem hiding this comment.
All three points fixed in c5564da:
- The decode loop now lives once, in
policy.DecodePkixParameterSet, returning aDecodedPkixParameterSetwith the recognised OIDs plus the decoded key types, RSA sizes and curves.allowedKeyConfigurationsFromPkixParameterSetis reduced to mapping that ontoAllowedKeyConfiguration, andBuildPolicySpecificationForTPPonto the policy specification. One error-string source, so no moretpp:divergence beyond a single wrapping prefix. PkixToKeyAlgorithmsis now derived fromKeyAlgorithmsToPKIXat initialisation rather than hand-maintained, so adding an OID is a single-site change.TestPkixToKeyAlgorithmsIsInverseOfKeyAlgorithmsToPKIXasserts the round trip for every entry — and, since theTppRsaKeySizegap you found below is exactly this class of drift, it also asserts every RSA size in the table is accepted by validation.- The anonymous struct is replaced by
policy.LockedArrayAttribute.
| for _, oid := range oids { | ||
| entry, ok := policy.PkixToKeyAlgorithms[oid] | ||
| if !ok { | ||
| return nil, fmt.Errorf("tpp: policy allows unrecognised PKIX parameter set OID %q; vcert's OID table may need updating", oid) |
There was a problem hiding this comment.
Nit: unrecognised → unrecognized (US spelling), here and at policyUtils.go:630, plus the test names and strings.Contains assertions that match these strings.
There was a problem hiding this comment.
Fixed in c5564da — the affected strings were rewritten by the shared-decoder change anyway, and both they and the test names/assertions now use "unrecognized"/"recognizes".
| "2048": "1.3.6.1.4.1.28783.10.1.1.2048", | ||
| "3072": "1.3.6.1.4.1.28783.10.1.1.3072", | ||
| "4096": "1.3.6.1.4.1.28783.10.1.1.4096", | ||
| "8192": "1.3.6.1.4.1.28783.10.1.1.8192", |
There was a problem hiding this comment.
8192 is added to both OID maps but not to TppRsaKeySize (policyUtils.go#L19), so even a single-value rsaKeySizes: [8192] spec — this PR's headline scenario — is rejected by validateKeyPair (policyUtils.go#L167-L169, "specified rsaKeySizes doesn't match with supported ones"). The default-key check at L315 has the same gap.
There was a problem hiding this comment.
Good catch, and it is exactly the drift the derived inverse map is meant to prevent. Fixed in c5564da: TppRsaKeySize gains 8192, which covers both validateKeyPair and the default-key check (that one reads the same slice). 8192 is a real supported size elsewhere in vcert — certificate.AllSupportedKeySizes() has included it for a while, and README-PLAYBOOK.md documents it — so the table was the outlier.
TestPkixToKeyAlgorithmsIsInverseOfKeyAlgorithmsToPKIX now fails if any RSA size gets a PKIX OID without also being added to TppRsaKeySize.
| } else { | ||
| shouldCreateDefKeyPair = true | ||
| defaultKeyPair.RsaKeySize = &policy.KeyPairResponse.KeySize.Value | ||
| if policy.KeyPairResponse.PkixParameterSet.Locked && len(policy.KeyPairResponse.PkixParameterSet.Value) > 0 { |
There was a problem hiding this comment.
Two things with this branch:
- It bypasses the
defaultKeyPair/shouldCreateDefKeyPairresolution the legacy branch performs, sodefault.keyPairdisappears fromgetpolicyoutput for TPP 25.1+ folders, and a subsequentsetpolicyround trip erases the folder's defaults. - When
Lockedis false the PKIX values (recommended-only) are dropped entirely —KeyPairResponsehas no field to carry a PKIX default.
Also, Locked && len(Values) == 0 falls through to the deprecated (now-unlocked) fields — see the matching comment on the tpp.go branch.
There was a problem hiding this comment.
Both fixed in c5564da:
- The unlocked case now populates
defaults.keyPair:pkixParameterSetDefaultfrom the first OID (TPP takes a single default), plus the decodedkeyType/rsaKeySize/ellipticCurvefor readability.BuildTppPolicyalready round-tripsPkixParameterSetDefault, so agetpolicy→setpolicyof a recommend-only folder now preserves the default instead of erasing it. Covered byTestBuildPolicySpecificationForTPPPkixParameterSetNotLocked. Locked && len == 0is now an error — see the reply on the tpp.go branch.
One thing I have deliberately not done: in the locked case no defaults.keyPair is emitted. That matches the legacy branch, which also only creates defaults when a field is unlocked, and CheckPolicy's KeyPair response has no "PKIX Parameter Set Default" field for us to read even if we wanted to.
| for _, oid := range policy.KeyPairResponse.PkixParameterSet.Value { | ||
| alg, ok := PkixToKeyAlgorithms[oid] | ||
| if !ok { | ||
| return nil, fmt.Errorf("policy allows unrecognised PKIX parameter set OID %q; vcert's OID table may need updating", oid) |
There was a problem hiding this comment.
One unrecognised OID aborts the entire policy read (and the tpp.go equivalent panics). A folder that adds, say, ML-DSA alongside RSA-2048 makes vcert unusable for that zone even for clients only requesting RSA, until a vcert with an updated table ships. Consider skipping unknown OIDs with a warning — still fail-closed for enforcement — and erroring only when every OID is unknown.
There was a problem hiding this comment.
Agreed — fail-closed shouldn't mean fail-shut. Fixed in c5564da: DecodePkixParameterSet skips OIDs it doesn't recognise with a log.Printf warning and errors only when nothing in the list is recognised (or the list is empty). Skipping narrows the algorithms vcert will permit relative to the server, so it is still fail-closed, and a folder that adds ML-DSA alongside RSA-2048 stays usable for RSA clients.
Covered by .../an_unrecognized_OID_alongside_a_recognized_one_is_ignored and TestBuildPolicySpecificationForTPPPkixParameterSetPartiallyUnrecognized, with the all-unknown case still erroring.
| curves = append(curves, alg.Curve) | ||
| } | ||
| } | ||
| keyPair.KeyTypes = keyTypes |
There was a problem hiding this comment.
Round-trip bug: keyTypes gets "ECDSA" here, but the setpolicy write-back map KeyAlgorithmsToPKIX is keyed "ECC" (policyUtils.go#L36). BuildTppPolicy copies KeyTypes[0] into KeyAlgorithm (policyUtils.go#L410), and the lookup at connector.go#L1179 misses with no else branch, so no PKIX Parameter Set attribute is written — and on TPP ≥ 25 the legacy Key Algorithm branch is skipped too. Net effect: getpolicy → setpolicy of an ECDSA-locked folder silently leaves the target folder with no key-algorithm restriction at all.
There was a problem hiding this comment.
Fixed in c5564da, at the root: PkixToKeyAlgorithms is now derived from KeyAlgorithmsToPKIX, so the decoded key type is whatever the forward map is keyed by — "ECC", not "ECDSA". The KeyAlgorithmsToPKIX lookup at connector.go:1179 therefore hits, and it cannot drift again without TestPkixToKeyAlgorithmsIsInverseOfKeyAlgorithmsToPKIX failing.
Belt and braces: the spec's pkixParameterSet is now populated too (see the reply below), so on TPP ≥ 25 the write-back takes the tppPolicy.PkixParameterSet != nil branch and never depends on the KeyAlgorithm lookup at all. TestBuildPolicySpecificationForTPPPkixParameterSetEcc asserts the emitted keyType has an entry in KeyAlgorithmsToPKIX.
| } | ||
| } | ||
| keyPair.KeyTypes = keyTypes | ||
| keyPair.RsaKeySizes = rsaKeySizes |
There was a problem hiding this comment.
The multi-valued output here (e.g. rsaKeySizes: [4096, 8192], this PR's own test case) is rejected by vcert's own setpolicy validation: validateKeyPair errors on more than one value (policyUtils.go#L156-L172), and mixed RSA+ECDSA fails on keyTypes — yet README-CLI-PLATFORM.md documents getpolicy --file output as setpolicy input. Populating the spec's existing pkixParameterSet field (policySpecification.go#L32), which BuildTppPolicy already round-trips verbatim, would make the round trip lossless and also sidestep the ECC/ECDSA mismatch above.
There was a problem hiding this comment.
Adopted in c5564da. getpolicy now emits pkixParameterSet with the OIDs alongside the decoded keyTypes/rsaKeySizes/ellipticCurves, and validateKeyPair returns early — validating the OIDs instead — when pkixParameterSet is populated, since it is what BuildTppPolicy writes back and the decoded arrays are only the readable view of it. So the multi-valued output no longer trips vcert's own setpolicy validation, and the round trip is lossless.
TestBuildPolicySpecificationForTPPPkixParameterSet now feeds its own output through validateKeyPair to keep that honest, and README-CLI-PLATFORM.md documents the new field next to the existing "only a single value is allowed" note.
|
The
All ten inline findings plus this one are addressed in c5564da. Two of them changed behaviour beyond a straight fix, so flagging them explicitly:
Kept as a new commit rather than an amend, deliberately: Still outstanding on this PR: the |
From TPP 25.1 onwards, the Certificates/CheckPolicy endpoint no longer locks the deprecated KeyPair.KeyAlgorithm/KeySize/EllipticCurve fields when a policy folder's allowed key algorithms are configured via the newer AlgorithmSelector API. Instead the allowed algorithms are expressed as a list of PKIX OIDs in KeyPair.PkixParameterSet, which vcert did not read at all. Two independent consumers of that response were affected: - serverPolicy.toPolicy(), behind ReadPolicyConfiguration and ReadZoneConfiguration, reported every TPP 25.1+ folder as placing no restriction on AllowedKeyConfigurations, whatever an administrator had configured. Callers enforcing that policy, such as approver-policy-enterprise, therefore silently allowed certificate requests with the wrong key type or a weak key size. - BuildPolicySpecificationForTPP, behind the getpolicy/setpolicy CLI commands, likewise reported no key type or size restriction. Both now decode the PKIX parameter set through a single shared decoder, policy.DecodePkixParameterSet, backed by a PkixToKeyAlgorithms table derived from the existing KeyAlgorithmsToPKIX at initialisation rather than hand-maintained alongside it. Deriving the inverse keeps the two directions from drifting: it spells ECC the way KeyAlgorithmsToPKIX and BuildTppPolicy do, so a getpolicy of an ECC-locked folder no longer emits a keyType that the setpolicy write-back silently drops. A test asserts every OID inverts cleanly and that every RSA size in the table is accepted by validation, which is how the missing RSA 8192 entry in TppRsaKeySize was found. Decoding is deliberately fail-closed but not brittle. A locked but empty parameter set is an error in both paths, because falling through to the deprecated fields, which TPP 25.1+ leaves unlocked, would report the folder as allowing every RSA size and curve. An individual OID that this build does not recognise is skipped with a warning, and only a list with nothing recognisable in it is an error, so a folder that adds a future algorithm alongside a known one stays usable; skipping narrows what vcert permits, so it cannot widen a policy. Relatedly, toPolicy now returns an error instead of panicking, since the algorithm list is server-controlled and an unknown value must not take down a calling application. toZoneConfig derives the zone's default key configuration from the PKIX parameter set too, taking the first algorithm TPP offers. Previously the default stayed nil on such a folder, UpdateCertificateRequest fell back to RSA-2048, and the same folder's AllowedKeyConfigurations then rejected it; a test asserts the zone default satisfies the zone policy. For the CLI, the OIDs are carried through to the policy specification's pkixParameterSet field, which BuildTppPolicy already round-trips verbatim and which is the only key-algorithm attribute written on TPP 25 and above. The single-value keyPair checks are skipped when it is populated, so getpolicy output for a folder allowing several algorithms is valid setpolicy input; keyTypes, rsaKeySizes and ellipticCurves become the readable decoding of it. An unlocked parameter set is reported as defaults.keyPair rather than dropped. README-CLI-PLATFORM.md documents the new field. Verified live: `vcert getpolicy` against a TPP 25.1+ policy folder locked to RSA 4096/8192 via PkixParameterSet now reports keyTypes: ["RSA"], rsaKeySizes: [4096, 8192] instead of no restriction at all. Signed-off-by: Richard Wall <richard.wall@cyberark.com>
c5564da to
74dcacc
Compare
wallrj
left a comment
There was a problem hiding this comment.
Re-reviewed at 74dcacc. All nine inline findings and the toZoneConfig one from the review body are addressed — I checked the code, not just the replies:
toPolicyreturns errors (pre-existingKeyType.Setpanics converted too) and both callers propagate.Lockedwith an empty or all-unrecognised OID list is an error, not a fall-through to the unlocked deprecated fields.- Single shared
policy.DecodePkixParameterSet;PkixToKeyAlgorithmsderived fromKeyAlgorithmsToPKIXat init; anonymous struct replaced bypolicy.LockedArrayAttribute. - Unknown OIDs skipped with a warning, error only when nothing is recognised.
TppRsaKeySizegains 8192, with the inverse-map test guarding future drift.- Decoded key type is now
"ECC"(derived from the forward map), andpkixParameterSetis emitted verbatim withvalidateKeyPairaccepting it, so thegetpolicy→setpolicyround trip is lossless; README updated. - Unlocked PKIX values become
defaults.keyPairwithpkixParameterSetDefault; agreed that the locked case emitting no default matches the legacy branch and CheckPolicy offers nothing to read there. toZoneConfigderives the zone default from the first PKIX OID and returns errors.- US spellings throughout the new strings and tests.
Verified locally at 74dcacc: go build ./... passes and the pkg/policy tests pass, including the new PKIX ones. The pkg/venafi/tpp tests dial a live TPP from init (connector_test.go:80) so I couldn't run the new conversion tests offline.
I can't resolve the threads from this account (no write access on this repo) — they're all ready to resolve from yours.
with claude fable-5
|
Rebased onto master and squashed to a single commit (74dcacc). No content change from the reviewed state — the tree is byte-identical to the previous head, and the only thing that came in from master was the NGTS/playbook docs commit. Re-ran the downstream proof against the live TPP 25.1+ test instance on this exact commit, and all ten subtests pass, including the three that were previously skipped because the key-algorithm policy was not being read:
@simeoncybr your approval was recorded against the pre-review commit, so it will need re-applying if you are happy with the changes made in response to the review. |
Problem
From TPP 25.1 onwards, the
Certificates/CheckPolicyendpoint no longer locks the deprecatedKeyPair.KeyAlgorithm/KeySize/EllipticCurvefields when a policy folder's allowed key algorithms are configured via the newer AlgorithmSelector API. Instead, the allowed algorithms are expressed as a list of PKIX OIDs inKeyPair.PkixParameterSet.Values.vcert has two independent consumers of this same
CheckPolicyresponse, both of which only read the deprecated fields:pkg/venafi/tpp/tpp.go'sserverPolicy.toPolicy(), used byReadPolicyConfiguration/ReadZoneConfigurationto populateendpoint.Policy.AllowedKeyConfigurations— this is what a calling application (e.g. cert-manager'sapprover-policyVenafi plugin) uses to actually enforce policy.pkg/policy/policyUtils.go'sBuildPolicySpecificationForTPP, used by thevcert getpolicy/setpolicyCLI commands to report/manage policy.Both silently treated any TPP 25.1+ policy folder using the new PKIX mechanism as unrestricted:
endpoint.Policy.AllowedKeyConfigurationsended up containing every supported RSA size and ECDSA curve, andvcert getpolicyreported no key type/size restriction underpolicy.keyPairat all — regardless of what the TPP administrator had actually configured.Fix
PkixParameterSetfield toserverPolicy.KeyPair(pkg/venafi/tpp/tpp.go) and toKeyPairResponse(pkg/policy/policyStructures.go) to captureKeyPair.PkixParameterSet.Valuesfrom theCheckPolicyresponse in both code paths.policy.PkixToKeyAlgorithms(pkg/policy/policyUtils.go), mapping the PKIX OIDs (Venafi's IANA private enterprise arc,1.3.6.1.4.1.28783...) back to key type/size/curve — the inverse of the existing forward mappingKeyAlgorithmsToPKIX, which is also extended to cover RSA 8192 (previously missing).serverPolicy.toPolicy()andBuildPolicySpecificationForTPPboth now preferKeyPair.PkixParameterSet.Valueswhen locked, falling back to the deprecatedKeyAlgorithm/KeySize/EllipticCurvefields for TPP versions before 25.1.TestConvertServerPolicyToInternalPolicy_PkixParameterSet(single/multiple RSA sizes, a single ECDSA curve, mixed RSA+ECDSA, the legacy fallback, an unrecognised-OID error) andTestBuildPolicySpecificationForTPPPkixParameterSet/...UnrecognisedOID.Test evidence
Enforcement path (
serverPolicy.toPolicy()): verified end-to-end against a live TPP 25.3 instance using our internal cert-managerapprover-policyVenafi plugin's integration test suite, which calls this exact code path (ReadPolicyConfiguration/ReadZoneConfiguration→toPolicy()) to enforce key-type/size policy on CertificateRequests. Three integration subtests exercise this scenario against a policy folder locked to RSA 4096/8192 via the new PKIX mechanism — submitting an ECDSA key, an unsupported RSA size, and an RSA key below the locked minimum. Before this fix vcert silently allowed all three; consuming this branch via ago.mod replace, all three now correctly deny the request.CLI path (
BuildPolicySpecificationForTPP): verified live withvcert getpolicyagainst the same TPP 25.3 policy folder (locked to RSA 4096/8192 via PkixParameterSet). Before this fix:After this fix:
Also ran
go build ./...,go vet ./..., and the fullpkg/policyunit test suite locally (all passing).pkg/venafi/tpp's own test suite requires live TPP test credentials not set up for this exact package's test harness, so it wasn't run directly here — the two live verifications above exercise the same code paths against a real server.