Add rotate_db_encryption_key management command#1532
Open
amasolov wants to merge 7 commits intoansible:mainfrom
Open
Add rotate_db_encryption_key management command#1532amasolov wants to merge 7 commits intoansible:mainfrom
amasolov wants to merge 7 commits intoansible:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new Django management command Changes
Sequence DiagramsequenceDiagram
participant User
participant Command
participant Apps as "Django Apps Registry"
participant DB as "Database"
participant Crypto as "Crypto Utils"
participant Env as "Env / Settings"
User->>Command: invoke rotate_db_encryption_key (--use-custom-key / --dry-run)
Command->>Env: read current SECRET_KEY (old_key)
alt use custom key
Command->>Env: read EDA_SECRET_KEY
Env-->>Command: new_key (or error)
else generate new key
Command->>Crypto: generate random base64 key
Crypto-->>Command: new_key
end
Command->>Apps: discover models with EncryptedTextField
Apps-->>Command: list of (model, field) pairs
Command->>DB: begin transaction
loop per field (batched reads)
DB-->>Command: fetch batch (pk > last_pk, LIMIT ...)
Command->>Command: filter values containing "$encrypted$"
loop per matching row
Command->>Crypto: decrypt(ciphertext, key=old_key)
Crypto-->>Command: plaintext
Command->>Crypto: encrypt(plaintext, key=new_key)
Crypto-->>Command: new_ciphertext
alt not dry-run
Command->>DB: UPDATE row SET field=new_ciphertext WHERE pk=...
end
end
end
alt error occurs
DB->>DB: rollback
Command-->>User: report failure / raise CommandError
else success
DB->>DB: commit
Command-->>User: print "X value(s) re-encrypted" and printed new key (if generated)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/unit/commands/test_rotate_db_encryption_key.py (1)
33-67: Add a non-dry-run test that verifies actual re-encryption.These tests only assert command output. Please add coverage that creates a row with an
EncryptedTextField, runs the command withoutdry_run, then verifies the stored ciphertext changed and decrypts with the new key. That would cover the critical path inrotate_db_encryption_key.py:122-154, not just reporting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/commands/test_rotate_db_encryption_key.py` around lines 33 - 67, Add a non-dry-run unit test that creates a model instance containing an EncryptedTextField, saves it with an initial SECRET_KEY, captures the original ciphertext, runs call_command("rotate_db_encryption_key", use_custom_key=True/False as appropriate, dry_run=False) to perform real rotation (and set new key via os.environ["EDA_SECRET_KEY"] or settings.SECRET_KEY), reloads the instance and asserts the stored ciphertext changed and that decrypting the field with the new key yields the original plaintext; refer to the rotate_db_encryption_key command, call_command, EncryptedTextField and the SECRET_KEY/EDA_SECRET_KEY setup so the test covers the code path in rotate_db_encryption_key.py lines 122-154.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/aap_eda/core/management/commands/rotate_db_encryption_key.py`:
- Around line 117-120: The command leaks the encryption key because handle()
returns self.new_key (which BaseCommand.execute() will write to stdout) and also
explicitly writes it when not use_custom_key; remove the sensitive return value
by stopping handle() from returning the key—ensure you only call
self.stdout.write(self.new_key) when not use_custom_key and change the final
line that currently returns self.new_key to return None (or just return) so
BaseCommand.execute() does not print the key; locate symbols:
BaseCommand.execute(), handle(), self.new_key, use_custom_key, and
self.stdout.write to make this change.
- Around line 131-153: The current loop in rotate_db_encryption_key that uses
connection.cursor().execute(...) followed by cur.fetchall() (inside the for
model, field in fields block) loads all rows into memory and uses manual
double-quote string interpolation for identifiers; change it to iterate in
batches (use cursor.fetchmany(batch_size) or a server-side iterator) to avoid
fetchall and large memory spikes, and build SQL identifiers using
connection.ops.quote_name(model._meta.db_table) / quote_name(field.column) /
quote_name(model._meta.pk.column) instead of raw f-strings; keep the same SELECT
and UPDATE logic but page results (or use PK range/ORDER BY PK with LIMIT) and
update rows per-batch (still honoring dry_run) so the migration works across
different DB backends and large tables without OOMs.
---
Nitpick comments:
In `@tests/unit/commands/test_rotate_db_encryption_key.py`:
- Around line 33-67: Add a non-dry-run unit test that creates a model instance
containing an EncryptedTextField, saves it with an initial SECRET_KEY, captures
the original ciphertext, runs call_command("rotate_db_encryption_key",
use_custom_key=True/False as appropriate, dry_run=False) to perform real
rotation (and set new key via os.environ["EDA_SECRET_KEY"] or
settings.SECRET_KEY), reloads the instance and asserts the stored ciphertext
changed and that decrypting the field with the new key yields the original
plaintext; refer to the rotate_db_encryption_key command, call_command,
EncryptedTextField and the SECRET_KEY/EDA_SECRET_KEY setup so the test covers
the code path in rotate_db_encryption_key.py lines 122-154.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ba7fb550-9cf5-4512-8ff9-1fd74fa53722
📒 Files selected for processing (4)
src/aap_eda/core/management/commands/rotate_db_encryption_key.pysrc/aap_eda/core/utils/crypto/fields.pytests/unit/commands/test_rotate_db_encryption_key.pytests/unit/test_encryption.py
amasolov
force-pushed
the
feature/rotate-db-encryption-key
branch
2 times, most recently
from
fb21333 to
7b57742
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/aap_eda/core/management/commands/rotate_db_encryption_key.py (1)
115-123:⚠️ Potential issue | 🟠 MajorNew key is still printed twice to stdout when
--use-custom-keyis not set.
BaseCommand.execute()writes any truthy value returned byhandle()toself.stdout. Line 121 writesself.new_key, and then line 122 returns it, so Django writes it a second time. Operators piping the output to capture the new key will see it duplicated (and potentially copied incorrectly). Either print it or return it — not both.🔧 Suggested fix
- if not use_custom_key: - self.stdout.write(self.new_key) - return self.new_key - return None + if not use_custom_key: + return self.new_key + return NoneNote: the test in
tests/unit/commands/test_rotate_db_encryption_key.py::test_auto_generated_key_printedcurrently asserts onlylen(lines) >= 2, which passes regardless of the duplicate. Consider tightening it to assert the key appears exactly once.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/aap_eda/core/management/commands/rotate_db_encryption_key.py` around lines 115 - 123, The command currently both writes self.new_key to stdout and returns it from handle(), causing BaseCommand.execute() to emit the key twice; modify rotate_db_encryption_key.handle() so it either prints the new key OR returns it but not both — e.g., if not use_custom_key, either keep the self.stdout.write(self.new_key) and return None, or remove the write and return self.new_key — and update the test test_auto_generated_key_printed to assert the key appears exactly once instead of len(lines) >= 2; ensure references to dry_run, use_custom_key, and self.new_key in handle() are adjusted accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/aap_eda/core/management/commands/rotate_db_encryption_key.py`:
- Around line 115-123: The command currently both writes self.new_key to stdout
and returns it from handle(), causing BaseCommand.execute() to emit the key
twice; modify rotate_db_encryption_key.handle() so it either prints the new key
OR returns it but not both — e.g., if not use_custom_key, either keep the
self.stdout.write(self.new_key) and return None, or remove the write and return
self.new_key — and update the test test_auto_generated_key_printed to assert the
key appears exactly once instead of len(lines) >= 2; ensure references to
dry_run, use_custom_key, and self.new_key in handle() are adjusted accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 85a4d463-fd34-4966-9cb1-cc32c9890dd4
📒 Files selected for processing (4)
src/aap_eda/core/management/commands/rotate_db_encryption_key.pysrc/aap_eda/core/utils/crypto/fields.pytests/unit/commands/test_rotate_db_encryption_key.pytests/unit/test_encryption.py
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## main #1532 +/- ##
==========================================
- Coverage 91.93% 91.91% -0.03%
==========================================
Files 239 241 +2
Lines 10810 10942 +132
==========================================
+ Hits 9938 10057 +119
- Misses 872 885 +13
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 2 files with indirect coverage changes 🚀 New features to boost your workflow:
|
amasolov
force-pushed
the
feature/rotate-db-encryption-key
branch
2 times, most recently
from
08ae384 to
d193b0a
Compare
Add a Django management command to re-encrypt all EncryptedTextField columns after rotating the SECRET_KEY, similar to the existing awx-manage regenerate_secret_key in Automation Controller. The command supports: - Generating a new key (printed to stdout for deployment update) - Using a pre-generated key via EDA_SECRET_KEY env (--use-custom-key) - Dry-run mode (--dry-run) to report affected rows without writing - Full transaction wrapping for atomic re-encryption - Batched row fetching via fetchmany() to avoid OOM on large datasets - Backend-portable identifier quoting via connection.ops.quote_name() Encrypted columns are discovered dynamically via EncryptedTextField introspection rather than a static model list, so newly added encrypted fields are covered without updating this command. To enable explicit key material without mutating settings.SECRET_KEY, encrypt_string and decrypt_string now accept an optional key_material keyword argument. Signed-off-by: Alexey Masolov <amasolov@redhat.com> Made-with: Cursor
amasolov
force-pushed
the
feature/rotate-db-encryption-key
branch
from
d193b0a to
73916ad
Compare
Contributor
Author
Contributor
|
@amasolov will take a look at it soon. In the meantime can you please attend to the two SonarCloud issues? https://sonarcloud.io/project/security_hotspots?id=ansible_eda-server&pullRequest=1532&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true |
Move raw SQL construction into dedicated _build_select_sql() and _build_update_sql() static methods so cursor.execute() receives a plain variable rather than an f-string. This satisfies SonarCloud's S3649/S608 SQL injection hotspot detection without requiring manual triage in the SonarCloud UI. Also replace "return self.new_key" with an explicit self.stdout.write(self.new_key) to prevent BaseCommand.execute() from writing the generated key a second time to stdout. Signed-off-by: Alexey Masolov <amasolov@redhat.com> Made-with: Cursor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/unit/test_encryption.py (1)
14-16: 🛠️ Refactor suggestion | 🟠 MajorAdd negative assertions to detect when
key_materialparameter is ignored.The test passes regardless of whether the encrypt/decrypt helpers actually respect the
key_materialparameter. If both functions silently ignore it and always usesettings.SECRET_KEY, this test would still pass because every operation would use the same key. Add assertions with mismatched keys to catch this regression:Suggested test tightening
import pytest +from cryptography.fernet import InvalidToken from aap_eda.core.utils.crypto.fields import decrypt_string, encrypt_string @@ assert ciphertext.startswith("$encrypted$fernet-256$") assert decrypt_string(ciphertext, key_material=old_k) == value + with pytest.raises(InvalidToken): + decrypt_string(ciphertext) + with pytest.raises(InvalidToken): + decrypt_string(ciphertext, key_material=new_k) rewrapped = encrypt_string( decrypt_string(ciphertext, key_material=old_k), key_material=new_k, ) assert decrypt_string(rewrapped, key_material=new_k) == value + with pytest.raises(InvalidToken): + decrypt_string(rewrapped, key_material=old_k)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test_encryption.py` around lines 14 - 16, The current test for encrypt_string/decrypt_string does not assert behavior when key_material differs, so add negative assertions to ensure key_material is actually used: call encrypt_string with one key_material and assert that decrypt_string fails (raises or returns incorrect value) when called with a different key_material, and also verify that using settings.SECRET_KEY vs a distinct key yields different ciphertexts or decryption failures; update tests referencing decrypt_string, encrypt_string and key_material to include these mismatched-key assertions so the functions cannot silently ignore the key_material parameter.
🧹 Nitpick comments (1)
tests/unit/commands/test_rotate_db_encryption_key.py (1)
37-52: Seed a row so dry-run proves it does not write.This test currently passes with zero encrypted rows and does not verify persistence is unchanged. Capture the ciphertext before/after dry-run and assert the count.
Suggested dry-run coverage
def test_dry_run_reports_without_writing(settings): """--dry-run completes and reports without committing.""" settings.SECRET_KEY = "test-secret-for-rotation-command" + Setting.objects.create(key="dry_run_rotation", value="dry-run-secret") + with connection.cursor() as cur: + cur.execute( + "SELECT value FROM core_setting WHERE key = %s", + ["dry_run_rotation"], + ) + old_cipher = cur.fetchone()[0] + out = io.StringIO() with patch.dict( os.environ, {"EDA_SECRET_KEY": "new-secret-key-for-rotation"}, @@ stdout=out, ) - assert "would be re-encrypted" in out.getvalue() + assert "1 value(s) would be re-encrypted." in out.getvalue() + with connection.cursor() as cur: + cur.execute( + "SELECT value FROM core_setting WHERE key = %s", + ["dry_run_rotation"], + ) + new_cipher = cur.fetchone()[0] + assert new_cipher == old_cipher🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/commands/test_rotate_db_encryption_key.py` around lines 37 - 52, In test_dry_run_reports_without_writing, seed a database row with an encrypted column before invoking call_command("rotate_db_encryption_key", ...), capture its ciphertext (e.g., read the model instance’s encrypted field or raw DB column), run the command with dry_run=True, then re-read the ciphertext/count and assert the ciphertext and row count are unchanged and no new ciphertext was written; this proves dry-run does not persist changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/aap_eda/core/management/commands/rotate_db_encryption_key.py`:
- Around line 100-110: Add a guard after computing self.old_key and self.new_key
in the command (where use_custom_key is processed) to detect when the two keys
are identical and abort with a clear error; specifically, in the same scope
where self.old_key and self.new_key are set (referenced by the symbols
self.old_key, self.new_key and use_custom_key), compare them and raise
CommandError if they match (with a message like "New encryption key is identical
to the current SECRET_KEY; rotation aborted") so the rotation does not silently
become a no-op.
- Around line 175-181: The current loop in rotate_db_encryption_key.py uses
connection.cursor() (client-side) which buffers the entire SELECT; change it to
use a server-side cursor or PK-window pagination so scans are bounded-memory:
either create a named cursor (e.g., connection.cursor(name='batch_cursor') and
then execute select_sql and fetchmany(_FETCH_BATCH_SIZE)) or rewrite the scan to
paginate by primary key range (looping by max PK fetched and re-running a SELECT
... WHERE pk > last_pk ORDER BY pk LIMIT _FETCH_BATCH_SIZE), keeping the
existing call to self._reencrypt_rows(rows, update_sql, dry_run) and preserving
dry_run behavior. Ensure you close the cursor after use and that variable names
select_sql, update_sql, _reencrypt_rows, _FETCH_BATCH_SIZE and dry_run are used
consistently.
---
Outside diff comments:
In `@tests/unit/test_encryption.py`:
- Around line 14-16: The current test for encrypt_string/decrypt_string does not
assert behavior when key_material differs, so add negative assertions to ensure
key_material is actually used: call encrypt_string with one key_material and
assert that decrypt_string fails (raises or returns incorrect value) when called
with a different key_material, and also verify that using settings.SECRET_KEY vs
a distinct key yields different ciphertexts or decryption failures; update tests
referencing decrypt_string, encrypt_string and key_material to include these
mismatched-key assertions so the functions cannot silently ignore the
key_material parameter.
---
Nitpick comments:
In `@tests/unit/commands/test_rotate_db_encryption_key.py`:
- Around line 37-52: In test_dry_run_reports_without_writing, seed a database
row with an encrypted column before invoking
call_command("rotate_db_encryption_key", ...), capture its ciphertext (e.g.,
read the model instance’s encrypted field or raw DB column), run the command
with dry_run=True, then re-read the ciphertext/count and assert the ciphertext
and row count are unchanged and no new ciphertext was written; this proves
dry-run does not persist changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d2125391-330b-4df3-9d84-c3fc009a9919
📒 Files selected for processing (4)
src/aap_eda/core/management/commands/rotate_db_encryption_key.pysrc/aap_eda/core/utils/crypto/fields.pytests/unit/commands/test_rotate_db_encryption_key.pytests/unit/test_encryption.py
- Guard against no-op rotation when new key equals old key - Switch from fetchmany to PK-window pagination so the database driver only buffers one page of rows at a time - Strengthen dry-run test: seed a row and verify ciphertext is unchanged after --dry-run - Add negative key assertions to test_encryption.py: decrypting with the wrong key must raise InvalidToken Signed-off-by: Alexey Masolov <amasolov@redhat.com> Made-with: Cursor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/unit/commands/test_rotate_db_encryption_key.py (1)
82-96: Consider seeding a row fortest_auto_generated_key_printed_once.As written,
total=0because noSettingis created, so the test only verifies the "0 value(s)" branch. Seeding a row would also cover the common case where rotation actually occurred and still keep the "printed once" invariant.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/commands/test_rotate_db_encryption_key.py` around lines 82 - 96, The test test_auto_generated_key_printed_once never creates any Setting row so the command exercise only the "total=0" branch; before invoking call_command("rotate_db_encryption_key", ...) create/seed a Setting record (via the Setting model or test factory) with appropriate key/value so the rotation logic runs and still assert the generated key line appears exactly once; ensure you reference the Setting model used by the command and keep the rest of the assertions the same.src/aap_eda/core/management/commands/rotate_db_encryption_key.py (1)
183-197:last_pk = 0is fragile but not currently broken—consider future-proofing against non-integer PKs.The code assumes integer PKs, which works for all current EncryptedTextField-bearing models. However, since encrypted columns are discovered dynamically, a future refactoring adding a UUID or string PK to any of these models would silently fail or error (e.g.,
operator does not exist: uuid > integeron Postgres). To prevent this fragility, either seed the initial window with a type-safe sentinel (e.g.,last_pk = None) or run the first page without the PK predicate. This is low-priority now but worth addressing to avoid subtle bugs from future schema changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/aap_eda/core/management/commands/rotate_db_encryption_key.py` around lines 183 - 197, The loop in _reencrypt_column assumes integer PKs by seeding last_pk = 0; change it to last_pk = None and modify how you call _build_select_page_sql/_execute so the first page runs without a "pk > %s" predicate: update _reencrypt_column to initialize last_pk = None, call _build_select_page_sql(model, field, last_pk) (add an optional last_pk param), and in _build_select_page_sql emit the WHERE clause only when last_pk is not None; when executing the query pass [] for params on the initial run and [last_pk] thereafter; leave _build_update_sql and _reencrypt_rows unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/aap_eda/core/management/commands/rotate_db_encryption_key.py`:
- Around line 49-54: The iterator _iter_encrypted_text_fields currently iterates
model._meta.get_fields(), which includes inherited fields and can cause
duplicate or wrong-column processing; restrict iteration to fields defined on
the model itself by using model._meta.local_fields (or by filtering get_fields()
with field.model is model) and yield only fields that are instances of
EncryptedTextField, so the produced (model_class, field) pairs refer only to
columns that belong to that model.
---
Nitpick comments:
In `@src/aap_eda/core/management/commands/rotate_db_encryption_key.py`:
- Around line 183-197: The loop in _reencrypt_column assumes integer PKs by
seeding last_pk = 0; change it to last_pk = None and modify how you call
_build_select_page_sql/_execute so the first page runs without a "pk > %s"
predicate: update _reencrypt_column to initialize last_pk = None, call
_build_select_page_sql(model, field, last_pk) (add an optional last_pk param),
and in _build_select_page_sql emit the WHERE clause only when last_pk is not
None; when executing the query pass [] for params on the initial run and
[last_pk] thereafter; leave _build_update_sql and _reencrypt_rows unchanged.
In `@tests/unit/commands/test_rotate_db_encryption_key.py`:
- Around line 82-96: The test test_auto_generated_key_printed_once never creates
any Setting row so the command exercise only the "total=0" branch; before
invoking call_command("rotate_db_encryption_key", ...) create/seed a Setting
record (via the Setting model or test factory) with appropriate key/value so the
rotation logic runs and still assert the generated key line appears exactly
once; ensure you reference the Setting model used by the command and keep the
rest of the assertions the same.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 44b7370a-abe8-4d7c-976d-9f1ed68f6c01
📒 Files selected for processing (3)
src/aap_eda/core/management/commands/rotate_db_encryption_key.pytests/unit/commands/test_rotate_db_encryption_key.pytests/unit/test_encryption.py
Filter _iter_encrypted_text_fields() to only yield fields where field.model is the current model, preventing double re-encryption when multi-table inheritance causes get_fields() to return the same column for both parent and child models. Signed-off-by: Alexey Masolov <amasolov@redhat.com> Made-with: Cursor
The first page is now fetched without a PK bound predicate, and subsequent pages use the actual PK value from the previous batch. This removes the assumption that PKs are integers (last_pk = 0) and works correctly with UUID or any other ordered PK type. Signed-off-by: Alexey Masolov <amasolov@redhat.com> Made-with: Cursor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/aap_eda/core/management/commands/rotate_db_encryption_key.py`:
- Around line 129-135: The command currently prints self.new_key even during a
dry-run; change the logic so the generated key is only emitted when the command
will actually perform writes: update the condition that writes the key
(currently "if not use_custom_key: self.stdout.write(self.new_key)") to also
check that dry_run is False (e.g. "if not use_custom_key and not dry_run: ...")
inside the RotateDBEncryptionKey command/handle method so a generated key is
never shown during --dry-run.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5eef8484-643d-4cd2-bccc-cefae2baa167
📒 Files selected for processing (1)
src/aap_eda/core/management/commands/rotate_db_encryption_key.py
An operator could mistakenly persist a key printed during dry-run and break decryption for the unchanged database. Only emit the auto-generated key when writes actually occur. Signed-off-by: Alexey Masolov <amasolov@redhat.com> Made-with: Cursor
The previous run failed on test_advisory_lock.py::test_job_uniqueness [monitor_project_tasks], which is a pre-existing integration test unrelated to the encryption key rotation changes. Signed-off-by: Alexey Masolov <amasolov@redhat.com> Made-with: Cursor
|
ttuffin
approved these changes
Apr 23, 2026
Contributor
ttuffin
left a comment
ttuffin
left a comment
There was a problem hiding this comment.
LGTM, thanks for the contribution
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
3 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.
Closes #1531
What is being changed?
encrypt_stringanddecrypt_stringincore/utils/crypto/fields.pynow accept an optionalkey_materialkeyword argument, allowing callers to specify the encryption key explicitly instead of relying solely onsettings.SECRET_KEY.rotate_db_encryption_keyis added undercore/management/commands/. It re-encrypts allEncryptedTextFieldcolumns with a new key inside a single database transaction.Why is this change needed?
Operators need to rotate
SECRET_KEYperiodically for security compliance. Automation Controller hasawx-manage regenerate_secret_keyand Automation Hub haspulpcore-manager rotate-db-key, but EDA has no equivalent. After rotating the key, all encrypted data is permanently lost.How does this change address the issue?
The command provides a safe, atomic re-encryption path modelled directly after
awx-manage regenerate_secret_key:regenerate_secret_keyrotate_db_encryption_key@transaction.atomiconhandle()--use-custom-keywith env varTOWER_SECRET_KEYEDA_SECRET_KEYbase64.encodebytes(os.urandom(33))handle()returns new keyTwo improvements over the AWX counterpart:
EncryptedTextFieldintrospection (no static model list to maintain when new encrypted fields are added).--dry-runmode to report affected rows without writing to the database.Does this change introduce any new dependencies, blockers or breaking changes?
No new dependencies. The
key_materialparameter is optional and defaults tosettings.SECRET_KEY, so all existing callers are unaffected.How can it be tested?
key_materialparameter (tests/unit/test_encryption.py) and the management command (tests/unit/commands/test_rotate_db_encryption_key.py).ruff checkandruff formatpass cleanly.aap-eda-manage rotate_db_encryption_key --dry-runMade with Cursor
Summary by CodeRabbit
New Features
Tests