You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "bjankie1 (via GitHub)" <gi...@apache.org> on 2023/06/20 07:26:01 UTC
[GitHub] [airflow] bjankie1 opened a new pull request, #32016: Fix an issue that crashes Airflow Webserver when passed invalid private key path to Snowflake
bjankie1 opened a new pull request, #32016:
URL: https://github.com/apache/airflow/pull/32016
Passing path on an excessively large file or path to a system device crashes Webserver. This fix validates the path against such situations.
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in a newsfragment file, named `{pr_number}.significant.rst` or `{issue_number}.significant.rst`, in [newsfragments](https://github.com/apache/airflow/tree/main/newsfragments).
--
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] uranusjr commented on a diff in pull request #32016: Fix an issue that crashes Airflow Webserver when passed invalid private key path to Snowflake
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #32016:
URL: https://github.com/apache/airflow/pull/32016#discussion_r1235030775
##########
airflow/providers/snowflake/hooks/snowflake.py:
##########
@@ -216,7 +216,16 @@ def _get_conn_params(self) -> dict[str, str | None]:
"Please remove one."
)
elif private_key_file:
- private_key_pem = Path(private_key_file).read_bytes()
+ private_key_file_path = Path(private_key_file)
+ if not private_key_file_path.is_file() or private_key_file_path.stat().st_size == 0:
+ raise AirflowException(
+ "The private_key_file path points to an empty or invalid file."
+ )
Review Comment:
> the private key path should point to a regular file
And the `is_file()` call checks that. What does the other check cover?
--
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] bjankie1 commented on a diff in pull request #32016: Fix an issue that crashes Airflow Webserver when passed invalid private key path to Snowflake
Posted by "bjankie1 (via GitHub)" <gi...@apache.org>.
bjankie1 commented on code in PR #32016:
URL: https://github.com/apache/airflow/pull/32016#discussion_r1234998423
##########
airflow/providers/snowflake/hooks/snowflake.py:
##########
@@ -216,7 +216,16 @@ def _get_conn_params(self) -> dict[str, str | None]:
"Please remove one."
)
elif private_key_file:
- private_key_pem = Path(private_key_file).read_bytes()
+ private_key_file_path = Path(private_key_file)
+ if not private_key_file_path.is_file() or private_key_file_path.stat().st_size == 0:
+ raise AirflowException(
+ "The private_key_file path points to an empty or invalid file."
+ )
Review Comment:
If we don't check if path is a file a system device can be passed. e.g. `/dev/random` That would also crash Webserver.
--
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 a diff in pull request #32016: Fix an issue that crashes Airflow Webserver when passed invalid private key path to Snowflake
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #32016:
URL: https://github.com/apache/airflow/pull/32016#discussion_r1234899084
##########
airflow/providers/snowflake/hooks/snowflake.py:
##########
@@ -216,7 +216,16 @@ def _get_conn_params(self) -> dict[str, str | None]:
"Please remove one."
)
elif private_key_file:
- private_key_pem = Path(private_key_file).read_bytes()
+ private_key_file_path = Path(private_key_file)
+ if not private_key_file_path.is_file() or private_key_file_path.stat().st_size == 0:
+ raise AirflowException(
+ "The private_key_file path points to an empty or invalid file."
+ )
Review Comment:
```Passing path on an excessively large file or path to a system device crashes Webserver. This fix validates the path against such situations.``` from the description of the PR.
--
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] bjankie1 commented on a diff in pull request #32016: Fix an issue that crashes Airflow Webserver when passed invalid private key path to Snowflake
Posted by "bjankie1 (via GitHub)" <gi...@apache.org>.
bjankie1 commented on code in PR #32016:
URL: https://github.com/apache/airflow/pull/32016#discussion_r1255711968
##########
airflow/providers/snowflake/hooks/snowflake.py:
##########
@@ -216,7 +216,16 @@ def _get_conn_params(self) -> dict[str, str | None]:
"Please remove one."
)
elif private_key_file:
- private_key_pem = Path(private_key_file).read_bytes()
+ private_key_file_path = Path(private_key_file)
+ if not private_key_file_path.is_file() or private_key_file_path.stat().st_size == 0:
+ raise ValueError(
+ "The private_key_file path points to an empty or invalid file."
+ )
+ if private_key_file_path.stat().st_size > 4096:
+ raise ValueError(
+ "File size of private_key_file is too big."
Review Comment:
What documentation do you suggest putting it to? The limitations is applied to the connection settings. I coulad add a note to the operator docs.
--
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] uranusjr commented on a diff in pull request #32016: Fix an issue that crashes Airflow Webserver when passed invalid private key path to Snowflake
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #32016:
URL: https://github.com/apache/airflow/pull/32016#discussion_r1234869449
##########
airflow/providers/snowflake/hooks/snowflake.py:
##########
@@ -216,7 +216,16 @@ def _get_conn_params(self) -> dict[str, str | None]:
"Please remove one."
)
elif private_key_file:
- private_key_pem = Path(private_key_file).read_bytes()
+ private_key_file_path = Path(private_key_file)
+ if not private_key_file_path.is_file() or private_key_file_path.stat().st_size == 0:
+ raise AirflowException(
+ "The private_key_file path points to an empty or invalid file."
+ )
Review Comment:
What happens if we don’t check for these?
--
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 pull request #32016: Fix an issue that crashes Airflow Webserver when passed invalid private key path to Snowflake
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #32016:
URL: https://github.com/apache/airflow/pull/32016#issuecomment-1598341979
Needs conflict resolving.
--
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 merged pull request #32016: Fix an issue that crashes Airflow Webserver when passed invalid private key path to Snowflake
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk merged PR #32016:
URL: https://github.com/apache/airflow/pull/32016
--
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] phanikumv commented on a diff in pull request #32016: Fix an issue that crashes Airflow Webserver when passed invalid private key path to Snowflake
Posted by "phanikumv (via GitHub)" <gi...@apache.org>.
phanikumv commented on code in PR #32016:
URL: https://github.com/apache/airflow/pull/32016#discussion_r1235033593
##########
airflow/providers/snowflake/hooks/snowflake.py:
##########
@@ -216,7 +216,16 @@ def _get_conn_params(self) -> dict[str, str | None]:
"Please remove one."
)
elif private_key_file:
- private_key_pem = Path(private_key_file).read_bytes()
+ private_key_file_path = Path(private_key_file)
+ if not private_key_file_path.is_file() or private_key_file_path.stat().st_size == 0:
+ raise ValueError(
+ "The private_key_file path points to an empty or invalid file."
+ )
+ if private_key_file_path.stat().st_size > 4096:
+ raise ValueError(
+ "File size of private_key_file is too big."
Review Comment:
Let's also add a note in the docs about the size limitation.
##########
airflow/providers/snowflake/hooks/snowflake.py:
##########
@@ -216,7 +216,16 @@ def _get_conn_params(self) -> dict[str, str | None]:
"Please remove one."
)
elif private_key_file:
- private_key_pem = Path(private_key_file).read_bytes()
+ private_key_file_path = Path(private_key_file)
+ if not private_key_file_path.is_file() or private_key_file_path.stat().st_size == 0:
+ raise ValueError(
+ "The private_key_file path points to an empty or invalid file."
+ )
+ if private_key_file_path.stat().st_size > 4096:
+ raise ValueError(
+ "File size of private_key_file is too big."
Review Comment:
```suggestion
"The private_key_file size is too big. Please keep it less than 4 KB"
```
--
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 pull request #32016: Fix an issue that crashes Airflow Webserver when passed invalid private key path to Snowflake
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #32016:
URL: https://github.com/apache/airflow/pull/32016#issuecomment-1624418565
Needs conflict resolving @bjankie1
--
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] uranusjr commented on a diff in pull request #32016: Fix an issue that crashes Airflow Webserver when passed invalid private key path to Snowflake
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #32016:
URL: https://github.com/apache/airflow/pull/32016#discussion_r1234946470
##########
airflow/providers/snowflake/hooks/snowflake.py:
##########
@@ -216,7 +216,16 @@ def _get_conn_params(self) -> dict[str, str | None]:
"Please remove one."
)
elif private_key_file:
- private_key_pem = Path(private_key_file).read_bytes()
+ private_key_file_path = Path(private_key_file)
+ if not private_key_file_path.is_file() or private_key_file_path.stat().st_size == 0:
+ raise AirflowException(
+ "The private_key_file path points to an empty or invalid file."
+ )
Review Comment:
That’s the check below, not this one.
--
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] uranusjr commented on a diff in pull request #32016: Fix an issue that crashes Airflow Webserver when passed invalid private key path to Snowflake
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #32016:
URL: https://github.com/apache/airflow/pull/32016#discussion_r1235016504
##########
airflow/providers/snowflake/hooks/snowflake.py:
##########
@@ -216,7 +216,16 @@ def _get_conn_params(self) -> dict[str, str | None]:
"Please remove one."
)
elif private_key_file:
- private_key_pem = Path(private_key_file).read_bytes()
+ private_key_file_path = Path(private_key_file)
+ if not private_key_file_path.is_file() or private_key_file_path.stat().st_size == 0:
+ raise AirflowException(
+ "The private_key_file path points to an empty or invalid file."
+ )
Review Comment:
Still does not explain the extra stat check though? `is_file` only returns true for regular files.
Also this can benefit from a better exception class than AirflowException. A ValueError, OSError, or PermissionError would be more appropriate IMO.
--
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] bjankie1 commented on a diff in pull request #32016: Fix an issue that crashes Airflow Webserver when passed invalid private key path to Snowflake
Posted by "bjankie1 (via GitHub)" <gi...@apache.org>.
bjankie1 commented on code in PR #32016:
URL: https://github.com/apache/airflow/pull/32016#discussion_r1234998423
##########
airflow/providers/snowflake/hooks/snowflake.py:
##########
@@ -216,7 +216,16 @@ def _get_conn_params(self) -> dict[str, str | None]:
"Please remove one."
)
elif private_key_file:
- private_key_pem = Path(private_key_file).read_bytes()
+ private_key_file_path = Path(private_key_file)
+ if not private_key_file_path.is_file() or private_key_file_path.stat().st_size == 0:
+ raise AirflowException(
+ "The private_key_file path points to an empty or invalid file."
+ )
Review Comment:
If we don't check if `private_key_file` is a file, a system device can be passed. e.g. `/dev/random`. That also crashes Webserver.
--
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 pull request #32016: Fix an issue that crashes Airflow Webserver when passed invalid private key path to Snowflake
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #32016:
URL: https://github.com/apache/airflow/pull/32016#issuecomment-1605038514
conflicts :(
--
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] bjankie1 commented on a diff in pull request #32016: Fix an issue that crashes Airflow Webserver when passed invalid private key path to Snowflake
Posted by "bjankie1 (via GitHub)" <gi...@apache.org>.
bjankie1 commented on code in PR #32016:
URL: https://github.com/apache/airflow/pull/32016#discussion_r1235028413
##########
airflow/providers/snowflake/hooks/snowflake.py:
##########
@@ -216,7 +216,16 @@ def _get_conn_params(self) -> dict[str, str | None]:
"Please remove one."
)
elif private_key_file:
- private_key_pem = Path(private_key_file).read_bytes()
+ private_key_file_path = Path(private_key_file)
+ if not private_key_file_path.is_file() or private_key_file_path.stat().st_size == 0:
+ raise AirflowException(
+ "The private_key_file path points to an empty or invalid file."
+ )
Review Comment:
That is the intention - the private key path should point to a regular file. Does it make sense to read the key from other type of files?
I'm ok with changing to `ValueError` - pushed the change.
--
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] bjankie1 commented on a diff in pull request #32016: Fix an issue that crashes Airflow Webserver when passed invalid private key path to Snowflake
Posted by "bjankie1 (via GitHub)" <gi...@apache.org>.
bjankie1 commented on code in PR #32016:
URL: https://github.com/apache/airflow/pull/32016#discussion_r1237031607
##########
airflow/providers/snowflake/hooks/snowflake.py:
##########
@@ -216,7 +216,16 @@ def _get_conn_params(self) -> dict[str, str | None]:
"Please remove one."
)
elif private_key_file:
- private_key_pem = Path(private_key_file).read_bytes()
+ private_key_file_path = Path(private_key_file)
+ if not private_key_file_path.is_file() or private_key_file_path.stat().st_size == 0:
+ raise AirflowException(
+ "The private_key_file path points to an empty or invalid file."
+ )
Review Comment:
I checked also that for system `st_size` reports size as 0 therefore it's an alternative way of checking if this is a valid file. On top of that it doesn't make sense to pass an empty file in this case.
##########
airflow/providers/snowflake/hooks/snowflake.py:
##########
@@ -216,7 +216,16 @@ def _get_conn_params(self) -> dict[str, str | None]:
"Please remove one."
)
elif private_key_file:
- private_key_pem = Path(private_key_file).read_bytes()
+ private_key_file_path = Path(private_key_file)
+ if not private_key_file_path.is_file() or private_key_file_path.stat().st_size == 0:
+ raise AirflowException(
+ "The private_key_file path points to an empty or invalid file."
+ )
Review Comment:
I checked also that for system `st_size` reports size as 0 therefore it's an alternative way of checking if this is a valid file. On top of that it doesn't make sense to pass an empty file in this case.
--
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] xrmr commented on a diff in pull request #32016: Fix an issue that crashes Airflow Webserver when passed invalid private key path to Snowflake
Posted by "xrmr (via GitHub)" <gi...@apache.org>.
xrmr commented on code in PR #32016:
URL: https://github.com/apache/airflow/pull/32016#discussion_r1253438074
##########
tests/providers/snowflake/hooks/test_snowflake.py:
##########
@@ -342,6 +344,24 @@ def test_get_conn_params_should_support_private_auth_with_unencrypted_key(
), pytest.raises(TypeError, match="Password was given but private key is not encrypted."):
SnowflakeHook(snowflake_conn_id='test_conn')._get_conn_params()
+ def test_get_conn_params_should_fail_on_invalid_key(self):
+ connection_kwargs = {
+ **BASE_CONNECTION_KWARGS,
+ 'password': None,
+ 'extra': {
+ 'database': 'db',
+ 'account': 'airflow',
+ 'warehouse': 'af_wh',
+ 'region': 'af_region',
+ 'role': 'af_role',
+ 'private_key_file': "/dev/urandom",
+ },
+ }
+ with unittest.mock.patch.dict(
Review Comment:
The `unittest` here is the root cause of the failed checks; delete it to make the checks pass.
```suggestion
with mock.patch.dict(
```
--
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