Skip to content

ENG-2860 Add encryption_keys table#7661

Merged
erosselli merged 5 commits intomainfrom
erosselli/ENG-2860
Mar 17, 2026
Merged

ENG-2860 Add encryption_keys table#7661
erosselli merged 5 commits intomainfrom
erosselli/ENG-2860

Conversation

@erosselli
Copy link
Copy Markdown
Contributor

@erosselli erosselli commented Mar 16, 2026

Ticket ENG-2860

Description Of Changes

Adds the encryption_keys model and table (not wired up to anything yet) and updates the encryption implementation plan to better reflect the planned changes.

Code Changes

Steps to Confirm

Pre-Merge Checklist

  • Issue requirements met
  • All CI pipelines succeeded
  • CHANGELOG.md updated
    • Add a db-migration This indicates that a change includes a database migration label to the entry if your change includes a DB migration
    • Add a high-risk This issue suggests changes that have a high-probability of breaking existing code label to the entry if your change includes a high-risk change (i.e. potential for performance impact or unexpected regression) that should be flagged
    • Updates unreleased work already in Changelog, no new entry necessary
  • UX feedback:
    • All UX related changes have been reviewed by a designer
    • No UX review needed
  • Followup issues:
    • Followup issues created
    • No followup issues
  • Database migrations:
    • Ensure that your downrev is up to date with the latest revision on main
    • Ensure that your downgrade() migration is correct and works
      • If a downgrade migration is not possible for this change, please call this out in the PR description!
    • No migrations
  • Documentation:
    • Documentation complete, PR opened in fidesdocs
    • Documentation issue created in fidesdocs
    • If there are any new client scopes created as part of the pull request, remember to update public-facing documentation that references our scope registry
    • No documentation updates required

@vercel
Copy link
Copy Markdown
Contributor

vercel bot commented Mar 16, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

2 Skipped Deployments
Project Deployment Actions Updated (UTC)
fides-plus-nightly Ignored Ignored Preview Mar 17, 2026 1:10pm
fides-privacy-center Ignored Ignored Mar 17, 2026 1:10pm

Request Review

@erosselli erosselli marked this pull request as ready for review March 16, 2026 20:31
@erosselli erosselli requested a review from a team as a code owner March 16, 2026 20:31
@erosselli erosselli requested review from adamsachs and removed request for a team March 16, 2026 20:31
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Mar 16, 2026

Greptile Summary

This PR adds a new EncryptionKey SQLAlchemy model and corresponding Alembic migration to create a database table for storing wrapped Data Encryption Keys (DEKs), as part of the envelope encryption initiative. It also updates the implementation plan documentation to reflect the revised PR breakdown, unified DB storage for all providers, TTL-based caching, and KEK rotation design.

  • New EncryptionKey model with wrapped_dek, kek_id_hash, and provider columns — table is not wired up to anything yet (no behavioral change)
  • Alembic migration creates the table with proper down_revision chaining
  • Model registered in db/base.py for Alembic discovery
  • Implementation plan updated with re-numbered PRs, simplified schema (removed key_id, is_active, rotated_at), and detailed KEK rotation / TTL caching design
  • The table name encryption_keys uses a plural form, which deviates from the project's singular naming convention (e.g., connectionconfig, manual_task, digest_config)

Confidence Score: 4/5

  • This PR is safe to merge — it adds a new table with no behavioral impact on existing functionality.
  • Score of 4 reflects that this is a straightforward, additive change (new model + migration + docs) with no wiring to existing code paths. The only concern is the plural table name which deviates from project convention, but this is a style issue rather than a functional risk.
  • src/fides/api/models/encryption_key.py — table name convention should be reviewed before more code depends on this table name

Important Files Changed

Filename Overview
src/fides/api/models/encryption_key.py New model for storing wrapped DEKs. Uses plural table name encryption_keys which deviates from the project's singular naming convention. Otherwise clean and minimal.
src/fides/api/alembic/migrations/versions/xx_2026_03_16_1519_ecd49f8108e0_add_encryption_keys_table.py Standard Alembic migration creating the encryption_keys table with correct schema. Follows existing migration patterns. Table name should match any rename in the model.
src/fides/api/db/base.py Single-line import of the new EncryptionKey model, correctly placed in alphabetical order. Required for Alembic to detect the model.
docs/fides/docs/encryption/implementation-plan.md Documentation updates reflecting revised PR plan: simplified schema, unified DB storage for both providers, added TTL caching details, KEK rotation design, and re-numbered PRs. Well-structured and thorough.

Last reviewed commit: 2dd7c8a

@erosselli erosselli requested review from JadeCara and removed request for adamsachs March 16, 2026 21:08
Copy link
Copy Markdown
Contributor

@JadeCara JadeCara left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this all looks reasonable, there is an existing/known thing with sa.PrimaryKeyConstraint("id") and op.create_index(op.f("ix_encryption_keys_id"), "encryption_keys", ["id"], unique=False)

The PK constraint already creates a unique B-tree index. I am pretty sure the additional non-unique index is redundant. This comes from Alembic autogenerate picking up index=True on FidesBase.id — it's consistent with other tables in the codebase (same pattern everywhere), so it's a known codebase-wide quirk, not specific to this PR. Low priority.

@erosselli erosselli merged commit 07379b3 into main Mar 17, 2026
57 checks passed
@erosselli erosselli deleted the erosselli/ENG-2860 branch March 17, 2026 14:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants