genecommerce / module-encryption-key-manager

Tooling to help generate and invalidate magento encryption keys
GNU Lesser General Public License v3.0
54 stars 14 forks source link

Feature: Add support for JSON blobs #28

Closed tedsalmon closed 1 month ago

tedsalmon commented 1 month ago

Hello,

Thanks for developing this module -- it accomplished everything I thought I was going to have to do myself :).

I ran into the use case of needing to re-encrypt a column with a JSON blob that included an encrypted secret. I have extended the re-encrypt column command to handle JSON fields using dot notation and I have submitted a pull request: https://github.com/genecommerce/module-encryption-key-manager/pull/27

Here is an example of the new functionality:

magento gene:encryption-key-manager:reencrypt-column firebear_export_jobs entity_id export_source.export_source_sftp_password

Let me know if you want this implemented differently -- I am happy to refactor it as required.

Thanks! -Ted

convenient commented 1 month ago

This has been resolved in your PR thanks

jorgehs91 commented 1 month ago

Hi @convenient and @tedsalmon,

I've been testing this new solution and faced an issue.

For example, when we have two encrypted values in JSON we'll need to update them separately, running for example: bin/magento gene:encryption-key-manager:reencrypt-column table_name table_key json.username bin/magento gene:encryption-key-manager:reencrypt-column table_name table_key json.password

Happens that the second time the command is run, it will not find the value to be reencrypted, due to this code: https://github.com/genecommerce/module-encryption-key-manager/blob/938f04f1f2d4a3ce254eb1ef56e56bfa4f0be403/Console/ReencryptColumn.php#L134

I've been thinking in possible solutions. Maybe we could accept multiple fields comma separate? Or maybe we could add a parameter to update all keys found in given column?

Let me know if I can help with something.

convenient commented 1 month ago

@tedsalmon @jorgehs91 maybe its worth removing that part of the query, and doing that check in the PHP level when the actual field is loaded and extracted from JSON?

tedsalmon commented 1 month ago

@convenient @jorgehs91 -- Yeah, I was just thinking that as well. I'll mull it over and implement a fix :)

tedsalmon commented 1 month ago

@convenient -

I updated the functionality and added a test case for it in https://github.com/genecommerce/module-encryption-key-manager/pull/43

Thanks! -Ted cc @jorgehs91

convenient commented 1 month ago

Thank you for engaging with the test suite @tedsalmon , i know its a bit ugly, but it was the quickest thing I could get out there to help prevent against regression.

In https://github.com/genecommerce/module-encryption-key-manager/pull/46 I've pushed an extra commit to fix em, see https://github.com/genecommerce/module-encryption-key-manager/commit/9e884d1be4f4c598a054d237ba87b1e714f4cfd4

convenient commented 1 month ago

Thanks @tedsalmon and @jorgehs91