You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2020/08/17 06:04:50 UTC
[GitHub] [airflow] houqp opened a new pull request #10356: add basic auth API auth backend
houqp opened a new pull request #10356:
URL: https://github.com/apache/airflow/pull/10356
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
Add auth backend for username password based auth.
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+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 [UPDATING.md](https://github.com/apache/airflow/blob/master/UPDATING.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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #10356: add basic auth API auth backend
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #10356:
URL: https://github.com/apache/airflow/pull/10356#discussion_r472526744
##########
File path: docs/security/api.rst
##########
@@ -116,3 +116,41 @@ look like the following.
-H 'Content-Type: application/json' \
-H 'Cache-Control: no-cache' \
-H "Authorization: Bearer ${ID_TOKEN}"
+
+Basic authentication
+''''''''''''''''''''
+
+`Basic username password authentication <https://tools.ietf.org/html/rfc7617
+https://en.wikipedia.org/wiki/Basic_access_authentication>`_ is currently
+supported for the API. This works for users created through LDAP login or
+within Airflow DB using password.
Review comment:
```suggestion
within Airflow Metadata DB using password.
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #10356: add basic auth API auth backend
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #10356:
URL: https://github.com/apache/airflow/pull/10356#discussion_r471876678
##########
File path: tests/api/auth/backend/test_basic_auth.py
##########
@@ -0,0 +1,106 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import unittest
+from base64 import b64encode
+
+from flask_login import current_user
+from parameterized import parameterized
+
+from airflow.www.app import create_app
+from tests.test_utils.config import conf_vars
+from tests.test_utils.db import clear_db_pools
+
+
+class TestBasicAuth(unittest.TestCase):
+ def setUp(self) -> None:
+ with conf_vars(
+ {("api", "auth_backend"): "airflow.api.auth.backend.basic_auth"}
+ ):
+ self.app = create_app(testing=True)
+
+ self.appbuilder = self.app.appbuilder # pylint: disable=no-member
+ role_admin = self.appbuilder.sm.find_role("Admin")
+ tester = self.appbuilder.sm.find_user(username="test")
+ if not tester:
+ self.appbuilder.sm.add_user(
+ username="test",
+ first_name="test",
+ last_name="test",
+ email="test@fab.org",
+ role=role_admin,
+ password="test",
+ )
+
+ def test_success(self):
+ token = "Basic " + b64encode(b"test:test").decode()
+ clear_db_pools()
+
+ with self.app.test_client() as test_client:
+ response = test_client.get(
+ "/api/v1/pools", headers={"Authorization": token}
+ )
+ assert current_user.email == "test@fab.org"
+
+ assert response.status_code == 200
+ assert response.json == {
+ "pools": [
+ {
+ "name": "default_pool",
+ "slots": 128,
+ "occupied_slots": 0,
+ "running_slots": 0,
+ "queued_slots": 0,
+ "open_slots": 128,
+ },
+ ],
+ "total_entries": 1,
+ }
+
+ @parameterized.expand([
+ ("basic",),
+ ("basic ",),
+ ("bearer",),
+ ("test:test",),
+ (b64encode(b"test:test").decode(),),
+ ("bearer ",),
+ ("basic: ",),
+ ("basic 123",),
+ ])
+ def test_malformed_headers(self, token):
+ with self.app.test_client() as test_client:
+ response = test_client.get(
+ "/api/v1/pools", headers={"Authorization": token}
+ )
+ assert response.status_code == 401
+ assert response.headers["WWW-Authenticate"] == "Basic"
+ assert response.data == b"Unauthorized"
+
+ @parameterized.expand([
+ ("basic " + b64encode(b"test").decode(),),
+ ("basic " + b64encode(b"test:").decode(),),
+ ("basic " + b64encode(b"test:123").decode(),),
+ ("basic " + b64encode(b"test test").decode(),),
+ ])
+ def test_invalid_auth_header(self, token):
+ with self.app.test_client() as test_client:
+ response = test_client.get(
+ "/api/v1/pools", headers={"Authorization": token}
+ )
+ assert response.status_code == 401
+ assert response.headers["WWW-Authenticate"] == "Basic"
+ assert response.data == b"Unauthorized"
Review comment:
For stable API, It should be in line with RFC-7807.
https://airflow.readthedocs.io/en/latest/stable-rest-api/redoc.html
For experimental, we don't have standardized response. I suspect that a correct answer is an object with the ok field set to False and then an error containing the error message.
```json
{
"ok": false,
"errors": "Unauthorized"
}
```
However, other backends don't implement it this way, so we can ignore it here too and focus only on the stable API.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] houqp commented on pull request #10356: add basic auth API auth backend
Posted by GitBox <gi...@apache.org>.
houqp commented on pull request #10356:
URL: https://github.com/apache/airflow/pull/10356#issuecomment-675210340
@jhtimmins would be nice to get a review from you as well :)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #10356: add basic auth API auth backend
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #10356:
URL: https://github.com/apache/airflow/pull/10356#discussion_r471876678
##########
File path: tests/api/auth/backend/test_basic_auth.py
##########
@@ -0,0 +1,106 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import unittest
+from base64 import b64encode
+
+from flask_login import current_user
+from parameterized import parameterized
+
+from airflow.www.app import create_app
+from tests.test_utils.config import conf_vars
+from tests.test_utils.db import clear_db_pools
+
+
+class TestBasicAuth(unittest.TestCase):
+ def setUp(self) -> None:
+ with conf_vars(
+ {("api", "auth_backend"): "airflow.api.auth.backend.basic_auth"}
+ ):
+ self.app = create_app(testing=True)
+
+ self.appbuilder = self.app.appbuilder # pylint: disable=no-member
+ role_admin = self.appbuilder.sm.find_role("Admin")
+ tester = self.appbuilder.sm.find_user(username="test")
+ if not tester:
+ self.appbuilder.sm.add_user(
+ username="test",
+ first_name="test",
+ last_name="test",
+ email="test@fab.org",
+ role=role_admin,
+ password="test",
+ )
+
+ def test_success(self):
+ token = "Basic " + b64encode(b"test:test").decode()
+ clear_db_pools()
+
+ with self.app.test_client() as test_client:
+ response = test_client.get(
+ "/api/v1/pools", headers={"Authorization": token}
+ )
+ assert current_user.email == "test@fab.org"
+
+ assert response.status_code == 200
+ assert response.json == {
+ "pools": [
+ {
+ "name": "default_pool",
+ "slots": 128,
+ "occupied_slots": 0,
+ "running_slots": 0,
+ "queued_slots": 0,
+ "open_slots": 128,
+ },
+ ],
+ "total_entries": 1,
+ }
+
+ @parameterized.expand([
+ ("basic",),
+ ("basic ",),
+ ("bearer",),
+ ("test:test",),
+ (b64encode(b"test:test").decode(),),
+ ("bearer ",),
+ ("basic: ",),
+ ("basic 123",),
+ ])
+ def test_malformed_headers(self, token):
+ with self.app.test_client() as test_client:
+ response = test_client.get(
+ "/api/v1/pools", headers={"Authorization": token}
+ )
+ assert response.status_code == 401
+ assert response.headers["WWW-Authenticate"] == "Basic"
+ assert response.data == b"Unauthorized"
+
+ @parameterized.expand([
+ ("basic " + b64encode(b"test").decode(),),
+ ("basic " + b64encode(b"test:").decode(),),
+ ("basic " + b64encode(b"test:123").decode(),),
+ ("basic " + b64encode(b"test test").decode(),),
+ ])
+ def test_invalid_auth_header(self, token):
+ with self.app.test_client() as test_client:
+ response = test_client.get(
+ "/api/v1/pools", headers={"Authorization": token}
+ )
+ assert response.status_code == 401
+ assert response.headers["WWW-Authenticate"] == "Basic"
+ assert response.data == b"Unauthorized"
Review comment:
For stable API, It should be in line with RFC-7807.
https://airflow.readthedocs.io/en/latest/stable-rest-api/redoc.html
For experimental API, we don't have standardized response. I suspect that a correct answer is an object with the ``ok`` field set to False and then an ``error`` field containing the error message.
```json
{
"ok": false,
"errors": "Unauthorized"
}
```
However, other backends don't implement it this way, so we can ignore it here too and focus only on the stable API.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil merged pull request #10356: Add basic auth API auth backend
Posted by GitBox <gi...@apache.org>.
kaxil merged pull request #10356:
URL: https://github.com/apache/airflow/pull/10356
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on pull request #10356: add basic auth API auth backend
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #10356:
URL: https://github.com/apache/airflow/pull/10356#issuecomment-675147837
cc @jhtimmins
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] houqp commented on a change in pull request #10356: add basic auth API auth backend
Posted by GitBox <gi...@apache.org>.
houqp commented on a change in pull request #10356:
URL: https://github.com/apache/airflow/pull/10356#discussion_r472523072
##########
File path: tests/api/auth/backend/test_basic_auth.py
##########
@@ -0,0 +1,135 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import unittest
+from base64 import b64encode
+
+from flask_login import current_user
+from parameterized import parameterized
+
+from airflow.www.app import create_app
+from tests.test_utils.config import conf_vars
+from tests.test_utils.db import clear_db_pools
+
+
+class TestBasicAuth(unittest.TestCase):
+ def setUp(self) -> None:
+ with conf_vars(
+ {("api", "auth_backend"): "airflow.api.auth.backend.basic_auth"}
+ ):
+ self.app = create_app(testing=True)
+
+ self.appbuilder = self.app.appbuilder # pylint: disable=no-member
+ role_admin = self.appbuilder.sm.find_role("Admin")
+ tester = self.appbuilder.sm.find_user(username="test")
+ if not tester:
+ self.appbuilder.sm.add_user(
+ username="test",
+ first_name="test",
+ last_name="test",
+ email="test@fab.org",
+ role=role_admin,
+ password="test",
+ )
+
+ def test_success(self):
+ token = "Basic " + b64encode(b"test:test").decode()
+ clear_db_pools()
+
+ with self.app.test_client() as test_client:
+ response = test_client.get(
+ "/api/v1/pools", headers={"Authorization": token}
+ )
+ assert current_user.email == "test@fab.org"
+
+ assert response.status_code == 200
+ assert response.json == {
+ "pools": [
+ {
+ "name": "default_pool",
+ "slots": 128,
+ "occupied_slots": 0,
+ "running_slots": 0,
+ "queued_slots": 0,
+ "open_slots": 128,
+ },
+ ],
+ "total_entries": 1,
+ }
+
+ @parameterized.expand([
+ ("basic",),
+ ("basic ",),
+ ("bearer",),
+ ("test:test",),
+ (b64encode(b"test:test").decode(),),
+ ("bearer ",),
+ ("basic: ",),
+ ("basic 123",),
+ ])
+ def test_malformed_headers(self, token):
+ with self.app.test_client() as test_client:
+ response = test_client.get(
+ "/api/v1/pools", headers={"Authorization": token}
+ )
+ assert response.status_code == 401
+ assert response.headers["Content-Type"] == "application/problem+json"
+ assert response.headers["WWW-Authenticate"] == "Basic"
+ assert response.json == {
+ 'detail': None,
+ 'status': 401,
+ 'title': 'Unauthorized',
+ 'type': 'about:blank',
+ }
+
+ @parameterized.expand([
+ ("basic " + b64encode(b"test").decode(),),
+ ("basic " + b64encode(b"test:").decode(),),
+ ("basic " + b64encode(b"test:123").decode(),),
+ ("basic " + b64encode(b"test test").decode(),),
+ ])
+ def test_invalid_auth_header(self, token):
+ with self.app.test_client() as test_client:
+ response = test_client.get(
+ "/api/v1/pools", headers={"Authorization": token}
+ )
+ assert response.status_code == 401
+ assert response.headers["Content-Type"] == "application/problem+json"
+ assert response.headers["WWW-Authenticate"] == "Basic"
+ assert response.json == {
+ 'detail': None,
+ 'status': 401,
+ 'title': 'Unauthorized',
+ 'type': 'about:blank',
+ }
+
+ def test_experimental_api(self):
Review comment:
@mik-laj packed experimental api related checks into this single test so it's easy to delete in the future.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #10356: add basic auth API auth backend
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #10356:
URL: https://github.com/apache/airflow/pull/10356#discussion_r471403019
##########
File path: airflow/api/auth/backend/basic_auth.py
##########
@@ -0,0 +1,74 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Basic authentication backend"""
+import binascii
+from base64 import b64decode
+from functools import wraps
+from typing import Callable, Optional, Tuple, TypeVar, Union, cast
+
+from flask import Response, _request_ctx_stack, current_app, request # type: ignore
+from flask_appbuilder.security.sqla.models import User
+from requests.auth import AuthBase
+
+CLIENT_AUTH: Optional[Union[Tuple[str, str], AuthBase]] = None
+
+
+def init_app(_):
+ """Initializes authentication backend"""
+
+
+T = TypeVar("T", bound=Callable) # pylint: disable=invalid-name
+
+
+def auth_current_user() -> Optional[User]:
+ """Authenticate and set current user if Authorization header exists"""
+ auth_header = request.headers.get("Authorization")
+ if not auth_header:
+ return None
+
+ auth_parts = auth_header.split(" ", 2)
+ if len(auth_parts) != 2:
+ return None
+ if auth_parts[0].lower() != "basic":
+ return None
+
+ try:
+ cred_parts = b64decode(auth_parts[1]).decode().split(":", 2)
+ except binascii.Error: # incorrect base64 encoding
+ return None
+
+ if len(cred_parts) != 2:
+ return None
+
+ (username_or_email, password) = cred_parts
+ user = current_app.appbuilder.sm.auth_user_db(username_or_email, password)
+ if user is not None:
+ ctx = _request_ctx_stack.top
+ ctx.user = user
+ return user
+
+
+def requires_authentication(function: T):
+ """Decorator for functions that require authentication"""
+ @wraps(function)
+ def decorated(*args, **kwargs):
+ if auth_current_user() is not None:
+ return function(*args, **kwargs)
+ else:
+ return Response("Unauthorized", 401)
Review comment:
Can you aadd WWW-Authenticate header? This will allow us to see this API through the browser.
https://developer.mozilla.org/en-US/docs/Web/HTTP/Authentication#WWW-Authenticate_and_Proxy-Authenticate_headers
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #10356: add basic auth API auth backend
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #10356:
URL: https://github.com/apache/airflow/pull/10356#discussion_r471876678
##########
File path: tests/api/auth/backend/test_basic_auth.py
##########
@@ -0,0 +1,106 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import unittest
+from base64 import b64encode
+
+from flask_login import current_user
+from parameterized import parameterized
+
+from airflow.www.app import create_app
+from tests.test_utils.config import conf_vars
+from tests.test_utils.db import clear_db_pools
+
+
+class TestBasicAuth(unittest.TestCase):
+ def setUp(self) -> None:
+ with conf_vars(
+ {("api", "auth_backend"): "airflow.api.auth.backend.basic_auth"}
+ ):
+ self.app = create_app(testing=True)
+
+ self.appbuilder = self.app.appbuilder # pylint: disable=no-member
+ role_admin = self.appbuilder.sm.find_role("Admin")
+ tester = self.appbuilder.sm.find_user(username="test")
+ if not tester:
+ self.appbuilder.sm.add_user(
+ username="test",
+ first_name="test",
+ last_name="test",
+ email="test@fab.org",
+ role=role_admin,
+ password="test",
+ )
+
+ def test_success(self):
+ token = "Basic " + b64encode(b"test:test").decode()
+ clear_db_pools()
+
+ with self.app.test_client() as test_client:
+ response = test_client.get(
+ "/api/v1/pools", headers={"Authorization": token}
+ )
+ assert current_user.email == "test@fab.org"
+
+ assert response.status_code == 200
+ assert response.json == {
+ "pools": [
+ {
+ "name": "default_pool",
+ "slots": 128,
+ "occupied_slots": 0,
+ "running_slots": 0,
+ "queued_slots": 0,
+ "open_slots": 128,
+ },
+ ],
+ "total_entries": 1,
+ }
+
+ @parameterized.expand([
+ ("basic",),
+ ("basic ",),
+ ("bearer",),
+ ("test:test",),
+ (b64encode(b"test:test").decode(),),
+ ("bearer ",),
+ ("basic: ",),
+ ("basic 123",),
+ ])
+ def test_malformed_headers(self, token):
+ with self.app.test_client() as test_client:
+ response = test_client.get(
+ "/api/v1/pools", headers={"Authorization": token}
+ )
+ assert response.status_code == 401
+ assert response.headers["WWW-Authenticate"] == "Basic"
+ assert response.data == b"Unauthorized"
+
+ @parameterized.expand([
+ ("basic " + b64encode(b"test").decode(),),
+ ("basic " + b64encode(b"test:").decode(),),
+ ("basic " + b64encode(b"test:123").decode(),),
+ ("basic " + b64encode(b"test test").decode(),),
+ ])
+ def test_invalid_auth_header(self, token):
+ with self.app.test_client() as test_client:
+ response = test_client.get(
+ "/api/v1/pools", headers={"Authorization": token}
+ )
+ assert response.status_code == 401
+ assert response.headers["WWW-Authenticate"] == "Basic"
+ assert response.data == b"Unauthorized"
Review comment:
For stable API, It should be in line with RFC-7807.
https://airflow.readthedocs.io/en/latest/stable-rest-api/redoc.html
For experimental, we don't have standardized response. I suspect that a correct answer is an object with the ok field set to False and then an error containing the error message.
```json
{
"ok": False,
"errors": "Unauthorized"
}
```
However, other backends don't implement it this way, so we can ignore it here too and focus only on the stable API.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #10356: add basic auth API auth backend
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #10356:
URL: https://github.com/apache/airflow/pull/10356#discussion_r471878722
##########
File path: tests/api/auth/backend/test_basic_auth.py
##########
@@ -0,0 +1,106 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import unittest
+from base64 import b64encode
+
+from flask_login import current_user
+from parameterized import parameterized
+
+from airflow.www.app import create_app
+from tests.test_utils.config import conf_vars
+from tests.test_utils.db import clear_db_pools
+
+
+class TestBasicAuth(unittest.TestCase):
+ def setUp(self) -> None:
+ with conf_vars(
+ {("api", "auth_backend"): "airflow.api.auth.backend.basic_auth"}
+ ):
+ self.app = create_app(testing=True)
+
+ self.appbuilder = self.app.appbuilder # pylint: disable=no-member
+ role_admin = self.appbuilder.sm.find_role("Admin")
+ tester = self.appbuilder.sm.find_user(username="test")
+ if not tester:
+ self.appbuilder.sm.add_user(
+ username="test",
+ first_name="test",
+ last_name="test",
+ email="test@fab.org",
+ role=role_admin,
+ password="test",
+ )
+
+ def test_success(self):
+ token = "Basic " + b64encode(b"test:test").decode()
+ clear_db_pools()
+
+ with self.app.test_client() as test_client:
+ response = test_client.get(
+ "/api/v1/pools", headers={"Authorization": token}
+ )
+ assert current_user.email == "test@fab.org"
+
+ assert response.status_code == 200
+ assert response.json == {
+ "pools": [
+ {
+ "name": "default_pool",
+ "slots": 128,
+ "occupied_slots": 0,
+ "running_slots": 0,
+ "queued_slots": 0,
+ "open_slots": 128,
+ },
+ ],
+ "total_entries": 1,
+ }
+
+ @parameterized.expand([
+ ("basic",),
+ ("basic ",),
+ ("bearer",),
+ ("test:test",),
+ (b64encode(b"test:test").decode(),),
+ ("bearer ",),
+ ("basic: ",),
+ ("basic 123",),
+ ])
+ def test_malformed_headers(self, token):
+ with self.app.test_client() as test_client:
+ response = test_client.get(
+ "/api/v1/pools", headers={"Authorization": token}
+ )
+ assert response.status_code == 401
+ assert response.headers["WWW-Authenticate"] == "Basic"
+ assert response.data == b"Unauthorized"
+
+ @parameterized.expand([
+ ("basic " + b64encode(b"test").decode(),),
+ ("basic " + b64encode(b"test:").decode(),),
+ ("basic " + b64encode(b"test:123").decode(),),
+ ("basic " + b64encode(b"test test").decode(),),
+ ])
+ def test_invalid_auth_header(self, token):
+ with self.app.test_client() as test_client:
+ response = test_client.get(
+ "/api/v1/pools", headers={"Authorization": token}
+ )
+ assert response.status_code == 401
+ assert response.headers["WWW-Authenticate"] == "Basic"
+ assert response.data == b"Unauthorized"
Review comment:
https://github.com/apache/airflow/blob/master/airflow/api/client/json_client.py#L35-L42
https://github.com/apache/airflow/blob/master/airflow/api/auth/backend/deny_all.py#L41
https://github.com/apache/airflow/blob/master/airflow/api/auth/backend/kerberos_auth.py#L106
https://github.com/apache/airflow/blob/master/airflow/api/auth/backend/kerberos_auth.py#L102
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #10356: add basic auth API auth backend
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #10356:
URL: https://github.com/apache/airflow/pull/10356#discussion_r471407992
##########
File path: docs/security.rst
##########
@@ -160,6 +160,26 @@ look like the following.
-H 'Cache-Control: no-cache' \
-H "Authorization: Bearer ${ID_TOKEN}"
+Basic authentication
+''''''''''''''''''''
+
+Basic username password authentication is currently supported for the API.
Review comment:
Can you add a link to an external site that will describe this authorization method in detail?
https://tools.ietf.org/html/rfc7617
https://en.wikipedia.org/wiki/Basic_access_authentication
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] houqp commented on a change in pull request #10356: add basic auth API auth backend
Posted by GitBox <gi...@apache.org>.
houqp commented on a change in pull request #10356:
URL: https://github.com/apache/airflow/pull/10356#discussion_r472370372
##########
File path: tests/api/auth/backend/test_basic_auth.py
##########
@@ -0,0 +1,106 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import unittest
+from base64 import b64encode
+
+from flask_login import current_user
+from parameterized import parameterized
+
+from airflow.www.app import create_app
+from tests.test_utils.config import conf_vars
+from tests.test_utils.db import clear_db_pools
+
+
+class TestBasicAuth(unittest.TestCase):
+ def setUp(self) -> None:
+ with conf_vars(
+ {("api", "auth_backend"): "airflow.api.auth.backend.basic_auth"}
+ ):
+ self.app = create_app(testing=True)
+
+ self.appbuilder = self.app.appbuilder # pylint: disable=no-member
+ role_admin = self.appbuilder.sm.find_role("Admin")
+ tester = self.appbuilder.sm.find_user(username="test")
+ if not tester:
+ self.appbuilder.sm.add_user(
+ username="test",
+ first_name="test",
+ last_name="test",
+ email="test@fab.org",
+ role=role_admin,
+ password="test",
+ )
+
+ def test_success(self):
+ token = "Basic " + b64encode(b"test:test").decode()
+ clear_db_pools()
+
+ with self.app.test_client() as test_client:
+ response = test_client.get(
+ "/api/v1/pools", headers={"Authorization": token}
+ )
+ assert current_user.email == "test@fab.org"
+
+ assert response.status_code == 200
+ assert response.json == {
+ "pools": [
+ {
+ "name": "default_pool",
+ "slots": 128,
+ "occupied_slots": 0,
+ "running_slots": 0,
+ "queued_slots": 0,
+ "open_slots": 128,
+ },
+ ],
+ "total_entries": 1,
+ }
+
+ @parameterized.expand([
+ ("basic",),
+ ("basic ",),
+ ("bearer",),
+ ("test:test",),
+ (b64encode(b"test:test").decode(),),
+ ("bearer ",),
+ ("basic: ",),
+ ("basic 123",),
+ ])
+ def test_malformed_headers(self, token):
+ with self.app.test_client() as test_client:
+ response = test_client.get(
+ "/api/v1/pools", headers={"Authorization": token}
+ )
+ assert response.status_code == 401
+ assert response.headers["WWW-Authenticate"] == "Basic"
+ assert response.data == b"Unauthorized"
+
+ @parameterized.expand([
+ ("basic " + b64encode(b"test").decode(),),
+ ("basic " + b64encode(b"test:").decode(),),
+ ("basic " + b64encode(b"test:123").decode(),),
+ ("basic " + b64encode(b"test test").decode(),),
+ ])
+ def test_invalid_auth_header(self, token):
+ with self.app.test_client() as test_client:
+ response = test_client.get(
+ "/api/v1/pools", headers={"Authorization": token}
+ )
+ assert response.status_code == 401
+ assert response.headers["WWW-Authenticate"] == "Basic"
+ assert response.data == b"Unauthorized"
Review comment:
updated, for experimental api, the response is set to the same as other auth backends.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #10356: add basic auth API auth backend
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #10356:
URL: https://github.com/apache/airflow/pull/10356#discussion_r471813795
##########
File path: docs/security.rst
##########
@@ -160,6 +160,43 @@ look like the following.
-H 'Cache-Control: no-cache' \
-H "Authorization: Bearer ${ID_TOKEN}"
+Basic authentication
+''''''''''''''''''''
+
+`Basic username password authentication <https://tools.ietf.org/html/rfc7617
+https://en.wikipedia.org/wiki/Basic_access_authentication>`_ is currently
+supported for the API. This works for users created through LDAP login or
+within Airflow DB using password.
+
+To enable basic authentication, set the following in the configuration:
+
+.. code-block:: ini
+
+ [api]
+ auth_backend = airflow.api.auth.backend.basic_auth
+
+Username and password needs to be base64 encoded and send through the
+``Authorization`` HTTP header in the following format:
+
+.. code-block:: text
+
+ Authorization: Basic Base64(username:password)
+
+Here is a sample curl command you can use to validate the setup:
+
+.. code-block:: bash
+
+ ENDPOINT_URL="http://locahost:8080/"
+ curl -X GET \
+ "${ENDPOINT_URL}/api/v1/pools" \
+ -H "Authorization: Basic $(printf username:password | base64)"
Review comment:
```suggestion
curl -X GET \
--user "username:password" \
"${ENDPOINT_URL}/api/v1/pools" \
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #10356: add basic auth API auth backend
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #10356:
URL: https://github.com/apache/airflow/pull/10356#discussion_r471400279
##########
File path: airflow/api/auth/backend/basic_auth.py
##########
@@ -0,0 +1,74 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Basic authentication backend"""
+import binascii
+from base64 import b64decode
+from functools import wraps
+from typing import Callable, Optional, Tuple, TypeVar, Union, cast
+
+from flask import Response, _request_ctx_stack, current_app, request # type: ignore
+from flask_appbuilder.security.sqla.models import User
+from requests.auth import AuthBase
+
+CLIENT_AUTH: Optional[Union[Tuple[str, str], AuthBase]] = None
+
+
+def init_app(_):
+ """Initializes authentication backend"""
+
+
+T = TypeVar("T", bound=Callable) # pylint: disable=invalid-name
+
+
+def auth_current_user() -> Optional[User]:
+ """Authenticate and set current user if Authorization header exists"""
+ auth_header = request.headers.get("Authorization")
+ if not auth_header:
+ return None
+
+ auth_parts = auth_header.split(" ", 2)
+ if len(auth_parts) != 2:
+ return None
+ if auth_parts[0].lower() != "basic":
+ return None
+
+ try:
+ cred_parts = b64decode(auth_parts[1]).decode().split(":", 2)
+ except binascii.Error: # incorrect base64 encoding
+ return None
+
+ if len(cred_parts) != 2:
+ return None
+
+ (username_or_email, password) = cred_parts
+ user = current_app.appbuilder.sm.auth_user_db(username_or_email, password)
Review comment:
```suggestion
auth_header = request.authorization
if not auth_header:
return None
if auth_header.type != "basic":
return None
user = current_app.appbuilder.sm.auth_user_db(auth_header.username, auth_header.password)
```
Werkzeug supports these methods, so we don't need to implement this code again.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #10356: add basic auth API auth backend
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #10356:
URL: https://github.com/apache/airflow/pull/10356#discussion_r471403786
##########
File path: airflow/api/auth/backend/basic_auth.py
##########
@@ -0,0 +1,74 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Basic authentication backend"""
+import binascii
+from base64 import b64decode
+from functools import wraps
+from typing import Callable, Optional, Tuple, TypeVar, Union, cast
+
+from flask import Response, _request_ctx_stack, current_app, request # type: ignore
+from flask_appbuilder.security.sqla.models import User
+from requests.auth import AuthBase
+
+CLIENT_AUTH: Optional[Union[Tuple[str, str], AuthBase]] = None
+
+
+def init_app(_):
+ """Initializes authentication backend"""
+
+
+T = TypeVar("T", bound=Callable) # pylint: disable=invalid-name
+
+
+def auth_current_user() -> Optional[User]:
+ """Authenticate and set current user if Authorization header exists"""
+ auth_header = request.headers.get("Authorization")
+ if not auth_header:
+ return None
+
+ auth_parts = auth_header.split(" ", 2)
+ if len(auth_parts) != 2:
+ return None
+ if auth_parts[0].lower() != "basic":
+ return None
+
+ try:
+ cred_parts = b64decode(auth_parts[1]).decode().split(":", 2)
+ except binascii.Error: # incorrect base64 encoding
+ return None
+
+ if len(cred_parts) != 2:
+ return None
+
+ (username_or_email, password) = cred_parts
+ user = current_app.appbuilder.sm.auth_user_db(username_or_email, password)
+ if user is not None:
+ ctx = _request_ctx_stack.top
+ ctx.user = user
Review comment:
```suggestion
login_user(user, remember=False)
```
This will generate a signal that other integrations can catch.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on pull request #10356: add basic auth API auth backend
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #10356:
URL: https://github.com/apache/airflow/pull/10356#issuecomment-675625402
LGTM. but I'd like to do a final check while I'm on my computer and able to run the code. Now I am wondering if the Content-Type headers are correct.
Thank you very much for your 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #10356: add basic auth API auth backend
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #10356:
URL: https://github.com/apache/airflow/pull/10356#discussion_r471814852
##########
File path: tests/api/auth/backend/test_basic_auth.py
##########
@@ -0,0 +1,102 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import unittest
+from base64 import b64encode
+
+from flask_login import current_user
+from parameterized import parameterized
+
+from airflow.www.app import create_app
+from tests.test_utils.config import conf_vars
+from tests.test_utils.db import clear_db_pools
+
+
+class TestBasicAuth(unittest.TestCase):
+ def setUp(self) -> None:
+ with conf_vars(
+ {("api", "auth_backend"): "airflow.api.auth.backend.basic_auth"}
+ ):
+ self.app = create_app(testing=True)
+
+ self.appbuilder = self.app.appbuilder # pylint: disable=no-member
+ role_admin = self.appbuilder.sm.find_role("Admin")
+ tester = self.appbuilder.sm.find_user(username="test")
+ if not tester:
+ self.appbuilder.sm.add_user(
+ username="test",
+ first_name="test",
+ last_name="test",
+ email="test@fab.org",
+ role=role_admin,
+ password="test",
+ )
+
+ def test_success(self):
+ token = "Basic " + b64encode(b"test:test").decode()
+ clear_db_pools()
+
+ with self.app.test_client() as test_client:
+ response = test_client.get(
+ "/api/v1/pools", headers={"Authorization": token}
+ )
+ assert current_user.email == "test@fab.org"
+
+ assert response.status_code == 200
+ assert response.json == {
+ "pools": [
+ {
+ "name": "default_pool",
+ "slots": 128,
+ "occupied_slots": 0,
+ "running_slots": 0,
+ "queued_slots": 0,
+ "open_slots": 128,
+ },
+ ],
+ "total_entries": 1,
+ }
+
+ @parameterized.expand([
+ ("basic",),
+ ("basic ",),
+ ("bearer",),
+ ("test:test",),
+ (b64encode(b"test:test").decode(),),
+ ("bearer ",),
+ ("basic: ",),
+ ("basic 123",),
+ ])
+ def test_malformed_headers(self, token):
+ with self.app.test_client() as test_client:
+ response = test_client.get(
+ "/api/v1/pools", headers={"Authorization": token}
+ )
+ assert response.status_code == 401
Review comment:
Can you assert response body and. headers also? In particular, I would like to check if the ``WWW-Authenticate`` header is set.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on pull request #10356: add basic auth API auth backend
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #10356:
URL: https://github.com/apache/airflow/pull/10356#issuecomment-675629482
Can you add tests for experimental API?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #10356: add basic auth API auth backend
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #10356:
URL: https://github.com/apache/airflow/pull/10356#discussion_r471406532
##########
File path: airflow/api/auth/backend/basic_auth.py
##########
@@ -0,0 +1,74 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Basic authentication backend"""
+import binascii
+from base64 import b64decode
+from functools import wraps
+from typing import Callable, Optional, Tuple, TypeVar, Union, cast
+
+from flask import Response, _request_ctx_stack, current_app, request # type: ignore
+from flask_appbuilder.security.sqla.models import User
+from requests.auth import AuthBase
+
+CLIENT_AUTH: Optional[Union[Tuple[str, str], AuthBase]] = None
+
+
+def init_app(_):
+ """Initializes authentication backend"""
+
+
+T = TypeVar("T", bound=Callable) # pylint: disable=invalid-name
+
+
+def auth_current_user() -> Optional[User]:
+ """Authenticate and set current user if Authorization header exists"""
+ auth_header = request.headers.get("Authorization")
+ if not auth_header:
+ return None
+
+ auth_parts = auth_header.split(" ", 2)
+ if len(auth_parts) != 2:
+ return None
+ if auth_parts[0].lower() != "basic":
+ return None
+
+ try:
+ cred_parts = b64decode(auth_parts[1]).decode().split(":", 2)
+ except binascii.Error: # incorrect base64 encoding
+ return None
+
+ if len(cred_parts) != 2:
+ return None
+
+ (username_or_email, password) = cred_parts
+ user = current_app.appbuilder.sm.auth_user_db(username_or_email, password)
Review comment:
```suggestion
user = current_app.appbuilder.sm.auth_user_db(username_or_email, password)
```
Can you add support for LDAP also? It will be code similar to the following:
```
if self.appbuilder.sm.auth_type == const.AUTH_DB:
user = self.appbuilder.sm.auth_user_db(username, password)
elif self.appbuilder.sm.auth_type == const.AUTH_LDAP:
user = self.appbuilder.sm.auth_user_ldap(username, password)
else:
raise Exception()
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] houqp commented on pull request #10356: add basic auth API auth backend
Posted by GitBox <gi...@apache.org>.
houqp commented on pull request #10356:
URL: https://github.com/apache/airflow/pull/10356#issuecomment-675626431
thanks @jhtimmins for the tip, I have it setup locally, but just forgot to run it for that commit :P
@mik-laj let me add a check for content type in test
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #10356: add basic auth API auth backend
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #10356:
URL: https://github.com/apache/airflow/pull/10356#discussion_r472527642
##########
File path: docs/security/api.rst
##########
@@ -116,3 +116,41 @@ look like the following.
-H 'Content-Type: application/json' \
-H 'Cache-Control: no-cache' \
-H "Authorization: Bearer ${ID_TOKEN}"
+
+Basic authentication
+''''''''''''''''''''
+
+`Basic username password authentication <https://tools.ietf.org/html/rfc7617
+https://en.wikipedia.org/wiki/Basic_access_authentication>`_ is currently
+supported for the API. This works for users created through LDAP login or
+within Airflow DB using password.
+
+To enable basic authentication, set the following in the configuration:
+
+.. code-block:: ini
+
+ [api]
+ auth_backend = airflow.api.auth.backend.basic_auth
+
+Username and password needs to be base64 encoded and send through the
+``Authorization`` HTTP header in the following format:
+
+.. code-block:: text
+
+ Authorization: Basic Base64(username:password)
+
+This conversation was marked as resolved by houqp
+
+Here is a sample curl command you can use to validate the setup:
+
+.. code-block:: bash
+
+ ENDPOINT_URL="http://locahost:8080/"
+ curl -X GET \
+ --user "username:password" \
+ "${ENDPOINT_URL}/api/v1/pools"
+
+Note, you can still enable this setting to allow API access through username
+password credential even though Airflow web is using other authentication
+method. Under this setup, only users created through LDAP or ``airflow users
+create`` command will be able to pass the API authentication.
Review comment:
```suggestion
Note, you can still enable this setting to allow API access through username
password credential even though Airflow Webserver might be using another authentication
method. Under this setup, only users created through LDAP or ``airflow users
create`` command will be able to pass the API authentication.
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #10356: add basic auth API auth backend
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #10356:
URL: https://github.com/apache/airflow/pull/10356#discussion_r471391807
##########
File path: docs/security.rst
##########
@@ -160,6 +160,26 @@ look like the following.
-H 'Cache-Control: no-cache' \
-H "Authorization: Bearer ${ID_TOKEN}"
+Basic authentication
+''''''''''''''''''''
+
+Basic username password authentication is currently supported for the API.
+
+To enable basic authentication, set the following in the configuration:
+
+.. code-block:: ini
+
+ [api]
+ auth_backend = airflow.api.auth.backend.basic_auth
+
+Username and password needs to be base64 encoded and send through the
+``Authorization`` HTTP header in the following format:
+
+.. code-block:: text
+
+ Authorization: Basic Base64(username:password)
Review comment:
Can you add an example using CURL? This will allow us to easily check if the configuration is correct.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] houqp commented on a change in pull request #10356: Add basic auth API auth backend
Posted by GitBox <gi...@apache.org>.
houqp commented on a change in pull request #10356:
URL: https://github.com/apache/airflow/pull/10356#discussion_r472578878
##########
File path: docs/security/api.rst
##########
@@ -116,3 +116,41 @@ look like the following.
-H 'Content-Type: application/json' \
-H 'Cache-Control: no-cache' \
-H "Authorization: Bearer ${ID_TOKEN}"
+
+Basic authentication
+''''''''''''''''''''
+
+`Basic username password authentication <https://tools.ietf.org/html/rfc7617
+https://en.wikipedia.org/wiki/Basic_access_authentication>`_ is currently
+supported for the API. This works for users created through LDAP login or
+within Airflow DB using password.
+
+To enable basic authentication, set the following in the configuration:
+
+.. code-block:: ini
+
+ [api]
+ auth_backend = airflow.api.auth.backend.basic_auth
+
+Username and password needs to be base64 encoded and send through the
+``Authorization`` HTTP header in the following format:
+
+.. code-block:: text
+
+ Authorization: Basic Base64(username:password)
+
+This conversation was marked as resolved by houqp
+
Review comment:
lol, good catch, the worst copy paste error ever.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] houqp commented on a change in pull request #10356: add basic auth API auth backend
Posted by GitBox <gi...@apache.org>.
houqp commented on a change in pull request #10356:
URL: https://github.com/apache/airflow/pull/10356#discussion_r471740036
##########
File path: docs/security.rst
##########
@@ -160,6 +160,26 @@ look like the following.
-H 'Cache-Control: no-cache' \
-H "Authorization: Bearer ${ID_TOKEN}"
+Basic authentication
+''''''''''''''''''''
+
+Basic username password authentication is currently supported for the API.
Review comment:
I have added more comment around this topic.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] houqp commented on a change in pull request #10356: add basic auth API auth backend
Posted by GitBox <gi...@apache.org>.
houqp commented on a change in pull request #10356:
URL: https://github.com/apache/airflow/pull/10356#discussion_r471743601
##########
File path: airflow/api/auth/backend/basic_auth.py
##########
@@ -0,0 +1,65 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Basic authentication backend"""
+from functools import wraps
+from typing import Callable, Optional, Tuple, TypeVar, Union, cast
+
+from flask import Response, current_app, request
+from flask_appbuilder.const import AUTH_LDAP
+from flask_appbuilder.security.sqla.models import User
+from flask_login import login_user
+from requests.auth import AuthBase
+
+CLIENT_AUTH: Optional[Union[Tuple[str, str], AuthBase]] = None
+
+
+def init_app(_):
+ """Initializes authentication backend"""
+
+
+T = TypeVar("T", bound=Callable) # pylint: disable=invalid-name
+
+
+def auth_current_user() -> Optional[User]:
+ """Authenticate and set current user if Authorization header exists"""
+ auth = request.authorization
+ if auth is None or not auth.username or not auth.password:
+ return None
+
+ ab_security_manager = current_app.appbuilder.sm
+ user = None
+ if ab_security_manager.auth_type == AUTH_LDAP:
+ user = ab_security_manager.auth_user_ldap(auth.username, auth.password)
+ if user is None:
+ user = ab_security_manager.auth_user_db(auth.username, auth.password)
Review comment:
@mik-laj i changed the logic so that db user auth is always used as the fallback. This way, we can use ldap for web login, while still manage "service accounts" manually as db users.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #10356: add basic auth API auth backend
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #10356:
URL: https://github.com/apache/airflow/pull/10356#discussion_r471409467
##########
File path: tests/api/auth/backend/test_basic_auth.py
##########
@@ -0,0 +1,102 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import unittest
+from base64 import b64encode
+
+from flask_login import current_user
+from parameterized import parameterized
+
+from airflow.www.app import create_app
+from tests.test_utils.config import conf_vars
+from tests.test_utils.db import clear_db_pools
+
+
+class TestBasicAuth(unittest.TestCase):
+ def setUp(self) -> None:
+ with conf_vars(
+ {("api", "auth_backend"): "airflow.api.auth.backend.basic_auth"}
+ ):
+ self.app = create_app(testing=True)
+
+ self.appbuilder = self.app.appbuilder # pylint: disable=no-member
+ role_admin = self.appbuilder.sm.find_role("Admin")
+ tester = self.appbuilder.sm.find_user(username="test")
+ if not tester:
+ self.appbuilder.sm.add_user(
+ username="test",
+ first_name="test",
+ last_name="test",
+ email="test@fab.org",
+ role=role_admin,
+ password="test",
+ )
+
+ def test_success(self):
+ token = "Basic " + b64encode(b"test:test").decode()
+ clear_db_pools()
+
+ with self.app.test_client() as test_client:
+ response = test_client.get(
+ "/api/v1/pools", headers={"Authorization": token}
+ )
+ assert current_user.email == "test@fab.org"
+
+ assert response.status_code == 200
+ assert response.json == {
+ "pools": [
+ {
+ "name": "default_pool",
+ "slots": 128,
+ "occupied_slots": 0,
+ "running_slots": 0,
+ "queued_slots": 0,
+ "open_slots": 128,
+ },
+ ],
+ "total_entries": 1,
+ }
+
+ @parameterized.expand([
+ ("basic",),
+ ("basic ",),
+ ("bearer",),
+ ("test:test",),
+ (b64encode(b"test:test").decode(),),
+ ("bearer ",),
+ ("basic: ",),
+ ("basic 123",),
+ ])
+ def test_malformed_headers(self, token):
+ with self.app.test_client() as test_client:
+ response = test_client.get(
+ "/api/v1/pools", headers={"Authorization": token}
+ )
+ assert response.status_code == 401
+
+ @parameterized.expand([
+ ("basic " + b64encode(b"test").decode(),),
+ ("basic " + b64encode(b"test:").decode(),),
+ ("basic " + b64encode(b"test:123").decode(),),
+ ("basic " + b64encode(b"test test").decode(),),
+ ])
+ def test_invalid_id_token(self, token):
Review comment:
```suggestion
def test_invalid_auth_header(self, 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #10356: add basic auth API auth backend
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #10356:
URL: https://github.com/apache/airflow/pull/10356#discussion_r471409256
##########
File path: docs/security.rst
##########
@@ -160,6 +160,26 @@ look like the following.
-H 'Cache-Control: no-cache' \
-H "Authorization: Bearer ${ID_TOKEN}"
+Basic authentication
+''''''''''''''''''''
+
+Basic username password authentication is currently supported for the API.
Review comment:
Does this authentication method always work? Are there configuration options that could prevent this authorization from working?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #10356: add basic auth API auth backend
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #10356:
URL: https://github.com/apache/airflow/pull/10356#discussion_r471876678
##########
File path: tests/api/auth/backend/test_basic_auth.py
##########
@@ -0,0 +1,106 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import unittest
+from base64 import b64encode
+
+from flask_login import current_user
+from parameterized import parameterized
+
+from airflow.www.app import create_app
+from tests.test_utils.config import conf_vars
+from tests.test_utils.db import clear_db_pools
+
+
+class TestBasicAuth(unittest.TestCase):
+ def setUp(self) -> None:
+ with conf_vars(
+ {("api", "auth_backend"): "airflow.api.auth.backend.basic_auth"}
+ ):
+ self.app = create_app(testing=True)
+
+ self.appbuilder = self.app.appbuilder # pylint: disable=no-member
+ role_admin = self.appbuilder.sm.find_role("Admin")
+ tester = self.appbuilder.sm.find_user(username="test")
+ if not tester:
+ self.appbuilder.sm.add_user(
+ username="test",
+ first_name="test",
+ last_name="test",
+ email="test@fab.org",
+ role=role_admin,
+ password="test",
+ )
+
+ def test_success(self):
+ token = "Basic " + b64encode(b"test:test").decode()
+ clear_db_pools()
+
+ with self.app.test_client() as test_client:
+ response = test_client.get(
+ "/api/v1/pools", headers={"Authorization": token}
+ )
+ assert current_user.email == "test@fab.org"
+
+ assert response.status_code == 200
+ assert response.json == {
+ "pools": [
+ {
+ "name": "default_pool",
+ "slots": 128,
+ "occupied_slots": 0,
+ "running_slots": 0,
+ "queued_slots": 0,
+ "open_slots": 128,
+ },
+ ],
+ "total_entries": 1,
+ }
+
+ @parameterized.expand([
+ ("basic",),
+ ("basic ",),
+ ("bearer",),
+ ("test:test",),
+ (b64encode(b"test:test").decode(),),
+ ("bearer ",),
+ ("basic: ",),
+ ("basic 123",),
+ ])
+ def test_malformed_headers(self, token):
+ with self.app.test_client() as test_client:
+ response = test_client.get(
+ "/api/v1/pools", headers={"Authorization": token}
+ )
+ assert response.status_code == 401
+ assert response.headers["WWW-Authenticate"] == "Basic"
+ assert response.data == b"Unauthorized"
+
+ @parameterized.expand([
+ ("basic " + b64encode(b"test").decode(),),
+ ("basic " + b64encode(b"test:").decode(),),
+ ("basic " + b64encode(b"test:123").decode(),),
+ ("basic " + b64encode(b"test test").decode(),),
+ ])
+ def test_invalid_auth_header(self, token):
+ with self.app.test_client() as test_client:
+ response = test_client.get(
+ "/api/v1/pools", headers={"Authorization": token}
+ )
+ assert response.status_code == 401
+ assert response.headers["WWW-Authenticate"] == "Basic"
+ assert response.data == b"Unauthorized"
Review comment:
This answer should be in line with RFC-7807.
https://airflow.readthedocs.io/en/latest/stable-rest-api/redoc.html
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #10356: add basic auth API auth backend
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #10356:
URL: https://github.com/apache/airflow/pull/10356#discussion_r472527063
##########
File path: docs/security/api.rst
##########
@@ -116,3 +116,41 @@ look like the following.
-H 'Content-Type: application/json' \
-H 'Cache-Control: no-cache' \
-H "Authorization: Bearer ${ID_TOKEN}"
+
+Basic authentication
+''''''''''''''''''''
+
+`Basic username password authentication <https://tools.ietf.org/html/rfc7617
+https://en.wikipedia.org/wiki/Basic_access_authentication>`_ is currently
+supported for the API. This works for users created through LDAP login or
+within Airflow DB using password.
+
+To enable basic authentication, set the following in the configuration:
+
+.. code-block:: ini
+
+ [api]
+ auth_backend = airflow.api.auth.backend.basic_auth
+
+Username and password needs to be base64 encoded and send through the
+``Authorization`` HTTP header in the following format:
+
+.. code-block:: text
+
+ Authorization: Basic Base64(username:password)
+
+This conversation was marked as resolved by houqp
+
Review comment:
```suggestion
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #10356: add basic auth API auth backend
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #10356:
URL: https://github.com/apache/airflow/pull/10356#discussion_r471406532
##########
File path: airflow/api/auth/backend/basic_auth.py
##########
@@ -0,0 +1,74 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Basic authentication backend"""
+import binascii
+from base64 import b64decode
+from functools import wraps
+from typing import Callable, Optional, Tuple, TypeVar, Union, cast
+
+from flask import Response, _request_ctx_stack, current_app, request # type: ignore
+from flask_appbuilder.security.sqla.models import User
+from requests.auth import AuthBase
+
+CLIENT_AUTH: Optional[Union[Tuple[str, str], AuthBase]] = None
+
+
+def init_app(_):
+ """Initializes authentication backend"""
+
+
+T = TypeVar("T", bound=Callable) # pylint: disable=invalid-name
+
+
+def auth_current_user() -> Optional[User]:
+ """Authenticate and set current user if Authorization header exists"""
+ auth_header = request.headers.get("Authorization")
+ if not auth_header:
+ return None
+
+ auth_parts = auth_header.split(" ", 2)
+ if len(auth_parts) != 2:
+ return None
+ if auth_parts[0].lower() != "basic":
+ return None
+
+ try:
+ cred_parts = b64decode(auth_parts[1]).decode().split(":", 2)
+ except binascii.Error: # incorrect base64 encoding
+ return None
+
+ if len(cred_parts) != 2:
+ return None
+
+ (username_or_email, password) = cred_parts
+ user = current_app.appbuilder.sm.auth_user_db(username_or_email, password)
Review comment:
```suggestion
user = current_app.appbuilder.sm.auth_user_db(username_or_email, password)
```
Can you add support for LDAP also? It will be code similar to the following:
```python
if self.appbuilder.sm.auth_type == const.AUTH_DB:
user = self.appbuilder.sm.auth_user_db(username, password)
elif self.appbuilder.sm.auth_type == const.AUTH_LDAP:
user = self.appbuilder.sm.auth_user_ldap(username, password)
else:
raise Exception()
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jhtimmins commented on pull request #10356: add basic auth API auth backend
Posted by GitBox <gi...@apache.org>.
jhtimmins commented on pull request #10356:
URL: https://github.com/apache/airflow/pull/10356#issuecomment-675292420
@houqp this looks good to me. Looks like some static check tests are failing. If you aren't using it already, pre-commit is quite helpful and will help catch those issues on your local environment. https://github.com/apache/airflow/blob/master/STATIC_CODE_CHECKS.rst#pre-commit-hooks
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org