hypnguyen1209 commented on code in PR #69792:
URL: https://github.com/apache/airflow/pull/69792#discussion_r3572034840
##########
airflow-core/src/airflow/jobs/triggerer_job_runner.py:
##########
@@ -1696,8 +1696,22 @@ def get_trigger_by_classpath(self, classpath: str) ->
type[BaseTrigger]:
"""
Get a trigger class by its classpath ("path.to.module.classname").
+ The resolved object must be a
:class:`~airflow.triggers.base.BaseTrigger`
+ subclass. This is validated before the class is cached and, crucially,
+ before it is ever instantiated in ``create_triggers`` -- ``classpath``
+ originates from the (attacker-influenceable) deferred-task payload, so
+ without this check an arbitrary importable callable could be invoked in
+ the triggerer process.
+
Uses a cache dictionary to speed up lookups after the first time.
"""
if classpath not in self.trigger_cache:
- self.trigger_cache[classpath] = import_string(classpath)
+ trigger_class = import_string(classpath)
+ if not (isinstance(trigger_class, type) and
issubclass(trigger_class, BaseTrigger)):
Review Comment:
> I think this should be pre-import check, unfortunately - by the time we
get here the class is already imported, and the whole idea is to avoid even
importing it - because just importing it can have side-effects. I am not sure
if that one is even reasonably doable - because in order to get class hierarchy
you need to import it. Just AST parsing will not solve it.
>
> So I am not sure if that is solving such defense-in-depth is even doable.
Correct - the issubclass check runs after import_string, so it doesn't stop
import-time side effects of an arbitrary module.
To be precise about what it does stop: the reported path is
classpath="subprocess.check_output" + trigger_kwargs, where the code executes
at trigger_class(**kwargs) (the call) - importing stdlib subprocess is
side-effect-free, so the issubclass gate blocks the actual execution. It does
not help when the classpath points at a module whose import has side effects
(e.g. an attacker-plantable module on sys.path).
A genuinely pre-import defense is doable, just not via issubclass (which
needs the class, hence the import). It has to be a string check on the
classpath before import_string - a module-namespace allow-list:
```
module = classpath.rpartition(".")[0]
if not (module == "airflow" or module.startswith("airflow.") or module in
allowed_extra):
raise TypeError(...) # before import_string()
```
That never imports anything outside the trusted namespace (core airflow.* +
providers airflow.providers.*), so import-time side effects of
untrusted/planted modules are never triggered. The issubclass check can stay
after import as defense-in-depth.
The trade-off is custom triggers in user modules (mycompany.triggers.X):
those would need a configurable prefix allow-list (similar to [core]
allowed_deserialization_classes) to keep working - a behaviour change plus a
new config option, which is why I wanted to check the direction before
implementing.
--
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]
