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.
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.
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.
It doesn't look like you updated this in the final version
There was a problem hiding this comment.
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.
ah, just realized it's that way because decrypted is also used in the catch body.
There was a problem hiding this comment.
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.
I don't like these non-standard "maybe" semantics one bit 😄
Co-authored-by: Chris Truter <crisptrutski@users.noreply.github.com>
qnkhuat
added
the
backport
| (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.
Doesn't it make more sense to just fetch and check the value of the secret key?
There was a problem hiding this comment.
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.interfacefor custom migration; instead, it adds dependency onmetabase.util.encryptionbut I think this ns is less likely to change and safe to use in migration.