Re: [PR] Add JWT/OIDC authentication support to Hashicorp Vault provider [airflow]
jason810496 merged PR #61439: URL: https://github.com/apache/airflow/pull/61439 -- 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]
Re: [PR] Add JWT/OIDC authentication support to Hashicorp Vault provider [airflow]
boring-cyborg[bot] commented on PR #61439: URL: https://github.com/apache/airflow/pull/61439#issuecomment-3871878985 Awesome work, congrats on your first merged pull request! You are invited to check our [Issue Tracker](https://github.com/apache/airflow/issues) for additional contributions. -- 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]
Re: [PR] Add JWT/OIDC authentication support to Hashicorp Vault provider [airflow]
piotrlinski commented on code in PR #61439:
URL: https://github.com/apache/airflow/pull/61439#discussion_r2781112347
##
providers/hashicorp/src/airflow/providers/hashicorp/hooks/vault.py:
##
@@ -376,6 +402,8 @@ def get_connection_form_widgets(cls) -> dict[str, Any]:
"kubernetes_jwt_path": StringField(
lazy_gettext("Kubernetes jwt path"),
widget=BS3TextFieldWidget()
),
+"jwt_role": StringField(lazy_gettext("JWT role"),
widget=BS3TextFieldWidget()),
Review Comment:
added
--
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]
Re: [PR] Add JWT/OIDC authentication support to Hashicorp Vault provider [airflow]
piotrlinski commented on code in PR #61439: URL: https://github.com/apache/airflow/pull/61439#discussion_r2780985809 ## providers/hashicorp/src/airflow/providers/hashicorp/_internal_client/vault_client.py: ## @@ -82,6 +84,7 @@ class _VaultClient(LoggingMixin): :param kubernetes_role: Role for Authentication (for ``kubernetes`` auth_type). :param kubernetes_jwt_path: Path for kubernetes jwt token (for ``kubernetes`` auth_type, default: ``/var/run/secrets/kubernetes.io/serviceaccount/token``). +default: ``/var/run/secrets/kubernetes.io/serviceaccount/token``). Review Comment: the default jwt token file path, is removed, the JWT is more generic auth method, there is no standard for the default locations for tokens. Previously I set it similar as for jwt token path like in kubernetes pods, this is not the case here. The logic now goes, you need to set or token ot token path, there are not defaults -- 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]
Re: [PR] Add JWT/OIDC authentication support to Hashicorp Vault provider [airflow]
jason810496 commented on code in PR #61439:
URL: https://github.com/apache/airflow/pull/61439#discussion_r2776807723
##
providers/hashicorp/src/airflow/providers/hashicorp/hooks/vault.py:
##
@@ -376,6 +402,8 @@ def get_connection_form_widgets(cls) -> dict[str, Any]:
"kubernetes_jwt_path": StringField(
lazy_gettext("Kubernetes jwt path"),
widget=BS3TextFieldWidget()
),
+"jwt_role": StringField(lazy_gettext("JWT role"),
widget=BS3TextFieldWidget()),
Review Comment:
`jwt_token` is missing I think.
```suggestion
"jwt_role": StringField(lazy_gettext("JWT role"),
widget=BS3TextFieldWidget()),
"jwt_token": StringField(lazy_gettext("JWT token"),
widget=BS3TextFieldWidget()),
```
##
providers/hashicorp/src/airflow/providers/hashicorp/_internal_client/vault_client.py:
##
@@ -82,6 +84,7 @@ class _VaultClient(LoggingMixin):
:param kubernetes_role: Role for Authentication (for ``kubernetes``
auth_type).
:param kubernetes_jwt_path: Path for kubernetes jwt token (for
``kubernetes`` auth_type, default:
``/var/run/secrets/kubernetes.io/serviceaccount/token``).
+default: ``/var/run/secrets/kubernetes.io/serviceaccount/token``).
Review Comment:
It seems this line should move to line 101 as we had already doc the default
path for `kubernetes_jwt_path` in line 86, but we're still missing the default
value doc for `jwt_token_path` in line 101.
--
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]
Re: [PR] Add JWT/OIDC authentication support to Hashicorp Vault provider [airflow]
Lee-W commented on PR #61439: URL: https://github.com/apache/airflow/pull/61439#issuecomment-3863385145 @jason810496 would appreciate a second pair of eyes. Thanks! -- 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]
Re: [PR] Add JWT/OIDC authentication support to Hashicorp Vault provider [airflow]
Lee-W commented on PR #61439: URL: https://github.com/apache/airflow/pull/61439#issuecomment-3858846440 > there is still one thing I am not sure, I made the default path for the jwt_token_path like for kubernetes (usually it goes with kubernets), however the JWT method does not have to be anyhow related to k8s, therefor not sure if we should keep it as default or remove - you need to pass the jwt_token (string) or jwt_token_path(path to a file with a token) Pass it might be better as it doesn't always goes with k8s -- 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]
Re: [PR] Add JWT/OIDC authentication support to Hashicorp Vault provider [airflow]
Lee-W commented on code in PR #61439:
URL: https://github.com/apache/airflow/pull/61439#discussion_r2772954535
##
providers/hashicorp/tests/unit/hashicorp/_internal_client/test_vault_client.py:
##
@@ -587,6 +587,120 @@ def test_kubernetes_kubernetes_jwt_path_none(self,
mock_hvac):
url="http://localhost:8180";,
)
+
@mock.patch("airflow.providers.hashicorp._internal_client.vault_client.hvac")
+def test_jwt_with_token(self, mock_hvac):
+mock_client = mock.MagicMock()
+mock_hvac.Client.return_value = mock_client
+vault_client = _VaultClient(
+auth_type="jwt",
+jwt_role="my-role",
+jwt_token="eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9.test",
+url="http://localhost:8180";,
+session=None,
+)
+client = vault_client.client
+mock_hvac.Client.assert_called_with(url="http://localhost:8180";,
session=None)
+client.auth.jwt.jwt_login.assert_called_with(
+role="my-role", jwt="eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9.test"
+)
+client.is_authenticated.assert_called_with()
Review Comment:
```suggestion
assert mock_hvac.Client.mock_calls == [
call(
url="http://localhost:8180";,
session=None
)
]
assert client.auth.jwt.jwt_login.mock_calls == [
call(
role="my-role",
jwt="eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9.test",
)
]
client.is_authenticated.mock_calls == [
call()
]
```
##
providers/hashicorp/tests/unit/hashicorp/hooks/test_vault.py:
##
@@ -733,6 +733,95 @@ def test_kubernetes_dejson(self, mock_kubernetes,
mock_hvac, mock_get_connection
test_client.is_authenticated.assert_called_with()
assert test_hook.vault_client.kv_engine_version == 2
+
@mock.patch("airflow.providers.hashicorp.hooks.vault.VaultHook.get_connection")
+
@mock.patch("airflow.providers.hashicorp._internal_client.vault_client.hvac")
+def test_jwt_init_params(self, mock_hvac, mock_get_connection):
+mock_client = mock.MagicMock()
+mock_hvac.Client.return_value = mock_client
+mock_connection = self.get_mock_connection()
+mock_get_connection.return_value = mock_connection
+
+connection_dict = {}
+
+mock_connection.extra_dejson.get.side_effect = connection_dict.get
+kwargs = {
+"auth_type": "jwt",
+"jwt_role": "my-role",
+"jwt_token": "eyJhbGciOiJSUzI1NiJ9.test",
+"vault_conn_id": "vault_conn_id",
+"session": None,
+}
+
+test_hook = VaultHook(**kwargs)
Review Comment:
Since this `kwargs` is not used elsewhere, we can just pass them into
`VaultHook`
##
providers/hashicorp/tests/unit/hashicorp/hooks/test_vault.py:
##
@@ -733,6 +733,95 @@ def test_kubernetes_dejson(self, mock_kubernetes,
mock_hvac, mock_get_connection
test_client.is_authenticated.assert_called_with()
assert test_hook.vault_client.kv_engine_version == 2
+
@mock.patch("airflow.providers.hashicorp.hooks.vault.VaultHook.get_connection")
+
@mock.patch("airflow.providers.hashicorp._internal_client.vault_client.hvac")
+def test_jwt_init_params(self, mock_hvac, mock_get_connection):
+mock_client = mock.MagicMock()
+mock_hvac.Client.return_value = mock_client
+mock_connection = self.get_mock_connection()
+mock_get_connection.return_value = mock_connection
+
+connection_dict = {}
+
+mock_connection.extra_dejson.get.side_effect = connection_dict.get
+kwargs = {
+"auth_type": "jwt",
+"jwt_role": "my-role",
+"jwt_token": "eyJhbGciOiJSUzI1NiJ9.test",
+"vault_conn_id": "vault_conn_id",
+"session": None,
+}
+
+test_hook = VaultHook(**kwargs)
Review Comment:
same elsewhere
##
providers/hashicorp/src/airflow/providers/hashicorp/secrets/vault.py:
##
@@ -120,6 +123,9 @@ def __init__(
radius_host: str | None = None,
radius_secret: str | None = None,
radius_port: int | None = None,
+jwt_role: str | None = None,
Review Comment:
```suggestion
*,
jwt_role: str | None = None,
```
##
providers/hashicorp/src/airflow/providers/hashicorp/hooks/vault.py:
##
@@ -124,6 +128,9 @@ def __init__(
azure_resource: str | None = None,
radius_host: str | None = None,
radius_port: int | None = None,
+jwt_role: str | None = None,
Review Comment:
```suggestion
*,
jwt_role: str | None = None,
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on
Re: [PR] Add JWT/OIDC authentication support to Hashicorp Vault provider [airflow]
piotrlinski commented on PR #61439: URL: https://github.com/apache/airflow/pull/61439#issuecomment-3854031253 there is still one thing I am not sure, I made the default path for the jwt_token_path like for kubernetes (usually it goes with kubernets), however the JWT method does not have to be anyhow related to k8s, therefor not sure if we should keep it as default or remove - you need to pass the jwt_token (string) or jwt_token_path(path to a file with a token) -- 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]
Re: [PR] Add JWT/OIDC authentication support to Hashicorp Vault provider [airflow]
piotrlinski commented on code in PR #61439: URL: https://github.com/apache/airflow/pull/61439#discussion_r2769433495 ## providers/hashicorp/src/airflow/providers/hashicorp/hooks/vault.py: ## @@ -89,6 +90,10 @@ class VaultHook(BaseHook): :param kubernetes_role: Role for Authentication (for ``kubernetes`` auth_type) :param kubernetes_jwt_path: Path for kubernetes jwt token (for ``kubernetes`` auth_type, default: ``/var/run/secrets/kubernetes.io/serviceaccount/token``) +:param jwt_role: Role for Authentication (for ``jwt`` auth_type) Review Comment: order fiex -- 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]
Re: [PR] Add JWT/OIDC authentication support to Hashicorp Vault provider [airflow]
Lee-W commented on code in PR #61439: URL: https://github.com/apache/airflow/pull/61439#discussion_r2769368316 ## providers/hashicorp/src/airflow/providers/hashicorp/hooks/vault.py: ## @@ -89,6 +90,10 @@ class VaultHook(BaseHook): :param kubernetes_role: Role for Authentication (for ``kubernetes`` auth_type) :param kubernetes_jwt_path: Path for kubernetes jwt token (for ``kubernetes`` auth_type, default: ``/var/run/secrets/kubernetes.io/serviceaccount/token``) +:param jwt_role: Role for Authentication (for ``jwt`` auth_type) Review Comment: the doc string also should reflect the arg order ## providers/hashicorp/src/airflow/providers/hashicorp/_internal_client/vault_client.py: ## @@ -121,6 +127,9 @@ def __init__( radius_host: str | None = None, radius_secret: str | None = None, radius_port: int | None = None, +jwt_role: str | None = None, Review Comment: ```suggestion *, jwt_role: str | None = None, ``` -- 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]
Re: [PR] Add JWT/OIDC authentication support to Hashicorp Vault provider [airflow]
piotrlinski commented on code in PR #61439:
URL: https://github.com/apache/airflow/pull/61439#discussion_r2768581588
##
providers/hashicorp/src/airflow/providers/hashicorp/hooks/vault.py:
##
@@ -240,6 +256,19 @@ def _get_kubernetes_parameters_from_connection(
kubernetes_role =
self.connection.extra_dejson.get("kubernetes_role")
return kubernetes_jwt_path, kubernetes_role
+def _get_jwt_parameters_from_connection(
+self, jwt_role: str | None, jwt_token: str | None, jwt_token_path: str
| None
+) -> tuple[str | None, str | None, str | None]:
+if not jwt_role:
+jwt_role = self.connection.extra_dejson.get("jwt_role")
+if not jwt_token:
+jwt_token = self.connection.extra_dejson.get("jwt_token")
+if not jwt_token_path:
+jwt_token_path = self.connection.extra_dejson.get("jwt_token_path")
+if not jwt_token_path:
+jwt_token_path = DEFAULT_JWT_TOKEN_PATH
Review Comment:
done
--
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]
Re: [PR] Add JWT/OIDC authentication support to Hashicorp Vault provider [airflow]
piotrlinski commented on code in PR #61439: URL: https://github.com/apache/airflow/pull/61439#discussion_r2768579708 ## providers/hashicorp/src/airflow/providers/hashicorp/hooks/vault.py: ## @@ -117,6 +122,9 @@ def __init__( region: str | None = None, kubernetes_role: str | None = None, kubernetes_jwt_path: str | None = None, +jwt_role: str | None = None, Review Comment: done ## providers/hashicorp/src/airflow/providers/hashicorp/secrets/vault.py: ## @@ -112,6 +115,9 @@ def __init__( region: str | None = None, kubernetes_role: str | None = None, kubernetes_jwt_path: str = "/var/run/secrets/kubernetes.io/serviceaccount/token", Review Comment: done -- 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]
Re: [PR] Add JWT/OIDC authentication support to Hashicorp Vault provider [airflow]
Lee-W commented on code in PR #61439:
URL: https://github.com/apache/airflow/pull/61439#discussion_r2768013383
##
providers/hashicorp/src/airflow/providers/hashicorp/secrets/vault.py:
##
@@ -112,6 +115,9 @@ def __init__(
region: str | None = None,
kubernetes_role: str | None = None,
kubernetes_jwt_path: str =
"/var/run/secrets/kubernetes.io/serviceaccount/token",
Review Comment:
same here
##
providers/hashicorp/src/airflow/providers/hashicorp/hooks/vault.py:
##
@@ -117,6 +122,9 @@ def __init__(
region: str | None = None,
kubernetes_role: str | None = None,
kubernetes_jwt_path: str | None = None,
+jwt_role: str | None = None,
Review Comment:
same here
##
providers/hashicorp/src/airflow/providers/hashicorp/_internal_client/vault_client.py:
##
@@ -113,6 +119,9 @@ def __init__(
region: str | None = None,
kubernetes_role: str | None = None,
kubernetes_jwt_path: str | None =
"/var/run/secrets/kubernetes.io/serviceaccount/token",
+jwt_role: str | None = None,
+jwt_token: str | None = None,
+jwt_token_path: str | None = DEFAULT_JWT_TOKEN_PATH,
Review Comment:
```suggestion
*,
jwt_role: str | None = None,
jwt_token: str | None = None,
jwt_token_path: str | None = DEFAULT_JWT_TOKEN_PATH,
```
Let's place it after `radius_port` and make it a keyword-only argument.
otherwise, it would be a breaking change
##
providers/hashicorp/src/airflow/providers/hashicorp/hooks/vault.py:
##
@@ -240,6 +256,19 @@ def _get_kubernetes_parameters_from_connection(
kubernetes_role =
self.connection.extra_dejson.get("kubernetes_role")
return kubernetes_jwt_path, kubernetes_role
+def _get_jwt_parameters_from_connection(
+self, jwt_role: str | None, jwt_token: str | None, jwt_token_path: str
| None
+) -> tuple[str | None, str | None, str | None]:
+if not jwt_role:
+jwt_role = self.connection.extra_dejson.get("jwt_role")
+if not jwt_token:
+jwt_token = self.connection.extra_dejson.get("jwt_token")
+if not jwt_token_path:
+jwt_token_path = self.connection.extra_dejson.get("jwt_token_path")
+if not jwt_token_path:
+jwt_token_path = DEFAULT_JWT_TOKEN_PATH
Review Comment:
```suggestion
jwt_token_path =
self.connection.extra_dejson.get("jwt_token_path") or DEFAULT_JWT_TOKEN_PATH
```
--
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]
Re: [PR] Add JWT/OIDC authentication support to Hashicorp Vault provider [airflow]
boring-cyborg[bot] commented on PR #61439: URL: https://github.com/apache/airflow/pull/61439#issuecomment-3846224105 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 (https://github.com/apache/airflow/blob/main/contributing-docs/README.rst) Here are some useful points: - Pay attention to the quality of your code (ruff, mypy and type annotations). Our [prek-hooks]( https://github.com/apache/airflow/blob/main/contributing-docs/08_static_code_checks.rst#prerequisites-for-prek-hooks) will help you with that. - In case of a new feature add useful documentation (in docstrings or in `docs/` directory). Adding a new operator? Check this short [guide](https://github.com/apache/airflow/blob/main/airflow-core/docs/howto/custom-operator.rst) Consider adding an example DAG that shows how users should use it. - Consider using [Breeze environment](https://github.com/apache/airflow/blob/main/dev/breeze/doc/README.rst) for testing locally, it's a heavy docker but it ships with a working Airflow and a lot of integrations. - Be patient and persistent. It might take some time to get a review or get the final approval from Committers. - Please follow [ASF Code of Conduct](https://www.apache.org/foundation/policies/conduct) for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack. - Be sure to read the [Airflow Coding style]( https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#coding-style-and-best-practices). - Always keep your Pull Requests rebased, otherwise your build might fail due to changes not related to your commits. Apache Airflow is a community-driven project and together we are making it better 🚀. In case of doubts contact the developers at: Mailing List: [email protected] Slack: https://s.apache.org/airflow-slack -- 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]
