Validate deferred trigger classpath resolves to a BaseTrigger subclass#69792
Validate deferred trigger classpath resolves to a BaseTrigger subclass#69792hypnguyen1209 wants to merge 1 commit into
Conversation
|
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide
|
There was a problem hiding this comment.
Pull request overview
Hardens the triggerer’s deferred-trigger loading path by validating that a deferred-task classpath resolves to a BaseTrigger subclass before caching/instantiation, preventing arbitrary importable callables from being invoked in the triggerer process.
Changes:
- Add a
BaseTrigger-subclass validation (and clearer docstring) inTriggerRunner.get_trigger_by_classpath. - Add a unit test covering both an allowed trigger class and a rejected non-trigger callable.
- Add a release-note newsfragment describing the security hardening (but the fragment filename needs correction).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| airflow-core/src/airflow/jobs/triggerer_job_runner.py | Validates imported objects are BaseTrigger subclasses before caching/instantiation. |
| airflow-core/tests/unit/jobs/test_triggerer_job.py | Adds regression test for accepting a real trigger and rejecting a non-trigger callable classpath. |
| airflow-core/newsfragments/99999.bugfix.rst | Adds release note entry for the hardening (currently named with a placeholder). |
The triggerer resolves a trigger from the ``classpath`` carried in the deferred-task Execution API payload via ``import_string(classpath)`` and then instantiates it with ``trigger_class(**kwargs)``. ``get_trigger_by_classpath`` did not check that the imported object is actually a ``BaseTrigger`` subclass, so any importable callable (e.g. ``subprocess.check_output``) could be loaded and invoked in the long-running triggerer process. Harden this by rejecting, before caching and before instantiation, any ``classpath`` that does not resolve to a ``type`` that is a ``BaseTrigger`` subclass. Legitimate triggers are unaffected (they are always ``BaseTrigger`` subclasses); an invalid classpath now fails the trigger cleanly instead of being instantiated. Adds a unit test covering both the accepted and rejected cases.
hypnguyen1209
force-pushed
the
harden-triggerer-classpath-validation
branch
from
7794470 to
672b5f1
Compare
| 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)): |
There was a problem hiding this 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.
There was a problem hiding this 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.
potiuk
left a comment
potiuk
left a comment
There was a problem hiding this comment.
Hmm... I think that's not really preventing the side effects it was supposed to prevent.
The triggerer resolves a trigger from the
classpathcarried in the deferred-task Execution API payload viaimport_string(classpath)and then instantiates it withtrigger_class(**kwargs).get_trigger_by_classpathdid not check that the imported object is actually aBaseTriggersubclass, so any importable callable (e.g.subprocess.check_output) could be loaded and invoked in the long-running triggerer process.Harden this by rejecting, before caching and before instantiation, any
classpaththat does not resolve to atypethat is aBaseTriggersubclass. Legitimate triggers are unaffected (they are alwaysBaseTriggersubclasses); an invalid classpath now fails the trigger cleanly instead of being instantiated. Adds a unit test covering both the accepted and rejected cases.Was generative AI tooling used to co-author this PR?
{pr_number}.significant.rst, in airflow-core/newsfragments. You can add this file in a follow-up commit after the PR is created so you know the PR number.