-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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.
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
Fix migration fail with encryption key #42617
Conversation
|
|
||
(defn- encrypted-json-out | ||
[v] | ||
(let [decrypted (encryption/maybe-decrypt v)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm surprised that this is outside the try, can you explain why that is? A comment in the code would be great.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh I copied the code without much thinking. Maybe it's like that because maybe-encrypt
already handles exception graciously?
It's probably better to have it inside the try-catch imo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't look like you updated this in the final version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's fine to change this only here even if you don't want to update the app code (yet?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, just realized it's that way because decrypted is also used in the catch body.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh, I see it also does a try catch, and will just return the raw value if there was a failure. Seems like this could fail horribly if the encrypted string is all numeric characters then... 😬
I see there is already a warning logged if the secret key is not preset, or valid. Perhaps we should just raise that log level?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like these non-standard "maybe" semantics one bit 😄
Co-authored-by: Chris Truter <crisptrutski@users.noreply.github.com>
(catch Throwable e | ||
(if (or (encryption/possibly-encrypted-string? decrypted) | ||
(encryption/possibly-encrypted-bytes? decrypted)) | ||
(log/error e "Could not decrypt encrypted field! Have you forgot to set MB_ENCRYPTION_SECRET_KEY?") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't it make more sense to just fetch and check the value of the secret key?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, we can probably get a better error message here by doing that.
But we have tests for it, and I don't want to change some existing behavior as part of an escalation fix.
@qnkhuat Did you forget to add a milestone to the issue for this PR? When and where should I add a milestone? |
Fixes #42612 and #42611
The custom migration was getting raw
db.details
, but it didn't take into account cases where db-details are encrypted.This PR fixes that and also:
metabase.models.interface
for custom migration; instead, it adds dependency onmetabase.util.encryption
but I think this ns is less likely to change and safe to use in migration.