From c4f7b620b12aed40634a16f8224c6f2d4ba5f61e Mon Sep 17 00:00:00 2001 From: Kegan Maher Date: Tue, 6 Feb 2024 01:01:39 +0000 Subject: [PATCH 1/9] feat(secrets): Django validator for secret names 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 --- benefits/core/models.py | 25 ++++++++++++++++++++ tests/pytest/core/test_models.py | 39 +++++++++++++++++++++++++++++++- 2 files changed, 63 insertions(+), 1 deletion(-) diff --git a/benefits/core/models.py b/benefits/core/models.py index 8de6ec1fe..ed5c1e3d9 100644 --- a/benefits/core/models.py +++ b/benefits/core/models.py @@ -4,7 +4,9 @@ import importlib import logging +import re +from django.core.validators import RegexValidator from django.conf import settings from django.db import models from django.urls import reverse @@ -15,6 +17,29 @@ logger = logging.getLogger(__name__) +class SecretNameValidator(RegexValidator): + """RegexValidator that validates a secret name. + + 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 + """ + + def __init__(self, *args, **kwargs): + kwargs["regex"] = re.compile(r"^[-a-zA-Z0-9]{1,127}$", re.ASCII) + kwargs["message"] = ( + "Enter a valid secret name of between 1-127 alphanumeric ASCII characters and the hyphen character only." + ) + super().__init__(*args, **kwargs) + + class PemData(models.Model): """API Certificate or Key in PEM format.""" diff --git a/tests/pytest/core/test_models.py b/tests/pytest/core/test_models.py index 6df589ce0..36e5a5b09 100644 --- a/tests/pytest/core/test_models.py +++ b/tests/pytest/core/test_models.py @@ -1,7 +1,44 @@ from django.conf import settings +from django.core.exceptions import ValidationError + import pytest -from benefits.core.models import EligibilityType, EligibilityVerifier, TransitAgency +from benefits.core.models import SecretNameValidator, EligibilityType, EligibilityVerifier, TransitAgency + + +@pytest.mark.parametrize( + "secret_name", + [ + "a", + "1", + "one", + "one-two-three", + "1-2-3", + "this-is-a-really-long-secret-name-in-fact-it-is-the-absolute-maximum-length-of-127-characters-to-be-exact-and-now-it-has-enough", # noqa: E501 + ], +) +def test_SecretNameValidator_valid(secret_name): + validator = SecretNameValidator() + + # a successful validation does not raise an Exception and returns None + assert validator(secret_name) is None + + +@pytest.mark.parametrize( + "secret_name", + [ + "", + "!", + "underscores_not_allowed", + "this-is-a-really-long-secret-name-in-fact-it-much-much-longer-than-the-absolute-maximum-length-of-127-characters-and-now-it-has-enough-to-be-too-long", # noqa: E501 + ], +) +def test_SecretNameValidator_invalid(secret_name): + validator = SecretNameValidator() + + # an unsuccessful validation raises django.core.exceptions.ValidationError + with pytest.raises(ValidationError): + validator(secret_name) @pytest.mark.django_db From 5c94f1256d2617211186e3f8187e523f14d81418 Mon Sep 17 00:00:00 2001 From: Kegan Maher Date: Tue, 6 Feb 2024 02:54:04 +0000 Subject: [PATCH 2/9] feat(secrets): Django field for storing secret names --- benefits/core/models.py | 32 ++++++++++++++++++++++++++++++++ tests/pytest/core/test_models.py | 14 +++++++++++++- 2 files changed, 45 insertions(+), 1 deletion(-) diff --git a/benefits/core/models.py b/benefits/core/models.py index ed5c1e3d9..d2db9664f 100644 --- a/benefits/core/models.py +++ b/benefits/core/models.py @@ -40,6 +40,38 @@ def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) +class SecretValueField(models.SlugField): + """Field that handles retrieving a value from a secret store. + + The field value is the name of the secret to be retrieved. + + The secret value itself MUST NEVER be stored in this field. + """ + + NAME_VALIDATOR = SecretNameValidator() + + description = """Field that handles retrieving a value from a secret store. + + The field value is the name of the secret to be retrieved. Must be between 1-127 alphanumeric ASCII characters or hyphen + characters. + + The secret value itself MUST NEVER be stored in this field. + """ + + def __init__(self, *args, **kwargs): + kwargs["validators"] = [self.NAME_VALIDATOR] + # although the validator also checks for a max length of 127 + # this setting enforces the length at the database column level as well + kwargs["max_length"] = 127 + # similar to max_length, enforce at the field (form) validation level to not allow blanks + kwargs["blank"] = False + # similar to blank, enforce at the database level that null is not allowed + kwargs["null"] = False + # the default is False, but this is more explicit + kwargs["allow_unicode"] = False + super().__init__(*args, **kwargs) + + class PemData(models.Model): """API Certificate or Key in PEM format.""" diff --git a/tests/pytest/core/test_models.py b/tests/pytest/core/test_models.py index 36e5a5b09..e8b9e62ab 100644 --- a/tests/pytest/core/test_models.py +++ b/tests/pytest/core/test_models.py @@ -3,7 +3,7 @@ import pytest -from benefits.core.models import SecretNameValidator, EligibilityType, EligibilityVerifier, TransitAgency +from benefits.core.models import SecretNameValidator, SecretValueField, EligibilityType, EligibilityVerifier, TransitAgency @pytest.mark.parametrize( @@ -41,6 +41,18 @@ def test_SecretNameValidator_invalid(secret_name): validator(secret_name) +def test_SecretValueField_init(): + field = SecretValueField() + + assert SecretValueField.NAME_VALIDATOR in field.validators + assert field.max_length == 127 + assert field.blank is False + assert field.null is False + assert field.allow_unicode is False + assert field.description is not None + assert field.description != "" + + @pytest.mark.django_db def test_PemData_str(model_PemData): assert str(model_PemData) == model_PemData.label From ca91759b0225af0a2f6dd5a02f0bec5bf6f4bfb0 Mon Sep 17 00:00:00 2001 From: Kegan Maher Date: Tue, 6 Feb 2024 03:57:31 +0000 Subject: [PATCH 3/9] refactor(models): AuthProvider.client_id is a secret field * update definition to use new field * update migrations * remove env var from terraform definitions * move default value to .env.sample --- .env.sample | 1 + benefits/core/migrations/0001_initial.py | 13 ++++++++++--- benefits/core/migrations/0002_data.py | 4 ++-- benefits/core/models.py | 8 +++++++- terraform/app_service.tf | 1 - tests/pytest/conftest.py | 2 +- tests/pytest/core/test_models.py | 13 +++++++++++++ 7 files changed, 34 insertions(+), 8 deletions(-) diff --git a/.env.sample b/.env.sample index e60bb329e..b2d3b49ef 100644 --- a/.env.sample +++ b/.env.sample @@ -1 +1,2 @@ testsecret=Hello from the local environment! +auth_provider_client_id=benefits-oauth-client-id diff --git a/benefits/core/migrations/0001_initial.py b/benefits/core/migrations/0001_initial.py index ad482b829..f1ff709af 100644 --- a/benefits/core/migrations/0001_initial.py +++ b/benefits/core/migrations/0001_initial.py @@ -1,10 +1,12 @@ -# Generated by Django 4.2.4 on 2023-08-16 15:06 +# Generated by Django 5.0.1 on 2024-02-06 03:46 -from django.db import migrations, models +import benefits.core.models import django.db.models.deletion +from django.db import migrations, models class Migration(migrations.Migration): + initial = True dependencies = [] @@ -17,7 +19,12 @@ class Migration(migrations.Migration): ("sign_out_button_template", models.TextField(null=True)), ("sign_out_link_template", models.TextField(null=True)), ("client_name", models.TextField()), - ("client_id", models.TextField()), + ( + "client_id_secret_name", + benefits.core.models.SecretValueField( + max_length=127, validators=[benefits.core.models.SecretNameValidator()] + ), + ), ("authority", models.TextField()), ("scope", models.TextField(null=True)), ("claim", models.TextField(null=True)), diff --git a/benefits/core/migrations/0002_data.py b/benefits/core/migrations/0002_data.py index c1807c2ea..14ea86a70 100644 --- a/benefits/core/migrations/0002_data.py +++ b/benefits/core/migrations/0002_data.py @@ -160,7 +160,7 @@ def load_data(app, *args, **kwargs): sign_out_button_template="core/includes/button--sign-out--login-gov.html", sign_out_link_template="core/includes/link--sign-out--login-gov.html", client_name=os.environ.get("SENIOR_AUTH_PROVIDER_CLIENT_NAME", "senior-benefits-oauth-client-name"), - client_id=os.environ.get("AUTH_PROVIDER_CLIENT_ID", "benefits-oauth-client-id"), + client_id_secret_name="auth-provider-client-id", authority=os.environ.get("AUTH_PROVIDER_AUTHORITY", "https://example.com"), scope=os.environ.get("SENIOR_AUTH_PROVIDER_SCOPE", "verify:senior"), claim=os.environ.get("SENIOR_AUTH_PROVIDER_CLAIM", "senior"), @@ -171,7 +171,7 @@ def load_data(app, *args, **kwargs): sign_out_button_template="core/includes/button--sign-out--login-gov.html", sign_out_link_template="core/includes/link--sign-out--login-gov.html", client_name=os.environ.get("VETERAN_AUTH_PROVIDER_CLIENT_NAME", "veteran-benefits-oauth-client-name"), - client_id=os.environ.get("AUTH_PROVIDER_CLIENT_ID", "benefits-oauth-client-id"), + client_id_secret_name="auth-provider-client-id", authority=os.environ.get("AUTH_PROVIDER_AUTHORITY", "https://example.com"), scope=os.environ.get("VETERAN_AUTH_PROVIDER_SCOPE", "verify:veteran"), claim=os.environ.get("VETERAN_AUTH_PROVIDER_CLAIM", "veteran"), diff --git a/benefits/core/models.py b/benefits/core/models.py index d2db9664f..5fc688010 100644 --- a/benefits/core/models.py +++ b/benefits/core/models.py @@ -13,6 +13,8 @@ import requests +from benefits.secrets import get_secret_by_name + logger = logging.getLogger(__name__) @@ -104,7 +106,7 @@ class AuthProvider(models.Model): sign_out_button_template = models.TextField(null=True) sign_out_link_template = models.TextField(null=True) client_name = models.TextField() - client_id = models.TextField() + client_id_secret_name = SecretValueField() authority = models.TextField() scope = models.TextField(null=True) claim = models.TextField(null=True) @@ -118,6 +120,10 @@ def supports_claims_verification(self): def supports_sign_out(self): return bool(self.sign_out_button_template) or bool(self.sign_out_link_template) + @property + def client_id(self): + return get_secret_by_name(self.client_id_secret_name) + class EligibilityType(models.Model): """A single conditional eligibility type.""" diff --git a/terraform/app_service.tf b/terraform/app_service.tf index bc8004731..f1f32161c 100644 --- a/terraform/app_service.tf +++ b/terraform/app_service.tf @@ -119,7 +119,6 @@ resource "azurerm_linux_web_app" "main" { "SBMTD_PAYMENT_PROCESSOR_CLIENT_CERT" = "${local.secret_prefix}sbmtd-payment-processor-client-cert)" "SBMTD_PAYMENT_PROCESSOR_CLIENT_CERT_PRIVATE_KEY" = "${local.secret_prefix}sbmtd-payment-processor-client-cert-private-key)" "SBMTD_PAYMENT_PROCESSOR_CLIENT_CERT_ROOT_CA" = "${local.secret_prefix}sbmtd-payment-processor-client-cert-root-ca)" - "AUTH_PROVIDER_CLIENT_ID" = "${local.secret_prefix}auth-provider-client-id)" "AUTH_PROVIDER_AUTHORITY" = "${local.secret_prefix}auth-provider-authority)" "SENIOR_AUTH_PROVIDER_CLIENT_NAME" = "${local.secret_prefix}senior-auth-provider-client-name)" "SENIOR_AUTH_PROVIDER_SCOPE" = "${local.secret_prefix}senior-auth-provider-scope)" diff --git a/tests/pytest/conftest.py b/tests/pytest/conftest.py index d3c78c512..883cb7c02 100644 --- a/tests/pytest/conftest.py +++ b/tests/pytest/conftest.py @@ -47,7 +47,7 @@ def model_AuthProvider(): sign_out_button_template="core/includes/button--sign-out--senior.html", sign_out_link_template="core/includes/link--sign-out--senior.html", client_name="Client", - client_id="1234", + client_id_secret_name="1234", authority="https://example.com", ) diff --git a/tests/pytest/core/test_models.py b/tests/pytest/core/test_models.py index e8b9e62ab..a5050cd0e 100644 --- a/tests/pytest/core/test_models.py +++ b/tests/pytest/core/test_models.py @@ -53,6 +53,11 @@ def test_SecretValueField_init(): assert field.description != "" +@pytest.fixture +def mock_get_secret_by_name(mocker): + return mocker.patch("benefits.core.models.get_secret_by_name", return_value="secret value!") + + @pytest.mark.django_db def test_PemData_str(model_PemData): assert str(model_PemData) == model_PemData.label @@ -88,6 +93,14 @@ def test_model_AuthProvider(model_AuthProvider): assert model_AuthProvider.supports_sign_out +@pytest.mark.django_db +def test_model_AuthProvider_client_id(model_AuthProvider, mock_get_secret_by_name): + secret_value = model_AuthProvider.client_id + + mock_get_secret_by_name.assert_called_once_with(model_AuthProvider.client_id_secret_name) + assert secret_value == mock_get_secret_by_name.return_value + + @pytest.mark.django_db def test_model_AuthProvider_with_verification(model_AuthProvider_with_verification): assert model_AuthProvider_with_verification.supports_claims_verification From cb315e6497e1f2073228a0066dfd839bbc1bb24c Mon Sep 17 00:00:00 2001 From: Kegan Maher Date: Tue, 6 Feb 2024 18:39:58 +0000 Subject: [PATCH 4/9] refactor(models): EligibilityVerifier.api_auth_key is a secret field * update definition to use new field * update migrations * remove env vars from terraform definitions * move default value to .env.sample --- .env.sample | 2 ++ benefits/core/migrations/0001_initial.py | 9 +++++++-- benefits/core/migrations/0002_data.py | 4 ++-- benefits/core/models.py | 11 ++++++++--- terraform/app_service.tf | 2 -- tests/pytest/conftest.py | 8 +++++++- tests/pytest/core/test_models.py | 19 +++++++++++-------- 7 files changed, 37 insertions(+), 18 deletions(-) diff --git a/.env.sample b/.env.sample index b2d3b49ef..4a2e616e9 100644 --- a/.env.sample +++ b/.env.sample @@ -1,2 +1,4 @@ testsecret=Hello from the local environment! auth_provider_client_id=benefits-oauth-client-id +courtesy_card_verifier_api_auth_key=server-auth-token +mobility_pass_verifier_api_auth_key=server-auth-token diff --git a/benefits/core/migrations/0001_initial.py b/benefits/core/migrations/0001_initial.py index f1ff709af..a3f3c630a 100644 --- a/benefits/core/migrations/0001_initial.py +++ b/benefits/core/migrations/0001_initial.py @@ -1,4 +1,4 @@ -# Generated by Django 5.0.1 on 2024-02-06 03:46 +# Generated by Django 5.0.1 on 2024-02-06 18:09 import benefits.core.models import django.db.models.deletion @@ -48,7 +48,12 @@ class Migration(migrations.Migration): ("active", models.BooleanField(default=False)), ("api_url", models.TextField(null=True)), ("api_auth_header", models.TextField(null=True)), - ("api_auth_key", models.TextField(null=True)), + ( + "api_auth_key_secret_name", + benefits.core.models.SecretValueField( + max_length=127, null=True, validators=[benefits.core.models.SecretNameValidator()] + ), + ), ("jwe_cek_enc", models.TextField(null=True)), ("jwe_encryption_alg", models.TextField(null=True)), ("jws_signing_alg", models.TextField(null=True)), diff --git a/benefits/core/migrations/0002_data.py b/benefits/core/migrations/0002_data.py index 14ea86a70..b509e3d11 100644 --- a/benefits/core/migrations/0002_data.py +++ b/benefits/core/migrations/0002_data.py @@ -203,7 +203,7 @@ def load_data(app, *args, **kwargs): active=os.environ.get("COURTESY_CARD_VERIFIER_ACTIVE", "True").lower() == "true", api_url=os.environ.get("COURTESY_CARD_VERIFIER_API_URL", "http://server:8000/verify"), api_auth_header=os.environ.get("COURTESY_CARD_VERIFIER_API_AUTH_HEADER", "X-Server-API-Key"), - api_auth_key=os.environ.get("COURTESY_CARD_VERIFIER_API_AUTH_KEY", "server-auth-token"), + api_auth_key_secret_name="courtesy-card-verifier-api-auth-key", eligibility_type=mst_courtesy_card_type, public_key=mst_server_public_key, jwe_cek_enc=os.environ.get("COURTESY_CARD_VERIFIER_JWE_CEK_ENC", "A256CBC-HS512"), @@ -238,7 +238,7 @@ def load_data(app, *args, **kwargs): active=os.environ.get("MOBILITY_PASS_VERIFIER_ACTIVE", "True").lower() == "true", api_url=os.environ.get("MOBILITY_PASS_VERIFIER_API_URL", "http://server:8000/verify"), api_auth_header=os.environ.get("MOBILITY_PASS_VERIFIER_API_AUTH_HEADER", "X-Server-API-Key"), - api_auth_key=os.environ.get("MOBILITY_PASS_VERIFIER_API_AUTH_KEY", "server-auth-token"), + api_auth_key_secret_name="mobility-pass-verifier-api-auth-key", eligibility_type=sbmtd_mobility_pass_type, public_key=sbmtd_server_public_key, jwe_cek_enc=os.environ.get("MOBILITY_PASS_VERIFIER_JWE_CEK_ENC", "A256CBC-HS512"), diff --git a/benefits/core/models.py b/benefits/core/models.py index 5fc688010..070b8ccb7 100644 --- a/benefits/core/models.py +++ b/benefits/core/models.py @@ -67,8 +67,6 @@ def __init__(self, *args, **kwargs): kwargs["max_length"] = 127 # similar to max_length, enforce at the field (form) validation level to not allow blanks kwargs["blank"] = False - # similar to blank, enforce at the database level that null is not allowed - kwargs["null"] = False # the default is False, but this is more explicit kwargs["allow_unicode"] = False super().__init__(*args, **kwargs) @@ -164,7 +162,7 @@ class EligibilityVerifier(models.Model): active = models.BooleanField(default=False) api_url = models.TextField(null=True) api_auth_header = models.TextField(null=True) - api_auth_key = models.TextField(null=True) + api_auth_key_secret_name = SecretValueField(null=True) eligibility_type = models.ForeignKey(EligibilityType, on_delete=models.PROTECT) # public key is used to encrypt requests targeted at this Verifier and to verify signed responses from this verifier public_key = models.ForeignKey(PemData, related_name="+", on_delete=models.PROTECT, null=True) @@ -183,6 +181,13 @@ class EligibilityVerifier(models.Model): def __str__(self): return self.name + @property + def api_auth_key(self): + if self.api_auth_key_secret_name is not None: + return get_secret_by_name(self.api_auth_key_secret_name) + else: + return None + @property def public_key_data(self): """This Verifier's public key as a string.""" diff --git a/terraform/app_service.tf b/terraform/app_service.tf index f1f32161c..3a95f334f 100644 --- a/terraform/app_service.tf +++ b/terraform/app_service.tf @@ -136,7 +136,6 @@ resource "azurerm_linux_web_app" "main" { "COURTESY_CARD_VERIFIER_ACTIVE" = "${local.secret_prefix}courtesy-card-verifier-active)" "COURTESY_CARD_VERIFIER_API_URL" = "${local.secret_prefix}courtesy-card-verifier-api-url)" "COURTESY_CARD_VERIFIER_API_AUTH_HEADER" = "${local.secret_prefix}courtesy-card-verifier-api-auth-header)" - "COURTESY_CARD_VERIFIER_API_AUTH_KEY" = "${local.secret_prefix}courtesy-card-verifier-api-auth-key)" "COURTESY_CARD_VERIFIER_JWE_CEK_ENC" = "${local.secret_prefix}courtesy-card-verifier-jwe-cek-enc)" "COURTESY_CARD_VERIFIER_JWE_ENCRYPTION_ALG" = "${local.secret_prefix}courtesy-card-verifier-jwe-encryption-alg)" "COURTESY_CARD_VERIFIER_JWS_SIGNING_ALG" = "${local.secret_prefix}courtesy-card-verifier-jws-signing-alg)" @@ -172,7 +171,6 @@ resource "azurerm_linux_web_app" "main" { "MOBILITY_PASS_VERIFIER_ACTIVE" = "${local.secret_prefix}mobility-pass-verifier-active)" "MOBILITY_PASS_VERIFIER_API_URL" = "${local.secret_prefix}mobility-pass-verifier-api-url)" "MOBILITY_PASS_VERIFIER_API_AUTH_HEADER" = "${local.secret_prefix}mobility-pass-verifier-api-auth-header)" - "MOBILITY_PASS_VERIFIER_API_AUTH_KEY" = "${local.secret_prefix}mobility-pass-verifier-api-auth-key)" "MOBILITY_PASS_VERIFIER_JWE_CEK_ENC" = "${local.secret_prefix}mobility-pass-verifier-jwe-cek-enc)" "MOBILITY_PASS_VERIFIER_JWE_ENCRYPTION_ALG" = "${local.secret_prefix}mobility-pass-verifier-jwe-encryption-alg)" "MOBILITY_PASS_VERIFIER_JWS_SIGNING_ALG" = "${local.secret_prefix}mobility-pass-verifier-jws-signing-alg)" diff --git a/tests/pytest/conftest.py b/tests/pytest/conftest.py index 883cb7c02..920aaa9e3 100644 --- a/tests/pytest/conftest.py +++ b/tests/pytest/conftest.py @@ -32,6 +32,12 @@ def app_request(rf): return app_request +# autouse this fixture so we never call out to the real secret store +@pytest.fixture(autouse=True) +def mock_models_get_secret_by_name(mocker): + return mocker.patch("benefits.core.models.get_secret_by_name", return_value="secret value!") + + @pytest.fixture def model_PemData(): data = PemData.objects.create( @@ -108,7 +114,7 @@ def model_EligibilityVerifier(model_PemData, model_EligibilityType): active=True, api_url="https://example.com/verify", api_auth_header="X-API-AUTH", - api_auth_key="secret-key", + api_auth_key_secret_name="secret-key", eligibility_type=model_EligibilityType, public_key=model_PemData, selection_label_template="eligibility/includes/selection-label.html", diff --git a/tests/pytest/core/test_models.py b/tests/pytest/core/test_models.py index a5050cd0e..8f959ddef 100644 --- a/tests/pytest/core/test_models.py +++ b/tests/pytest/core/test_models.py @@ -53,11 +53,6 @@ def test_SecretValueField_init(): assert field.description != "" -@pytest.fixture -def mock_get_secret_by_name(mocker): - return mocker.patch("benefits.core.models.get_secret_by_name", return_value="secret value!") - - @pytest.mark.django_db def test_PemData_str(model_PemData): assert str(model_PemData) == model_PemData.label @@ -94,11 +89,11 @@ def test_model_AuthProvider(model_AuthProvider): @pytest.mark.django_db -def test_model_AuthProvider_client_id(model_AuthProvider, mock_get_secret_by_name): +def test_model_AuthProvider_client_id(model_AuthProvider, mock_models_get_secret_by_name): secret_value = model_AuthProvider.client_id - mock_get_secret_by_name.assert_called_once_with(model_AuthProvider.client_id_secret_name) - assert secret_value == mock_get_secret_by_name.return_value + mock_models_get_secret_by_name.assert_called_once_with(model_AuthProvider.client_id_secret_name) + assert secret_value == mock_models_get_secret_by_name.return_value @pytest.mark.django_db @@ -266,6 +261,14 @@ def test_EligibilityVerifier_without_AuthProvider(model_EligibilityVerifier): assert not model_EligibilityVerifier.uses_auth_verification +@pytest.mark.django_db +def test_EligiblityVerifier_api_auth_key(model_EligibilityVerifier, mock_models_get_secret_by_name): + secret_value = model_EligibilityVerifier.api_auth_key + + mock_models_get_secret_by_name.assert_called_once_with(model_EligibilityVerifier.api_auth_key_secret_name) + assert secret_value == mock_models_get_secret_by_name.return_value + + @pytest.mark.django_db def test_PaymentProcessor_str(model_PaymentProcessor): assert str(model_PaymentProcessor) == model_PaymentProcessor.name From a4b367584a6b54b28f960c11f162c276705e219d Mon Sep 17 00:00:00 2001 From: Kegan Maher Date: Tue, 6 Feb 2024 19:50:25 +0000 Subject: [PATCH 5/9] refactor(models): PemData.data could come from a secret field 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 --- .env.sample | 63 +++++++++++++++++++++ benefits/core/migrations/0001_initial.py | 7 ++- benefits/core/migrations/0002_data.py | 70 ++++-------------------- benefits/core/models.py | 29 ++++++---- terraform/app_service.tf | 11 ---- tests/pytest/conftest.py | 4 +- tests/pytest/core/test_models.py | 61 +++++++++++++++++---- 7 files changed, 149 insertions(+), 96 deletions(-) diff --git a/.env.sample b/.env.sample index 4a2e616e9..b5aedbf6d 100644 --- a/.env.sample +++ b/.env.sample @@ -2,3 +2,66 @@ testsecret=Hello from the local environment! auth_provider_client_id=benefits-oauth-client-id courtesy_card_verifier_api_auth_key=server-auth-token mobility_pass_verifier_api_auth_key=server-auth-token +client_private_key='-----BEGIN RSA PRIVATE KEY----- +MIIEpAIBAAKCAQEA1pt0ZoOuPEVPJJS+5r884zcjZLkZZ2GcPwr79XOLDbOi46on +Ca79kjRnhS0VUK96SwUPS0z9J5mDA5LSNL2RoxFb5QGaevnJY828NupzTNdUd0sY +JK3kRjKUggHWuB55hwJcH/Dx7I3DNH4NL68UAlK+VjwJkfYPrhq/bl5z8ZiurvBa +5C1mDxhFpcTZlCfxQoas7D1d+uPACF6mEMbQNd3RaIaSREO50NvNywXIIt/OmCiR +qI7JtOcn4eyh1I4j9WtlbMhRJLfwPMAgY5epTsWcURmhVofF2wVoFbib3JGCfA7t +z/gmP5YoEKnf/cumKmF3e9LrZb8zwm7bTHUViwIDAQABAoIBAQCIv0XMjNvZS9DC +XoXGQtVpcxj6dXfaiDgnc7hZDubsNCr3JtT5NqgdIYdVNQUABNDIPNEiCkzFjuwM +uuF2+dRzM/x6UCs/cSsCjXYBCCOwMwV/fjpEJQnwMQqwTLulVsXZYYeSUtXVBf/8 +0tVULRty34apLFhsyX30UtboXQdESfpmm5ZsqsZJlYljw+M7JxRMneQclI19y/ya +hPWlfhLB9OffVEJXGaWx1NSYnKoCMKqE/+4krROr6V62xXaNyX6WtU6XiT7C6R5A +PBxfhmoeFdVCF6a+Qq0v2fKThYoZnV4sn2q2An9YPfynFYnlgzdfnAFSejsqxQd0 +fxYLOtMBAoGBAP1jxjHDJngZ1N+ymw9MIpRgr3HeuMP5phiSTbY2tu9lPzQd+TMX +fhr1bQh2Fd/vU0u7X0yPnTWtUrLlCdGnWPpXivx95GNGgUUIk2HStFdrRx+f2Qvk +G8vtLgmSbjQ26UiHzxi9Wa0a41PWIA3TixkcFrS2X29Qc4yd6pVHmicfAoGBANjR +Z8aaDkSKLkq5Nk1T7I0E1+mtPoH1tPV/FJClXjJrvfDuYHBeOyUpipZddnZuPGWA +IW2tFIsMgJQtgpvgs52NFI7pQGJRUPK/fTG+Ycocxo78TkLr/RIj8Kj5brXsbZ9P +3/WBX5GAISTSp1ab8xVgK/Tm07hGupKVqnY2lCAVAoGAIql0YjhE2ecGtLcU+Qm8 +LTnwpg4GjmBnNTNGSCfB7IuYEsQK489R49Qw3xhwM5rkdRajmbCHm+Eiz+/+4NwY +kt5I1/NMu7vYUR40MwyEuPSm3Q+bvEGu/71pL8wFIUVlshNJ5CN60fA8qqo+5kVK +4Ntzy7Kq6WpC9Dhh75vE3ZcCgYEAty99uXtxsJD6+aEwcvcENkUwUztPQ6ggAwci +je9Z/cmwCj6s9mN3HzfQ4qgGrZsHpk4ycCK655xhilBFOIQJ3YRUKUaDYk4H0YDe +Osf6gTP8wtQDH2GZSNlavLk5w7UFDYQD2b47y4fw+NaOEYvjPl0p5lmb6ebAPZb8 +FbKZRd0CgYBC1HTbA+zMEqDdY4MWJJLC6jZsjdxOGhzjrCtWcIWEGMDF7oDDEoix +W3j2hwm4C6vaNkH9XX1dr5+q6gq8vJQdbYoExl22BGMiNbfI3+sLRk0zBYL//W6c +tSREgR4EjosqQfbkceLJ2JT1wuNjInI0eR9H3cRugvlDTeWtbdJ5qA== +-----END RSA PRIVATE KEY-----' +client_public_key='-----BEGIN PUBLIC KEY----- +MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEA1pt0ZoOuPEVPJJS+5r88 +4zcjZLkZZ2GcPwr79XOLDbOi46onCa79kjRnhS0VUK96SwUPS0z9J5mDA5LSNL2R +oxFb5QGaevnJY828NupzTNdUd0sYJK3kRjKUggHWuB55hwJcH/Dx7I3DNH4NL68U +AlK+VjwJkfYPrhq/bl5z8ZiurvBa5C1mDxhFpcTZlCfxQoas7D1d+uPACF6mEMbQ +Nd3RaIaSREO50NvNywXIIt/OmCiRqI7JtOcn4eyh1I4j9WtlbMhRJLfwPMAgY5ep +TsWcURmhVofF2wVoFbib3JGCfA7tz/gmP5YoEKnf/cumKmF3e9LrZb8zwm7bTHUV +iwIDAQAB +-----END PUBLIC KEY-----' +mst_payment_processor_client_cert='-----BEGIN CERTIFICATE----- +PEM DATA +-----END CERTIFICATE-----' +mst_payment_processor_client_cert_private_key='-----BEGIN RSA PRIVATE KEY----- +PEM DATA +-----END RSA PRIVATE KEY-----' +mst_payment_processor_client_cert_root_ca='-----BEGIN CERTIFICATE----- +PEM DATA +-----END CERTIFICATE-----' +sacrt_payment_processor_client_cert='-----BEGIN CERTIFICATE----- +PEM DATA +-----END CERTIFICATE-----' +sacrt_payment_processor_client_cert_private_key='-----BEGIN RSA PRIVATE KEY----- +PEM DATA +-----END RSA PRIVATE KEY-----' +sacrt_payment_processor_client_cert_root_ca='-----BEGIN CERTIFICATE----- +PEM DATA +-----END CERTIFICATE-----' +sbmtd_payment_processor_client_cert='-----BEGIN CERTIFICATE----- +PEM DATA +-----END CERTIFICATE-----' +sbmtd_payment_processor_client_cert_private_key='-----BEGIN RSA PRIVATE KEY----- +PEM DATA +-----END RSA PRIVATE KEY-----' +sbmtd_payment_processor_client_cert_root_ca='-----BEGIN CERTIFICATE----- +PEM DATA +-----END CERTIFICATE-----' diff --git a/benefits/core/migrations/0001_initial.py b/benefits/core/migrations/0001_initial.py index a3f3c630a..db5f9936d 100644 --- a/benefits/core/migrations/0001_initial.py +++ b/benefits/core/migrations/0001_initial.py @@ -92,8 +92,13 @@ class Migration(migrations.Migration): fields=[ ("id", models.AutoField(primary_key=True, serialize=False)), ("label", models.TextField()), - ("text", models.TextField(null=True)), ("remote_url", models.TextField(null=True)), + ( + "text_secret_name", + benefits.core.models.SecretValueField( + max_length=127, null=True, validators=[benefits.core.models.SecretNameValidator()] + ), + ), ], ), migrations.CreateModel( diff --git a/benefits/core/migrations/0002_data.py b/benefits/core/migrations/0002_data.py index b509e3d11..a78b8e495 100644 --- a/benefits/core/migrations/0002_data.py +++ b/benefits/core/migrations/0002_data.py @@ -51,106 +51,58 @@ def load_data(app, *args, **kwargs): ), ) - default_client_private_key = """ ------BEGIN RSA PRIVATE KEY----- -MIIEpAIBAAKCAQEA1pt0ZoOuPEVPJJS+5r884zcjZLkZZ2GcPwr79XOLDbOi46on -Ca79kjRnhS0VUK96SwUPS0z9J5mDA5LSNL2RoxFb5QGaevnJY828NupzTNdUd0sY -JK3kRjKUggHWuB55hwJcH/Dx7I3DNH4NL68UAlK+VjwJkfYPrhq/bl5z8ZiurvBa -5C1mDxhFpcTZlCfxQoas7D1d+uPACF6mEMbQNd3RaIaSREO50NvNywXIIt/OmCiR -qI7JtOcn4eyh1I4j9WtlbMhRJLfwPMAgY5epTsWcURmhVofF2wVoFbib3JGCfA7t -z/gmP5YoEKnf/cumKmF3e9LrZb8zwm7bTHUViwIDAQABAoIBAQCIv0XMjNvZS9DC -XoXGQtVpcxj6dXfaiDgnc7hZDubsNCr3JtT5NqgdIYdVNQUABNDIPNEiCkzFjuwM -uuF2+dRzM/x6UCs/cSsCjXYBCCOwMwV/fjpEJQnwMQqwTLulVsXZYYeSUtXVBf/8 -0tVULRty34apLFhsyX30UtboXQdESfpmm5ZsqsZJlYljw+M7JxRMneQclI19y/ya -hPWlfhLB9OffVEJXGaWx1NSYnKoCMKqE/+4krROr6V62xXaNyX6WtU6XiT7C6R5A -PBxfhmoeFdVCF6a+Qq0v2fKThYoZnV4sn2q2An9YPfynFYnlgzdfnAFSejsqxQd0 -fxYLOtMBAoGBAP1jxjHDJngZ1N+ymw9MIpRgr3HeuMP5phiSTbY2tu9lPzQd+TMX -fhr1bQh2Fd/vU0u7X0yPnTWtUrLlCdGnWPpXivx95GNGgUUIk2HStFdrRx+f2Qvk -G8vtLgmSbjQ26UiHzxi9Wa0a41PWIA3TixkcFrS2X29Qc4yd6pVHmicfAoGBANjR -Z8aaDkSKLkq5Nk1T7I0E1+mtPoH1tPV/FJClXjJrvfDuYHBeOyUpipZddnZuPGWA -IW2tFIsMgJQtgpvgs52NFI7pQGJRUPK/fTG+Ycocxo78TkLr/RIj8Kj5brXsbZ9P -3/WBX5GAISTSp1ab8xVgK/Tm07hGupKVqnY2lCAVAoGAIql0YjhE2ecGtLcU+Qm8 -LTnwpg4GjmBnNTNGSCfB7IuYEsQK489R49Qw3xhwM5rkdRajmbCHm+Eiz+/+4NwY -kt5I1/NMu7vYUR40MwyEuPSm3Q+bvEGu/71pL8wFIUVlshNJ5CN60fA8qqo+5kVK -4Ntzy7Kq6WpC9Dhh75vE3ZcCgYEAty99uXtxsJD6+aEwcvcENkUwUztPQ6ggAwci -je9Z/cmwCj6s9mN3HzfQ4qgGrZsHpk4ycCK655xhilBFOIQJ3YRUKUaDYk4H0YDe -Osf6gTP8wtQDH2GZSNlavLk5w7UFDYQD2b47y4fw+NaOEYvjPl0p5lmb6ebAPZb8 -FbKZRd0CgYBC1HTbA+zMEqDdY4MWJJLC6jZsjdxOGhzjrCtWcIWEGMDF7oDDEoix -W3j2hwm4C6vaNkH9XX1dr5+q6gq8vJQdbYoExl22BGMiNbfI3+sLRk0zBYL//W6c -tSREgR4EjosqQfbkceLJ2JT1wuNjInI0eR9H3cRugvlDTeWtbdJ5qA== ------END RSA PRIVATE KEY----- -""" - client_private_key = PemData.objects.create( - text=os.environ.get("CLIENT_PRIVATE_KEY", default_client_private_key), + text_secret_name="client-private-key", label="Benefits client private key", ) - default_client_public_key = """ ------BEGIN PUBLIC KEY----- -MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEA1pt0ZoOuPEVPJJS+5r88 -4zcjZLkZZ2GcPwr79XOLDbOi46onCa79kjRnhS0VUK96SwUPS0z9J5mDA5LSNL2R -oxFb5QGaevnJY828NupzTNdUd0sYJK3kRjKUggHWuB55hwJcH/Dx7I3DNH4NL68U -AlK+VjwJkfYPrhq/bl5z8ZiurvBa5C1mDxhFpcTZlCfxQoas7D1d+uPACF6mEMbQ -Nd3RaIaSREO50NvNywXIIt/OmCiRqI7JtOcn4eyh1I4j9WtlbMhRJLfwPMAgY5ep -TsWcURmhVofF2wVoFbib3JGCfA7tz/gmP5YoEKnf/cumKmF3e9LrZb8zwm7bTHUV -iwIDAQAB ------END PUBLIC KEY----- -""" - client_public_key = PemData.objects.create( - text=os.environ.get("CLIENT_PUBLIC_KEY", default_client_public_key), + text_secret_name="client-public-key", label="Benefits client public key", ) - dummy_cert_text = """ ------BEGIN CERTIFICATE----- -PEM DATA ------END CERTIFICATE----- -""" - mst_payment_processor_client_cert = PemData.objects.create( - text=os.environ.get("MST_PAYMENT_PROCESSOR_CLIENT_CERT", dummy_cert_text), + text_secret_name="mst-payment-processor-client-cert", label="MST payment processor client certificate", ) mst_payment_processor_client_cert_private_key = PemData.objects.create( - text=os.environ.get("MST_PAYMENT_PROCESSOR_CLIENT_CERT_PRIVATE_KEY", client_private_key.text), + text_secret_name="mst-payment-processor-client-cert-private-key", label="MST payment processor client certificate private key", ) mst_payment_processor_client_cert_root_ca = PemData.objects.create( - text=os.environ.get("MST_PAYMENT_PROCESSOR_CLIENT_CERT_ROOT_CA", dummy_cert_text), + text_secret_name="mst-payment-processor-client-cert-root-ca", label="MST payment processor client certificate root CA", ) sacrt_payment_processor_client_cert = PemData.objects.create( - text=os.environ.get("SACRT_PAYMENT_PROCESSOR_CLIENT_CERT", dummy_cert_text), + text_secret_name="sacrt-payment-processor-client-cert", label="SacRT payment processor client certificate", ) sacrt_payment_processor_client_cert_private_key = PemData.objects.create( - text=os.environ.get("SACRT_PAYMENT_PROCESSOR_CLIENT_CERT_PRIVATE_KEY", client_private_key.text), + text_secret_name="sacrt-payment-processor-client-cert-private-key", label="SacRT payment processor client certificate private key", ) sacrt_payment_processor_client_cert_root_ca = PemData.objects.create( - text=os.environ.get("SACRT_PAYMENT_PROCESSOR_CLIENT_CERT_ROOT_CA", dummy_cert_text), + text_secret_name="sacrt-payment-processor-client-cert-root-ca", label="SacRT payment processor client certificate root CA", ) sbmtd_payment_processor_client_cert = PemData.objects.create( - text=os.environ.get("SBMTD_PAYMENT_PROCESSOR_CLIENT_CERT", dummy_cert_text), + text_secret_name="sbmtd-payment-processor-client-cert", label="SBMTD payment processor client certificate", ) sbmtd_payment_processor_client_cert_private_key = PemData.objects.create( - text=os.environ.get("SBMTD_PAYMENT_PROCESSOR_CLIENT_CERT_PRIVATE_KEY", client_private_key.text), + text_secret_name="sbmtd-payment-processor-client-cert-private-key", label="SBMTD payment processor client certificate private key", ) sbmtd_payment_processor_client_cert_root_ca = PemData.objects.create( - text=os.environ.get("SBMTD_PAYMENT_PROCESSOR_CLIENT_CERT_ROOT_CA", dummy_cert_text), + text_secret_name="sbmtd-payment-processor-client-cert-root-ca", label="SBMTD payment processor client certificate root CA", ) diff --git a/benefits/core/models.py b/benefits/core/models.py index 070b8ccb7..2280e55a1 100644 --- a/benefits/core/models.py +++ b/benefits/core/models.py @@ -2,6 +2,7 @@ The core application: Common model definitions. """ +from functools import cached_property import importlib import logging import re @@ -78,23 +79,31 @@ class PemData(models.Model): id = models.AutoField(primary_key=True) # Human description of the PEM data label = models.TextField() - # The data in utf-8 encoded PEM text format - text = models.TextField(null=True) + # The name of a secret with data in utf-8 encoded PEM text format + text_secret_name = SecretValueField(null=True) # Public URL hosting the utf-8 encoded PEM text remote_url = models.TextField(null=True) def __str__(self): return self.label - @property + @cached_property def data(self): - if self.text: - return self.text - elif self.remote_url: - self.text = requests.get(self.remote_url, timeout=settings.REQUESTS_TIMEOUT).text - - self.save() - return self.text + """ + Attempts to get data from `remote_url` or `text_secret_name`, with the latter taking precendence if both are defined. + """ + remote_data = None + secret_data = None + + if self.remote_url: + remote_data = requests.get(self.remote_url, timeout=settings.REQUESTS_TIMEOUT).text + if self.text_secret_name: + try: + secret_data = get_secret_by_name(self.text_secret_name) + except Exception: + secret_data = None + + return secret_data if secret_data is not None else remote_data class AuthProvider(models.Model): diff --git a/terraform/app_service.tf b/terraform/app_service.tf index 3a95f334f..a59e7dd30 100644 --- a/terraform/app_service.tf +++ b/terraform/app_service.tf @@ -106,19 +106,8 @@ resource "azurerm_linux_web_app" "main" { "SACRT_SENIOR_GROUP_ID" = "${local.secret_prefix}sacrt-senior-group-id)" "SBMTD_SENIOR_GROUP_ID" = "${local.secret_prefix}sbmtd-senior-group-id)", "SBMTD_MOBILITY_PASS_GROUP_ID" = "${local.secret_prefix}sbmtd-mobility-pass-group-id)" - "CLIENT_PRIVATE_KEY" = "${local.secret_prefix}client-private-key)" - "CLIENT_PUBLIC_KEY" = "${local.secret_prefix}client-public-key)" "MST_SERVER_PUBLIC_KEY_URL" = "${local.secret_prefix}mst-server-public-key-url)" "SBMTD_SERVER_PUBLIC_KEY_URL" = "${local.secret_prefix}sbmtd-server-public-key-url)" - "MST_PAYMENT_PROCESSOR_CLIENT_CERT" = "${local.secret_prefix}mst-payment-processor-client-cert)" - "MST_PAYMENT_PROCESSOR_CLIENT_CERT_PRIVATE_KEY" = "${local.secret_prefix}mst-payment-processor-client-cert-private-key)" - "MST_PAYMENT_PROCESSOR_CLIENT_CERT_ROOT_CA" = "${local.secret_prefix}mst-payment-processor-client-cert-root-ca)" - "SACRT_PAYMENT_PROCESSOR_CLIENT_CERT" = "${local.secret_prefix}sacrt-payment-processor-client-cert)" - "SACRT_PAYMENT_PROCESSOR_CLIENT_CERT_PRIVATE_KEY" = "${local.secret_prefix}sacrt-payment-processor-client-cert-private-key)" - "SACRT_PAYMENT_PROCESSOR_CLIENT_CERT_ROOT_CA" = "${local.secret_prefix}sacrt-payment-processor-client-cert-root-ca)" - "SBMTD_PAYMENT_PROCESSOR_CLIENT_CERT" = "${local.secret_prefix}sbmtd-payment-processor-client-cert)" - "SBMTD_PAYMENT_PROCESSOR_CLIENT_CERT_PRIVATE_KEY" = "${local.secret_prefix}sbmtd-payment-processor-client-cert-private-key)" - "SBMTD_PAYMENT_PROCESSOR_CLIENT_CERT_ROOT_CA" = "${local.secret_prefix}sbmtd-payment-processor-client-cert-root-ca)" "AUTH_PROVIDER_AUTHORITY" = "${local.secret_prefix}auth-provider-authority)" "SENIOR_AUTH_PROVIDER_CLIENT_NAME" = "${local.secret_prefix}senior-auth-provider-client-name)" "SENIOR_AUTH_PROVIDER_SCOPE" = "${local.secret_prefix}senior-auth-provider-scope)" diff --git a/tests/pytest/conftest.py b/tests/pytest/conftest.py index 920aaa9e3..cc62540bc 100644 --- a/tests/pytest/conftest.py +++ b/tests/pytest/conftest.py @@ -40,9 +40,7 @@ def mock_models_get_secret_by_name(mocker): @pytest.fixture def model_PemData(): - data = PemData.objects.create( - text="-----BEGIN PUBLIC KEY-----\nPEM DATA\n-----END PUBLIC KEY-----\n", label="Test public key" - ) + data = PemData.objects.create(text_secret_name="pem-secret-data", label="Test public key") return data diff --git a/tests/pytest/core/test_models.py b/tests/pytest/core/test_models.py index 8f959ddef..ee8345213 100644 --- a/tests/pytest/core/test_models.py +++ b/tests/pytest/core/test_models.py @@ -6,6 +6,12 @@ from benefits.core.models import SecretNameValidator, SecretValueField, EligibilityType, EligibilityVerifier, TransitAgency +@pytest.fixture +def mock_requests_get_pem_data(mocker): + # intercept and spy on the GET request + return mocker.patch("benefits.core.models.requests.get", return_value=mocker.Mock(text="PEM text")) + + @pytest.mark.parametrize( "secret_name", [ @@ -59,27 +65,58 @@ def test_PemData_str(model_PemData): @pytest.mark.django_db -def test_PemData_data_text(model_PemData): - assert model_PemData.text - assert model_PemData.data == model_PemData.text +def test_PemData_data_text_secret_name(model_PemData, mock_models_get_secret_by_name): + # a secret name and not remote URL, should use secret value + + data = model_PemData.data + + mock_models_get_secret_by_name.assert_called_once_with(model_PemData.text_secret_name) + assert data == mock_models_get_secret_by_name.return_value @pytest.mark.django_db -def test_PemData_data_remote(model_PemData, mocker): - model_PemData.text = None +def test_PemData_data_remote(model_PemData, mock_requests_get_pem_data): + # a remote URL and no secret name, should use remote value + + model_PemData.text_secret_name = None model_PemData.remote_url = "http://localhost/publickey" - # intercept and spy on the GET request - requests_spy = mocker.patch("benefits.core.models.requests.get", return_value=mocker.Mock(text="PEM text")) + assert not model_PemData.text_secret_name + + data = model_PemData.data + + mock_requests_get_pem_data.assert_called_once_with(model_PemData.remote_url, timeout=settings.REQUESTS_TIMEOUT) + assert data == mock_requests_get_pem_data.return_value.text + - assert not model_PemData.text +@pytest.mark.django_db +def test_PemData_data_text_secret_name_and_remote__uses_text_secret( + model_PemData, mock_models_get_secret_by_name, mock_requests_get_pem_data +): + # a remote URL and the secret value is not None, should use the secret value + + model_PemData.remote_url = "http://localhost/publickey" + + data = model_PemData.data + + mock_models_get_secret_by_name.assert_called_once_with(model_PemData.text_secret_name) + mock_requests_get_pem_data.assert_called_once_with(model_PemData.remote_url, timeout=settings.REQUESTS_TIMEOUT) + assert data == mock_models_get_secret_by_name.return_value + + +@pytest.mark.django_db +def test_PemData_data_text_secret_name_and_remote__uses_remote( + model_PemData, mock_models_get_secret_by_name, mock_requests_get_pem_data +): + # a remote URL and the secret value is None, should use remote value + model_PemData.remote_url = "http://localhost/publickey" + mock_models_get_secret_by_name.return_value = None data = model_PemData.data - assert model_PemData.text - assert data == "PEM text" - assert data == model_PemData.text - requests_spy.assert_called_once_with(model_PemData.remote_url, timeout=settings.REQUESTS_TIMEOUT) + mock_models_get_secret_by_name.assert_called_once_with(model_PemData.text_secret_name) + mock_requests_get_pem_data.assert_called_once_with(model_PemData.remote_url, timeout=settings.REQUESTS_TIMEOUT) + assert data == mock_requests_get_pem_data.return_value.text @pytest.mark.django_db From bce9ab32c41b8eab285ac7f576b15d32b96c5b39 Mon Sep 17 00:00:00 2001 From: Kegan Maher Date: Thu, 8 Feb 2024 00:53:16 +0000 Subject: [PATCH 6/9] refactor(secrets): enforce name validation in helper function --- benefits/core/migrations/0001_initial.py | 9 ++-- benefits/core/models.py | 31 +----------- benefits/secrets.py | 29 ++++++++++++ tests/pytest/core/test_models.py | 41 ++-------------- tests/pytest/test_secrets.py | 60 ++++++++++++++++++++++-- 5 files changed, 95 insertions(+), 75 deletions(-) diff --git a/benefits/core/migrations/0001_initial.py b/benefits/core/migrations/0001_initial.py index db5f9936d..5eaa52e69 100644 --- a/benefits/core/migrations/0001_initial.py +++ b/benefits/core/migrations/0001_initial.py @@ -1,6 +1,7 @@ # Generated by Django 5.0.1 on 2024-02-06 18:09 import benefits.core.models +import benefits.secrets import django.db.models.deletion from django.db import migrations, models @@ -21,9 +22,7 @@ class Migration(migrations.Migration): ("client_name", models.TextField()), ( "client_id_secret_name", - benefits.core.models.SecretValueField( - max_length=127, validators=[benefits.core.models.SecretNameValidator()] - ), + benefits.core.models.SecretValueField(max_length=127, validators=[benefits.secrets.SecretNameValidator()]), ), ("authority", models.TextField()), ("scope", models.TextField(null=True)), @@ -51,7 +50,7 @@ class Migration(migrations.Migration): ( "api_auth_key_secret_name", benefits.core.models.SecretValueField( - max_length=127, null=True, validators=[benefits.core.models.SecretNameValidator()] + max_length=127, null=True, validators=[benefits.secrets.SecretNameValidator()] ), ), ("jwe_cek_enc", models.TextField(null=True)), @@ -96,7 +95,7 @@ class Migration(migrations.Migration): ( "text_secret_name", benefits.core.models.SecretValueField( - max_length=127, null=True, validators=[benefits.core.models.SecretNameValidator()] + max_length=127, null=True, validators=[benefits.secrets.SecretNameValidator()] ), ), ], diff --git a/benefits/core/models.py b/benefits/core/models.py index 2280e55a1..7f5124447 100644 --- a/benefits/core/models.py +++ b/benefits/core/models.py @@ -5,44 +5,19 @@ from functools import cached_property import importlib import logging -import re -from django.core.validators import RegexValidator from django.conf import settings from django.db import models from django.urls import reverse import requests -from benefits.secrets import get_secret_by_name +from benefits.secrets import NAME_VALIDATOR, get_secret_by_name logger = logging.getLogger(__name__) -class SecretNameValidator(RegexValidator): - """RegexValidator that validates a secret name. - - 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 - """ - - def __init__(self, *args, **kwargs): - kwargs["regex"] = re.compile(r"^[-a-zA-Z0-9]{1,127}$", re.ASCII) - kwargs["message"] = ( - "Enter a valid secret name of between 1-127 alphanumeric ASCII characters and the hyphen character only." - ) - super().__init__(*args, **kwargs) - - class SecretValueField(models.SlugField): """Field that handles retrieving a value from a secret store. @@ -51,8 +26,6 @@ class SecretValueField(models.SlugField): The secret value itself MUST NEVER be stored in this field. """ - NAME_VALIDATOR = SecretNameValidator() - description = """Field that handles retrieving a value from a secret store. The field value is the name of the secret to be retrieved. Must be between 1-127 alphanumeric ASCII characters or hyphen @@ -62,7 +35,7 @@ class SecretValueField(models.SlugField): """ def __init__(self, *args, **kwargs): - kwargs["validators"] = [self.NAME_VALIDATOR] + kwargs["validators"] = [NAME_VALIDATOR] # although the validator also checks for a max length of 127 # this setting enforces the length at the database column level as well kwargs["max_length"] = 127 diff --git a/benefits/secrets.py b/benefits/secrets.py index 293b6d909..0d1668d5e 100644 --- a/benefits/secrets.py +++ b/benefits/secrets.py @@ -1,11 +1,13 @@ import logging import os +import re import sys from azure.core.exceptions import ClientAuthenticationError from azure.identity import DefaultAzureCredential from azure.keyvault.secrets import SecretClient from django.conf import settings +from django.core.validators import RegexValidator logger = logging.getLogger(__name__) @@ -13,11 +15,38 @@ KEY_VAULT_URL = "https://kv-cdt-pub-calitp-{env}-001.vault.azure.net/" +class SecretNameValidator(RegexValidator): + """RegexValidator that validates a secret name. + + 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 + """ + + def __init__(self, *args, **kwargs): + kwargs["regex"] = re.compile(r"^[-a-zA-Z0-9]{1,127}$", re.ASCII) + kwargs["message"] = ( + "Enter a valid secret name of between 1-127 alphanumeric ASCII characters and the hyphen character only." + ) + super().__init__(*args, **kwargs) + + +NAME_VALIDATOR = SecretNameValidator() + + def get_secret_by_name(secret_name, client=None): """Read a value from the secret store, currently Azure KeyVault. When `settings.RUNTIME_ENVIRONMENT() == "local"`, reads from the environment instead. """ + NAME_VALIDATOR(secret_name) runtime_env = settings.RUNTIME_ENVIRONMENT() diff --git a/tests/pytest/core/test_models.py b/tests/pytest/core/test_models.py index ee8345213..8637fde50 100644 --- a/tests/pytest/core/test_models.py +++ b/tests/pytest/core/test_models.py @@ -1,9 +1,9 @@ from django.conf import settings -from django.core.exceptions import ValidationError import pytest -from benefits.core.models import SecretNameValidator, SecretValueField, EligibilityType, EligibilityVerifier, TransitAgency +from benefits.core.models import SecretValueField, EligibilityType, EligibilityVerifier, TransitAgency +import benefits.secrets @pytest.fixture @@ -12,45 +12,10 @@ def mock_requests_get_pem_data(mocker): return mocker.patch("benefits.core.models.requests.get", return_value=mocker.Mock(text="PEM text")) -@pytest.mark.parametrize( - "secret_name", - [ - "a", - "1", - "one", - "one-two-three", - "1-2-3", - "this-is-a-really-long-secret-name-in-fact-it-is-the-absolute-maximum-length-of-127-characters-to-be-exact-and-now-it-has-enough", # noqa: E501 - ], -) -def test_SecretNameValidator_valid(secret_name): - validator = SecretNameValidator() - - # a successful validation does not raise an Exception and returns None - assert validator(secret_name) is None - - -@pytest.mark.parametrize( - "secret_name", - [ - "", - "!", - "underscores_not_allowed", - "this-is-a-really-long-secret-name-in-fact-it-much-much-longer-than-the-absolute-maximum-length-of-127-characters-and-now-it-has-enough-to-be-too-long", # noqa: E501 - ], -) -def test_SecretNameValidator_invalid(secret_name): - validator = SecretNameValidator() - - # an unsuccessful validation raises django.core.exceptions.ValidationError - with pytest.raises(ValidationError): - validator(secret_name) - - def test_SecretValueField_init(): field = SecretValueField() - assert SecretValueField.NAME_VALIDATOR in field.validators + assert benefits.secrets.NAME_VALIDATOR in field.validators assert field.max_length == 127 assert field.blank is False assert field.null is False diff --git a/tests/pytest/test_secrets.py b/tests/pytest/test_secrets.py index 1029538b0..0d4341530 100644 --- a/tests/pytest/test_secrets.py +++ b/tests/pytest/test_secrets.py @@ -1,7 +1,8 @@ -import pytest from azure.core.exceptions import ClientAuthenticationError +from django.core.exceptions import ValidationError +import pytest -from benefits.secrets import KEY_VAULT_URL, get_secret_by_name +from benefits.secrets import KEY_VAULT_URL, SecretNameValidator, NAME_VALIDATOR, get_secret_by_name @pytest.fixture(autouse=True) @@ -14,7 +15,7 @@ def mock_DefaultAzureCredential(mocker): @pytest.fixture def secret_name(): - return "the secret name" + return "the-secret-name" @pytest.fixture @@ -22,6 +23,59 @@ def secret_value(): return "the secret value" +@pytest.mark.parametrize( + "secret_name", + [ + "a", + "1", + "one", + "one-two-three", + "1-2-3", + "this-is-a-really-long-secret-name-in-fact-it-is-the-absolute-maximum-length-of-127-characters-to-be-exact-and-now-it-has-enough", # noqa: E501 + ], +) +def test_SecretNameValidator_valid(secret_name): + validator = SecretNameValidator() + + # a successful validation does not raise an Exception and returns None + assert validator(secret_name) is None + assert NAME_VALIDATOR(secret_name) is None + + +@pytest.mark.parametrize( + "secret_name", + [ + "", + "!", + "underscores_not_allowed", + "this-is-a-really-long-secret-name-in-fact-it-much-much-longer-than-the-absolute-maximum-length-of-127-characters-and-now-it-has-enough-to-be-too-long", # noqa: E501 + ], +) +def test_SecretNameValidator_invalid(secret_name): + validator = SecretNameValidator() + + # an unsuccessful validation raises django.core.exceptions.ValidationError + with pytest.raises(ValidationError): + validator(secret_name) + + with pytest.raises(ValidationError): + NAME_VALIDATOR(secret_name) + + +@pytest.mark.parametrize( + "secret_name", + [ + "", + "!", + "underscores_not_allowed", + "this-is-a-really-long-secret-name-in-fact-it-much-much-longer-than-the-absolute-maximum-length-of-127-characters-and-now-it-has-enough-to-be-too-long", # noqa: E501 + ], +) +def test_get_secret_by_name__invalid_name(secret_name): + with pytest.raises(ValidationError): + get_secret_by_name(secret_name) + + @pytest.mark.parametrize("runtime_env", ["dev", "test", "prod"]) def test_get_secret_by_name__with_client__returns_secret_value(mocker, runtime_env, settings, secret_name, secret_value): settings.RUNTIME_ENVIRONMENT = lambda: runtime_env From 4ae08a91c3d672208c8a73aa5d97f8df8aa7d64f Mon Sep 17 00:00:00 2001 From: Kegan Maher Date: Thu, 8 Feb 2024 01:04:13 +0000 Subject: [PATCH 7/9] fix(secrets): env vars can't contain hyphens --- benefits/secrets.py | 3 ++- tests/pytest/test_secrets.py | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/benefits/secrets.py b/benefits/secrets.py index 0d1668d5e..08de13692 100644 --- a/benefits/secrets.py +++ b/benefits/secrets.py @@ -52,7 +52,8 @@ def get_secret_by_name(secret_name, client=None): if runtime_env == "local": logger.debug("Runtime environment is local, reading from environment instead of Azure KeyVault.") - return os.environ.get(secret_name) + env_secret_name = secret_name.replace("-", "_") + return os.environ.get(env_secret_name) elif client is None: # construct the KeyVault URL from the runtime environment diff --git a/tests/pytest/test_secrets.py b/tests/pytest/test_secrets.py index 0d4341530..53359a982 100644 --- a/tests/pytest/test_secrets.py +++ b/tests/pytest/test_secrets.py @@ -142,6 +142,7 @@ def test_get_secret_by_name__local__returns_environment_variable(mocker, setting settings.RUNTIME_ENVIRONMENT = lambda: "local" env_spy = mocker.patch("benefits.secrets.os.environ.get", return_value=secret_value) + env_secret_name = secret_name.replace("-", "_") client_cls = mocker.patch("benefits.secrets.SecretClient") client = client_cls.return_value @@ -149,5 +150,5 @@ def test_get_secret_by_name__local__returns_environment_variable(mocker, setting client_cls.assert_not_called() client.get_secret.assert_not_called() - env_spy.assert_called_once_with(secret_name) + env_spy.assert_called_once_with(env_secret_name) assert actual_value == secret_value From 0caa85143a65d31035c60b74aeb70cd995050c8d Mon Sep 17 00:00:00 2001 From: Kegan Maher Date: Thu, 8 Feb 2024 01:28:03 +0000 Subject: [PATCH 8/9] fix(ci): start from the .env.sample --- .github/workflows/tests-cypress.yml | 2 +- .github/workflows/tests-ui.yml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/tests-cypress.yml b/.github/workflows/tests-cypress.yml index 17420a0c0..eaaa1f222 100644 --- a/.github/workflows/tests-cypress.yml +++ b/.github/workflows/tests-cypress.yml @@ -15,7 +15,7 @@ jobs: - name: Start app run: | - touch .env + cp .env.sample .env docker compose up --detach client server - name: Run Cypress tests diff --git a/.github/workflows/tests-ui.yml b/.github/workflows/tests-ui.yml index ffb5f6858..f7020e1be 100644 --- a/.github/workflows/tests-ui.yml +++ b/.github/workflows/tests-ui.yml @@ -18,7 +18,7 @@ jobs: - name: Start app run: | - touch .env + cp .env.sample .env docker compose up --detach client - name: Run Lighthouse tests for a11y From 3721d8c9a953fa4405c3532afe73f0c95a3475d2 Mon Sep 17 00:00:00 2001 From: Kegan Maher Date: Thu, 8 Feb 2024 01:49:14 +0000 Subject: [PATCH 9/9] refactor(models): rename secret field for clarity --- benefits/core/migrations/0001_initial.py | 6 +++--- benefits/core/models.py | 17 +++++++---------- tests/pytest/core/test_models.py | 6 +++--- 3 files changed, 13 insertions(+), 16 deletions(-) diff --git a/benefits/core/migrations/0001_initial.py b/benefits/core/migrations/0001_initial.py index 5eaa52e69..1d7ec7edc 100644 --- a/benefits/core/migrations/0001_initial.py +++ b/benefits/core/migrations/0001_initial.py @@ -22,7 +22,7 @@ class Migration(migrations.Migration): ("client_name", models.TextField()), ( "client_id_secret_name", - benefits.core.models.SecretValueField(max_length=127, validators=[benefits.secrets.SecretNameValidator()]), + benefits.core.models.SecretNameField(max_length=127, validators=[benefits.secrets.SecretNameValidator()]), ), ("authority", models.TextField()), ("scope", models.TextField(null=True)), @@ -49,7 +49,7 @@ class Migration(migrations.Migration): ("api_auth_header", models.TextField(null=True)), ( "api_auth_key_secret_name", - benefits.core.models.SecretValueField( + benefits.core.models.SecretNameField( max_length=127, null=True, validators=[benefits.secrets.SecretNameValidator()] ), ), @@ -94,7 +94,7 @@ class Migration(migrations.Migration): ("remote_url", models.TextField(null=True)), ( "text_secret_name", - benefits.core.models.SecretValueField( + benefits.core.models.SecretNameField( max_length=127, null=True, validators=[benefits.secrets.SecretNameValidator()] ), ), diff --git a/benefits/core/models.py b/benefits/core/models.py index 7f5124447..8473fc2cc 100644 --- a/benefits/core/models.py +++ b/benefits/core/models.py @@ -18,18 +18,15 @@ logger = logging.getLogger(__name__) -class SecretValueField(models.SlugField): - """Field that handles retrieving a value from a secret store. - - The field value is the name of the secret to be retrieved. +class SecretNameField(models.SlugField): + """Field that stores the name of a secret held in a secret store. The secret value itself MUST NEVER be stored in this field. """ - description = """Field that handles retrieving a value from a secret store. + description = """Field that stores the name of a secret held in a secret store. - The field value is the name of the secret to be retrieved. Must be between 1-127 alphanumeric ASCII characters or hyphen - characters. + Secret names must be between 1-127 alphanumeric ASCII characters or hyphen characters. The secret value itself MUST NEVER be stored in this field. """ @@ -53,7 +50,7 @@ class PemData(models.Model): # Human description of the PEM data label = models.TextField() # The name of a secret with data in utf-8 encoded PEM text format - text_secret_name = SecretValueField(null=True) + text_secret_name = SecretNameField(null=True) # Public URL hosting the utf-8 encoded PEM text remote_url = models.TextField(null=True) @@ -86,7 +83,7 @@ class AuthProvider(models.Model): sign_out_button_template = models.TextField(null=True) sign_out_link_template = models.TextField(null=True) client_name = models.TextField() - client_id_secret_name = SecretValueField() + client_id_secret_name = SecretNameField() authority = models.TextField() scope = models.TextField(null=True) claim = models.TextField(null=True) @@ -144,7 +141,7 @@ class EligibilityVerifier(models.Model): active = models.BooleanField(default=False) api_url = models.TextField(null=True) api_auth_header = models.TextField(null=True) - api_auth_key_secret_name = SecretValueField(null=True) + api_auth_key_secret_name = SecretNameField(null=True) eligibility_type = models.ForeignKey(EligibilityType, on_delete=models.PROTECT) # public key is used to encrypt requests targeted at this Verifier and to verify signed responses from this verifier public_key = models.ForeignKey(PemData, related_name="+", on_delete=models.PROTECT, null=True) diff --git a/tests/pytest/core/test_models.py b/tests/pytest/core/test_models.py index 8637fde50..2069b4d22 100644 --- a/tests/pytest/core/test_models.py +++ b/tests/pytest/core/test_models.py @@ -2,7 +2,7 @@ import pytest -from benefits.core.models import SecretValueField, EligibilityType, EligibilityVerifier, TransitAgency +from benefits.core.models import SecretNameField, EligibilityType, EligibilityVerifier, TransitAgency import benefits.secrets @@ -12,8 +12,8 @@ def mock_requests_get_pem_data(mocker): return mocker.patch("benefits.core.models.requests.get", return_value=mocker.Mock(text="PEM text")) -def test_SecretValueField_init(): - field = SecretValueField() +def test_SecretNameField_init(): + field = SecretNameField() assert benefits.secrets.NAME_VALIDATOR in field.validators assert field.max_length == 127