PMM-14979 Detect and reject a mismatched encryption key - #5735
Draft
ademidoff wants to merge 17 commits into
Draft
PMM-14979 Detect and reject a mismatched encryption key#5735ademidoff wants to merge 17 commits into
ademidoff wants to merge 17 commits into
Conversation
…columns Encryption key rotation re-encrypted the JSON "option" columns (mysql_options, mongo_options, aws_options, azure_options, postgresql_options) during the DECRYPT phase instead of decrypting them, adding another encryption layer on every rotation. Root cause: encryption.Column exposed a single CustomHandler that was wired to the Encrypt* handler in DefaultAgentEncryptionColumnsV3. Both EncryptItems and DecryptItems invoked that same handler, so the decrypt pass of a rotation actually encrypted the values again. Each rotation therefore stacked two extra layers (decrypt-phase encrypt + encrypt-phase encrypt), growing tls_cert/tls_key ~80% per cycle until pmm-agent could no longer parse them ("tls: failed to find any PEM data in certificate input") and MySQL TLS monitoring broke. Fix: give Column separate CustomEncryptHandler and CustomDecryptHandler fields. EncryptItems uses the encrypt handler, DecryptItems uses the decrypt handler, and DefaultAgentEncryptionColumnsV3 wires both. The already-present but previously unused Decrypt*OptionsHandler functions are now correctly used during the decrypt phase. Also extends TestEncryptionRotation to store real encrypted PEM cert/key in mysql_options and assert they still decrypt to the original PEM across two consecutive rotations. The test fails on the old code and passes with the fix. Co-Authored-By: Claude Opus 4.8 <[email protected]>
Signed-off-by: theTibi <[email protected]>
Enforce the encrypt/decrypt handler pair invariant documented on encryption.Column with a generic round trip over every column of DefaultAgentEncryptionColumnsV3, so a column wired with a single handler for both directions fails a test instead of silently double-encrypting. Also harden the rotation test: use a real self-signed PEM key pair and assert it still loads via tls.X509KeyPair after rotation, drop the dependency on when the lazily initialized default encryption is created, remove the key files in t.Cleanup, and run the second rotation through rotateEncryptionKey to skip the supervisorctl round trips.
In HA every PMM Server node shares one PostgreSQL database but reads its encryption key from a local file. A node that generated its own key could not decrypt the credentials stored by the others, and that failure was silent: Decrypt returned the input ciphertext alongside the error, and agentEncryption logged a warning and assigned that value anyway. The ciphertext then travelled through Agent.DSN into SetState, so pmm-agent received it in place of a username and reported pq: password authentication failed for user "AQ+rKT/93psPS..." while QAN and postgres_exporter data disappeared for the affected services. Fail closed instead of degrading: - Decrypt and Encrypt now return an empty string on failure, so a caller that ignores the error cannot pass ciphertext off as plaintext, nor persist a secret unencrypted. The ciphertext is also no longer included in the base64 error message, which put secret material in the log. - EncryptAgent and DecryptAgent return an error, and the call sites in models, service_info_broker and realtimeanalytics propagate it. The error names the agent and the column, so a bad row can be identified. - Settings gains EncryptionKeyFingerprint, recorded in the same transaction that marks the columns encrypted. A node compares its own key against it at startup: under HA a mismatch is fatal, since the remedy is copying one file and continuing would hand out unusable credentials and write rows the other nodes cannot read. Standalone PMM logs the problem and exposes pmm_managed_encryption_key_mismatch so an installation whose key went missing does not stop booting on upgrade. Databases with no fingerprint recorded adopt the local key only if it decrypts what is already stored. - pmm-managed-init refuses to start an HA node that has no key, rather than letting one be generated per node. The documentation described the key file as 32 raw bytes, but the code writes a base64-encoded Tink keyset, so anyone following it to pre-provision a shared key would have hit a startup failure. It now documents the real format, how to generate a key, and that HA nodes must share one.
This was referenced Aug 5, 2026
…ey-between-nodes Signed-off-by: Alex Demidoff <[email protected]> # Conflicts: # managed/models/encryption_helpers_test.go
Extract decryptAgents/insertAgent helpers in models, reuse UpdateAgent in the service layer, and turn agentEncryption's copy-pasted per-field blocks into a field table. Gate the no-fingerprint key probe on agents.username actually being encrypted, and trim comments to the intent. Signed-off-by: Alex Demidoff <[email protected]>
Three ChangeAgent subtests inserted agents with plaintext credentials, and TestListSessions wrote a decrypted agent back with db.Update, so decryption rejected the rows once it started returning an error instead of a warning. Signed-off-by: Alex Demidoff <[email protected]>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #5735 +/- ##
==========================================
+ Coverage 43.59% 45.23% +1.63%
==========================================
Files 415 218 -197
Lines 43134 27914 -15220
==========================================
- Hits 18804 12626 -6178
+ Misses 22454 13905 -8549
+ Partials 1876 1383 -493
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Ticket number: PMM-14979
Feature build: Percona-Lab/pmm-submodules#4510
Important
Based on #5587 (
PMM-15188) to avoid conflicts: both changemanaged/utils/encryption/encryption.goandmanaged/models/database.go. Retarget tomainonce #5587 merges. Review the diff against that base.Problem
Reported on the forum against a 3-node Docker HA cluster: after pmm-agents reconnected, QAN and
postgres_exporterdata vanished for several PostgreSQL services, and agents loggedThat "username" is the encrypted value from the
agentstable. Followers only loggeddecryption: aead_factory: decryption failedat warning level.In HA all nodes share one PostgreSQL database, but each reads its encryption key from a local file, and
New()inmanaged/utils/encryption/encryption.gogenerates one whenever the file is absent.migrateDBencrypts on whichever node migrates first and records the columns insettings.EncryptedItems, so the other nodes skip encryption and keep an unrelated key.The failure was silent because
Decryptreturned the input ciphertext next to the error, andagentEncryptioninmanaged/models/encryption_helpers.gologged a warning and assigned that value anyway. The ciphertext then flowed throughAgent.DSN(managed/models/agent_model.go:543) intoSetState.A worse variant was reachable: if a follower won the migration race on a fresh database, the leader's later writes used a different key, leaving one table encrypted under two keys, which
pmm-encryption-rotationcannot repair.Changes
Fail closed rather than degrade
DecryptandEncryptreturn an empty string on failure, so a caller that ignores the error cannot pass ciphertext off as plaintext or persist a secret unencrypted. The ciphertext is no longer interpolated into the base64 error message, which was putting secret material in logs.EncryptAgent/DecryptAgentreturn an error; all 20 call sites inmanaged/models/agent_helpers.go,managed/services/agents/service_info_broker.goandmanaged/services/realtimeanalytics/service.gopropagate it. Errors name the agent and column.Detect a mismatched key
Settings.EncryptionKeyFingerprintplusencryption.Fingerprint(). Recorded indbEncryptionin the same transaction that marks the columns encrypted, so a concurrently starting node cannot see encrypted data with no fingerprint and adopt its own key. Cleared on the decrypt path, which keeps key rotation (decrypt -> new key -> encrypt) correct.models.VerifyEncryptionKeycompares the local key at startup. Under HA a mismatch is fatal: the remedy is copying one file, and continuing means handing out unusable credentials. Standalone PMM logs it and setspmm_managed_encryption_key_mismatch, so an installation whose key went missing does not stop booting on upgrade. Databases with no fingerprint adopt the local key only if it decrypts what is already stored.Prevent it at the source
pmm-managed-initrefuses to start an HA node with no key file instead of letting one be generated per node.build/docker/server/entrypoint.shruns underset -o errexit, so the non-zero exit does abort startup.Documentation
documentation/docs/admin/security/data_encryption.mddescribed the key as 32 raw bytes; the code writes a base64-encoded Tink keyset, so anyone following it to pre-provision a shared key would have hit a startup failure. Corrected, with an HA section.documentation/docs/install-pmm/install-HA-clustered.md(which had no mention of encryption) and a warning to the DockerPMM_HA_*table indocumentation/docs/install-pmm/install-pmm-server/deployment-options/docker/preview_env_var.md.Related
Testing
Added: encrypt/decrypt round trip and a regression test that a value this node cannot decrypt surfaces as an error instead of being returned (
managed/models/encryption_helpers_test.go); five fingerprint cases via sqlmock, including the reported follower scenario (managed/models/encryption_key_test.go); the HA preflight (managed/cmd/pmm-managed-init/main_test.go).go build ./...andgo vet ./...clean, golangci-lint reports 0 new issues against the base.Not verified locally:
testdb-backed suites need/srv/.postgres_password, so they fail withpq: password authentication failedhere. Failure counts are identical on this branch and the base, butTestDefaultAgentEncryptionColumnsRoundTripand therealtimeanalyticssuite are unexercised against these changes and need CI.