Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for OIDC API Auth and OIDC integration tests #16977

Merged
merged 26 commits into from
Jan 2, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
6182222
Add test harness for oidc login against keycloak
nuwang Oct 8, 2023
d1aa6d1
Add oidc logout tests and fix bug in server side logout
nuwang Oct 8, 2023
9d3d45a
Accept access token for api auth
nuwang Oct 15, 2023
32dc586
Make bearer token optional and test for API calls by users who have n…
nuwang Oct 16, 2023
9367a69
Only run the cleanup if job state is available
nuwang Oct 16, 2023
257ebbc
Refactor method name
nuwang Oct 19, 2023
e4fd79b
Move method to correct class after rebase
nuwang Nov 2, 2023
a7a0492
Add test for token expiry
nuwang Nov 2, 2023
54d0bd2
Add audience configs with additional clients
nuwang Nov 3, 2023
9992c38
Make sure that audiences are respected
nuwang Nov 3, 2023
41e70f2
Add test for unauthorized client
nuwang Nov 3, 2023
4e2f56a
Add default scope
nuwang Nov 3, 2023
c78788e
Add test for unauthorized audience
nuwang Nov 3, 2023
9cc1a6a
Reformat code
nuwang Nov 3, 2023
cc2d8c1
Reduce access token lifespan
nuwang Nov 3, 2023
92851c0
Refactor method names and lint fixes
nuwang Nov 4, 2023
7f05d3f
Fix issues flagged by mypy
nuwang Nov 4, 2023
e1323b7
Start the keycloak container
nuwang Nov 4, 2023
65ee1e6
Change keycloak docker hostname
nuwang Nov 5, 2023
3d1244d
Make keycloak port configurable
nuwang Nov 6, 2023
517f0e7
Verify client scopes before accepting token
nuwang Nov 6, 2023
ca8cb47
Add integration tests for oidc account linkup
nuwang Nov 6, 2023
5d316fb
Improved error messages and exception handling
nuwang Nov 7, 2023
56edd29
Merge branch 'dev' into oidc_api_auth
nuwang Dec 19, 2023
fed04ed
More descriptive help text for oidc_scope_prefix
nuwang Dec 20, 2023
a21c9b2
Also update oidc_scope_prefix description in config_schema.yml
nuwang Dec 20, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions lib/galaxy/authnz/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -87,3 +87,19 @@ def logout(self, trans, post_user_logout_href=None):
:param post_user_logout_href: Optional URL to redirect to after logging out of IDP.
"""
raise NotImplementedError()

def decode_user_access_token(self, sa_session, access_token):
"""
Verifies and decodes an access token against this provider, returning the user and
a dict containing the decoded token data.

:type sa_session: sqlalchemy.orm.scoping.scoped_session
:param sa_session: SQLAlchemy database handle.

:type access_token: string
:param access_token: An OIDC access token

