From 254dac3f467a6ade344be59075bd58b0d2e96cfd Mon Sep 17 00:00:00 2001 From: Pauline Ribeyre <4224001+paulineribeyre@users.noreply.github.com> Date: Wed, 30 Nov 2022 14:31:58 -0600 Subject: [PATCH] PXP-10250 PXP-10268 OIDC clients expliration (#1057) --- .secrets.baseline | 6 +- README.md | 25 +++- bin/fence_create.py | 24 ++++ fence/models.py | 44 ++++++- fence/scripting/fence_create.py | 109 ++++++++++++++++- fence/utils.py | 2 + tests/scripting/test_fence-create.py | 170 +++++++++++++++++++++++++++ 7 files changed, 367 insertions(+), 13 deletions(-) diff --git a/.secrets.baseline b/.secrets.baseline index 7974f4d3f..7cb5eeda5 100644 --- a/.secrets.baseline +++ b/.secrets.baseline @@ -189,7 +189,7 @@ "filename": "fence/utils.py", "hashed_secret": "8318df9ecda039deac9868adf1944a29a95c7114", "is_verified": false, - "line_number": 110 + "line_number": 112 } ], "migrations/versions/e4c7b0ab68d3_create_tables.py": [ @@ -321,7 +321,7 @@ "filename": "tests/scripting/test_fence-create.py", "hashed_secret": "e5e9fa1ba31ecd1ae84f75caaa474f3a663f05f4", "is_verified": false, - "line_number": 221 + "line_number": 264 } ], "tests/test-fence-config.yaml": [ @@ -334,5 +334,5 @@ } ] }, - "generated_at": "2022-08-12T15:25:33Z" + "generated_at": "2022-11-10T22:37:01Z" } diff --git a/README.md b/README.md index 165c5e5d7..fb4da28d3 100644 --- a/README.md +++ b/README.md @@ -491,9 +491,11 @@ WARNING: fence-create directly modifies the database in some cases and may circu As a Gen3 commons administrator, if you want to create an oauth client that skips user consent step, use the following command: ```bash -fence-create client-create --client CLIENT_NAME --urls OAUTH_REDIRECT_URL --username USERNAME --auto-approve +fence-create client-create --client CLIENT_NAME --urls OAUTH_REDIRECT_URL --username USERNAME --auto-approve (--expires-in 30) ``` +The optional `--expires-in` parameter allows specifying the number of days until this client expires. + #### Register an Implicit Oauth Client As a Gen3 commons administrator, if you want to create an implicit oauth client for a webapp: @@ -521,7 +523,7 @@ The OAuth2 Client Credentials flow is used for machine-to-machine communication As a Gen3 commons administrator, if you want to create an OAuth client for a client credentials flow: ```bash -fence-create client-create --client CLIENT_NAME --grant-types client_credentials +fence-create client-create --client CLIENT_NAME --grant-types client_credentials (--expires-in 30) ``` This command will return a client ID and client secret, which you can then use to obtain an access token: @@ -530,12 +532,12 @@ This command will return a client ID and client secret, which you can then use t curl --request POST https://FENCE_URL/oauth2/token?grant_type=client_credentials -d scope="openid user" --user CLIENT_ID:CLIENT_SECRET ``` +The optional `--expires-in` parameter allows specifying the number of *days* until this client expires. The recommendation is to rotate credentials with the `client_credentials` grant at least once a year. + NOTE: In Gen3, you can grant specific access to a client the same way you would to a user. See the [user.yaml guide](https://github.com/uc-cdis/fence/blob/master/docs/user.yaml_guide.md) for more details. NOTE: Client credentials tokens are not linked to a user. They are not supported by all Gen3 endpoints. -NOTE: The recommendation is to rotate these credentials at least once a year. Credentials expiration is not enforced at the moment but may be in the future. - #### Modify OAuth Client ```bash @@ -547,7 +549,7 @@ allowed here too. Add `--append` argument to add new callback urls or allowed scopes to existing client (instead of replacing them) using `--append --urls` or `--append --allowed-scopes` ```bash -fence-create client-modify --client CLIENT_NAME --urls http://localhost/api/v0/new/oauth2/authorize --append +fence-create client-modify --client CLIENT_NAME --urls http://localhost/api/v0/new/oauth2/authorize --append (--expires-in 30) ``` #### Delete OAuth Client @@ -557,6 +559,19 @@ fence-create client-delete --client CLIENT_NAME ``` That command should output the result of the deletion attempt. +#### Delete Expired OAuth Clients + +```bash +fence-create client-delete-expired +``` + +To post a warning in Slack about any clients that expired or are about to expire: + +```bash +fence-create client-delete-expired --slack-webhook --warning-days +``` + + #### List OAuth Clients ```bash diff --git a/bin/fence_create.py b/bin/fence_create.py index dfe3e713b..cec5b2337 100755 --- a/bin/fence_create.py +++ b/bin/fence_create.py @@ -16,6 +16,7 @@ create_google_logging_bucket, create_sample_data, delete_client_action, + delete_expired_clients_action, delete_users, delete_expired_google_access, cleanup_expired_ga4gh_information, @@ -103,6 +104,9 @@ def parse_arguments(): client_create.add_argument( "--allowed-scopes", help="which scopes are allowed for this client", nargs="+" ) + client_create.add_argument( + "--expires-in", help="days until this client expires", required=False + ) client_modify = subparsers.add_parser("client-modify") client_modify.add_argument("--client", required=True) @@ -137,12 +141,28 @@ def parse_arguments(): "previous policies will be revoked", nargs="*", ) + client_modify.add_argument( + "--expires-in", help="days until this client expires", required=False + ) client_list = subparsers.add_parser("client-list") client_delete = subparsers.add_parser("client-delete") client_delete.add_argument("--client", required=True) + client_delete_expired = subparsers.add_parser("client-delete-expired") + client_delete_expired.add_argument( + "--slack-webhook", + help="Slack webhook to post warnings when clients expired or are about to expire", + required=False, + ) + client_delete_expired.add_argument( + "--warning-days", + help="how many days before a client expires should we post a warning on Slack", + required=False, + default=7, + ) + user_delete = subparsers.add_parser("user-delete") user_delete.add_argument("--users", required=True, nargs="+") @@ -433,6 +453,7 @@ def main(): arborist=arborist, policies=args.policies, allowed_scopes=args.allowed_scopes, + expires_in=args.expires_in, ) elif args.action == "client-modify": modify_client_action( @@ -448,11 +469,14 @@ def main(): policies=args.policies, allowed_scopes=args.allowed_scopes, append=args.append, + expires_in=args.expires_in, ) elif args.action == "client-delete": delete_client_action(DB, args.client) elif args.action == "client-list": list_client_action(DB) + elif args.action == "client-delete-expired": + delete_expired_clients_action(DB, args.slack_webhook, args.warning_days) elif args.action == "user-delete": delete_users(DB, args.users) elif args.action == "expired-service-account-delete": diff --git a/fence/models.py b/fence/models.py index 985db5f7d..14f1df973 100644 --- a/fence/models.py +++ b/fence/models.py @@ -11,6 +11,8 @@ from authlib.flask.oauth2.sqla import OAuth2AuthorizationCodeMixin, OAuth2ClientMixin import bcrypt +from datetime import datetime, timedelta +import flask from sqlalchemy import ( Integer, BigInteger, @@ -51,6 +53,7 @@ from fence import logger from fence.config import config +from fence.errors import UserError def query_for_user(session, username): @@ -123,6 +126,37 @@ def get_project_to_authz_mapping(session): return output +def get_client_expires_at(expires_in, grant_types): + """ + Given an `expires_in` value (days from now), return an `expires_at` value (timestamp). + + expires_in (int/float/str): days until this client expires + grant_types (str): list of the client's grants joined by "\n" + """ + expires_at = None + + if expires_in: + try: + expires_in = float(expires_in) + assert expires_in > 0 + except (ValueError, AssertionError): + raise UserError( + f"Requested expiry must be a positive integer; instead got: {expires_in}" + ) + + # for backwards compatibility, 0 means no expiration + if expires_in != 0: + expires_at = (datetime.utcnow() + timedelta(days=expires_in)).timestamp() + + if "client_credentials" in grant_types.split("\n"): + if not expires_in or expires_in <= 0 or expires_in > 366: + logger.warning( + "Credentials with the 'client_credentials' grant which will be used externally are required to expire within 12 months. Use the `--expires-in` parameter to add an expiration." + ) + + return expires_at + + class ClientAuthType(Enum): """ List the possible types of OAuth client authentication, which are @@ -184,9 +218,11 @@ class Client(Base, OAuth2ClientMixin): _default_scopes = Column(Text) _scopes = ["compute", "storage", "user"] + expires_at = Column(Integer, nullable=False, default=0) + # note that authlib adds a response_type column which is not used here - def __init__(self, client_id, **kwargs): + def __init__(self, client_id, expires_in=0, **kwargs): """ NOTE that for authlib, the client must have an attribute ``redirect_uri`` which is a newline-delimited list of valid redirect URIs. @@ -233,6 +269,12 @@ def __init__(self, client_id, **kwargs): "redirect_uri" ), "Redirect URL(s) are required for the 'authorization_code' grant" + expires_at = get_client_expires_at( + expires_in=expires_in, grant_types=kwargs["grant_type"] + ) + if expires_at: + kwargs["expires_at"] = expires_at + super(Client, self).__init__(client_id=client_id, **kwargs) @property diff --git a/fence/scripting/fence_create.py b/fence/scripting/fence_create.py index 8f8b1f839..55024b082 100644 --- a/fence/scripting/fence_create.py +++ b/fence/scripting/fence_create.py @@ -1,5 +1,7 @@ +from datetime import datetime, timedelta import os import os.path +import requests import time from yaml import safe_load import json @@ -50,6 +52,7 @@ ServiceAccountToGoogleBucketAccessGroup, query_for_user, GA4GHVisaV1, + get_client_expires_at, ) from fence.scripting.google_monitor import email_users_without_access, validation_check from fence.config import config @@ -84,12 +87,14 @@ def modify_client_action( policies=None, allowed_scopes=None, append=False, + expires_in=None, ): driver = SQLAlchemyDriver(DB) with driver.session as s: - client = s.query(Client).filter(Client.name == client).first() + client_name = client + client = s.query(Client).filter(Client.name == client_name).first() if not client: - raise Exception("client {} does not exist".format(client)) + raise Exception("client {} does not exist".format(client_name)) if urls: if append: client.redirect_uris += urls @@ -120,13 +125,23 @@ def modify_client_action( else: client._allowed_scopes = " ".join(allowed_scopes) logger.info("Updating allowed_scopes to {}".format(allowed_scopes)) + if expires_in: + client.expires_at = get_client_expires_at( + expires_in=expires_in, grant_types=client.grant_type + ) s.commit() if arborist is not None and policies: arborist.update_client(client.client_id, policies) def create_client_action( - DB, username=None, client=None, urls=None, auto_approve=False, **kwargs + DB, + username=None, + client=None, + urls=None, + auto_approve=False, + expires_in=None, + **kwargs, ): print("\nSave these credentials! Fence will not save the unhashed client secret.") res = create_client( @@ -135,6 +150,7 @@ def create_client_action( urls=urls, name=client, auto_approve=auto_approve, + expires_in=expires_in, **kwargs, ) print("client id, client secret:") @@ -170,11 +186,96 @@ def delete_client_action(DB, client_name): current_session.delete(client) current_session.commit() - logger.info("Client {} deleted".format(client_name)) + logger.info("Client '{}' deleted".format(client_name)) except Exception as e: logger.error(str(e)) +def delete_expired_clients_action(DB, slack_webhook=None, warning_days=None): + """ + Args: + slack_webhook (str): Slack webhook to post warnings when clients expired or are about to expire + warning_days (int): how many days before a client expires should we post a warning on + Slack (default: 7) + """ + try: + cirrus_config.update(**config["CIRRUS_CFG"]) + except AttributeError: + # no cirrus config, continue anyway. we don't have client service accounts + # to delete + pass + + def split_uris(uris): + if not uris: + return uris + return uris.split("\n") + + now = datetime.utcnow().timestamp() + driver = SQLAlchemyDriver(DB) + expired_messages = ["Some expired OIDC clients have been deleted!"] + with driver.session as current_session: + clients = ( + current_session.query(Client) + # for backwards compatibility, 0 means no expiration + .filter(Client.expires_at != 0) + .filter(Client.expires_at <= now) + .all() + ) + + for client in clients: + expired_messages.append( + f"Client '{client.name}' (ID '{client.client_id}') expired at {datetime.fromtimestamp(client.expires_at)} UTC. Redirect URIs: {split_uris(client.redirect_uri)})" + ) + _remove_client_service_accounts(current_session, client) + current_session.delete(client) + current_session.commit() + + # get the clients that are expiring soon + warning_days = float(warning_days) if warning_days else 7 + warning_days_in_secs = warning_days * 24 * 60 * 60 # days to seconds + warning_expiry = ( + datetime.utcnow() + timedelta(seconds=warning_days_in_secs) + ).timestamp() + expiring_clients = ( + current_session.query(Client) + .filter(Client.expires_at != 0) + .filter(Client.expires_at <= warning_expiry) + .all() + ) + + expiring_messages = ["Some OIDC clients are expiring soon!"] + expiring_messages.extend( + [ + f"Client '{client.name}' (ID '{client.client_id}') expires at {datetime.fromtimestamp(client.expires_at)} UTC. Redirect URIs: {split_uris(client.redirect_uri)}" + for client in expiring_clients + ] + ) + + for post_msgs, nothing_to_do_msg in ( + (expired_messages, "No expired clients to delete"), + (expiring_messages, "No clients are close to expiring"), + ): + if len(post_msgs) > 1: + for e in post_msgs: + logger.info(e) + if slack_webhook: # post a warning on Slack + logger.info("Posting to Slack...") + payload = { + "attachments": [ + { + "fallback": post_msgs[0], + "title": post_msgs[0], + "text": "\n- " + "\n- ".join(post_msgs[1:]), + "color": "#FF5F15", + } + ] + } + resp = requests.post(slack_webhook, json=payload) + resp.raise_for_status() + else: + logger.info(nothing_to_do_msg) + + def _remove_client_service_accounts(db_session, client): client_service_accounts = ( db_session.query(GoogleServiceAccount) diff --git a/fence/utils.py b/fence/utils.py index 89be6b933..28fb995e9 100644 --- a/fence/utils.py +++ b/fence/utils.py @@ -47,6 +47,7 @@ def create_client( arborist=None, policies=None, allowed_scopes=None, + expires_in=None, ): client_id = random_str(40) if arborist is not None: @@ -97,6 +98,7 @@ def create_client( grant_types=grant_types, is_confidential=confidential, token_endpoint_auth_method=auth_method, + expires_in=expires_in, ) s.add(client) s.commit() diff --git a/tests/scripting/test_fence-create.py b/tests/scripting/test_fence-create.py index 192d0447c..65842d9b2 100644 --- a/tests/scripting/test_fence-create.py +++ b/tests/scripting/test_fence-create.py @@ -1,3 +1,4 @@ +from datetime import datetime, timedelta import time import mock @@ -10,6 +11,7 @@ from userdatamodel.driver import SQLAlchemyDriver from fence.config import config +from fence.errors import UserError from fence.jwt.validate import validate_jwt from fence.utils import create_client from fence.models import ( @@ -35,6 +37,7 @@ JWTCreator, create_client_action, delete_client_action, + delete_expired_clients_action, delete_expired_service_accounts, delete_expired_google_access, link_external_bucket, @@ -66,6 +69,7 @@ def create_client_action_wrapper( username="exampleuser", urls=["https://betawebapp.example/fence", "https://webapp.example/fence"], grant_types=["authorization_code", "refresh_token", "implicit"], + expires_in=None, **kwargs, ): """ @@ -79,6 +83,7 @@ def create_client_action_wrapper( username=username, urls=urls, grant_types=grant_types, + expires_in=expires_in, **kwargs, ) to_test() @@ -210,6 +215,44 @@ def to_test(): ) +@pytest.mark.parametrize("expires_in", [None, 0, 1000, 0.5, -10, "not-valid"]) +@pytest.mark.parametrize("grant_type", ["authorization_code", "client_credentials"]) +def test_create_client_with_expiration(db_session, grant_type, expires_in): + """ + Test that a client can be created with a valid expiration. + """ + client_name = "client_with_expiration" + grant_types = [grant_type] + now = datetime.utcnow() + + def to_test(): + saved_client = db_session.query(Client).filter_by(name=client_name).first() + assert saved_client.grant_types == grant_types + if not expires_in: + assert saved_client.expires_at == 0 + else: + expected_expires_at = (now + timedelta(days=expires_in)).timestamp() + # allow up to 1 second variation to account for test execution + assert saved_client.expires_at <= expected_expires_at + 1 + assert saved_client.expires_at >= expected_expires_at - 1 + + if expires_in in [-10, "not-valid"]: + with pytest.raises(UserError): + create_client_action_wrapper( + to_test, + client_name=client_name, + grant_types=grant_types, + expires_in=expires_in, + ) + else: + create_client_action_wrapper( + to_test, + client_name=client_name, + grant_types=grant_types, + expires_in=expires_in, + ) + + def test_client_delete(app, db_session, cloud_manager, test_user_a): """ Test that the client delete function correctly cleans up the client's @@ -294,6 +337,93 @@ def test_client_delete_error(app, db_session, cloud_manager, test_user_a): assert len(client_service_account_after) == 1 +@pytest.mark.parametrize("post_to_slack", [False, True]) +def test_client_delete_expired(app, db_session, cloud_manager, post_to_slack): + """ + Test that the expired clients are correctly deleted along with their service accounts. + Clients with "None" or "0" expiration do not expire. + """ + # create a set of clients with different expirations + user = User(username="client_user") + for i, expires_in in enumerate([0.0000001, 0.000005, 1, 1000, None, 0]): + client = Client( + client_id=f"test_client_id_{i}", + client_secret=f"secret_{i}", + name=f"test_client_{i}", + user=user, + redirect_uris=["localhost", "other-uri"], + expires_in=expires_in, + ) + db_session.add(client) + db_session.commit() + + # create a service account for one of the clients that will be removed + client_service_account = GoogleServiceAccount( + google_unique_id="jf09238ufposijf", + client_id="test_client_id_0", + user_id=user.id, + google_project_id="test", + email="someemail@something.com", + ) + db_session.add(client_service_account) + db_session.commit() + + # empty return means success + cloud_manager.return_value.__enter__.return_value.delete_service_account.return_value = ( + {} + ) + + # wait 1 second for the clients to expire + time.sleep(1) + + requests_mocker = mock.patch( + "fence.scripting.fence_create.requests", new_callable=mock.Mock + ) + with requests_mocker as mocked_requests: + # delete the expired clients + if not post_to_slack: + delete_expired_clients_action(config["DB"]) + else: + slack_webhook = "test-webhook" + delete_expired_clients_action( + config["DB"], slack_webhook=slack_webhook, warning_days=2 + ) + calls = mocked_requests.post.call_args_list + assert ( + len(calls) == 2 + ), f"Expected 2 Slack webhook calls, but got {len(calls)}." + + # check the call about clients that have expired + args, kwargs = calls[0] + assert len(args) == 1 and args[0] == slack_webhook + msg = kwargs.get("json", {}).get("attachments", [{}])[0].get("text") + assert "test_client_0" in msg + assert "test_client_1" in msg + + # check the call about clients that expire soon + args, kwargs = calls[1] + assert len(args) == 1 and args[0] == slack_webhook + msg = kwargs.get("json", {}).get("attachments", [{}])[0].get("text") + assert "test_client_2" in msg + + # make sure expired clients are deleted + clients_after = db_session.query(Client).all() + assert sorted([c.name for c in clients_after]) == [ + "test_client_2", + "test_client_3", + "test_client_4", + "test_client_5", + ] + + # make sure the service account for the expired client are deleted + client_sa_after = ( + db_session.query(GoogleServiceAccount) + .filter_by(client_id="test_client_id_0") + .all() + ) + assert len(client_sa_after) == 0 + + def test_delete_users(app, db_session, example_usernames): """ Test the basic functionality of ``delete_users``. @@ -1535,3 +1665,43 @@ def test_modify_client_action_modify_append_url(db_session): assert client.name == "test321" assert client.description == "test client" assert client.redirect_uris == ["abcd", "test1", "test2", "test3"] + + +@pytest.mark.parametrize("expires_in", [None, 0, 1000, 0.5, -10, "not-valid"]) +@pytest.mark.parametrize("existing_expiration", [True, False]) +def test_modify_client_expiration(db_session, expires_in, existing_expiration): + """ + Test that a client can be modified with a valid expiration. + """ + # create a client + client_name = "test_client" + client = Client( + client_id="test_client_id", + client_secret="secret", + name=client_name, + user=User(username="client_user"), + redirect_uris="localhost", + expires_in=(2 if existing_expiration else None), + ) + db_session.add(client) + db_session.commit() + original_expires_at = client.expires_at + + # modify the client's expiration + now = datetime.utcnow() + if expires_in in [-10, "not-valid"]: + with pytest.raises(UserError): + modify_client_action( + DB=db_session, client=client_name, expires_in=expires_in + ) + else: + modify_client_action(DB=db_session, client=client_name, expires_in=expires_in) + + # make sure the expiration was updated if necessary + if not expires_in: + assert client.expires_at == original_expires_at + else: + expected_expires_at = (now + timedelta(days=expires_in)).timestamp() + # allow up to 1 second variation to account for test execution + assert client.expires_at <= expected_expires_at + 1 + assert client.expires_at >= expected_expires_at - 1