Merge pull request #359 from TeskaLabs/feature/client-secret

Client secret management

.github/workflows/ci.yml

@@ -58,6 +58,7 @@ jobs:
         pip install bson
         pip install pymongo
         pip install jwcrypto
+        pip install passlib
         pip install git+https://github.com/TeskaLabs/asab.git#egg=asab[encryption]
     
     - name: Test with unittest

CHANGELOG.md

@@ -3,6 +3,7 @@
 ## v24.06
 
 ### Pre-releases
+- `v24.06-alpha11`
 - `v24.06-alpha10`
 - `v24.06-beta3`
 - `v24.06-alpha9`
@@ -29,6 +30,7 @@
 - Fix the initialization of NoTenantsError (#346, `v24.06-alpha2`)
 
 ### Features
+- Client secret management (#359, `v24.06-alpha11`)
 - External login provider label contains just the display name (#352, `v24.06-alpha10`)
 - ElasticSearch index and Kibana space authorization (#353, `v24.06-alpha7.2`)
 - Disable special characters in tenant ID (#349, `v24.06-alpha6`)

Dockerfile

@@ -37,6 +37,7 @@ RUN apk add --no-cache  \
     fastjsonschema \
     passlib \
     bcrypt \
+    argon2_cffi \
     python-ldap \
     aiomysql \
     jinja2 \

seacatauth/client/handler.py

@@ -5,7 +5,7 @@
 import asab.exceptions
 
 from seacatauth.decorators import access_control
-from .service import REGISTER_CLIENT_SCHEMA, UPDATE_CLIENT_SCHEMA, CLIENT_TEMPLATES
+from .service import REGISTER_CLIENT_SCHEMA, UPDATE_CLIENT_SCHEMA, CLIENT_TEMPLATES, is_client_confidential
 
 #
 
@@ -84,12 +84,8 @@ async def get(self, request):
 		Get client by client_id
 		"""
 		client_id = request.match_info["client_id"]
-		result = self._rest_normalize(
-			await self.ClientService.get(client_id),
-			include_client_secret=True)
-		return asab.web.rest.json_response(
-			request, result
-		)
+		result = self._rest_normalize(await self.ClientService.get(client_id))
+		return asab.web.rest.json_response(request, result)
 
 
 	@access_control("seacat:client:access")
@@ -120,11 +116,18 @@ async def register(self, request, *, json_data):
 			if not self.ClientService._AllowCustomClientID:
 				raise asab.exceptions.ValidationError("Specifying custom client_id is not allowed.")
 			json_data["_custom_client_id"] = json_data.pop("preferred_client_id")
-		result = await self.ClientService.register(**json_data)
-		data = self._rest_normalize(
-			await self.ClientService.get(result["client_id"]),
-			include_client_secret=True)
-		return asab.web.rest.json_response(request, data=data)
+		client_id = await self.ClientService.register(**json_data)
+		client = await self.ClientService.get(client_id)
+		response_data = self._rest_normalize(client)
+
+		if is_client_confidential(client):
+			# Set a secret for confidential client
+			client_secret, client_secret_expires_at = await self.ClientService.reset_secret(client_id)
+			response_data["client_secret"] = client_secret
+			if client_secret_expires_at:
+				response_data["client_secret_expires_at"] = client_secret_expires_at
+
+		return asab.web.rest.json_response(request, data=response_data)
 
 
 	@asab.web.rest.json_schema_handler(UPDATE_CLIENT_SCHEMA)
@@ -151,10 +154,13 @@ async def reset_secret(self, request):
 		Reset client secret
 		"""
 		client_id = request.match_info["client_id"]
-		response = await self.ClientService.reset_secret(client_id)
+		client_secret, client_secret_expires_at = await self.ClientService.reset_secret(client_id)
+		response_data = {"client_secret": client_secret}
+		if client_secret_expires_at:
+			response_data["client_secret_expires_at"] = client_secret_expires_at
 		return asab.web.rest.json_response(
 			request,
-			data=response,
+			data=response_data,
 		)
 
 
@@ -171,7 +177,7 @@ async def delete(self, request):
 		)
 
 
-	def _rest_normalize(self, client: dict, include_client_secret: bool = False):
+	def _rest_normalize(self, client: dict):
 		cookie_service = self.ClientService.App.get_service("seacatauth.CookieService")
 
 		rest_data = {
@@ -181,8 +187,8 @@ def _rest_normalize(self, client: dict, include_client_secret: bool = False):
 		}
 		rest_data["client_id"] = rest_data["_id"]
 		rest_data["client_id_issued_at"] = int(rest_data["_c"].timestamp())
-		if include_client_secret and "__client_secret" in client:
-			rest_data["client_secret"] = client["__client_secret"]
+		if "__client_secret" in client:
+			rest_data["client_secret"] = True
 			if "client_secret_expires_at" in rest_data:
 				rest_data["client_secret_expires_at"] = int(rest_data["client_secret_expires_at"].timestamp())
 		rest_data["cookie_name"] = cookie_service.get_cookie_name(rest_data["_id"])

seacatauth/client/service.py

@@ -3,6 +3,7 @@
 import re
 import secrets
 import urllib.parse
+import passlib.hash
 
 import asab.storage.exceptions
 import asab.exceptions
@@ -280,9 +281,7 @@ async def count(self, query_filter: dict = None):
 
 	async def get(self, client_id: str):
 		cookie_svc = self.App.get_service("seacatauth.CookieService")
-		client = await self.StorageService.get(self.ClientCollection, client_id, decrypt=["__client_secret"])
-		if "__client_secret" in client:
-			client["__client_secret"] = client["__client_secret"].decode("ascii")
+		client = await self.StorageService.get(self.ClientCollection, client_id)
 		client["cookie_name"] = cookie_svc.get_cookie_name(client_id)
 		return client
 
@@ -303,61 +302,27 @@ async def register(
 		:type _custom_client_id: str
 		:return: Dict containing the issued client_id and client_secret.
 		"""
-		response_types = kwargs.get("response_types", frozenset(["code"]))
+		response_types = kwargs.get("response_types", {"code"})
 		for v in response_types:
 			assert v in RESPONSE_TYPES
 
-		grant_types = kwargs.get("grant_types", frozenset(["authorization_code"]))
+		grant_types = kwargs.get("grant_types", {"authorization_code"})
 		for v in grant_types:
 			assert v in GRANT_TYPES
 
 		application_type = kwargs.get("application_type", "web")
 		assert application_type in APPLICATION_TYPES
 
-		token_endpoint_auth_method = kwargs.get("token_endpoint_auth_method", "none")
-		# TODO: The default should be "client_secret_basic". Change this once implemented.
-		assert token_endpoint_auth_method in TOKEN_ENDPOINT_AUTH_METHODS
-
 		if _custom_client_id is not None:
 			client_id = _custom_client_id
-			L.warning("Creating a client with custom ID", struct_data={"client_id": client_id})
+			L.warning("Creating a client with custom ID.", struct_data={"client_id": client_id})
 		else:
 			client_id = secrets.token_urlsafe(self.ClientIdLength)
 		upsertor = self.StorageService.upsertor(self.ClientCollection, obj_id=client_id)
 
-		if token_endpoint_auth_method == "none":
-			# The client is PUBLIC
-			# Clients incapable of maintaining the confidentiality of their
-			# credentials (e.g., clients executing on the device used by the
-			# resource owner, such as an installed native application or a web
-			# browser-based application), and incapable of secure client
-			# authentication via any other means.
-			# The authorization server MAY establish a client authentication method
-			# with public clients. However, the authorization server MUST NOT rely
-			# on public client authentication for the purpose of identifying the
-			# client.
-			client_secret = None
-			client_secret_expires_at = None
-		elif token_endpoint_auth_method == "client_secret_basic":
-			raise NotImplementedError("Token endpoint auth method 'client_secret_basic' is not supported.")
-			# TODO: Finish implementing authorization with client secret
-			# The client is CONFIDENTIAL
-			# Clients capable of maintaining the confidentiality of their
-			# credentials (e.g., client implemented on a secure server with
-			# restricted access to the client credentials), or capable of secure
-			# client authentication using other means.
-			# Confidential clients are typically issued (or establish) a set of
-			# client credentials used for authenticating with the authorization
-			# server (e.g., password, public/private key pair).
-			client_secret, client_secret_expires_at = self._generate_client_secret()
-			upsertor.set("__client_secret", client_secret.encode("ascii"), encrypt=True)
-			if client_secret_expires_at is not None:
-				upsertor.set("client_secret_expires_at", client_secret_expires_at)
-		else:
-			# The client is CONFIDENTIAL
-			# Valid method type, not implemented yet
-			raise NotImplementedError("token_endpoint_auth_method = {!r}".format(token_endpoint_auth_method))
-
+		# TODO: The default should be "client_secret_basic".
+		token_endpoint_auth_method = kwargs.get("token_endpoint_auth_method", "none")
+		assert token_endpoint_auth_method in TOKEN_ENDPOINT_AUTH_METHODS
 		upsertor.set("token_endpoint_auth_method", token_endpoint_auth_method)
 
 		self._check_redirect_uris(redirect_uris, application_type, grant_types)
@@ -385,10 +350,11 @@ async def register(
 			upsertor.set("session_expiration", session_expiration)
 
 		# Optional client metadata
-		for k in frozenset([
+		for k in {
 			"client_name", "client_uri", "logout_uri", "cookie_domain", "custom_data", "login_uri",
 			"authorize_anonymous_users", "authorize_uri", "cookie_webhook_uri", "cookie_entry_uri",
-			"anonymous_cid"]):
+			"anonymous_cid"
+		}:
 			v = kwargs.get(k)
 			if v is not None and not (isinstance(v, str) and len(v) == 0):
 				upsertor.set(k, v)
@@ -398,40 +364,26 @@ async def register(
 		except asab.storage.exceptions.DuplicateError:
 			raise asab.exceptions.Conflict(key="client_id", value=client_id)
 
-		L.log(asab.LOG_NOTICE, "Client created", struct_data={"client_id": client_id})
-
-		response = {
-			"client_id": client_id,
-			"client_id_issued_at": int(datetime.datetime.now(datetime.timezone.utc).timestamp())}
-
-		if client_secret is not None:
-			response["client_secret"] = client_secret
-			if client_secret_expires_at is not None:
-				response["client_secret_expires_at"] = client_secret_expires_at
-
-		return response
+		L.log(asab.LOG_NOTICE, "Client created.", struct_data={"client_id": client_id})
+		return client_id
 
 
 	async def reset_secret(self, client_id: str):
+		"""
+		Set or reset client secret
+		"""
+		# TODO: Use M2M credentials provider.
 		client = await self.get(client_id)
-		if client["token_endpoint_auth_method"] == "none":
-			# The authorization server MAY establish a client authentication method with public clients.
-			# However, the authorization server MUST NOT rely on public client authentication for the purpose
-			# of identifying the client. [rfc6749#section-3.1.2]
-			raise asab.exceptions.ValidationError("Cannot set secret for public client")
 		upsertor = self.StorageService.upsertor(self.ClientCollection, obj_id=client_id, version=client["_v"])
 		client_secret, client_secret_expires_at = self._generate_client_secret()
-		upsertor.set("__client_secret", client_secret.encode("ascii"), encrypt=True)
+		client_secret_hash = passlib.hash.argon2.hash(client_secret)
+		upsertor.set("__client_secret", client_secret_hash)
 		if client_secret_expires_at is not None:
 			upsertor.set("client_secret_expires_at", client_secret_expires_at)
-		await upsertor.execute(event_type=EventTypes.CLIENT_SECRET_RESET)
-		L.log(asab.LOG_NOTICE, "Client secret updated", struct_data={"client_id": client_id})
 
-		response = {"client_secret": client_secret}
-		if client_secret_expires_at is not None:
-			response["client_secret_expires_at"] = client_secret_expires_at
-
-		return response
+		await upsertor.execute(event_type=EventTypes.CLIENT_SECRET_RESET)
+		L.log(asab.LOG_NOTICE, "Client secret updated.", struct_data={"client_id": client_id})
+		return client_secret, client_secret_expires_at
 
 
 	async def update(self, client_id: str, **kwargs):
@@ -471,14 +423,15 @@ async def update(self, client_id: str, **kwargs):
 				upsertor.set(k, v)
 
 		await upsertor.execute(event_type=EventTypes.CLIENT_UPDATED)
-		L.log(asab.LOG_NOTICE, "Client updated", struct_data={
+		L.log(asab.LOG_NOTICE, "Client updated.", struct_data={
 			"client_id": client_id,
-			"fields": " ".join(client_update.keys())})
+			"fields": " ".join(client_update.keys())
+		})
 
 
 	async def delete(self, client_id: str):
 		await self.StorageService.delete(self.ClientCollection, client_id)
-		L.log(asab.LOG_NOTICE, "Client deleted", struct_data={"client_id": client_id})
+		L.log(asab.LOG_NOTICE, "Client deleted.", struct_data={"client_id": client_id})
 
 
 	async def validate_client_authorize_options(
@@ -504,22 +457,39 @@ async def validate_client_authorize_options(
 		return True
 
 
-	async def authenticate_client(self, client: dict, client_secret: str = None):
+	async def authenticate_client(self, client: dict, client_id: str, client_secret: str = None):
 		"""
-		Verify client credentials (client_id and client_secret).
+		Verify client ID and secret.
 		"""
-		# TODO: Use a client credential provider.
+		if client_id != client["_id"]:
+			raise exceptions.ClientError(
+				"Client IDs do not match.",
+				client_id=client["_id"],
+				attempted_client_id=client_id
+			)
 		if client_secret is None:
 			# The client MAY omit the parameter if the client secret is an empty string.
 			# [rfc6749#section-2.3.1]
 			client_secret = ""
+
+		client_secret_hash = client.get("__client_secret")
+		if not client_secret_hash:
+			# No client secret is set, none is expected!
+			if not client_secret:
+				return True
+			else:
+				raise exceptions.ClientError(
+					"Got non-empty secret from non-confidential client.", client_id=client["_id"])
+
 		if "client_secret_expires_at" in client \
 			and client["client_secret_expires_at"] != 0 \
 			and client["client_secret_expires_at"] < datetime.datetime.now(datetime.timezone.utc):
-			L.warning("Client secret expired.", struct_data={"client_id": client["_id"]})
-		if client_secret != client.get("__client_secret", ""):
-			L.warning("Incorrect client secret.", struct_data={"client_id": client["_id"]})
-		return True
+			raise exceptions.ClientError("Expired client secret.", client_id=client["_id"])
+
+		if not passlib.hash.argon2.verify(client_secret, client_secret_hash):
+			raise exceptions.ClientError("Incorrect client secret.", client_id=client["_id"])
+		else:
+			return True
 
 
 	def _check_grant_types(self, grant_types, response_types):
@@ -622,3 +592,13 @@ def validate_redirect_uri(redirect_uri: str, registered_uris: list, validation_m
 		raise ValueError("Unsupported redirect_uri_validation_method: {!r}".format(validation_method))
 
 	return False
+
+
+def is_client_confidential(client: dict):
+	token_endpoint_auth_method = client.get("token_endpoint_auth_method", "none")
+	if token_endpoint_auth_method == "none":
+		return False
+	elif token_endpoint_auth_method in {"client_secret_basic", "client_secret_post"}:
+		return True
+	else:
+		raise NotImplementedError("Unsupported token_endpoint_auth_method: {!r}".format(token_endpoint_auth_method))