You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@airflow.apache.org by Jarek Potiuk <ja...@potiuk.com> on 2025/02/19 09:26:29 UTC
[DISCUSS] Getting rid of regular expressions (and getting rid of re2 as result of it) ?
Hello Here,
I wanted to discuss something that bothered me for a while and I think
makes a good case for a small breaking change in our public interfaces for
Airflow 3 a slack conversation from today (where re2 was evidently crashing
due to some long regexp passed to it) triggered me to call for action.
I would like to propose that we get rid of regexp in all the places where
we have user controllable input where user can supply a regexp (for example
in REST APIs, APIs used by UI, CLI, operator APIs) and other places where
the user can provide their own regexp now. And that should allow us to get
rid of google-re2 as a dependency (google-re2 is a somewhat problematic
dependency to have).
*A bit of context:*
We've replaced stdlib re usage with re2 in June 2023 as a result of a
security report we got. In one (and likely few other API calls of ours) we
used parameter that we passed (root) as regex - and that parameter was
controllable by the user - and they could potentially cause ReDOS
https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
issue (i.e. crash our webserver by providing a very small carefully crafted
regexp in root parameter). google-re2 is (theoretically) protecting
about this kind of issues b y limiting regexp supported and using c regexp
library written by google that is faster.
So as a result - as we were not sure if there are other cases (it was not
really obvious that "root" passed message is then parsed as regexp - we
used re2 everywhere in airflow, mandated use of re2 and added pre-commit
checking it. For those who have access to our security mailing list
https://lists.apache.org/thread/3mjt86r7djzdg5t22tby431t9cvg8drl is the
relevant thread.
*Problem*:
That caused a number of problems:
* google-re2 is binary package and it does not have binary wheel for newer
Python versions and some platforms - and it has to be compiled on those,
and it often fails, we had for example problem with installing airflow on
conda
* https://github.com/apache/airflow/discussions/32852
*
https://stackoverflow.com/questions/76701323/trying-to-download-apache-airflow-with-pip-but-error-pops-up-when-building-whee
* https://github.com/apache/airflow/issues/32849
*
https://erogluegemen.medium.com/how-to-install-apache-airflow-on-mac-df9bd5cf1ff8
*
https://stackoverflow.com/questions/77575842/unable-to-install-google-re2-on-macos-14-1-1
And the list goes on-and-on. Actually in my "Beach cleaning" review of
project - re2 is high on the list to "forego" - also because it's written
in C, and while it is developed in google, it might be that there are
similar regexp vulnerabilities as the original re vulnerabilities
*Proposed solution:*
I think a better approach would be to make sure that we never, ever parse a
string passed by a user as regexp. This means that we remove all the
regex-parsed user-controlled parameters in Airflow 3 (including REST API,
CLI, the APIs used by the UI, Task SDK if it's there, parameters of the
operators etc.). But also that means that we should add mechanisms
(automation/pre-commit. code review awareness with maintainers) - to
prevent adding such cases in the future.
Then we could just get rid of re2 and switch to stdlib re - if we control
all the regexp, it's safe to use.
I have not done an inventory of those usages, but there are a few places
where regexp can be provided from a client side (and maybe even fewer or
none) in the new UI. We have literally a few CLI commands in Airflow CLI
that take regex as input (dag and task command) - and there are probably a
few more places we will have to look at. In all those cases - I THINK - we
should be able to use other, safer pattern types (`glob` for one is a good
alternative in many cases and it's also a simpler one to grasp even if less
flexible).
That means "breaking change" for all those usages, but one that is rather
unlikely to cause a great problem and if we provide globs or other patterns
as replacement, it should be quite easy to handle by our users.
So my recommendation would be to:
* remove and add protection so that we do not use regexp for any user input
* remove google-re2 as dependency
BTW. This s a pure embodiment of the famous saying "If you have a problem,
introduce a regexp, now you have two problems".
WDYT?
J.
Re: [DISCUSS] Getting rid of regular expressions (and getting rid of re2 as result of it) ?
Posted by Jarek Potiuk <ja...@potiuk.com>.
Thanks Buğra!
On Thu, Feb 20, 2025 at 2:22 AM Buğra Öztürk <oz...@gmail.com>
wrote:
> Thanks for bringing this up Jarek! I like the idea of removing regexp
> definitions from everywhere.
>
> First of all, for the CLI part, when we go remote, agree. We can make these
> changes and include them as breaking since we will already make a bunch of
> breaking changes there. For backward compatibility, it will vary between
> local and remote. Local commands shouldn't be backward compatible and we
> should document that. The remote will go to the API, and since the public
> API will be backwards compatible, this should be backwards compatible, too.
> Please correct me if I'm wrong. I would love to have no backwards
> compatibility there to be more flexible. I am up for `Please use it Wisely`
> documentation (should be one of the must-haves for local_commands) and most
> likely update with glob in the next available iteration.
>
Yeah. Agree. It's good for now - but when we turn admin to "remote" we can
change. I am not too worried about those
> I think we also have in structure.py too where it is for UI. Again we have
> root there used for task_ids_or_regex.
Yep. Same story. I think this is the most "contentious" use and my question
is -
can we get rid of that regexp there. I don't know the full story on how
"root"
should be used there and whether other pattern matching can be used or
maybe we should do something else - but I see that one
(in multiple places as you noticed) as the only blocker from removing
google-re2 as a dependency.
> Maybe I am missing a use case here but the method is only used in the API
> and old UI where it will be gone soon.
I also found it used in the fast_api - but yes - maybe the new UI does not
need the
'regexp' part there? I think there are few people who are way better
equipped than
me here :) - Brent, Pierre (others) ? Would love to hear some more about
"root" usage.
> I am not sure how easy would be to cover all cases of introducing regexp.
> We may end up maintaining a step where it may miss lots of edge cases. I
> don't have any strong opinion on this though, we may simply have checks and
> that would help to increase awareness already.
>
That discussion alone might be a good protection for a while at least, but
I am still
trying to figure out some light-weight way to flag/ surface potential
user-side
regexp usage. But I am not too worried - so far the only one that is
problematic
is exactly the one we already knew about (via security report) - and It
seems we
did not have a temptation to use regex for other API calls. It also does
not "feel"
right to use regex in APIs.
> --
> Bugra Ozturk
>
Re: [DISCUSS] Getting rid of regular expressions (and getting rid of re2 as result of it) ?
Posted by Buğra Öztürk <oz...@gmail.com>.
Thanks for bringing this up Jarek! I like the idea of removing regexp
definitions from everywhere.
First of all, for the CLI part, when we go remote, agree. We can make these
changes and include them as breaking since we will already make a bunch of
breaking changes there. For backward compatibility, it will vary between
local and remote. Local commands shouldn't be backward compatible and we
should document that. The remote will go to the API, and since the public
API will be backwards compatible, this should be backwards compatible, too.
Please correct me if I'm wrong. I would love to have no backwards
compatibility there to be more flexible. I am up for `Please use it Wisely`
documentation (should be one of the must-haves for local_commands) and most
likely update with glob in the next available iteration.
I think we also have in structure.py too where it is for UI. Again we have
root there used for task_ids_or_regex. It seems the task groups and tasks
are already loaded from the files. Should we need to support regexp here?
Maybe I am missing a use case here but the method is only used in the API
and old UI where it will be gone soon. Other usages use the method with
task_id rather than regexp. The least effort could be dropping the support
and updating the downstream usages accordingly. What do you think?
I am not sure how easy would be to cover all cases of introducing regexp.
We may end up maintaining a step where it may miss lots of edge cases. I
don't have any strong opinion on this though, we may simply have checks and
that would help to increase awareness already.
On Thu, Feb 20, 2025 at 12:41 AM Jarek Potiuk <ja...@potiuk.com> wrote:
> > Challenge would be... who has capacity to make it?
>
> This seems to be not much work - assuming that we agree on how to approach
> it, I can do it with the help of involved people to understand
> the single case where I am not sure. I would love some feedback.
>
> I actually did an "inventory" of sorts in the Python 3.13 first-pass PR
> https://github.com/apache/airflow/pull/46891 - and I can extract google-re
> removal from it while fixing the dependency issues. so part of the work is
> already done.
>
> And I have concrete proposals for the approach (with one unsure thing).
>
> *1. Internal regexps - just use re*
>
> We can just use re here. I already removed and reviewed re2 where only
> "internal" regexps are used.
>
> *2. Very likely no changes needed.*
>
> We can assume that an admin person configuring it will not be "rogue", so
> likely setting regexp in configuration does not have to be "protected".
>
> * airflow/metrics/validators.py -> metrics allow/block list from
> configuration
> * airflow/serialization/serde.py -> allowed_deserialization_classes_regexp
> from configuration
> * airflow/models/dagrun.py -> allowed_run_id_pattern
> * task_sdk/src/airflow/sdk/execution_time/secrets_masker.py -> patterns
> configured by secret adapters
>
> *3. Security model clarification maybe? Or breaking changes?*
>
> I think while we could add some breaking changes, it will be better to
> clarify things in security mode.
>
> * airflow/cli/commands/remote_commands/dag_command.py : dag command,
> mark_success_pattern
> * airflow/models.dag.py : mark_success_pattern: dag/test dag command
> * airflow/utils/cli.py: get_dags : dags command
>
> Those regexps are provided by potentially "low access" CLI (especially
> when later we do remote CLI) - so we could likely change it to glob. But
> instead we can also make a security model explanation and explain
> that whoever has access to CLI and those specific commands should be
> trusted not to abuse it and cause ReDoc. Also - we can worry about it when
> we go remot, and we can also mention that the CLI command line is not
> covered by "back-compatibility" guarantee (so that we can change it later).
>
> * airflow/utils/file.py: those are .*ariflowignore* rules. DAG authors
> could potentially modify them, but DAG authors can still do "dos" pretty
> freely with just TOP level code and task doing "stuff" - so while we could
> consider dropping regexp, I see no big value in it and we can use "re".
>
> *4. Unsure (here I need some help) *
>
> * task_ids_or_regex (airflow/models/dag.py, sdk/definitions, grid.py ->
> this is the one that was flagged by the security report (passed as root
> parameter) - this is used by both UI and exposed via task_sdk. I am not
> really sure - if we really need to use regex here, maybe glob (which is
> less flexible and safer) can be used instead of regex even if it will be
> somewhat breaking/new behaviour for the user?
>
> I think Brent, Ash, Amogh, Pierre - you've been touching that one
> recently. Would love to hear what you think whether we can do something
> with it?
>
> *5. Future protection*
>
> Looking at the "number of cases" above and how those regexes could be
> passed between UI/ task SDK API etc - I am not sure if we can automate
> "future" avoidance easily. I think we might simply start paying attention
> and treat the "introduce regexp - you have two problems" saying more
> seriously during reviews and possibly maybe - similar to the "internal-api"
> label - flag and fail if a PR introduces a regexp :) ?. Not sure if it's
> worth it though
>
> WDYT? Does the approach look good? Any comments?
>
> J.
>
>
>
>
> On Wed, Feb 19, 2025 at 9:35 PM Jens Scheffler <j_scheffler@gmx.de.invalid
> >
> wrote:
>
> > Sounds good to me - I was not aware of user-facing interfaces... and if
> > removing support then 3.0 is the best point.
> >
> > Challenge would be... who has capacity to make it?
> >
> > On 19.02.25 15:39, Jed Cunningham wrote:
> > > Sounds good to me.
> > >
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: dev-unsubscribe@airflow.apache.org
> > For additional commands, e-mail: dev-help@airflow.apache.org
> >
> >
>
--
Bugra Ozturk
Re: [DISCUSS] Getting rid of regular expressions (and getting rid of re2 as result of it) ?
Posted by Jarek Potiuk <ja...@potiuk.com>.
> Challenge would be... who has capacity to make it?
This seems to be not much work - assuming that we agree on how to approach
it, I can do it with the help of involved people to understand
the single case where I am not sure. I would love some feedback.
I actually did an "inventory" of sorts in the Python 3.13 first-pass PR
https://github.com/apache/airflow/pull/46891 - and I can extract google-re
removal from it while fixing the dependency issues. so part of the work is
already done.
And I have concrete proposals for the approach (with one unsure thing).
*1. Internal regexps - just use re*
We can just use re here. I already removed and reviewed re2 where only
"internal" regexps are used.
*2. Very likely no changes needed.*
We can assume that an admin person configuring it will not be "rogue", so
likely setting regexp in configuration does not have to be "protected".
* airflow/metrics/validators.py -> metrics allow/block list from
configuration
* airflow/serialization/serde.py -> allowed_deserialization_classes_regexp
from configuration
* airflow/models/dagrun.py -> allowed_run_id_pattern
* task_sdk/src/airflow/sdk/execution_time/secrets_masker.py -> patterns
configured by secret adapters
*3. Security model clarification maybe? Or breaking changes?*
I think while we could add some breaking changes, it will be better to
clarify things in security mode.
* airflow/cli/commands/remote_commands/dag_command.py : dag command,
mark_success_pattern
* airflow/models.dag.py : mark_success_pattern: dag/test dag command
* airflow/utils/cli.py: get_dags : dags command
Those regexps are provided by potentially "low access" CLI (especially
when later we do remote CLI) - so we could likely change it to glob. But
instead we can also make a security model explanation and explain
that whoever has access to CLI and those specific commands should be
trusted not to abuse it and cause ReDoc. Also - we can worry about it when
we go remot, and we can also mention that the CLI command line is not
covered by "back-compatibility" guarantee (so that we can change it later).
* airflow/utils/file.py: those are .*ariflowignore* rules. DAG authors
could potentially modify them, but DAG authors can still do "dos" pretty
freely with just TOP level code and task doing "stuff" - so while we could
consider dropping regexp, I see no big value in it and we can use "re".
*4. Unsure (here I need some help) *
* task_ids_or_regex (airflow/models/dag.py, sdk/definitions, grid.py ->
this is the one that was flagged by the security report (passed as root
parameter) - this is used by both UI and exposed via task_sdk. I am not
really sure - if we really need to use regex here, maybe glob (which is
less flexible and safer) can be used instead of regex even if it will be
somewhat breaking/new behaviour for the user?
I think Brent, Ash, Amogh, Pierre - you've been touching that one
recently. Would love to hear what you think whether we can do something
with it?
*5. Future protection*
Looking at the "number of cases" above and how those regexes could be
passed between UI/ task SDK API etc - I am not sure if we can automate
"future" avoidance easily. I think we might simply start paying attention
and treat the "introduce regexp - you have two problems" saying more
seriously during reviews and possibly maybe - similar to the "internal-api"
label - flag and fail if a PR introduces a regexp :) ?. Not sure if it's
worth it though
WDYT? Does the approach look good? Any comments?
J.
On Wed, Feb 19, 2025 at 9:35 PM Jens Scheffler <j_...@gmx.de.invalid>
wrote:
> Sounds good to me - I was not aware of user-facing interfaces... and if
> removing support then 3.0 is the best point.
>
> Challenge would be... who has capacity to make it?
>
> On 19.02.25 15:39, Jed Cunningham wrote:
> > Sounds good to me.
> >
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@airflow.apache.org
> For additional commands, e-mail: dev-help@airflow.apache.org
>
>
Re: [DISCUSS] Getting rid of regular expressions (and getting rid of re2 as result of it) ?
Posted by Jens Scheffler <j_...@gmx.de.INVALID>.
Sounds good to me - I was not aware of user-facing interfaces... and if
removing support then 3.0 is the best point.
Challenge would be... who has capacity to make it?
On 19.02.25 15:39, Jed Cunningham wrote:
> Sounds good to me.
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@airflow.apache.org
For additional commands, e-mail: dev-help@airflow.apache.org
Re: [DISCUSS] Getting rid of regular expressions (and getting rid of re2 as result of it) ?
Posted by Jed Cunningham <je...@apache.org>.
Sounds good to me.
Re: [DISCUSS] Getting rid of regular expressions (and getting rid of re2 as result of it) ?
Posted by Daniel Standish <da...@astronomer.io.INVALID>.
Doesn't bother me.
On Wed, Feb 19, 2025 at 2:55 AM Jarek Potiuk <ja...@potiuk.com> wrote:
> And one comment as well. I **just** tried to attempt to add Python 3.13
> support (finally):
>
> And of course - the binary wheels google-re2 released for Python 3.13 are
> not enough to be used on Debian Bookworm( 1) and the build process fails:
>
> 5.974 Built crcmod==1.7
> 6.196 × Failed to build `google-re2==1.1.20240702`
> 6.196 ├─▶ The build backend returned an error
> 6.196 ╰─▶ Call to `setuptools.build_meta:__legacy__.build_wheel` failed
> (exit
> 6.196 status: 1)
> 6.196
> 6.196 [stdout]
> 6.196 running bdist_wheel
> 6.196 running build
> 6.196 running build_py
> 6.196 copying re2/__init__.py ->
> build/lib.linux-x86_64-cpython-313/re2
> 6.196 running build_ext
> 6.196 building '_re2' extension
> 6.196 g++ -fno-strict-overflow -Wsign-compare -DNDEBUG -g -O3
> 6.196 -Wall -fPIC -I/root/.cache/uv/builds-v0/.tmp7L9INs/include
> 6.196 -I/usr/local/include/python3.13 -c _re2.cc -o
> 6.196 build/temp.linux-x86_64-cpython-313/_re2.o -fvisibility=hidden
> 6.196
> 6.196 [stderr]
> 6.196 _re2.cc:15:10: fatal error: absl/strings/string_view.h: No such
> file
> 6.196 or directory
> 6.196 15 | #include "absl/strings/string_view.h"
> 6.196 | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 6.196 compilation terminated.
> 6.196 error: command '/usr/bin/g++' failed with exit code 1
> 6.196
> 6.196 hint: This usually indicates a problem with the package or the
> build
> 6.196 environment.
> 6.196 help: `google-re2` (v1.1.20240702) was included because
> `apache-airflow`
> 6.196 (v3.0.0.dev0) depends on `google-re2`
> ------
> ERROR: failed to solve: process "/bin/bash -o pipefail -o errexit -o
> nounset -o nolog -c bash /scripts/docker/install_airflow.sh" did not
> complete successfully: exit code: 1
>
> So - as far as I am concerned - google-re2 should go away
>
> On Wed, Feb 19, 2025 at 10:26 AM Jarek Potiuk <ja...@potiuk.com> wrote:
>
> > Hello Here,
> >
> > I wanted to discuss something that bothered me for a while and I think
> > makes a good case for a small breaking change in our public interfaces
> for
> > Airflow 3 a slack conversation from today (where re2 was evidently
> crashing
> > due to some long regexp passed to it) triggered me to call for action.
> >
> > I would like to propose that we get rid of regexp in all the places where
> > we have user controllable input where user can supply a regexp (for
> example
> > in REST APIs, APIs used by UI, CLI, operator APIs) and other places where
> > the user can provide their own regexp now. And that should allow us to
> get
> > rid of google-re2 as a dependency (google-re2 is a somewhat problematic
> > dependency to have).
> >
> > *A bit of context:*
> >
> > We've replaced stdlib re usage with re2 in June 2023 as a result of a
> > security report we got. In one (and likely few other API calls of ours)
> we
> > used parameter that we passed (root) as regex - and that parameter was
> > controllable by the user - and they could potentially cause ReDOS
> >
> https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
> > issue (i.e. crash our webserver by providing a very small carefully
> crafted
> > regexp in root parameter). google-re2 is (theoretically) protecting
> > about this kind of issues b y limiting regexp supported and using c
> regexp
> > library written by google that is faster.
> >
> > So as a result - as we were not sure if there are other cases (it was not
> > really obvious that "root" passed message is then parsed as regexp - we
> > used re2 everywhere in airflow, mandated use of re2 and added pre-commit
> > checking it. For those who have access to our security mailing list
> > https://lists.apache.org/thread/3mjt86r7djzdg5t22tby431t9cvg8drl is the
> > relevant thread.
> >
> > *Problem*:
> >
> > That caused a number of problems:
> > * google-re2 is binary package and it does not have binary wheel for
> newer
> > Python versions and some platforms - and it has to be compiled on those,
> > and it often fails, we had for example problem with installing airflow on
> > conda
> >
> > * https://github.com/apache/airflow/discussions/32852
> > *
> >
> https://stackoverflow.com/questions/76701323/trying-to-download-apache-airflow-with-pip-but-error-pops-up-when-building-whee
> > * https://github.com/apache/airflow/issues/32849
> > *
> >
> https://erogluegemen.medium.com/how-to-install-apache-airflow-on-mac-df9bd5cf1ff8
> > *
> >
> https://stackoverflow.com/questions/77575842/unable-to-install-google-re2-on-macos-14-1-1
> >
> > And the list goes on-and-on. Actually in my "Beach cleaning" review of
> > project - re2 is high on the list to "forego" - also because it's written
> > in C, and while it is developed in google, it might be that there are
> > similar regexp vulnerabilities as the original re vulnerabilities
> >
> > *Proposed solution:*
> >
> > I think a better approach would be to make sure that we never, ever parse
> > a string passed by a user as regexp. This means that we remove all the
> > regex-parsed user-controlled parameters in Airflow 3 (including REST API,
> > CLI, the APIs used by the UI, Task SDK if it's there, parameters of the
> > operators etc.). But also that means that we should add mechanisms
> > (automation/pre-commit. code review awareness with maintainers) - to
> > prevent adding such cases in the future.
> >
> > Then we could just get rid of re2 and switch to stdlib re - if we control
> > all the regexp, it's safe to use.
> >
> > I have not done an inventory of those usages, but there are a few places
> > where regexp can be provided from a client side (and maybe even fewer or
> > none) in the new UI. We have literally a few CLI commands in Airflow CLI
> > that take regex as input (dag and task command) - and there are
> probably a
> > few more places we will have to look at. In all those cases - I THINK -
> we
> > should be able to use other, safer pattern types (`glob` for one is a
> good
> > alternative in many cases and it's also a simpler one to grasp even if
> less
> > flexible).
> >
> > That means "breaking change" for all those usages, but one that is rather
> > unlikely to cause a great problem and if we provide globs or other
> patterns
> > as replacement, it should be quite easy to handle by our users.
> >
> > So my recommendation would be to:
> > * remove and add protection so that we do not use regexp for any user
> input
> > * remove google-re2 as dependency
> >
> > BTW. This s a pure embodiment of the famous saying "If you have a
> problem,
> > introduce a regexp, now you have two problems".
> >
> >
> > WDYT?
> >
> > J.
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
>
Re: [DISCUSS] Getting rid of regular expressions (and getting rid of re2 as result of it) ?
Posted by Jarek Potiuk <ja...@potiuk.com>.
And one comment as well. I **just** tried to attempt to add Python 3.13
support (finally):
And of course - the binary wheels google-re2 released for Python 3.13 are
not enough to be used on Debian Bookworm( 1) and the build process fails:
5.974 Built crcmod==1.7
6.196 × Failed to build `google-re2==1.1.20240702`
6.196 ├─▶ The build backend returned an error
6.196 ╰─▶ Call to `setuptools.build_meta:__legacy__.build_wheel` failed
(exit
6.196 status: 1)
6.196
6.196 [stdout]
6.196 running bdist_wheel
6.196 running build
6.196 running build_py
6.196 copying re2/__init__.py ->
build/lib.linux-x86_64-cpython-313/re2
6.196 running build_ext
6.196 building '_re2' extension
6.196 g++ -fno-strict-overflow -Wsign-compare -DNDEBUG -g -O3
6.196 -Wall -fPIC -I/root/.cache/uv/builds-v0/.tmp7L9INs/include
6.196 -I/usr/local/include/python3.13 -c _re2.cc -o
6.196 build/temp.linux-x86_64-cpython-313/_re2.o -fvisibility=hidden
6.196
6.196 [stderr]
6.196 _re2.cc:15:10: fatal error: absl/strings/string_view.h: No such
file
6.196 or directory
6.196 15 | #include "absl/strings/string_view.h"
6.196 | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
6.196 compilation terminated.
6.196 error: command '/usr/bin/g++' failed with exit code 1
6.196
6.196 hint: This usually indicates a problem with the package or the
build
6.196 environment.
6.196 help: `google-re2` (v1.1.20240702) was included because
`apache-airflow`
6.196 (v3.0.0.dev0) depends on `google-re2`
------
ERROR: failed to solve: process "/bin/bash -o pipefail -o errexit -o
nounset -o nolog -c bash /scripts/docker/install_airflow.sh" did not
complete successfully: exit code: 1
So - as far as I am concerned - google-re2 should go away
On Wed, Feb 19, 2025 at 10:26 AM Jarek Potiuk <ja...@potiuk.com> wrote:
> Hello Here,
>
> I wanted to discuss something that bothered me for a while and I think
> makes a good case for a small breaking change in our public interfaces for
> Airflow 3 a slack conversation from today (where re2 was evidently crashing
> due to some long regexp passed to it) triggered me to call for action.
>
> I would like to propose that we get rid of regexp in all the places where
> we have user controllable input where user can supply a regexp (for example
> in REST APIs, APIs used by UI, CLI, operator APIs) and other places where
> the user can provide their own regexp now. And that should allow us to get
> rid of google-re2 as a dependency (google-re2 is a somewhat problematic
> dependency to have).
>
> *A bit of context:*
>
> We've replaced stdlib re usage with re2 in June 2023 as a result of a
> security report we got. In one (and likely few other API calls of ours) we
> used parameter that we passed (root) as regex - and that parameter was
> controllable by the user - and they could potentially cause ReDOS
> https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
> issue (i.e. crash our webserver by providing a very small carefully crafted
> regexp in root parameter). google-re2 is (theoretically) protecting
> about this kind of issues b y limiting regexp supported and using c regexp
> library written by google that is faster.
>
> So as a result - as we were not sure if there are other cases (it was not
> really obvious that "root" passed message is then parsed as regexp - we
> used re2 everywhere in airflow, mandated use of re2 and added pre-commit
> checking it. For those who have access to our security mailing list
> https://lists.apache.org/thread/3mjt86r7djzdg5t22tby431t9cvg8drl is the
> relevant thread.
>
> *Problem*:
>
> That caused a number of problems:
> * google-re2 is binary package and it does not have binary wheel for newer
> Python versions and some platforms - and it has to be compiled on those,
> and it often fails, we had for example problem with installing airflow on
> conda
>
> * https://github.com/apache/airflow/discussions/32852
> *
> https://stackoverflow.com/questions/76701323/trying-to-download-apache-airflow-with-pip-but-error-pops-up-when-building-whee
> * https://github.com/apache/airflow/issues/32849
> *
> https://erogluegemen.medium.com/how-to-install-apache-airflow-on-mac-df9bd5cf1ff8
> *
> https://stackoverflow.com/questions/77575842/unable-to-install-google-re2-on-macos-14-1-1
>
> And the list goes on-and-on. Actually in my "Beach cleaning" review of
> project - re2 is high on the list to "forego" - also because it's written
> in C, and while it is developed in google, it might be that there are
> similar regexp vulnerabilities as the original re vulnerabilities
>
> *Proposed solution:*
>
> I think a better approach would be to make sure that we never, ever parse
> a string passed by a user as regexp. This means that we remove all the
> regex-parsed user-controlled parameters in Airflow 3 (including REST API,
> CLI, the APIs used by the UI, Task SDK if it's there, parameters of the
> operators etc.). But also that means that we should add mechanisms
> (automation/pre-commit. code review awareness with maintainers) - to
> prevent adding such cases in the future.
>
> Then we could just get rid of re2 and switch to stdlib re - if we control
> all the regexp, it's safe to use.
>
> I have not done an inventory of those usages, but there are a few places
> where regexp can be provided from a client side (and maybe even fewer or
> none) in the new UI. We have literally a few CLI commands in Airflow CLI
> that take regex as input (dag and task command) - and there are probably a
> few more places we will have to look at. In all those cases - I THINK - we
> should be able to use other, safer pattern types (`glob` for one is a good
> alternative in many cases and it's also a simpler one to grasp even if less
> flexible).
>
> That means "breaking change" for all those usages, but one that is rather
> unlikely to cause a great problem and if we provide globs or other patterns
> as replacement, it should be quite easy to handle by our users.
>
> So my recommendation would be to:
> * remove and add protection so that we do not use regexp for any user input
> * remove google-re2 as dependency
>
> BTW. This s a pure embodiment of the famous saying "If you have a problem,
> introduce a regexp, now you have two problems".
>
>
> WDYT?
>
> J.
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>