From bef84c11b0500259815389384ce88567b0fac3de Mon Sep 17 00:00:00 2001 From: Brian Caswell Date: Mon, 19 Jul 2021 13:09:11 -0400 Subject: [PATCH 1/4] delete secret on object delete --- src/api-service/__app__/onefuzzlib/orm.py | 24 ++++++- src/api-service/__app__/onefuzzlib/secrets.py | 5 ++ src/api-service/tests/test_secrets.py | 70 ++++++++++++++++++- 3 files changed, 97 insertions(+), 2 deletions(-) diff --git a/src/api-service/__app__/onefuzzlib/orm.py b/src/api-service/__app__/onefuzzlib/orm.py index 6e4257e24f..d47e114a06 100644 --- a/src/api-service/__app__/onefuzzlib/orm.py +++ b/src/api-service/__app__/onefuzzlib/orm.py @@ -40,7 +40,7 @@ from typing_extensions import Protocol from .azure.table import get_client -from .secrets import save_to_keyvault +from .secrets import delete_secret_data, save_to_keyvault from .telemetry import track_event_filtered from .updates import queue_update @@ -249,6 +249,26 @@ def hide_secrets(data: B, hider: Callable[[SecretData], SecretData]) -> B: return data +def delete_secrets(data: B, deleter: Callable[[SecretData], None]) -> None: + for field in data.__fields__: + field_data = getattr(data, field) + if isinstance(field_data, SecretData): + deleter(field_data) + elif isinstance(field_data, BaseModel): + delete_secrets(field_data, deleter) + elif isinstance(field_data, list): + for entry in field_data: + if isinstance(entry, BaseModel): + delete_secrets(entry, deleter) + elif isinstance(entry, SecretData): + deleter(field_data) + elif isinstance(field_data, dict): + for value in field_data.values(): + if isinstance(value, BaseModel): + delete_secrets(value, deleter) + elif isinstance(value, SecretData): + deleter(field_data) + # NOTE: if you want to include Timestamp in a model that uses ORMMixin, # it must be maintained as part of the model. class ORMMixin(ModelMixin): @@ -363,6 +383,8 @@ def save(self, new: bool = False, require_etag: bool = False) -> Optional[Error] def delete(self) -> None: partition_key, row_key = self.get_keys() + delete_secrets(self, delete_secret_data) + client = get_client() try: client.delete_entity( diff --git a/src/api-service/__app__/onefuzzlib/secrets.py b/src/api-service/__app__/onefuzzlib/secrets.py index eaba556e53..8ddb50390f 100644 --- a/src/api-service/__app__/onefuzzlib/secrets.py +++ b/src/api-service/__app__/onefuzzlib/secrets.py @@ -80,3 +80,8 @@ def delete_secret(secret_url: str) -> None: (vault_url, secret_name) = parse_secret_url(secret_url) keyvault_client = get_keyvault_client(vault_url) keyvault_client.begin_delete_secret(secret_name) + + +def delete_secret_data(data: SecretData) -> None: + if isinstance(data.secret, SecretAddress): + delete_secret(data.secret.url) diff --git a/src/api-service/tests/test_secrets.py b/src/api-service/tests/test_secrets.py index e484cf9ee1..d4f89b74ad 100644 --- a/src/api-service/tests/test_secrets.py +++ b/src/api-service/tests/test_secrets.py @@ -7,7 +7,10 @@ import pathlib import unittest +from typing import List, Dict +from pydantic import BaseModel from onefuzztypes.enums import OS, ContainerType +from onefuzztypes.models import SecretData from onefuzztypes.job_templates import ( JobTemplate, JobTemplateIndex, @@ -27,7 +30,7 @@ from onefuzztypes.primitives import Container from onefuzztypes.requests import NotificationCreate -from __app__.onefuzzlib.orm import hide_secrets +from __app__.onefuzzlib.orm import hide_secrets, delete_secrets def hider(secret_data: SecretData) -> SecretData: @@ -36,7 +39,72 @@ def hider(secret_data: SecretData) -> SecretData: return secret_data +class WithSecret(BaseModel): + a: SecretData[str] + + +class WithList(BaseModel): + a: List[WithSecret] + + +class WithDict(BaseModel): + a: Dict[str, SecretData[str]] + b: Dict[str, WithSecret] + + +class Nested(BaseModel): + a: WithSecret + b: WithDict + c: WithList + + class TestSecret(unittest.TestCase): + def test_delete(self) -> None: + self.count = 0 + + def deleter(secret_data: SecretData) -> None: + self.count += 1 + + delete_secrets(WithSecret(a=SecretData("a")), deleter) + self.assertEqual(self.count, 1) + + delete_secrets( + WithList( + a=[ + WithSecret(a=SecretData("a")), + WithSecret(a=SecretData("b")), + ] + ), + deleter, + ) + self.assertEqual(self.count, 3) + + delete_secrets( + WithDict( + a={"a": SecretData("a"), "b": SecretData("b")}, + b={ + "a": WithSecret(a=SecretData("a")), + "b": WithSecret(a=SecretData("a")), + }, + ), + deleter, + ) + self.assertEqual(self.count, 7) + + delete_secrets( + Nested( + a=WithSecret(a=SecretData("a")), + b=WithDict( + a={"a": SecretData("a")}, b={"a": WithSecret(a=SecretData("a"))} + ), + c=WithList( + a=[WithSecret(a=SecretData("a")), WithSecret(a=SecretData("b"))] + ), + ), + deleter, + ) + self.assertEqual(self.count, 12) + def test_hide(self) -> None: notification = Notification( container=Container("data"), From c8f1005b5c498ba097a1866f1af21d96374c60a1 Mon Sep 17 00:00:00 2001 From: Brian Caswell Date: Mon, 19 Jul 2021 13:29:06 -0400 Subject: [PATCH 2/4] fix issues found via lint --- src/api-service/__app__/onefuzzlib/orm.py | 5 +++-- src/api-service/tests/test_secrets.py | 7 +++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/api-service/__app__/onefuzzlib/orm.py b/src/api-service/__app__/onefuzzlib/orm.py index d47e114a06..0599a26bff 100644 --- a/src/api-service/__app__/onefuzzlib/orm.py +++ b/src/api-service/__app__/onefuzzlib/orm.py @@ -261,13 +261,14 @@ def delete_secrets(data: B, deleter: Callable[[SecretData], None]) -> None: if isinstance(entry, BaseModel): delete_secrets(entry, deleter) elif isinstance(entry, SecretData): - deleter(field_data) + deleter(entry) elif isinstance(field_data, dict): for value in field_data.values(): if isinstance(value, BaseModel): delete_secrets(value, deleter) elif isinstance(value, SecretData): - deleter(field_data) + deleter(value) + # NOTE: if you want to include Timestamp in a model that uses ORMMixin, # it must be maintained as part of the model. diff --git a/src/api-service/tests/test_secrets.py b/src/api-service/tests/test_secrets.py index d4f89b74ad..9792a3c72d 100644 --- a/src/api-service/tests/test_secrets.py +++ b/src/api-service/tests/test_secrets.py @@ -6,11 +6,9 @@ import json import pathlib import unittest +from typing import Dict, List -from typing import List, Dict -from pydantic import BaseModel from onefuzztypes.enums import OS, ContainerType -from onefuzztypes.models import SecretData from onefuzztypes.job_templates import ( JobTemplate, JobTemplateIndex, @@ -29,8 +27,9 @@ ) from onefuzztypes.primitives import Container from onefuzztypes.requests import NotificationCreate +from pydantic import BaseModel -from __app__.onefuzzlib.orm import hide_secrets, delete_secrets +from __app__.onefuzzlib.orm import delete_secrets, hide_secrets def hider(secret_data: SecretData) -> SecretData: From 6cea51f2bea9f7eef8a2b240a70727894791dfe1 Mon Sep 17 00:00:00 2001 From: Brian Caswell Date: Tue, 20 Jul 2021 07:29:00 -0400 Subject: [PATCH 3/4] address feedback --- src/api-service/__app__/onefuzzlib/orm.py | 4 ++-- src/api-service/__app__/onefuzzlib/secrets.py | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/api-service/__app__/onefuzzlib/orm.py b/src/api-service/__app__/onefuzzlib/orm.py index 0599a26bff..a567a05781 100644 --- a/src/api-service/__app__/onefuzzlib/orm.py +++ b/src/api-service/__app__/onefuzzlib/orm.py @@ -40,7 +40,7 @@ from typing_extensions import Protocol from .azure.table import get_client -from .secrets import delete_secret_data, save_to_keyvault +from .secrets import delete_remote_secret_data, save_to_keyvault from .telemetry import track_event_filtered from .updates import queue_update @@ -384,7 +384,7 @@ def save(self, new: bool = False, require_etag: bool = False) -> Optional[Error] def delete(self) -> None: partition_key, row_key = self.get_keys() - delete_secrets(self, delete_secret_data) + delete_secrets(self, delete_remote_secret_data) client = get_client() try: diff --git a/src/api-service/__app__/onefuzzlib/secrets.py b/src/api-service/__app__/onefuzzlib/secrets.py index 8ddb50390f..f7904b0706 100644 --- a/src/api-service/__app__/onefuzzlib/secrets.py +++ b/src/api-service/__app__/onefuzzlib/secrets.py @@ -82,6 +82,6 @@ def delete_secret(secret_url: str) -> None: keyvault_client.begin_delete_secret(secret_name) -def delete_secret_data(data: SecretData) -> None: +def delete_remote_secret_data(data: SecretData) -> None: if isinstance(data.secret, SecretAddress): delete_secret(data.secret.url) From 32f1e775770cd5655c03e3d570e6b64f2ed523f2 Mon Sep 17 00:00:00 2001 From: Brian Caswell Date: Wed, 21 Jul 2021 13:43:34 -0400 Subject: [PATCH 4/4] address feedback --- src/api-service/__app__/onefuzzlib/orm.py | 1 + 1 file changed, 1 insertion(+) diff --git a/src/api-service/__app__/onefuzzlib/orm.py b/src/api-service/__app__/onefuzzlib/orm.py index a567a05781..0f30da4224 100644 --- a/src/api-service/__app__/onefuzzlib/orm.py +++ b/src/api-service/__app__/onefuzzlib/orm.py @@ -249,6 +249,7 @@ def hide_secrets(data: B, hider: Callable[[SecretData], SecretData]) -> B: return data +# NOTE: the actual deletion must come from the `deleter` callback function def delete_secrets(data: B, deleter: Callable[[SecretData], None]) -> None: for field in data.__fields__: field_data = getattr(data, field)