nic-6443 commented on PR #14162: URL: https://github.com/apache/apisix/pull/14162#issuecomment-6679254062
I think this can be a lot smaller. The point of the PR is to switch to a self-describing encrypted format, so decryption no longer depends on each release's own `encrypt_fields`. I don't think the new format also needs to stay compatible with releases that predate it. That's the same deal we already have whenever a field is added to `encrypt_fields`: once the new release encrypts it, an older release reads ciphertext it doesn't know to decrypt. Users avoid writing through the Admin API during a rolling upgrade, and rolling back means restoring the data backed up before the upgrade. Switching to `$ENC$v1$` can follow the same rule, so I'd drop `write_marker` and always write the marked form. The release note just needs to say that once resources are re-saved, going back to a release without marker support requires restoring that backup, and this time it affects every encrypted field, not only a newly added one. With that, I think the PR comes down to: - `data_encryption`: `has_marker` / `encrypt_with_marker` / `decrypt_marked`, and `encrypt_conf` always using the marked form. - On read, run the existing `encrypt_fields` path first and have it skip values that carry the marker, then one in-place recursive pass that decrypts every string starting with `$ENC$v1$`. Call it from `plugin_checker`, `stream_plugin_checker` and `check_plugin_metadata`, and from `decrypt_params` for Admin API reads. If a marked value fails to decrypt, log it and keep the value as-is, the same way the `encrypt_fields` path treats old ciphertext today. Unmarked values keep going through `encrypt_fields` as before. With that ordering a decrypted plaintext is never decrypted again, so the `marked` argument threaded through `decrypt_conf` / `process_encrypt_field` isn't needed. What I think can go: `check_reserved_prefix` / `find_reserved_prefix` / `has_reserved_prefix`, and making the resource invalid when a marked value doesn't decrypt. Once a failed decryption just logs and keeps the value, a user-supplied string that happens to start with `$ENC$` is kept as it is, so there's nothing left to reject on input. That also removes both behaviour changes the description lists as applying regardless of the option. `decrypt_patch_target` / `find_marked` / `unchanged_marked` / `restore_marked` and the related `resource.lua` changes. With the marker always written, PATCH works the way it does on master: decrypt, merge, and `check_conf` re-encrypts, now in the marked form. The only case these functions cover is a marked field missing from the current release's `encrypt_fields`, which only happens after a rollback without restoring the backup, and I don't think we need to support that. `collect_marked` / `apply_marked` (the all-or-nothing decryption). With failures no longer making the resource invalid, there's no partially decrypted failure state left to guard. And only `encrypt_fields` values get marked, so decrypting them in place before `check_schema` exposes exactly what master already exposes when validation fails, and #14157 already deals with those log lines. The same goes for the log changes in `http/route.lua`, `http/service.lua`, `init.lua`, `utils/log-util.lua` and `plugin.conf_version`: with `enable_encrypt_fields` on, master already logs those plaintexts, so the marker doesn't introduce them. Decrypting plugin metadata on a copy. The leak membphis pointed out comes from moving decryption ahead of the schema check. If metadata is decrypted after the check, as master does for `encrypt_fields` today, the copy isn't needed. Decrypting the plugin metadata list in `decrypt_params`. Master already returns ciphertext there, so it's unrelated to the marker. -- 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]
