You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "barrysteyn (via GitHub)" <gi...@apache.org> on 2023/09/05 01:00:50 UTC
[GitHub] [airflow] barrysteyn opened a new issue, #34093: The allowed_deserialization_classes regex is broken
barrysteyn opened a new issue, #34093:
URL: https://github.com/apache/airflow/issues/34093
### Apache Airflow version
Other Airflow 2 version (please specify below)
### What happened
In #28829, the `allowed_deserialization_classes` regex was made more intuitive. It's aim (I guess) was to ensure that a period `.` be treated as a period and not as the wildcard regex character. The justification (given in #28829) was the following:
> Typically someone would like to allow any classes below 'mymodule' to match. For example, 'mymodule.dataclasses' by setting allowed_deserialization_classes to 'mymodule.*'. However this matches everything starting with mymodule, so also mymodulemalicious.'
To correct the above, the following substitution was provided to each space separated string provided by `allowed_deserialization_classes`:
```
re.sub(r"(\w)\.", r"\1\..", p)
```
Assume I have a class that I want to be serialized called `this.is.an.example`. Here is the results of the following inputs to `allowed_deserialization_classes`
* `this.is.an.example` --> This would **not match** (note it is the identical class name). The regular expression would change it to `this\\..is\\..an\\..example`. Which means that there is an extra wildcard `.` that would need be matched.
* `this.is.an*` --> This would **not match**. The regular expression would change it to `this\\..is\\..an*` which suffer from the problem in the first example.
* `this.*` --> This would **match** (note that this example is given in the unit tests for #28829). The regular expression would would change it to `this\\..*`. It will match because it will look for the word *this* followed by *.* and then the wildcard character with the wildstar character (which will match zero or all of anything). To note that this is *THE* only way to make things match without altering the input string.
### What you think should happen instead
In order to make a match for `this.is.an.example`, I need to put as input in `allowed_deserialization_classes` **this[.]is[.]an[.]example**:
* The above input would be left alone by the regex expression substitution.
* It also does what the author wants (which is to match the period `.`)
* To note that one can also just escape the `.` by doing **this\.is\.an\.example** which would have the identical result.
In fact, any correct regex would match (even **this[.]is[.]an***) as long as `re.sub(r"(\w)\.", r"\1\..", p)` does alter things.
### What I think should happen
It is dubious to alter a regex in the first place. Airflow is aimed at technical people, and as such, proper documentation would suffice. There I suggest the following:
1. Remove the `re.sub(r"(\w)\.", r"\1\..", p)`
2. Correct the documentation and warn people to escape the wildcard character.
3. If the intention is still to make it easier (without needing to escape the wildcard character, then the regex should be changed to `re.sub(r"(\w)\.", r"\1\.", p)` (note the removal of the last period compared to what is in the code base now)
### How to reproduce
I reproduced by copying the logic and putting it in a test task. I did the following:
```python
@dag(
dag_id="test-dag-used-to-test",
start_date=datetime(2021, 1, 1), # start sometime in the past
catchup=False, # important, since we started in the past
)
def test_dag():
@task
def test_match():
input = 'this.is.an.example' # Note that is the exact class name
p = re.sub(r"(\w)\.", r"\1\..", input)
result = any(p.match(classname) # Returns false
test_match()
test_dag()
```
### Operating System
MacOS
### Versions of Apache Airflow Providers
The change was introduced in #28829 which was from AirFlow `2.6.0`
### Deployment
Amazon (AWS) MWAA
### Deployment details
_No response_
### Anything else
_No response_
### Are you willing to submit PR?
- [X] Yes I am willing to submit a PR!
### Code of Conduct
- [X] I agree to follow this project's [Code of Conduct](https://github.com/apache/airflow/blob/main/CODE_OF_CONDUCT.md)
--
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: commits-unsubscribe@airflow.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] barrysteyn commented on issue #34093: The allowed_deserialization_classes regex is broken
Posted by "barrysteyn (via GitHub)" <gi...@apache.org>.
barrysteyn commented on issue #34093:
URL: https://github.com/apache/airflow/issues/34093#issuecomment-1706773954
Thanks @potiuk. The ultimate aim of `allowed_deserialization_classes` is provide protection against unexpected class deserialization. Using a regex is goes against that - as @bolkedebruin pointed out, you can have unexpected results and end up allowing things you didn't want. However, I am guessing one of the things that @bolkedebruin wanted to do was to switch things off completely by allowing the regex `.*` which would match everything.
That said, I think we should do the following:
1. If `allowed_deserialization_classes` is None, then operate as per normal (which would be not to allow cusomt class deserialization)
2. If `allowed_deserialization_classes` is `''` (empty string), turn the feature off (I am still not sure how useful this feature is, but I bet there are lots of people who would like to turn it off).
3. If `allowed_deserialization_classes` has strings in it, then use [fnmatch](https://docs.python.org/3/library/fnmatch.html#fnmatch.filter) to do (as you suggest) a **glob** like pattern matching search.
4. Do not do any regex matching (and since #28829, this feature has been broken and so I don't think it is used much - except maybe to turn things off)
A note on (4) above: I do not advocate for trying string match, then glob match and then regex. This is way too complex for what is aimed at securing the system. It opens up a can of worms. Rather, let's just use `fnmatch` if we need to match. And... we need to vastly improve the documentation:
1. Remove cryptic comments like `Bare “.” will be replaced so you can set airflow*` - I am a first language English speaker and I have no idea what that implies.
2. State that `allowed_deserialization_classes` is just a space separated string that will be used with `split()` to make lists.
3. Give the rules as I mention above (`None` will enforce things, `''` will stop enforcement, all other strings will be split and matched with `fnmatch`)
What do you think?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on issue #34093: The allowed_deserialization_classes regex is broken
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on issue #34093:
URL: https://github.com/apache/airflow/issues/34093#issuecomment-1707020916
Absolutely - feel free to open a new PR (even now before getting feedback from the others). We can always iterate over the PR and get to the final solution there.
And we have quite good CONTRIBUTING doss to get you started (just find it in the top level of the repo) - including some useful guidelines on setting up your IDEs and we also have `breeze` development environment based on docker-compose that is super easy to setup and keep updated and it providers 1-1 local reproducibility of everything that happens in CI - so it's easy to reproduce and see (and fix)) problems our CI / static checks /docs building requires
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] barrysteyn commented on issue #34093: The allowed_deserialization_classes regex is broken
Posted by "barrysteyn (via GitHub)" <gi...@apache.org>.
barrysteyn commented on issue #34093:
URL: https://github.com/apache/airflow/issues/34093#issuecomment-1706906717
@potiuk Thanks for the reply. I have just one final question (mainly because I am new to the AirFlow community): How does it work from here? Would you like me to submit a PR (which would take me some time to set up things - but would be a pleasure to help out the community) or will someone just pick this up?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] The allowed_deserialization_classes regex is broken [airflow]
Posted by "bolkedebruin (via GitHub)" <gi...@apache.org>.
bolkedebruin closed issue #34093: The allowed_deserialization_classes regex is broken
URL: https://github.com/apache/airflow/issues/34093
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on issue #34093: The allowed_deserialization_classes regex is broken
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on issue #34093:
URL: https://github.com/apache/airflow/issues/34093#issuecomment-1705991452
Agree. It makes very little sense how it is defined. Especially the comment `Bare “.” will be replaced so you can set airflow*` is pretty misleaning in the context of your examples.
I think - if we wanted to do it more intuitively (but keep the regexp compatibility) we should do it differently
1) Check if the pattern string ends with `.*`. If so use `glob` matching rather than regexp matching
2) Check if the string fully matches existing class (we can attempt to import the pattern and see if it works).
3) If none of the above, fallback to regexp matching
4) Update the documentation describing it (and treat it as a bugfix not breaking change).
I believe this will strike the right balance between the intuitiveness of full-class specification, ability to glob-match when you want to exclude whole packages while keeping the power of regular expressions (both 1) and 2) are highly unlikely to be specified with the intention of being regular expressions).
@kaxil @bolkedebruin - does it cover the intentions of the https://github.com/apache/airflow/pull/28829 ? I believe that was the intention. The `\,,.` replacement indeed does not properly match expressions that contain more than one top-level package and make the matching far less intuitive if you have hierarchical packages.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on issue #34093: The allowed_deserialization_classes regex is broken
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on issue #34093:
URL: https://github.com/apache/airflow/issues/34093#issuecomment-1706899136
I am also fine for that. Regexp fallback was only there as a compatibility measure and I am perfectly fine to do "glob-only" - that would by my way of implemeting it if I were to implement it from scratch.
However indeed this is an obscure feature (I'd say) and using regexp was (somewhat - not completely) broken since that change and this is the first time we hear something about it, so I would not be surprised if the feature is hardly used. So it's really on the border of "is it really breaking someone's workflow".
We could really treat it as a bugiix and add release notes /significant newsfragment that announcing that this feature was using regexp was a bug that is going to be fixed in the new release. I think also the radius blast of it is very limited - the worst thing that happens, the serialisation errors will fail everything if the match stops working and we can even likely update the error message to explain that this might be due to the bugfixing of the parameter and link to documentation explaining how to fix it.
@kaxil @bolkedebruin - WDYT?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org