:return: A tuple containing the user and decoded jwt data
:rtype: Tuple[User, dict]
"""
raise NotImplementedError()
69 changes: 68 additions & 1 deletion lib/galaxy/authnz/custos_authnz.py
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ class CustosAuthnzConfiguration:
redirect_uri: str
ca_bundle: Optional[str]
pkce_support: bool
accepted_audiences: List[str]
extra_params: Optional[dict]
authorization_endpoint: Optional[str]
token_endpoint: Optional[str]
Expand All @@ -68,11 +69,14 @@ class CustosAuthnzConfiguration:
iam_client_secret: Optional[str]
userinfo_endpoint: Optional[str]
credential_url: Optional[str]
issuer: Optional[str]
jwks_uri: Optional[str]


class OIDCAuthnzBase(IdentityProvider):
def __init__(self, provider, oidc_config, oidc_backend_config, idphint=None):
provider = provider.lower()
self.jwks_client: Optional[jwt.PyJWKClient]
self.config = CustosAuthnzConfiguration(
provider=provider,
verify_ssl=oidc_config["VERIFY_SSL"],
Expand All @@ -84,6 +88,15 @@ def __init__(self, provider, oidc_config, oidc_backend_config, idphint=None):
redirect_uri=oidc_backend_config["redirect_uri"],
ca_bundle=oidc_backend_config.get("ca_bundle", None),
pkce_support=oidc_backend_config.get("pkce_support", False),
accepted_audiences=list(
filter(
None,
map(
str.strip,
oidc_backend_config.get("accepted_audiences", oidc_backend_config["client_id"]).split(","),
),
)
),
extra_params={},
authorization_endpoint=None,
token_endpoint=None,
Expand All @@ -92,6 +105,8 @@ def __init__(self, provider, oidc_config, oidc_backend_config, idphint=None):
iam_client_secret=None,
userinfo_endpoint=None,
credential_url=None,
issuer=None,
jwks_uri=None,
)

def _decode_token_no_signature(self, token):
Expand Down Expand Up @@ -221,7 +236,7 @@ def callback(self, state_token, authz_code, trans, login_redirect_url):
if trans.app.config.fixed_delegated_auth:
user = existing_user
else:
message = f"There already exists a user with email {email}. To associate this external login, you must first be logged in as that existing account."
message = f"There already exists a user with email {email}. To associate this external login, you must first be logged in as that existing account."
log.info(message)
login_redirect_url = (
f"{login_redirect_url}login/start"
Expand Down Expand Up @@ -449,6 +464,12 @@ def _load_well_known_oidc_config(self, well_known_oidc_config):
self.config.token_endpoint = well_known_oidc_config["token_endpoint"]
self.config.userinfo_endpoint = well_known_oidc_config["userinfo_endpoint"]
self.config.end_session_endpoint = well_known_oidc_config.get("end_session_endpoint")
self.config.issuer = well_known_oidc_config.get("issuer")
self.config.jwks_uri = well_known_oidc_config.get("jwks_uri")
if self.config.jwks_uri:
self.jwks_client = jwt.PyJWKClient(self.config.jwks_uri, cache_jwk_set=True, lifespan=360)
else:
self.jwks_client = None

def _get_verify_param(self):
"""Return 'ca_bundle' if 'verify_ssl' is true and 'ca_bundle' is configured."""
Expand All @@ -473,6 +494,52 @@ def _username_from_userinfo(trans, userinfo):
else:
return username

def decode_user_access_token(self, sa_session, access_token):
"""Verifies and decodes an access token against this provider, returning the user and
a dict containing the decoded token data.

:type sa_session: sqlalchemy.orm.scoping.scoped_session
:param sa_session: SQLAlchemy database handle.

:type access_token: string
:param access_token: An OIDC access token

:return: A tuple containing the user and decoded jwt data or [None, None]
if the access token does not belong to this provider.
:rtype: Tuple[User, dict]
"""
if not self.jwks_client:
return None
try:
signing_key = self.jwks_client.get_signing_key_from_jwt(access_token)
decoded_jwt = jwt.decode(
access_token,
signing_key.key,
algorithms=["RS256"],
issuer=self.config.issuer,
audience=self.config.accepted_audiences,
options={
"verify_signature": True,
"verify_exp": True,
"verify_nbf": True,
"verify_iat": True,
"verify_aud": bool(self.config.accepted_audiences),
"verify_iss": True,
},
)
except jwt.exceptions.PyJWKClientError:
log.debug(f"Could not get signing keys for access token with provider: {self.config.provider}. Ignoring...")
return None, None
except jwt.exceptions.InvalidIssuerError:
# An Invalid issuer means that the access token is not relevant to this provider.
# All other exceptions are bubbled up
return None, None
# jwt verified, we can now fetch the user
user_id = decoded_jwt["sub"]
custos_authnz_token = self._get_custos_authnz_token(sa_session, user_id, self.config.provider)
user = custos_authnz_token.user if custos_authnz_token else None
return user, decoded_jwt


class OIDCAuthnzBaseKeycloak(OIDCAuthnzBase):
def __init__(self, provider, oidc_config, oidc_backend_config, idphint=None):
Expand Down
52 changes: 52 additions & 0 deletions lib/galaxy/authnz/managers.py
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,8 @@ def _parse_idp_config(self, config_xml):
rtv["tenant_id"] = config_xml.find("tenant_id").text
if config_xml.find("pkce_support") is not None:
rtv["pkce_support"] = asbool(config_xml.find("pkce_support").text)
if config_xml.find("accepted_audiences") is not None:
rtv["accepted_audiences"] = config_xml.find("accepted_audiences").text
# this is a EGI Check-in specific config
if config_xml.find("checkin_env") is not None:
rtv["checkin_env"] = config_xml.find("checkin_env").text
Expand Down Expand Up @@ -195,6 +197,8 @@ def _parse_custos_config(self, config_xml):
rtv["icon"] = config_xml.find("icon").text
if config_xml.find("pkce_support") is not None:
rtv["pkce_support"] = asbool(config_xml.find("pkce_support").text)
if config_xml.find("accepted_audiences") is not None:
rtv["accepted_audiences"] = config_xml.find("accepted_audiences").text
return rtv

def get_allowed_idps(self):
Expand Down Expand Up @@ -407,6 +411,54 @@ def create_user(self, provider, token, trans, login_redirect_url):
log.exception(msg)
return False, msg, (None, None)

def _assert_jwt_contains_scopes(self, user, jwt, required_scopes):
if not jwt:
raise exceptions.AuthenticationFailed(
err_msg=f"User: {user.username} does not have the required scopes: [{required_scopes}]"
)
scopes = jwt.get("scope") or ""
if not set(required_scopes).issubset(scopes.split(" ")):
raise exceptions.AuthenticationFailed(
err_msg=f"User: {user.username} has JWT with scopes: [{scopes}] but not required scopes: [{required_scopes}]"
)

def _validate_permissions(self, user, jwt):
required_scopes = [f"{self.app.config.oidc_scope_prefix}:*"]
self._assert_jwt_contains_scopes(user, jwt, required_scopes)

def _match_access_token_to_user_in_provider(self, sa_session, provider, access_token):
try:
success, message, backend = self._get_authnz_backend(provider)
if success is False:
msg = f"An error occurred when obtaining user by token with provider `{provider}`: {message}"
log.error(msg)
return None
user, jwt = None, None
try:
user, jwt = backend.decode_user_access_token(sa_session, access_token)
except Exception:
log.exception("Could not decode access token")
raise exceptions.AuthenticationFailed(err_msg="Invalid access token or an unexpected error occurred.")
if user and jwt:
self._validate_permissions(user, jwt)
return user
elif not user and jwt:
# jwt was decoded, but no user could be matched
raise exceptions.AuthenticationFailed(
err_msg="Cannot locate user by access token. The user should log into Galaxy at least once with this OIDC provider."
)
# Both jwt and user are empty, which means that this provider can't process this access token
return None
except NotImplementedError:
return None

def match_access_token_to_user(self, sa_session, access_token):
for provider in self.oidc_backends_config:
user = self._match_access_token_to_user_in_provider(sa_session, provider, access_token)
if user:
return user
return None

def logout(self, provider, trans, post_user_logout_href=None):
"""
Log the user out of the identity provider.
Expand Down
9 changes: 9 additions & 0 deletions lib/galaxy/config/sample/galaxy.yml.sample
Original file line number Diff line number Diff line change
Expand Up @@ -2179,6 +2179,15 @@ galaxy:
# <config_dir>.
#oidc_backends_config_file: oidc_backends_config.xml

