potiuk commented on code in PR #36801:
URL: https://github.com/apache/airflow/pull/36801#discussion_r1452776572


##########
airflow/jobs/triggerer_job_runner.py:
##########
@@ -715,13 +715,20 @@ def trigger_row_to_trigger_instance(self, trigger_row: 
Trigger, trigger_class: t
         """Convert a Trigger row into a Trigger instance."""
         from airflow.models.crypto import get_fernet
 
-        decrypted_kwargs = {}
         fernet = get_fernet()
-        for k, v in trigger_row.kwargs.items():
-            if k.startswith(ENCRYPTED_KWARGS_PREFIX):
-                decrypted_kwargs[k[len(ENCRYPTED_KWARGS_PREFIX) :]] = 
fernet.decrypt(
-                    v.encode("utf-8")
-                ).decode("utf-8")
-            else:
-                decrypted_kwargs[k] = v
+
+        def _decrypt(_value: Any) -> Any:
+            if isinstance(_value, str):
+                return fernet.decrypt(_value.encode("utf-8")).decode("utf-8")
+            if isinstance(_value, dict):
+                return {k: _decrypt(v) for k, v in _value.items()}
+            if isinstance(_value, list):
+                return [_decrypt(v) for v in _value]
+            if isinstance(_value, tuple):
+                return tuple(_decrypt(v) for v in _value)
+            return _value
+
+        decrypted_kwargs = {}
+        for key, value in trigger_row.kwargs.items():
+            decrypted_kwargs[key] = _decrypt(value)

Review Comment:
   Also one more thing here - encrypting all strings vs. the whole blob might 
be a performance hit and it will produce strange results if strings are small. 
I had a discussion about it with @bolkedebruin - if you have quite a number o 
of small strings that have predictable values encrypted, there might be ways 
that this can be utilised to guess the encryption keys (rainbow tables and the 
like) - in this case it's not likely to be possible because fernet key is 
generally a rather long random bate array, but I think a good `security` 
practice is to encrypt whole blob rather than individual fields witin it 
(especially if some of those fields are small and predictable). 
   
   I do not have an exact attack in mind - it's just intelligent 
guess/intuition that in this case encrypting all kwargs fields serialized as a 
single blob will have much better performance and security properties. 



-- 
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]

Reply via email to