fix(internal-plugin-encryption): fail closed on KMS x5c chain validation (FPV-531) - #5154
fix(internal-plugin-encryption): fail closed on KMS x5c chain validation (FPV-531)#5154akulakum wants to merge 4 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e09577aeee
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| * list is empty; deployments MUST provide the Webex KMS issuing roots. | ||
| * @type {Array<string>} | ||
| */ | ||
| caroots: DEFAULT_KMS_CAROOTS, |
There was a problem hiding this comment.
When a consumer supplies config.encryption.caroots: [], WebexCore.initialize() combines it with these defaults using lodash merge, which retains both default array entries; a one-element custom root similarly retains the second GoDaddy root. Consequently, the new empty-root guard never fails closed for an explicit empty array, and private-KMS deployments cannot restrict trust to a single custom CA. Ensure this array is replaced rather than index-merged during configuration normalization.
Useful? React with 👍 / 👎.
github-actions
Bot
removed
the
validated
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7cdd9752e2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| * @type {string[]} | ||
| */ | ||
| export const DEFAULT_KMS_CAROOTS = [ | ||
| // Go Daddy Root Certificate Authority - G2 (gdroot-g2.crt) |
There was a problem hiding this comment.
When an attacker can substitute the GET /kms/{userId} response—the scenario this change is intended to stop—these public GoDaddy roots do not authenticate Webex KMS specifically. validateKidHeader() only requires the attacker-supplied kid to use the kms: scheme, and validateCommonName() compares the certificate against that same attacker-supplied value. An attacker with a valid GoDaddy-issued certificate for a controlled domain can therefore provide kid: kms://attacker.example plus matching x5c and key parameters, pass the new chain validation, and retain control of the KMS identity. Bind kid to an independently trusted KMS hostname or use a trust anchor/constraint that identifies only Webex KMS.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d78ada7832
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| if (has(overrideConfig, 'encryption.caroots')) { | ||
| webexConfig.encryption.caroots = overrideConfig.encryption.caroots; |
There was a problem hiding this comment.
When a consumer retains and later mutates the array passed in config.encryption.caroots, this direct assignment mutates the SDK's active trust store as well. This differs from the normal merge({}, …) configuration path, which creates a separate array, and can unexpectedly add or remove trusted roots after initialization without calling setConfig(). Clone the explicit replacement array while preserving replacement rather than index-merge semantics.
Useful? React with 👍 / 👎.
1 participant
COMPLETES #FPV-531
This pull request addresses
FPV-531 — Webex JS SDK skips KMS certificate chain validation (ASIG / Flashpoint, P2)
Before talking to KMS, the SDK validates the KMS static-key identity from
GET /kms/{userId}(rsaPublicKeyJWK withx5cchain). The only step that actually authenticates the certificate —validateCertificatesSignature()— was conditional onconfig.encryption.caroots. That config key was never defined, socarootswas alwaysundefinedand chain validation was silently skipped in every deployment.An attacker who substitutes a self-signed KMS cert can impersonate KMS, harvest the OAuth bearer token embedded in KMS requests, and control all E2E encryption keys. This was demonstrated end-to-end against the integration environment (see Jira for reproduction).
by making the following changes
Fail closed in
kms-certificate-validation.js— treat missing or emptycarootsas a validation failure instead of skippingvalidateCertificatesSignature()(per ASIGfix.patch).Ship default CA roots in
config.encryption.caroots— addkms-default-caroots.jswith base64 DER trusted roots used by Webex KMS x5c chains (GoDaddy G2 roots, per CE-59676 / kms-1023-client-pkix-validation and the GoDaddy certificate repository).Fix unit tests — remove the test that accepted self-signed certs when no CA roots were configured; add tests asserting self-signed rejection with empty/default roots and that default roots are shipped.
Samples — remove
caroots: nulloverride indocs/samples/calling/app.jsso the SDK defaults apply.Preserve explicit
encryption.carootsoverrides —WebexCore.initialize()/setConfig()use lodashmerge, which combines arrays by index. That meantcaroots: []still kept default GoDaddy roots (fail-closed never triggered) andcaroots: [customCA]kept a second default root. After merge, replaceencryption.carootswhen the consumer explicitly sets it so private-KMS / empty-root deployments behave as intended.Change Type
The following scenarios were tested
yarn workspace @webex/internal-plugin-encryption test:unit --targets kms-certificate-validation.js— 20/20 passedyarn workspace @webex/internal-plugin-encryption test:unit --targets encryption-config.js— 3/3 passedx5cis rejected with emptycarootsand with default config rootscaroots: []and single custom root replace defaults (no index-merge with GoDaddy roots)The GAI Coding Policy And Copyright Annotation Best Practices
I certified that
Note for reviewers / ASIG: Default CA roots are GoDaddy public roots aligned with historical Webex KMS chain analysis (CE-59676). Please confirm with ASIG (Phillip McKnight) that current prod/integration KMS 3-cert chains still anchor to these roots; additional roots can be added to
kms-default-caroots.jsif needed.Config override behavior: Consumers can still set
config.encryption.carootsto[](fail closed) or a single custom CA for private KMS; those values replace defaults rather than merging with them.Make sure to have followed the contributing guidelines before submitting.