From a6b346835d641ffe41a93f96431ae03c3751d587 Mon Sep 17 00:00:00 2001 From: Russ Allbery Date: Fri, 27 Oct 2023 14:23:58 -0700 Subject: [PATCH 1/9] Move generation of auth URL for Ingress Move construction of the auth URL for an Ingress created from a GafaelfawrIngress to the model, since it's a mechanical transformation of the ingress configuration. This eliminates a warning for an excessively complex method. --- src/gafaelfawr/models/kubernetes.py | 32 +++++++++++++++++++++++++++ src/gafaelfawr/services/kubernetes.py | 28 ++--------------------- 2 files changed, 34 insertions(+), 26 deletions(-) diff --git a/src/gafaelfawr/models/kubernetes.py b/src/gafaelfawr/models/kubernetes.py index df9f9a6c8..cea23839f 100644 --- a/src/gafaelfawr/models/kubernetes.py +++ b/src/gafaelfawr/models/kubernetes.py @@ -7,6 +7,7 @@ from datetime import datetime, timedelta from enum import Enum from typing import Literal, Self +from urllib.parse import urlencode from kubernetes_asyncio.client import ( V1HTTPIngressPath, @@ -296,6 +297,37 @@ def _validate_conflicts(self) -> Self: return self + def to_auth_url(self) -> str: + """Generate the auth URL corresponding to this ingress configuration. + + Returns + ------- + str + Authentication request URL for the Gafaelfawr ``/auth`` route that + corresponds to this ingress configuration. + """ + base_url = self.base_url.rstrip("/") + query = [("scope", s) for s in self.scopes.scopes] + if self.scopes.satisfy != Satisfy.ALL: + query.append(("satisfy", self.scopes.satisfy.value)) + if self.delegate: + if self.delegate.notebook: + query.append(("notebook", "true")) + elif self.delegate.internal: + service = self.delegate.internal.service + query.append(("delegate_to", service)) + scopes = ",".join(self.delegate.internal.scopes) + query.append(("delegate_scope", scopes)) + if self.delegate.minimum_lifetime: + minimum_lifetime = self.delegate.minimum_lifetime + minimum_str = str(int(minimum_lifetime.total_seconds())) + query.append(("minimum_lifetime", minimum_str)) + if self.delegate.use_authorization: + query.append(("use_authorization", "true")) + if self.auth_type: + query.append(("auth_type", self.auth_type.value)) + return f"{base_url}/auth?" + urlencode(query) + class GafaelfawrIngressMetadata(CamelCaseModel): """Metadata used to create an ``Ingress`` object.""" diff --git a/src/gafaelfawr/services/kubernetes.py b/src/gafaelfawr/services/kubernetes.py index e1725241b..9602fa900 100644 --- a/src/gafaelfawr/services/kubernetes.py +++ b/src/gafaelfawr/services/kubernetes.py @@ -3,7 +3,6 @@ from __future__ import annotations from base64 import b64decode -from urllib.parse import urlencode from kubernetes_asyncio.client import ( V1Ingress, @@ -22,7 +21,6 @@ KubernetesError, PermissionDeniedError, ) -from ..models.auth import Satisfy from ..models.kubernetes import ( GafaelfawrIngress, GafaelfawrIngressScopesBase, @@ -107,29 +105,7 @@ async def update( def _build_annotations(self, ingress: GafaelfawrIngress) -> dict[str, str]: """Build annotations for an ``Ingress``.""" - base_url = ingress.config.base_url.rstrip("/") - - query = [("scope", s) for s in ingress.config.scopes.scopes] - if ingress.config.scopes.satisfy != Satisfy.ALL: - query.append(("satisfy", ingress.config.scopes.satisfy.value)) - if ingress.config.delegate: - if ingress.config.delegate.notebook: - query.append(("notebook", "true")) - elif ingress.config.delegate.internal: - service = ingress.config.delegate.internal.service - query.append(("delegate_to", service)) - scopes = ",".join(ingress.config.delegate.internal.scopes) - query.append(("delegate_scope", scopes)) - if ingress.config.delegate.minimum_lifetime: - minimum_lifetime = ingress.config.delegate.minimum_lifetime - minimum_str = str(int(minimum_lifetime.total_seconds())) - query.append(("minimum_lifetime", minimum_str)) - if ingress.config.delegate.use_authorization: - query.append(("use_authorization", "true")) - if ingress.config.auth_type: - query.append(("auth_type", ingress.config.auth_type.value)) - auth_url = f"{base_url}/auth?" + urlencode(query) - + auth_url = ingress.config.to_auth_url() snippet_key = "nginx.ingress.kubernetes.io/configuration-snippet" snippet = ingress.template.metadata.annotations.get(snippet_key, "") if snippet and not snippet.endswith("\n"): @@ -144,7 +120,7 @@ def _build_annotations(self, ingress: GafaelfawrIngress) -> dict[str, str]: snippet_key: snippet, } if ingress.config.login_redirect: - url = f"{base_url}/login" + url = ingress.config.base_url.rstrip("/") + "/login" annotations["nginx.ingress.kubernetes.io/auth-signin"] = url return annotations From 20cfe1f6cb44605a35e3f9e0d9be1781b85779f1 Mon Sep 17 00:00:00 2001 From: Russ Allbery Date: Fri, 27 Oct 2023 14:31:06 -0700 Subject: [PATCH 2/9] Simplify get_user_info_from_token Separate the code to obtain groups from LDAP and possibly add GIDs from Firestore into a separate internal method to reduce the complexity of get_user_info_from_token in UserInfoService. --- src/gafaelfawr/services/userinfo.py | 64 ++++++++++++++++++++++------- 1 file changed, 49 insertions(+), 15 deletions(-) diff --git a/src/gafaelfawr/services/userinfo.py b/src/gafaelfawr/services/userinfo.py index fa1cdbbf1..14a9ea0a7 100644 --- a/src/gafaelfawr/services/userinfo.py +++ b/src/gafaelfawr/services/userinfo.py @@ -76,7 +76,7 @@ def __init__( self._firestore = firestore self._logger = logger - async def get_user_info_from_token( # noqa: PLR0912,C901 + async def get_user_info_from_token( self, token_data: TokenData ) -> TokenUserInfo: """Get the user information from a token. @@ -98,8 +98,7 @@ async def get_user_info_from_token( # noqa: PLR0912,C901 Raises ------ FirestoreError - UID/GID allocation using Firestore failed, probably because the UID - or GID space has been exhausted. + UID/GID allocation using Firestore failed. LDAPError Gafaelfawr was configured to get user groups, username, or numeric UID from LDAP, but the attempt failed due to some error. @@ -134,18 +133,7 @@ async def get_user_info_from_token( # noqa: PLR0912,C901 groups = token_data.groups if groups is None: - if self._firestore: - group_names = await self._ldap.get_group_names(username, gid) - groups = [] - for group_name in group_names: - try: - group_gid = await self._firestore.get_gid(group_name) - except FirestoreError as e: - e.user = username - raise - groups.append(TokenGroup(name=group_name, id=group_gid)) - else: - groups = await self._ldap.get_groups(username, gid) + groups = await self._get_groups_from_ldap(username, gid) # When adding the user private group, be careful not to change # the groups array, since it may be cached in the LDAP cache @@ -298,6 +286,52 @@ def _calculate_quota( api[service] = extra.api[service] return Quota(api=api, notebook=notebook) + async def _get_groups_from_ldap( + self, username: str, primary_gid: int | None + ) -> list[TokenGroup]: + """Get user group information from LDAP. + + Add GIDs from Firestore if configured to do so. + + Parameters + ---------- + username + Username of the user. + primary_gid + Primary GID if set. If not `None`, the user's groups will be + checked for this GID. If it's not found, search for the group with + this GID and add it to the user's group memberships. This handles + LDAP configurations where the user's primary group is represented + only by their GID and not their group memberships. + + Returns + ------- + list of TokenGroup + User's groups from LDAP. + + Raises + ------ + FirestoreError + GID allocation using Firestore failed. + LDAPError + Raised if some error occurred when searching LDAP. + """ + if not self._ldap: + raise RuntimeError("LDAP not configured") + if self._firestore: + names = await self._ldap.get_group_names(username, primary_gid) + groups = [] + for group_name in names: + try: + group_gid = await self._firestore.get_gid(group_name) + except FirestoreError as e: + e.user = username + raise + groups.append(TokenGroup(name=group_name, id=group_gid)) + return groups + else: + return await self._ldap.get_groups(username, primary_gid) + class OIDCUserInfoService(UserInfoService): """Retrieve user metadata from external systems for OIDC authentication. From 55187e58b90f99f7c54076c5318777a2d4897703 Mon Sep 17 00:00:00 2001 From: Russ Allbery Date: Fri, 27 Oct 2023 14:56:03 -0700 Subject: [PATCH 3/9] Simplify getting LDAP user data Move the construction of the attribute list to query into another method to simplify the get_data method of LDAPStorage. --- src/gafaelfawr/storage/ldap.py | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/src/gafaelfawr/storage/ldap.py b/src/gafaelfawr/storage/ldap.py index 498df91bb..57f263c77 100644 --- a/src/gafaelfawr/storage/ldap.py +++ b/src/gafaelfawr/storage/ldap.py @@ -121,7 +121,7 @@ async def get_groups( return groups - async def get_data(self, username: str) -> LDAPUserData: # noqa: C901 + async def get_data(self, username: str) -> LDAPUserData: """Get the data for an LDAP user. Parameters @@ -148,20 +148,11 @@ async def get_data(self, username: str) -> LDAPUserData: # noqa: C901 search = f"({self._config.user_search_attr}={username})" logger = self._logger.bind(ldap_search=search, user=username) - attrs = [] - if self._config.name_attr: - attrs.append(self._config.name_attr) - if self._config.email_attr: - attrs.append(self._config.email_attr) - if self._config.uid_attr: - attrs.append(self._config.uid_attr) - if self._config.gid_attr: - attrs.append(self._config.gid_attr) results = await self._query( self._config.user_base_dn, bonsai.LDAPSearchScope.ONE, search, - attrs, + self._build_user_search_attrs(), username, ) logger.debug("LDAP entries for user data", ldap_results=results) @@ -217,6 +208,19 @@ def _build_group_member_search(self, username: str) -> str: target = username return f"(&(objectClass={group_class})({member_attr}={target}))" + def _build_user_search_attrs(self) -> list[str]: + """Construct the list of user attributes to search for.""" + attrs = [] + if self._config.name_attr: + attrs.append(self._config.name_attr) + if self._config.email_attr: + attrs.append(self._config.email_attr) + if self._config.uid_attr: + attrs.append(self._config.uid_attr) + if self._config.gid_attr: + attrs.append(self._config.gid_attr) + return attrs + async def _find_group_for_gid( self, gid: int, username: str ) -> TokenGroup | None: From 66fa85709a774471199d1a2e0ffcbec1ecd54473 Mon Sep 17 00:00:00 2001 From: Russ Allbery Date: Fri, 27 Oct 2023 16:04:18 -0700 Subject: [PATCH 4/9] Remove some old handle terminology The documentation for the generic auth functions still included some old handle terminology. We now only use tokens, although they come in two forms (internal tokens and JWTs). --- src/gafaelfawr/auth.py | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/src/gafaelfawr/auth.py b/src/gafaelfawr/auth.py index 0d9e0a699..b3997f30c 100644 --- a/src/gafaelfawr/auth.py +++ b/src/gafaelfawr/auth.py @@ -277,9 +277,9 @@ def generate_unauthorized_challenge( def parse_authorization(context: RequestContext) -> str | None: - """Find a handle or token in the Authorization header. + """Find a token in the Authorization header. - Supports either ``Bearer`` or ``Basic`` authorization types. Rebinds the + Supports either ``Bearer`` or ``Basic`` authorization types. Rebinds the logging context to include the source of the token, if one is found. Parameters @@ -290,21 +290,18 @@ def parse_authorization(context: RequestContext) -> str | None: Returns ------- str or None - The handle or token if one was found, otherwise `None`. + Token if one was found, otherwise `None`. Raises ------ InvalidRequestError - If the Authorization header is malformed. + Raised if the Authorization header is malformed. Notes ----- A Basic Auth authentication string is normally a username and a password - separated by colon and then base64-encoded. Support a username of the - token (or session handle) and a password of ``x-oauth-basic``, or a - username of ``x-oauth-basic`` and a password of the token (or session - handle). If neither is the case, assume the token or session handle is - the username. + separated by colon and then base64-encoded. This method accepts a token in + either the username or the password field. """ header = context.request.headers.get("Authorization") From 41056d145afb1cc83c95f988ae5c86fb684ead03 Mon Sep 17 00:00:00 2001 From: Russ Allbery Date: Fri, 27 Oct 2023 16:04:54 -0700 Subject: [PATCH 5/9] Simplify authenticate method in dependency Move the code to find the token into a private method to simplify the authenticate method. --- src/gafaelfawr/dependencies/auth.py | 51 +++++++++++++++++++++-------- 1 file changed, 38 insertions(+), 13 deletions(-) diff --git a/src/gafaelfawr/dependencies/auth.py b/src/gafaelfawr/dependencies/auth.py index 118ebb27f..31c023ba2 100644 --- a/src/gafaelfawr/dependencies/auth.py +++ b/src/gafaelfawr/dependencies/auth.py @@ -73,7 +73,7 @@ def __init__( self.auth_type = auth_type self.ajax_forbidden = ajax_forbidden - async def authenticate( # noqa: C901 + async def authenticate( self, context: RequestContext, x_csrf_token: str | None = None ) -> TokenData: """Authenticate the request. @@ -104,19 +104,11 @@ async def authenticate( # noqa: C901 fastapi.HTTPException Raised if authentication is not provided or is not valid. """ - token = context.state.token - if token: - context.rebind_logger(token_source="cookie") + token = self._find_token(context) + + # Check CSRF if and only if the token was found in a cookie. + if context.state.token: self._verify_csrf(context, x_csrf_token) - elif not self.require_session: - try: - token_str = parse_authorization(context) - if token_str: - token = Token.from_str(token_str) - except (InvalidRequestError, InvalidTokenError) as e: - raise generate_challenge(context, self.auth_type, e) from None - if not token: - raise self._redirect_or_error(context) if self.allow_bootstrap_token: if token == context.config.bootstrap_token: @@ -150,6 +142,39 @@ async def authenticate( # noqa: C901 return data + def _find_token(self, context: RequestContext) -> Token: + """Find the authentication tomen in the request. + + Parameters + ---------- + context + The request context. + + Returns + ------- + Token + Token found in the request. + + Raises + ------ + fastapi.HTTPException + Raised if authentication is not provided or is not valid. + """ + token = context.state.token + if token: + context.rebind_logger(token_source="cookie") + return token + elif not self.require_session: + try: + token_str = parse_authorization(context) + if token_str: + return Token.from_str(token_str) + except (InvalidRequestError, InvalidTokenError) as e: + raise generate_challenge(context, self.auth_type, e) from None + + # Fall through when no token was found. + raise self._redirect_or_error(context) + def _redirect_or_error(self, context: RequestContext) -> HTTPException: """Redirect to the ``/login`` route or return a 401 error. From dc78534f1b03c04e469b529c74057a993a686d54 Mon Sep 17 00:00:00 2001 From: Russ Allbery Date: Fri, 27 Oct 2023 16:14:22 -0700 Subject: [PATCH 6/9] Simplify build_success_headers Move the code for constructing a delegated token into a separate function to simplify build_success_headers in the /auth handler. --- src/gafaelfawr/handlers/auth.py | 81 +++++++++++++++++++++------------ 1 file changed, 53 insertions(+), 28 deletions(-) diff --git a/src/gafaelfawr/handlers/auth.py b/src/gafaelfawr/handlers/auth.py index 7c10052ec..38ec38cca 100644 --- a/src/gafaelfawr/handlers/auth.py +++ b/src/gafaelfawr/handlers/auth.py @@ -344,7 +344,7 @@ async def get_anonymous( return {"status": "ok"} -async def build_success_headers( # noqa: C901 +async def build_success_headers( context: RequestContext, auth_config: AuthConfig, token_data: TokenData ) -> list[tuple[str, str]]: """Construct the headers for successful authorization. @@ -393,7 +393,54 @@ async def build_success_headers( # noqa: C901 if user_info.email: headers.append(("X-Auth-Request-Email", user_info.email)) - delegated_token = None + # Add the delegated token, if there should be one. + delegated = await build_delegated_token(context, auth_config, token_data) + if delegated: + headers.append(("X-Auth-Request-Token", delegated)) + + # If told to put the delegated token in the Authorization header, do that. + # Otherwise, strip authentication tokens from the Authorization headers of + # the incoming request and reflect the remainder back in the response. + # Always do this with the Cookie header. ingress-nginx can then be + # configured to lift those headers up into the proxy request, preventing + # the user's cookie from being passed down to the protected application. + if auth_config.use_authorization: + if delegated: + headers.append(("Authorization", f"Bearer {delegated}")) + elif "Authorization" in context.request.headers: + raw_authorizations = context.request.headers.getlist("Authorization") + authorizations = clean_authorization(raw_authorizations) + if authorizations: + headers.extend(("Authorization", v) for v in authorizations) + if "Cookie" in context.request.headers: + raw_cookies = context.request.headers.getlist("Cookie") + cookies = clean_cookies(raw_cookies) + if cookies: + headers.extend(("Cookie", v) for v in cookies) + + return headers + + +async def build_delegated_token( + context: RequestContext, auth_config: AuthConfig, token_data: TokenData +) -> str | None: + """Construct the delegated token for this request. + + Parameters + ---------- + context + Context of the incoming request. + auth_config + Configuration parameters for the authorization. + token_data + Data from the authentication token. + + Returns + ------- + str or None + Delegated token to include in the request, or `None` if none should be + included. + """ if auth_config.notebook: token_service = context.factory.create_token_service() async with context.session.begin(): @@ -402,8 +449,7 @@ async def build_success_headers( # noqa: C901 ip_address=context.ip_address, minimum_lifetime=auth_config.minimum_lifetime, ) - delegated_token = str(token) - headers.append(("X-Auth-Request-Token", delegated_token)) + return str(token) elif auth_config.delegate_to: # Delegated scopes are optional; if the authenticating token doesn't # have the scope, it's omitted from the delegated token. (To make it @@ -422,27 +468,6 @@ async def build_success_headers( # noqa: C901 ip_address=context.ip_address, minimum_lifetime=auth_config.minimum_lifetime, ) - delegated_token = str(token) - headers.append(("X-Auth-Request-Token", delegated_token)) - - # If told to put the delegated token in the Authorization header, do that. - # Otherwise, strip authentication tokens from the Authorization headers of - # the incoming request and reflect the remainder back in the response. - # Always do this with the Cookie header. ingress-nginx can then be - # configured to lift those headers up into the proxy request, preventing - # the user's cookie from being passed down to the protected application. - if auth_config.use_authorization: - if delegated_token: - headers.append(("Authorization", f"Bearer {delegated_token}")) - elif "Authorization" in context.request.headers: - raw_authorizations = context.request.headers.getlist("Authorization") - authorizations = clean_authorization(raw_authorizations) - if authorizations: - headers.extend(("Authorization", v) for v in authorizations) - if "Cookie" in context.request.headers: - raw_cookies = context.request.headers.getlist("Cookie") - cookies = clean_cookies(raw_cookies) - if cookies: - headers.extend(("Cookie", v) for v in cookies) - - return headers + return str(token) + else: + return None From 36df1b5408bf9e5cfa36ea2d982e2c71d7ff2ffa Mon Sep 17 00:00:00 2001 From: Russ Allbery Date: Fri, 27 Oct 2023 16:53:03 -0700 Subject: [PATCH 7/9] Update Ruff configuration Bump the McCabe complexity threshold slightly, since handling a return from an authentication provider should not be simplified more than it already is. Add some more exclusions from the Nublado controller source. --- pyproject.toml | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index 251c05774..97b5d23e6 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -163,10 +163,12 @@ ignore = [ "FIX002", # point of a TODO comment is that we're not ready to fix it "G004", # forbidding logging f-strings is appealing, but not our style "RET505", # disagree that omitting else always makes code more readable + "PLR0911", # often many returns is clearer and simpler style "PLR0913", # factory pattern uses constructors with many arguments "PLR2004", # too aggressive about magic values "S105", # good idea but too many false positives on non-passwords "S106", # good idea but too many false positives on non-passwords + "S107", # good idea but too many false positives on non-passwords "S603", # not going to manually mark every subprocess call as reviewed "S607", # using PATH is not a security vulnerability "SIM102", # sometimes the formatting of nested if statements is clearer @@ -227,11 +229,8 @@ builtins-ignorelist = [ fixture-parentheses = false mark-parentheses = false -[tool.ruff.pep8-naming] -classmethod-decorators = [ - "pydantic.root_validator", - "pydantic.validator", -] +[tool.ruff.mccabe] +max-complexity = 11 [tool.ruff.pydocstyle] convention = "numpy" From 72543ea50032d34a56094d1b299b07010bb330d1 Mon Sep 17 00:00:00 2001 From: Russ Allbery Date: Fri, 27 Oct 2023 16:54:03 -0700 Subject: [PATCH 8/9] Simplify code to handle login return Get handle_provider_return down to its minimal reasonable complexity by breaking te token construction into a separate method and unifying some of the error handling with a new exception. --- src/gafaelfawr/exceptions.py | 8 +++ src/gafaelfawr/handlers/login.py | 111 +++++++++++++++++++------------ 2 files changed, 76 insertions(+), 43 deletions(-) diff --git a/src/gafaelfawr/exceptions.py b/src/gafaelfawr/exceptions.py index 59424788d..0f861d10d 100644 --- a/src/gafaelfawr/exceptions.py +++ b/src/gafaelfawr/exceptions.py @@ -44,6 +44,7 @@ "MissingUsernameClaimError", "NoAvailableGidError", "NoAvailableUidError", + "NoScopesError", "NotConfiguredError", "NotFoundError", "OAuthError", @@ -152,6 +153,13 @@ def __init__(self, message: str) -> None: super().__init__(message, ErrorLocation.body, ["scopes"]) +class NoScopesError(InputValidationError): + """The user has no valid scopes and therefore cannot log in.""" + + error = "permission_denied" + status_code = status.HTTP_403_FORBIDDEN + + class NotConfiguredError(InputValidationError): """The requested operation was not configured.""" diff --git a/src/gafaelfawr/handlers/login.py b/src/gafaelfawr/handlers/login.py index 38232a958..2850122a5 100644 --- a/src/gafaelfawr/handlers/login.py +++ b/src/gafaelfawr/handlers/login.py @@ -15,11 +15,13 @@ FirestoreError, InvalidReturnURLError, LDAPError, + NoScopesError, OIDCNotEnrolledError, PermissionDeniedError, ProviderError, ProviderWebError, ) +from ..models.token import Token, TokenUserInfo from ..templates import templates router = APIRouter(route_class=SlackRouteErrorHandler) @@ -102,6 +104,8 @@ async def get_login( updated with the ``/login`` route. """ if code: + if not state: + return _error_user(context, LoginError.STATE_MISSING) return await handle_provider_return(code, state, context) else: return await redirect_to_provider(return_url, context) @@ -172,8 +176,8 @@ async def redirect_to_provider( return RedirectResponse(redirect_url) -async def handle_provider_return( # noqa: PLR0911,PLR0912,C901 - code: str, state: str | None, context: RequestContext +async def handle_provider_return( + code: str, state: str, context: RequestContext ) -> Response: """Handle the return from an external authentication provider. @@ -183,20 +187,19 @@ async def handle_provider_return( # noqa: PLR0911,PLR0912,C901 Parameters ---------- code - The authentication code from the provider. + Authentication code from the provider. state - The opaque state used to verify that this user initiated the - authentication. This can be `None`, but that will always be an - error. + Opaque state used to verify that this user initiated the + authentication. context - The context of the incoming request. + Context of the incoming request. Returns ------- fastapi.Response Either a redirect to the resource the user was trying to reach before - authentication or an HTML page with an error message if the - authentication failed. + authentication, to the login URL, or an HTML page with an error + message if the authentication failed. Raises ------ @@ -204,10 +207,6 @@ async def handle_provider_return( # noqa: PLR0911,PLR0912,C901 The authentication request is invalid or retrieving authentication information from the provider failed. """ - if not state: - return _error_user(context, LoginError.STATE_MISSING) - - # Extract details from the reply, check state, and get the return URL. if state != context.state.state: return _error_user(context, LoginError.STATE_INVALID) return_url = context.state.return_url @@ -216,10 +215,11 @@ async def handle_provider_return( # noqa: PLR0911,PLR0912,C901 context.rebind_logger(return_url=return_url) # Retrieve the user identity and authorization information based on the - # reply from the authentication provider. + # reply from the authentication provider, and construct a token. provider = context.factory.create_provider() try: user_info = await provider.create_user_info(code, state, context.state) + token = await _construct_token(context, user_info) except OIDCNotEnrolledError as e: if context.config.oidc and context.config.oidc.enrollment_url: url = context.config.oidc.enrollment_url @@ -234,41 +234,66 @@ async def handle_provider_return( # noqa: PLR0911,PLR0912,C901 return await _error_system(context, LoginError.PROVIDER_NETWORK, e) except LDAPError as e: return await _error_system(context, LoginError.LDAP_FAILED, e) + except NoScopesError as e: + await provider.logout(context.state) + return _error_user(context, LoginError.GROUPS_MISSING, str(e)) except ProviderError as e: return await _error_system(context, LoginError.PROVIDER_FAILED, e) except PermissionDeniedError as e: await provider.logout(context.state) return _error_user(context, LoginError.INVALID_USERNAME, str(e)) - # Get the scopes for this user. + # Successful login, so store the token, clear the login state, and send + # the user back to what they were doing. + context.state.token = token + context.state.state = None + context.state.return_url = None + return RedirectResponse(return_url) + + +async def _construct_token( + context: RequestContext, user_info: TokenUserInfo +) -> Token: + """Construct a token for the authenticated user. + + Parameters + ---------- + context + Context of the incoming request. + user_info + User information for the user. + + Returns + ------- + Token + Newly-minted token for the user. + + Raises + ------ + NoScopesError + Raised if the user's group memberships do not entitle them to any + scopes, and therefore they cannot log in. + PermissionDeniedError + Raised if the user's username is invalid. + """ user_info_service = context.factory.create_user_info_service() + admin_service = context.factory.create_admin_service() + token_service = context.factory.create_token_service() + + # Get the user's scopes. scopes = await user_info_service.get_scopes(user_info) if scopes is None: - await provider.logout(context.state) await user_info_service.invalidate_cache(user_info.username) msg = f"{user_info.username} is not a member of any authorized groups" - return _error_user(context, LoginError.GROUPS_MISSING, msg) + raise NoScopesError(msg) # Construct a token. - admin_service = context.factory.create_admin_service() - token_service = context.factory.create_token_service() - try: - async with context.session.begin(): - if await admin_service.is_admin(user_info.username): - scopes = sorted([*scopes, "admin:token"]) - token = await token_service.create_session_token( - user_info, scopes=scopes, ip_address=context.ip_address - ) - except PermissionDeniedError as e: - await provider.logout(context.state) - return _error_user(context, LoginError.INVALID_USERNAME, str(e)) - context.state.token = token - - # Successful login, so clear the login state and send the user back to - # what they were doing. - context.state.state = None - context.state.return_url = None - return RedirectResponse(return_url) + async with context.session.begin(): + if await admin_service.is_admin(user_info.username): + scopes = sorted([*scopes, "admin:token"]) + return await token_service.create_session_token( + user_info, scopes=scopes, ip_address=context.ip_address + ) async def _error_system( @@ -284,16 +309,16 @@ async def _error_system( Parameters ---------- context - The context of the incoming request. + Context of the incoming request. error - The type of error. + Type of error. exc - The exception representing the error. + Exception representing the error. Returns ------- fastapi.Response - The response to send back to the user. + Response to send back to the user. """ context.logger.error(error.value, error=str(exc)) slack_client = context.factory.create_slack_client() @@ -325,16 +350,16 @@ def _error_user( Parameters ---------- context - The context of the incoming request. + Context of the incoming request. error - The type of error. + Type of error. details Additional error details, if provided. Returns ------- fastapi.Response - The response to send back to the user. + Response to send back to the user. """ if details: context.logger.warning(error.value, error=details) From 0313fb17e41d652214ed07cb3ceeb22f23c1a9df Mon Sep 17 00:00:00 2001 From: Russ Allbery Date: Fri, 27 Oct 2023 17:23:12 -0700 Subject: [PATCH 9/9] Refactor token auditing Move portions of token auditing into private methods to reduce the complexity of the method. More of this could be done, but this gets the audit method below the complexity threshold, so good enough for now. --- src/gafaelfawr/services/token.py | 211 ++++++++++++++++++++----------- 1 file changed, 138 insertions(+), 73 deletions(-) diff --git a/src/gafaelfawr/services/token.py b/src/gafaelfawr/services/token.py index 9f03517f1..9de38bf9a 100644 --- a/src/gafaelfawr/services/token.py +++ b/src/gafaelfawr/services/token.py @@ -4,6 +4,7 @@ import ipaddress import re +from collections.abc import Iterable from datetime import datetime, timedelta from safir.datetime import current_datetime, format_datetime_for_logging @@ -79,9 +80,7 @@ def __init__( self._token_change_store = token_change_store self._logger = logger - async def audit( # noqa: PLR0912,PLR0915,C901 - self, *, fix: bool = False - ) -> list[str]: + async def audit(self, *, fix: bool = False) -> list[str]: """Check Gafaelfawr data stores for consistency. If any errors are found and Slack is configured, report them to Slack @@ -98,15 +97,15 @@ async def audit( # noqa: PLR0912,PLR0915,C901 list of str A list of human-readable alert messages formatted in Markdown. """ + alert: str | None alerts = [] now = current_datetime() db_tokens = { t.token: t for t in await self._token_db_store.list_with_parents() } db_token_keys = set(db_tokens.keys()) - redis_token_keys = set(await self._token_redis_store.list()) redis_tokens = {} - for key in redis_token_keys: + for key in await self._token_redis_store.list(): token_data = await self._token_redis_store.get_data_by_key(key) if token_data: redis_tokens[key] = token_data @@ -147,83 +146,22 @@ async def audit( # noqa: PLR0912,PLR0915,C901 alert += " (fixed)" alerts.append(alert) - # Check that the data matches between the database and Redis. Older + # Check that the data matches between the database and Redis. Older # versions of Gafaelfawr didn't sort the scopes in Redis, so we have # to sort them here or we get false positives with old tokens. for key in db_token_keys & redis_token_keys: db = db_tokens[key] redis = redis_tokens[key] - mismatches = [] - if db.username != redis.username: - mismatches.append("username") - if db.token_type != redis.token_type: - mismatches.append("type") - if db.scopes != sorted(redis.scopes): - # There was a bug where Redis wasn't updated when the scopes - # were changed but the database was. Redis is canonical, so - # set the database scopes to match. - if fix: - await self._token_db_store.modify(key, scopes=redis.scopes) - mismatches.append("scopes [fixed]") - else: - mismatches.append("scopes") - if db.created != redis.created: - mismatches.append("created") - if db.expires != redis.expires: - mismatches.append("expires") - if mismatches: - self._logger.warning( - "Token does not match between database and Redis", - token=key, - user=redis.username, - mismatches=mismatches, - ) - alerts.append( - f"Token `{key}` for `{redis.username}` does not match" - f' between database and Redis ({", ".join(mismatches)})' - ) - if db.parent and db.parent in db_tokens: - parent = db_tokens[db.parent] - exp = db.expires - if parent.expires and (not exp or exp > parent.expires): - self._logger.warning( - "Token expires after its parent", - token=key, - user=redis.username, - expires=exp, - parent_expires=parent.expires, - ) - alerts.append( - f"Token `{key}` for `{redis.username}` expires after" - " its parent token" - ) + parent = db_tokens.get(db.parent) if db.parent else None + alerts.extend( + await self._audit_token(key, db, redis, parent, fix=fix) + ) # Check for orphaned tokens. - for token in await self._token_db_store.list_orphaned(): - self._logger.warning( - "Token has no parent", token=token.token, user=token.username - ) - alerts.append( - f"Token `{token.token}` for `{token.username}` has no parent" - " token" - ) + alerts.extend(await self._audit_orphaned()) # Check for unknown scopes. - for token_data in redis_tokens.values(): - known_scopes = set(self._config.known_scopes.keys()) - for scope in token_data.scopes: - if scope not in known_scopes: - self._logger.warning( - "Token has unknown scope", - token=token_data.token.key, - user=token_data.username, - scope=scope, - ) - alerts.append( - f"Token `{token_data.token.key}` for" - f" `{token_data.username}` has unknown scope" - f" (`{scope}`)" - ) + alerts.extend(self._audit_unknown_scopes(redis_tokens.values())) # Return any errors. return alerts @@ -1052,6 +990,133 @@ def _check_authorization( self._logger.warning("Permission denied", error=msg) raise PermissionDeniedError(msg) + async def _audit_orphaned(self) -> list[str]: + """Audit for orphaned tokens. + + Returns + ------- + list of str + Alerts to report. + """ + alerts = [] + for token in await self._token_db_store.list_orphaned(): + key = token.token + username = token.username + msg = "Token has no parent" + self._logger.warning(msg, token=key, user=username) + alert = f"Token `{key}` for `{username}` has no parent token" + alerts.append(alert) + return alerts + + async def _audit_token( + self, + key: str, + db: TokenInfo, + redis: TokenData, + parent: TokenInfo | None, + *, + fix: bool = False, + ) -> list[str]: + """Audit a single token. + + Compares the Redis data for a token against the database data for a + token and reports and optionally fixes any issues. + + Parameters + ---------- + key + Key of the token. + db + Database version of token. + parent + Database record of parent of token, if any. + redis + Redis version of token. + fix + Whether to fix any issues found. + + Returns + ------- + str or `None` + List of alerts to report. + """ + alerts = [] + mismatches = [] + if db.username != redis.username: + mismatches.append("username") + if db.token_type != redis.token_type: + mismatches.append("type") + if db.scopes != sorted(redis.scopes): + # There was a bug where Redis wasn't updated when the scopes were + # changed but the database was. Redis is canonical, so set the + # database scopes to match. + if fix: + await self._token_db_store.modify(key, scopes=redis.scopes) + mismatches.append("scopes [fixed]") + else: + mismatches.append("scopes") + if db.created != redis.created: + mismatches.append("created") + if db.expires != redis.expires: + mismatches.append("expires") + if mismatches: + self._logger.warning( + "Token does not match between database and Redis", + token=key, + user=redis.username, + mismatches=mismatches, + ) + alerts.append( + f"Token `{key}` for `{redis.username}` does not match" + f' between database and Redis ({", ".join(mismatches)})' + ) + if parent: + exp = db.expires + if parent.expires and (not exp or exp > parent.expires): + self._logger.warning( + "Token expires after its parent", + token=key, + user=redis.username, + expires=exp, + parent_expires=parent.expires, + ) + alerts.append( + f"Token `{key}` for `{redis.username}` expires after" + " its parent token" + ) + return alerts + + def _audit_unknown_scopes(self, tokens: Iterable[TokenData]) -> list[str]: + """Audit Redis tokens for unknown scopes. + + Parameters + ---------- + tokens + List of all Redis tokens to audit. + + Returns + ------- + list of str + List of alerts to report. + """ + alerts = [] + for token_data in tokens: + known_scopes = set(self._config.known_scopes.keys()) + for scope in token_data.scopes: + if scope not in known_scopes: + self._logger.warning( + "Token has unknown scope", + token=token_data.token.key, + user=token_data.username, + scope=scope, + ) + alerts.append( + f"Token `{token_data.token.key}` for" + f" `{token_data.username}` has unknown scope" + f" (`{scope}`)" + ) + return alerts + async def _delete_one_token( self, key: str,