-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
docs(encryption): Add docs for encrypted field usage #15888
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
| - backend | ||
| - encryption | ||
| - django | ||
| sidebar_order: 10 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This puts the document at the top of Backend -> Application Domains - don't think we should be putting it there, since this will not be used by most people. Would rather put it after Metrics, which would be order 190.
| sidebar_order: 10 | |
| sidebar_order: 190 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make sense
|
|
||
| ### [EncryptedJSONField](#encryptedjsonfield) | ||
|
|
||
| A drop-in replacement for Django's `JSONField` that encrypts JSON data. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this replacement support the arguments that the original JSONField supports, especially special encoders & decoders?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, unfortunately it doesn't, i'll remove a term "drop-in" as it is not really a drop-in replacement.
|
Reads well already! I think structurally, we can improve this by reordering a bit and grouping this slightly differently. It may pay off to have two sub-pages for using encrypted fields, and for all of the admin work around it: User-facing:
Admin-related:
|
| secret = EncryptedCharField() | ||
| secret_hash = models.CharField(max_length=64, db_index=True) | ||
|
|
||
| def save(self, *args, **kwargs): | ||
| if self.secret: | ||
| self.secret_hash = hashlib.sha256(self.secret.encode()).hexdigest() | ||
| super().save(*args, **kwargs) | ||
|
|
||
| # Query by hash | ||
| MyModel.objects.filter(secret_hash=hashlib.sha256(b"my-value").hexdigest()) | ||
| ``` |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| # Using the model | ||
| creds = TempestCredentials.objects.create( | ||
| client_id="my-client", | ||
| client_secret="super-secret-value", | ||
| metadata={"api_version": "v2", "scopes": ["read", "write"]} | ||
| ) | ||
|
|
||
| # Reading works transparently | ||
| print(creds.client_secret) # Prints: "super-secret-value" | ||
| print(creds.metadata) # Prints: {"api_version": "v2", "scopes": ["read", "write"]} | ||
| ``` |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| ### Adding New Encrypted Fields | ||
|
|
||
| Add the field to your model and generate a migration: | ||
|
|
||
| ```python | ||
| class MyModel(models.Model): | ||
| api_key = EncryptedCharField(null=True, blank=True) | ||
| ``` | ||
|
|
||
| ```bash | ||
| sentry django makemigrations | ||
| sentry upgrade | ||
| ``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ohh damn, nice catch
| ```python | ||
| from cryptography.fernet import Fernet | ||
|
|
||
| key = Fernet.generate_key() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there any cryptographic options we should put here? Or are these all centrally "managed"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nothing, there are not special options for Fernet
shellmayr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great now from my perspective 👍
| class Migration(CheckedMigration): | ||
| is_post_deployment = True | ||
|
|
||
| dependencies = [ | ||
| ("myapp", "0002_alter_mymodel_api_key"), | ||
| ] | ||
|
|
||
| operations = [ | ||
| migrations.RunPython(encrypt_existing_data, migrations.RunPython.noop), | ||
| ] | ||
| ``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: The documentation code example for forcing encryption uses migrations.RunPython without importing the migrations module, which will cause a NameError during migration.
Severity: MEDIUM
🔍 Detailed Analysis
The code example provided in the encrypted fields documentation for creating a data migration is incomplete. It uses migrations.RunPython within the operations list, but it fails to import the migrations module. When a developer copies this code to create a migration file and runs it, the process will fail with a NameError: name 'migrations' is not defined. This will block the migration process, preventing developers from successfully following the documentation to encrypt fields.
💡 Suggested Fix
Add the line from django.db import migrations at the top of the code example to ensure the migrations module is available when the code is executed.
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: develop-docs/backend/application-domains/encrypted-fields/index.mdx#L156-L174
Potential issue: The code example provided in the encrypted fields documentation for
creating a data migration is incomplete. It uses `migrations.RunPython` within the
`operations` list, but it fails to import the `migrations` module. When a developer
copies this code to create a migration file and runs it, the process will fail with a
`NameError: name 'migrations' is not defined`. This will block the migration process,
preventing developers from successfully following the documentation to encrypt fields.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 8479019

Adds docs about usage of encryption fields at sentry.
it also includes a more in-detail section on how things work under the hood (for SRE and infra teams)
Closes TET-523: Docs for adding encrypted field to the model and TET-522: Docs for migrating existing fields to encrypted ones