Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Refactor: model secret fields #1874

Merged
merged 9 commits into from Feb 12, 2024
Merged

Refactor: model secret fields #1874

merged 9 commits into from Feb 12, 2024

Conversation

thekaveman
Copy link
Member

@thekaveman thekaveman commented Feb 6, 2024 •
edited

Defines a new field type SecretNameField that manages storage of the name of the secret for models that need it.

Defines a custom RegexValidator to ensure secret names are valid according to Azure's rules.

Refactors secret fields on models according to the field mapping spreadsheet to store the name of their corresponding secret using the new field type. Secret names are defined in new fields on each model named with the postfix _secret_name.

Models then use the helper defined in #1859 to get the value on-demand via a @property using the original (sans-postfix) field name. This means calling code doesn't have to change.

Closes #1847

Models to refactor

  • AuthProvider
  • EligiblityVerifier
  • PemData
  • PaymentProcessor Since the secret fields here are references to PemData, and that model has already been refactored, this model does not need to be further refactored.

Reviewing this PR

It should be possible to set up your local environment to read secrets from dev:

  1. Override DJANGO_ALLOWED_HOSTS to include dev-benefits.calitp.org,localhost
  2. Override other environment variables for non-secret fields, e.g. AUTH_PROVIDER_AUTHORITY
  3. Run bin/init.sh to recreate the database
  4. F5 to launch the app locally
  5. Verify the complete enrollment flow for the pathway(s) you set up

@github-actions github-actions bot added tests Related to automated testing (unit, UI, integration, etc.) migrations [auto] Review for potential model changes/needed data migrations updates back-end Django views, sessions, middleware, models, migrations etc. labels Feb 6, 2024
@thekaveman thekaveman added the security Changes to improve or maintain the availability and resilience of the app label Feb 6, 2024
@thekaveman thekaveman self-assigned this Feb 6, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Feb 6, 2024 •
edited

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)		Lines missing
  benefits
  secrets.py
  benefits/core
  models.py 73-74, 168
Project Total  

This report was generated by python-coverage-comment-action

@thekaveman thekaveman mentioned this pull request Feb 6, 2024
Closed
4 tasks
@thekaveman thekaveman added this to the Admin tool: foundation milestone Feb 6, 2024
@thekaveman thekaveman linked an issue Feb 6, 2024 that may be closed by this pull request
Refactor handling of secret model fields #1847
Closed
4 tasks
@thekaveman thekaveman force-pushed the refactor/model-secret-fields branch 3 times, most recently from b235a42 to 22db7a6 Compare February 8, 2024 01:32
Base automatically changed from feat/secrets-helper to dev February 8, 2024 01:36
@thekaveman
feat(secrets): Django validator for secret names
c4f7b62 
Azure KeyVault currently enforces the following rules:

    * The value must be between 1 and 127 characters long.
    * Secret names can only contain alphanumeric characters and dashes.

Read more about Azure KeyVault naming rules:
https://learn.microsoft.com/en-us/azure/azure-resource-manager/management/resource-name-rules#microsoftkeyvault

Read more about Django validators:
https://docs.djangoproject.com/en/5.0/ref/validators/#module-django.core.validators
@thekaveman
feat(secrets): Django field for storing secret names
5c94f12
@thekaveman
refactor(models): AuthProvider.client_id is a secret field
ca91759 
* update definition to use new field
* update migrations
* remove env var from terraform definitions
* move default value to .env.sample
@thekaveman
refactor(models): EligibilityVerifier.api_auth_key is a secret field
cb315e6 
* update definition to use new field
* update migrations
* remove env vars from terraform definitions
* move default value to .env.sample
@thekaveman
refactor(models): PemData.data could come from a secret field
a4b3675 
PemData favors the secret, but fallback to a remote URL
this is to allow for simpler turn-key local development

* update definition to use new field
* update migrations
* remove env vars from terraform definitions
* move default keys to .env.sample
@thekaveman
refactor(secrets): enforce name validation in helper function
bce9ab3
@thekaveman
fix(secrets): env vars can't contain hyphens
4ae08a9
@thekaveman
fix(ci): start from the .env.sample
0caa851
@thekaveman
refactor(models): rename secret field for clarity
3721d8c
@thekaveman thekaveman marked this pull request as ready for review February 8, 2024 02:04
@thekaveman thekaveman requested a review from a team as a code owner February 8, 2024 02:04
angela-tran
angela-tran approved these changes Feb 8, 2024
View reviewed changes
Copy link
Member

@angela-tran angela-tran left a comment

Choose a reason for hiding this comment

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

This looks good to me.

Since we'll be connecting to the Key Vault every time we need a secret, I looked up the Key Vault service limits: https://learn.microsoft.com/en-us/azure/key-vault/general/service-limits#secrets-managed-storage-account-keys-and-vault-transactions

I think it should be fine for the scale at which we currently operate.

@thekaveman
Copy link
Member Author

This looks good to me.

Since we'll be connecting to the Key Vault every time we need a secret, I looked up the Key Vault service limits: https://learn.microsoft.com/en-us/azure/key-vault/general/service-limits#secrets-managed-storage-account-keys-and-vault-transactions

I think it should be fine for the scale at which we currently operate.

Good point! I hadn't thought of that.

4,000 transactions allowed in 10 seconds, per vault per region

Seems like WAY more than enough for now 😅

However we could look at adding more of the @cached_property decorators like was done for PemData if we're worried about lag time.

@machikoyasuda machikoyasuda self-requested a review February 8, 2024 22:17
machikoyasuda
machikoyasuda approved these changes Feb 8, 2024
View reviewed changes
Copy link
Member

@machikoyasuda machikoyasuda left a comment

Choose a reason for hiding this comment

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

  • Ran locally sucessfully, going thru reading dev secrets

@thekaveman thekaveman merged commit eac947e into dev Feb 12, 2024
13 checks passed
@thekaveman thekaveman deleted the refactor/model-secret-fields branch February 12, 2024 23:18
@thekaveman thekaveman mentioned this pull request Feb 20, 2024
Closed
22 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@machikoyasuda machikoyasuda machikoyasuda approved these changes

@angela-tran angela-tran angela-tran approved these changes

Assignees

@thekaveman thekaveman

Labels
back-end Django views, sessions, middleware, models, migrations etc. migrations [auto] Review for potential model changes/needed data migrations updates security Changes to improve or maintain the availability and resilience of the app tests Related to automated testing (unit, UI, integration, etc.)
Projects
None yet
Milestone
Admin tool: foundation
Development

Successfully merging this pull request may close these issues.

Refactor handling of secret model fields
3 participants
@thekaveman @machikoyasuda @angela-tran