# Sets the prefix for OIDC scopes specific to this Galaxy instance.
# If an API call is made against this Galaxy instance using an OIDC bearer token,
# any scopes must be prefixed with this value e.g. https://galaxyproject.org/api.
# More concretely, to request all permissions that the user has, the scope
# would have to be specified as "<prefix>:*". e.g "https://galaxyproject.org/api:*".
# Currently, only * is recognised as a valid scope, and future iterations may
# provide more fine-grained scopes.
#oidc_scope_prefix: https://galaxyproject.org/api
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the :* missing here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The config setting should specify only the prefix. The * is the exact scope that's requested, meaning all permissions. That leaves space for future expansion, like perhaps <prefix>:datasets to request access to datasets etc. For now, we only support <prefix>:*

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should then e.g "https://galaxyproject.org/api" be changed to be clear?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've expanded the description a bit. Let me know whether this looks better.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, thanks a lot!


# XML config file that allows the use of different authentication
# providers (e.g. LDAP) instead or in addition to local authentication
# (.sample is used if default does not exist).
Expand Down
13 changes: 13 additions & 0 deletions lib/galaxy/config/schemas/config_schema.yml
Original file line number Diff line number Diff line change
Expand Up @@ -2903,6 +2903,19 @@ mapping:
desc: |
Sets the path to OIDC backends configuration file.

oidc_scope_prefix:
type: str
default: https://galaxyproject.org/api
required: false
desc: |
Sets the prefix for OIDC scopes specific to this Galaxy instance.
If an API call is made against this Galaxy instance using an OIDC bearer token,
any scopes must be prefixed with this value e.g. https://galaxyproject.org/api.
More concretely, to request all permissions that the user has, the scope
would have to be specified as "<prefix>:*". e.g "https://galaxyproject.org/api:*".
Currently, only * is recognised as a valid scope, and future iterations may
provide more fine-grained scopes.

auth_config_file:
type: str
default: auth_conf.xml
Expand Down
7 changes: 7 additions & 0 deletions lib/galaxy/managers/users.py
Original file line number Diff line number Diff line change
Expand Up @@ -294,6 +294,13 @@ def by_api_key(self, api_key: str, sa_session=None):
raise exceptions.AuthenticationFailed("Provided API key has expired.")
return provided_key.user

