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

Add per-project hashing salt #371

Merged
merged 67 commits into from
Apr 15, 2024
Merged

Add per-project hashing salt #371

merged 67 commits into from
Apr 15, 2024

Conversation

milanmlft
Copy link
Member

@milanmlft milanmlft commented Apr 11, 2024 •
edited
Loading

Fixes #343

  • Use core.project_config.secrets.AzureKeyVault in hasher to set up Azure Key Vault connection
  • Hashing salts are now retrieved from the key vault, using the project-name as key
    • If no salt exists, a new one is created and stored in the key vault
  • Removed the old hasher key vault functionality
  • Salt generation is moved into the hashing functionality, so only hasher knows about it

To do

@milanmlft
Refactor: move AzureKeyVault to core.project_config
75055d3
@milanmlft
Delay loading of PROJECT_CONFIGS_DIR envvar until it's needed
ce3cb6e 
Avoids import errors in tests where the envvar is only set after the
imports.
@milanmlft
Merge branch 'main' into milanmlft/343-per-project-hashing-salt
2a23529
@milanmlft
Move project config tests into their own subdir
752e659
@milanmlft
Add basic tests for Keyvault secret fetching
19f3f60
@milanmlft
Also install pixl_core in hasher Docker
d2e5589
@milanmlft
Refactor AzureKeyVault and add create_secret method
ccae234
@milanmlft
Add fetch_salt to get project-salts from Keyvault
d9c0fdc
@milanmlft
Set up Keyvault connection during function call
e0383b2
@milanmlft
Move azure_keyvault fixture to pytest-pixl
3c1978f 
Also remove `test_fetch_secret` as it's not testing our actual
`AzureKeyVault` class anyway
@milanmlft
Refactor: move hashing functionality inside Hasher class
0115e89 
`Hasher` class sets up Azure Key vault connection and handles fetching
hashing keys and project salts.
@milanmlft
Mock Hasher class for tests
9dc0b8e
@milanmlft
Don't set a global Hasher instance
8952827 
This was a bit of a pain to monkeypatch in the tests, as the call to
`Hasher()` here kept trying to initiate an actual AzureKeyVault
instance, which would fail because the proper environment variables are
missing. Doing this at least allows us to monkeypatch the `Hasher` class
in the tests so that it uses a mock keyvault instance.
@milanmlft
Monkeypatch the Hasher class with a mock keyvault for tests
3c52439
@milanmlft
Update endpoint tests with new fixtures
a32fb37
@milanmlft
MockKeyVault: mimic real keyvault behaviour by raising ValueError…
84dc3a8 
… for non-existing secrets
@milanmlft
Remove obsolete hasher settings
fb1bb1f
@milanmlft
Generate new salt if different length requested
970f9c1
@milanmlft
Fix imports
25e6aac
@milanmlft
Add test for when different salt length is requested
f814467
@milanmlft
Update tests
077252f
@milanmlft milanmlft requested a review from a team April 11, 2024 16:37
stefpiatek
stefpiatek reviewed Apr 12, 2024
View reviewed changes
Copy link
Contributor

@stefpiatek stefpiatek left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looking nice, but refactoring of keyvault won't actually cache sadly

pytest-pixl/src/pytest_pixl/plugin.py Outdated Show resolved Hide resolved
pytest-pixl/src/pytest_pixl/keyvault.py Outdated Show resolved Hide resolved
pixl_core/src/core/project_config/secrets.py Outdated Show resolved Hide resolved
pixl_core/src/core/project_config/secrets.py Outdated Show resolved Hide resolved
hasher/src/hasher/endpoints.py Outdated Show resolved Hide resolved
.github/workflows/main.yml Outdated Show resolved Hide resolved
hasher/src/hasher/hashing.py Outdated Show resolved Hide resolved
hasher/src/hasher/hashing.py Outdated Show resolved Hide resolved
stefpiatek
stefpiatek approved these changes Apr 15, 2024
View reviewed changes
Copy link
Contributor

@stefpiatek stefpiatek left a comment

Choose a reason for hiding this comment

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

Nice! I think some env changes are needed and a couple of questions about the public API

hasher/pyproject.toml Outdated Show resolved Hide resolved
hasher/pyproject.toml Outdated Show resolved Hide resolved
hasher/src/hasher/endpoints.py Outdated Show resolved Hide resolved
hasher/src/hasher/endpoints.py Show resolved Hide resolved
hasher/src/hasher/hashing.py Outdated Show resolved Hide resolved
hasher/src/hasher/main.py Show resolved Hide resolved
hasher/tests/conftest.py Outdated Show resolved Hide resolved
hasher/src/hasher/endpoints.py Outdated Show resolved Hide resolved
pixl_core/src/core/project_config/secrets.py Outdated Show resolved Hide resolved
test/.secrets.env.sample Show resolved Hide resolved
@milanmlft
Update dependencies
f0cb2e0 
`environs` not used anymore, `loguru` already in core
@milanmlft
Change logging level
ba9525b
@milanmlft
Configure logging level at app startup
b654da7
@milanmlft
Small refactor of AzureKeyVault, don't cache create_secret
edce95b
@milanmlft
Set hasher API AZ secrets
f7c649d
@milanmlft
Update main README
5ab3616
@milanmlft
Set default logging level and set in tests
b8b2b00
@milanmlft
Get rid of ENV environment variable
cf4c84c
@milanmlft
Make imaging API's log level envvar consistent
1f9d1ce
@milanmlft
Avoid hasher import in test fixture
6940fd8
@milanmlft
Allow creating salts interactively, use sensible defaults during hashing
648ef9d
@milanmlft
Update hasher docs
12d3435
@milanmlft
Merge branch 'main' into milanmlft/343-per-project-hashing-salt
95a01d8
@milanmlft
Use updated hasher API in new anonymisation
09d127c
@milanmlft
Hashing now also requires project slug as input
d7433b9
@milanmlft
Raise ValueError when keyvault secret not found
672490a
@milanmlft milanmlft enabled auto-merge (squash) April 15, 2024 16:23
@milanmlft milanmlft disabled auto-merge April 15, 2024 17:25
@milanmlft
Copy link
Member Author

Had to create a new secret in our dev Azure Keyvault under hasher--key to get system test passing. The Hasher API reads the secret configured with HASHER_API_AZ_KEY_VAULT_SECRET_NAME from the vault as key for the hashing. Added this to test/.env as well.

@milanmlft milanmlft enabled auto-merge (squash) April 15, 2024 17:47
@milanmlft milanmlft merged commit e1b9072 into main Apr 15, 2024
8 checks passed
@milanmlft milanmlft deleted the milanmlft/343-per-project-hashing-salt branch April 15, 2024 17:59
@peshence peshence mentioned this pull request Apr 24, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stefpiatek stefpiatek stefpiatek approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Per project salt in PIXL hashing
2 participants
@milanmlft @stefpiatek