feat(team): Add sensitive data auth, store secrets securely #22294
Conversation
| def create(self, validated_data: dict[str, Any], **kwargs) -> Team: | ||
| serializers.raise_errors_on_nested_writes("create", self, validated_data) | ||
| request = self.context["request"] | ||
| organization = self.context["view"].organization # Use the org we used to validate permissions | ||
|
|
||
| validated_data = self.encrypt_sensitive_fields(validated_data) |
There was a problem hiding this comment.
hmm, instead of doing it like this, which is a bit wonky / easy to forget to encrypt and then bork, maybe better to take the API token approach, where we generate the shared secret, vs. depending on the user for it, and they can input it in their systems. And then we can reset it. This also implies we don't necessarily need encryption on these fields, since they're not coming from the user anyway.
There was a problem hiding this comment.
.. only reason I can think of not to use the personal API key as the secret is because it's at the org level, and per user, rather than per team.
There was a problem hiding this comment.
Curious what you think Ben, currently I'm thinking:
- Keep the encryption
- Move it to a separate patch request which is the only way to generate / reset this key, and encryption happens here
- Don't show it on the UI after generation
There was a problem hiding this comment.
For the demo will continue like it is right now
There was a problem hiding this comment.
I think we're being overly careful about it. Most UIs that I've seen with this kind of thing display it in plain text.
As it's only a shared secret for signing the level of risk is low. Versus an api key that is actually an authentication key which is what makes it super sensitive
There was a problem hiding this comment.
That said I think we should encrypt it because why not 😅 but I wouldn't stress about never making it displayable again
| @@ -59,6 +59,9 @@ | |||
| # SECURITY WARNING: keep the secret key used in production secret! | |||
| SECRET_KEY: str = os.getenv("SECRET_KEY", DEFAULT_SECRET_KEY) | |||
|
|
|||
| # SECURITY WARNING: keep the encryption secret key used in production secret! | |||
| ENCRYPTION_SECRET_KEY: str = os.getenv("ENCRYPTION_SECRET_KEY", "sNxC-b8AkSkNqs8nVfUz4_gn89MzuQmtckgmxm81LP8=") | |||
There was a problem hiding this comment.
We might want to add a debug thing here so it only uses a default if in DEBUG otherwise throws?
| @@ -390,6 +391,14 @@ def render_template( | |||
| posthog_app_context["frontend_apps"] = get_frontend_apps(user.team.pk) | |||
| posthog_app_context["default_event_name"] = get_default_event_name(user.team) | |||
|
|
|||
| if user.email: | |||
| # TODO: Convert to an SDK function call | |||
| posthog_app_context["current_user_hash"] = hmac.new( | |||
There was a problem hiding this comment.
One change we should make here is to have two clear values current_user_id_hash and current_user_email_hash as otherwise it will be confusing what they are used for. The ID one would be great for "secure mode" in the future and the email for anything that requires emails (like this feature or maybe sending emails in the future)
| """ | ||
| Based on hmac validation | ||
| """ | ||
| # TODO: Do I need to consider any time-based validation here? |
There was a problem hiding this comment.
Ideally yeah although its definitely harder for the implementer. Would be awesome for very sensitive operations to include a ?t=NOW param as part of the message that can then be validated both in terms of time and content. That can always be added later though when we have a really high need for validation.
|
This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the |
|
Just out of interest, how does this differ from using |
|
oh I saw that package, didn't realise we were already using it! ( https://pypi.org/project/django-encrypted-model-fields/ ). Seeing the 0.X.X versions and the 2 year ago last update didn't inspire too much confidence 🙈 Tech wise, just that not doing it at the model level gives us more freedom to customise and recover from problems (like rotating encryption keys), and also customise when we want to allow decrypting / sending back a serialised decrypted field. But, not married to the current PR, potentially this customisation is unnecessary for now. |
Problem
Still need to test everything, add migration, but this should add support for:
Any API endpoint can make use of these to validate a given payload + hash.
Changes
👉 Stay up-to-date with PostHog coding conventions for a smoother review.
Does this work well for both Cloud and self-hosted?
How did you test this code?