def by_oidc_access_token(self, access_token: str):
if hasattr(self.app, "authnz_manager") and self.app.authnz_manager:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be more explicit to find which type of app we need (is it UniverseApplication) with an isinstance check ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's not great, I've simply followed the pattern used elsewhere. This is because authnz_manager is assigned conditionally even in the UniverseApplication constructor and is undefined by default. This is something that should probably be handled during the proposed refactoring run.

user = self.app.authnz_manager.match_access_token_to_user(self.app.model.session, access_token) # type: ignore[attr-defined]
return user
else:
return None

def check_bootstrap_admin_api_key(self, api_key):
bootstrap_admin_api_key = getattr(self.app.config, "bootstrap_admin_api_key", None)
if not bootstrap_admin_api_key:
Expand Down
12 changes: 12 additions & 0 deletions lib/galaxy/webapps/base/webapp.py
Original file line number Diff line number Diff line change
Expand Up @@ -534,6 +534,10 @@ def _authenticate_api(self, session_cookie: str) -> Optional[str]:
"""
Authenticate for the API via key or session (if available).
"""
oidc_access_token = self.request.headers.get("Authorization", None)
oidc_token_supplied = (
self.environ.get("is_api_request", False) and oidc_access_token and "Bearer " in oidc_access_token
)
api_key = self.request.params.get("key", None) or self.request.headers.get("x-api-key", None)
secure_id = self.get_cookie(name=session_cookie)
api_key_supplied = self.environ.get("is_api_request", False) and api_key
Expand All @@ -556,6 +560,14 @@ def _authenticate_api(self, session_cookie: str) -> Optional[str]:
)
self.user = None
self.galaxy_session = None
elif oidc_token_supplied:
# Sessionless API transaction with oidc token, we just need to associate a user.
oidc_access_token = oidc_access_token.replace("Bearer ", "")
try:
user = self.user_manager.by_oidc_access_token(oidc_access_token)
except AuthenticationFailed as e:
return str(e)
self.set_user(user)
else:
# Anonymous API interaction -- anything but @expose_api_anonymous will fail past here.
self.user = None
Expand Down
11 changes: 9 additions & 2 deletions lib/galaxy/webapps/galaxy/api/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@
APIKeyCookie,
APIKeyHeader,
APIKeyQuery,
HTTPAuthorizationCredentials,
HTTPBearer,
)
from fastapi_utils.cbv import cbv
from pydantic import ValidationError
Expand Down Expand Up @@ -80,6 +82,7 @@
api_key_query = APIKeyQuery(name="key", auto_error=False)
api_key_header = APIKeyHeader(name="x-api-key", auto_error=False)
api_key_cookie = APIKeyCookie(name="galaxysession", auto_error=False)
api_bearer_token = HTTPBearer(auto_error=False)


def get_app() -> StructuredApp:
Expand Down Expand Up @@ -139,6 +142,7 @@ def get_api_user(
user_manager: UserManager = depends(UserManager),
key: str = Security(api_key_query),
x_api_key: str = Security(api_key_header),
bearer_token: HTTPAuthorizationCredentials = Security(api_bearer_token),
run_as: Optional[DecodedDatabaseIdField] = Header(
default=None,
title="Run as User",
Expand All @@ -149,9 +153,12 @@ def get_api_user(
),
) -> Optional[User]:
api_key = key or x_api_key
if not api_key:
if api_key:
user = user_manager.by_api_key(api_key=api_key)
elif bearer_token:
user = user_manager.by_oidc_access_token(access_token=bearer_token.credentials)
else:
return None
user = user_manager.by_api_key(api_key=api_key)
if run_as:
if user_manager.user_can_do_run_as(user):
return user_manager.by_id(run_as)
Expand Down
6 changes: 4 additions & 2 deletions lib/galaxy_test/base/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -98,8 +98,10 @@ class UsesApiTestCaseMixin:
def tearDown(self):
if os.environ.get("GALAXY_TEST_EXTERNAL") is None:
# Only kill running jobs after test for managed test instances
for job in self.galaxy_interactor.get("jobs?state=running").json():
self._delete(f"jobs/{job['id']}")
response = self.galaxy_interactor.get("jobs?state=running")
if response.ok:
for job in response.json():
self._delete(f"jobs/{job['id']}")

def _api_url(self, path, params=None, use_key=None, use_admin_key=None):
if not params:
Expand Down
Loading
Loading