fix(server-charm): add pydantic validation to CSFLE master key#1154
fix(server-charm): add pydantic validation to CSFLE master key#1154rene-oromtz wants to merge 2 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1154 +/- ##
==========================================
+ Coverage 78.01% 78.04% +0.03%
==========================================
Files 118 118
Lines 12412 12431 +19
Branches 1023 1026 +3
==========================================
+ Hits 9683 9702 +19
Misses 2508 2508
Partials 221 221
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
ajzobro
left a comment
ajzobro
left a comment
There was a problem hiding this comment.
Looks good, minor questions.
| # If the key was unset, clear the stored key so the next | ||
| # config-changed with a valid key is treated as a fresh deployment. | ||
| if not new_key: | ||
| logger.info("Master key unset, clearing stored key") |
There was a problem hiding this comment.
Do we really want to support on-the-fly removal of the secret master key?
Is this also going to disable secrets when/if someone clears out the master key?
There was a problem hiding this comment.
Removing it will disable the secret store but also additional cleanup is needed (manually removing the relation), this is also needed because the stored state can't be modified easily, so you need to unset to not force a rotation
| return | ||
|
|
||
| # Rotate only if master key was previously defined, has changed, | ||
| # and this is the leader unit. |
There was a problem hiding this comment.
Here we only change the key if we are the leader unit... should we consider that when we are clearing the key as well?
There was a problem hiding this comment.
Good question, normally this is required when sending changes directly to a DB (such as Mongo). Given this is internal state, perhaps its also a DB 🤔 I can add this, doesn't hurt to have it
There was a problem hiding this comment.
ah perhaps I was wrong? It seems that the data may be per unit if I get this right:
https://documentation.ubuntu.com/ops/latest/reference/ops/#ops.StoredState
Data is stored alongside the charm (in the charm container for Kubernetes sidecar charms, and on the machine for machine charms).
If I get this right, each container have its own data store, so we need to clear them from all units
| for state in state_out.stored_states | ||
| if state.owner_path == "TestflingerCharm" and state.name == "_stored" | ||
| ) | ||
| assert stored.content["previous_master_key"] == "" |
There was a problem hiding this comment.
This is verifying the "next" state has a "previous" master key that is blank; shouldn't we be verifying the current key is empty?
There was a problem hiding this comment.
The previous_master_key is the value of the key internally stored by Juju which needs to be cleared if it was unset
2 participants
Description
This add a pydantic validation at charm level so MongoDB CSFLE encryption key is valid base64 with 96-byte length.
When unset, this will also clear the stored CSFLE key in charm.
Resolved issues
NA
Documentation
NA
Web service API changes
Tests
Added unit tests for this behavior