sha174n commented on PR #40654: URL: https://github.com/apache/superset/pull/40654#issuecomment-4708414083
Reviewed the engine selection and the migrator in depth. The fail-closed resolver (an unknown engine raises at startup rather than degrading to CBC, with the config value kept out of the error) and the migrator's authenticated-wins classification (so a GCM value can't be mis-skipped on a CBC rollback) are both well done, and the run is correctly all-or-nothing (rolls back on any failure). Two non-blocking notes: 1. The whole re-encryption runs in a single `engine.begin()` transaction. That's the right safety tradeoff, but on an install with a very large encrypted table (e.g. many stored OAuth tokens) it's a long-held transaction; worth a caveat in the runbook or a future per-table batch option. 2. AES-GCM is randomized, so encrypted fields stop being queryable by value (unlike CBC). No functional break today since nothing queries encrypted columns by value, just worth keeping in mind for future code. LGTM. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
