Repository navigation
Ruby SDK v17.2.1 - #1108
Ruby SDK v17.2.1#1108
Conversation
…1107) prepare_create_payload assigned the raw AES-256 record key to payload.record_key with no encryption, while the same request carried the record data encrypted under that key. The server therefore received both a record's ciphertext and the key that opens it, which breaks the zero-knowledge property for every record the Ruby SDK creates. Present since 17.1.0, so 17.1.0 and 17.2.0 are both published with it. The fix reads appOwnerPublicKey from storage, fails if it is absent, and wraps the record key with Crypto.encrypt_ec, mirroring Python prepare_create_payload. Ruby's upload_file path already did this correctly, so only the create-record payload was affected. The hard failure is deliberate: a fallback to sending the key unwrapped would leave the hole open for exactly the older applications most likely to lack the key. Three regression tests assert the property on the payload bytes rather than on the code path. The second one is the defect stated as an assertion: it first confirms the record key really does decrypt payload.data, then requires that the value on the wire is not those bytes. The third forbids the silent-fallback fix. Before the fix all three fail, and the second reports expected and actual as byte-identical 32-byte strings. Five existing create_secret specs shared a config fixture with no owner public key, which is why the defect shipped twice with a green suite. The fixture now carries a real P-256 key. No assertion in those specs changed. Verified: 783 examples, 0 failures. Live check against QA created a record, confirmed payload.recordKey is 125 bytes beginning with 0x04 rather than the 32-byte plaintext key, read the record back with its field values intact, and deleted it.
The entry named 17.1.0 and 17.2.0. All four published versions carry the defect, including 17.0.3 and 17.0.4. Verified by downloading each gem from RubyGems and reading prepare_create_payload: the unwrapped assignment is present in every one, byte-identical. It is also present in the SDK's first commit, d026001, so it was never correct rather than introduced by a later change. The git history misled the original entry because the Ruby SDK's history in this repository begins at the v17.1.0 release, four months after 17.0.3 had already published. This matters to a reader rather than only to the record: naming two versions implicitly cleared the other two, so a customer on 17.0.4 would have taken a security note as an all-clear.
mgallego-keeper
left a comment
mgallego-keeper
left a comment
There was a problem hiding this comment.
Approving. I verified the fix independently end to end; details below. The asks in the inline comments are non-blocking, but the docs ones only count if they land before the gem publish dispatch, since README, CHANGELOG, and gemspec metadata are baked into the 17.2.1 package.
Verification performed
- Full suite locally on Ruby 3.3.8: 783 examples, 0 failures, 13 pre-existing pending; the
KEEPER_MOCK_MODEoffline integration test is green. - All three new specs fail on the unfixed code for the advertised reasons (raw key on the wire, EC point parse failure on decrypt, no raise).
core_spec.rbalone passes 77/77 in both fixed and unfixed states, which confirms the description's explanation of why the old suite never caught this. - Cross-language wire check: bytes from Ruby
Crypto.encrypt_ecdecrypt with the Python SDK'sdecrypt_ecrecipe; identical ECIES construction (ephemeral P-256, ECDH x-coordinate, SHA-256 KDF, AES-256-GCM), 125 bytes with leading0x04for a 32-byte key, matching the live QA observation in the description. - Owner-key decode verified against all four stored formats (url-safe or standard alphabet, padded or unpadded), including keys whose base64 contains
+or/, so Commander-written configs, Web Vault configs, and the SDK's own binding format all work. - All six other SDKs hard-fail without
appOwnerPublicKeyas claimed; the new message is byte-identical to Python's current one. client_versionderives fromVERSION, so fixed clients stampmb17.2.1, cleanly separable frommr17.0.0/mr17.91.0/mb17.2.0for server-side forensics.- The branch carries exactly these six files over master (ahead 3, behind 0), and the affected-version list matches RubyGems (17.0.0-17.0.2 were never published).
- Payload sweep found no sibling issue: every other key the SDK transmits is wrapped (folderKey, file keys, shared-folder traversal), and
UpdatePayloadhas no key field, confirming the cannot-rotate statement.
One ask that cannot be an inline comment (file not in the diff): keeper_secrets_manager.gemspec:18 sets changelog_uri to a repo-root CHANGELOG.md that does not exist, so the Changelog link on the 17.2.1 rubygems.org page will 404 at exactly the moment users come looking for this advisory. One-line change to .../blob/master/sdk/ruby/CHANGELOG.md; worth including in this release since the metadata ships in the gem.
PR description nit: "Breaking Changes: None for any configuration that can create records today" contradicts the sentence that follows it; a configuration without the owner key can create records today (that is the vulnerability) and will raise after upgrading. "Any correctly provisioned configuration" is probably the intended meaning.
Optional, while both docs files are open: the neighboring dates look wrong: 17.1.0 is dated 2025-01-06 in both files while the gem published on 2025-11-06 (looks like a 01/11 transposition), and the README's 17.2.0 entry says 2025-11-14 while the gem published on 2026-07-20. If 17.2.1 gets a date, best not to propagate the pattern.
Pre-existing items found during the review are filed separately so nothing extra lands on this security patch: KSM-1223 (undefined KEY_ENCRYPTED_APP_KEY raises NameError on the no-appKey path), KSM-1224 (get_file_data output incompatible with download_file), KSM-1225 (redact key material from debug logging), KSM-1226 (owner-key handling unification and payload-builder hardening), KSM-1227 (remove the silent AES-GCM to CBC fallbacks), KSM-1228 (0600 on ksm_cache.bin). The application-log aspect of the exposure is noted on KSM-1193.
8525296
|
Thank ya, here are my additions:
|
mgallego-keeper
left a comment
mgallego-keeper
left a comment
There was a problem hiding this comment.
Re-reviewed at 8525296; everything verified on the new head.
- CHANGELOG: entry now under
### Security(matching this file's own 17.0.2 precedent),[17.2.1]compare link defined, and both wording fixes are in (never-published clause, scoped re-creation sentence). - README: documents the new raise and the actual remedy (re-bind with a fresh one-time token, or
KSM_APPOWNERPUBLICKEYon the read-onlyEnvironmentStorage). - Gemspec:
changelog_urifixed. I built the gem from this head and the packaged metadata carries the corrected link (keeper_secrets_manager-17.2.1.gem->blob/master/sdk/ruby/CHANGELOG.md). - Dates: removing them from all version headers instead of adding one is a sensible resolution; the provably wrong dates are gone and rubygems.org remains the single source of truth for publish timestamps.
- Deferrals (guard error contract, spec strengthenings) match what KSM-1226 already tracks; agreed they do not belong on this security patch.
- Re-verified locally on 8525296: 783 examples, 0 failures, 13 pre-existing pending; offline mock test green;
gem buildclean at 17.2.1.
Resolving the five threads. Still approved; good to publish and merge.
- move KSM-1193 CHANGELOG entry under ### Security (matches file's own precedent for security items) - clarify 17.0.0-17.0.2 were never published to RubyGems - scope "existing records" wording to records created by an earlier version - add missing [17.2.1] compare link - drop dates from all version headers in CHANGELOG.md/README.md - README: document create_secret's new raise and the re-bind/env-var remedy - gemspec: fix changelog_uri to point at sdk/ruby/CHANGELOG.md, not a nonexistent repo-root file
8525296 to
dd78c5a
Compare
mgallego-keeper
left a comment
mgallego-keeper
left a comment
There was a problem hiding this comment.
Re-approved at dd78c5a. The branch was force-pushed over my previous approval with one amendment: two lines of advisory wording in CHANGELOG.md and README.md ("placed the record key in the create payload unencrypted" becomes "without that wrap"), which is the more precise phrasing given the encrypted transport envelope. Verified the full tree delta from the previously approved 8525296 is exactly those two lines and nothing else, and that the published gem (17.2.1, sha256 ca73f928d6d51e421394ecfb1ca03181e5a3a4c8823157a906494b0b0206a7a7) is byte-identical to this head: lib/ and the gemspec match the reviewed code, the docs match the amendment. Cumulative diff vs master remains the same seven files. Still approved.
This branch was previously deployed
3 participants
Summary
Ruby SDK v17.2.1: a single security fix in `create_secret`. This is a patch release and carries nothing else.
Changes
Security Fixes
Maintenance
Security Impact
`create_secret` was omitting the record key wrap step. All records created via the Ruby SDK through v17.2.0 are affected. Upgrade to 17.2.1 to protect newly created records. Existing records are not remediable through the SDK.
Breaking Changes
`create_secret` now raises when the application config carries no `appOwnerPublicKey`, where it previously continued silently. This brings Ruby in line with all other SDKs.
Related Issues