PXP-10541 Client credentials rotation (#1068)

.secrets.baseline

@@ -198,7 +198,16 @@
         "filename": "fence/utils.py",
         "hashed_secret": "8318df9ecda039deac9868adf1944a29a95c7114",
         "is_verified": false,
-        "line_number": 112
+        "line_number": 129
+      }
+    ],
+    "migrations/versions/a04a70296688_non_unique_client_name.py": [
+      {
+        "type": "Hex High Entropy String",
+        "filename": "migrations/versions/a04a70296688_non_unique_client_name.py",
+        "hashed_secret": "bb2372fb50034d559d2920e7229fb5879cf1be72",
+        "is_verified": false,
+        "line_number": 13
       }
     ],
     "migrations/versions/e4c7b0ab68d3_create_tables.py": [
@@ -306,20 +315,29 @@
         "line_number": 48
       }
     ],
+    "tests/migrations/test_a04a70296688.py": [
+      {
+        "type": "Hex High Entropy String",
+        "filename": "tests/migrations/test_a04a70296688.py",
+        "hashed_secret": "bb2372fb50034d559d2920e7229fb5879cf1be72",
+        "is_verified": false,
+        "line_number": 27
+      }
+    ],
     "tests/migrations/test_ea7e1b843f82.py": [
       {
         "type": "Hex High Entropy String",
         "filename": "tests/migrations/test_ea7e1b843f82.py",
         "hashed_secret": "adb1fcd33b07abf9b6a064745759accea5cb341f",
         "is_verified": false,
-        "line_number": 27
+        "line_number": 26
       },
       {
         "type": "Hex High Entropy String",
         "filename": "tests/migrations/test_ea7e1b843f82.py",
         "hashed_secret": "bb2372fb50034d559d2920e7229fb5879cf1be72",
         "is_verified": false,
-        "line_number": 44
+        "line_number": 43
       }
     ],
     "tests/ras/test_ras.py": [
@@ -337,7 +355,7 @@
         "filename": "tests/scripting/test_fence-create.py",
         "hashed_secret": "e5e9fa1ba31ecd1ae84f75caaa474f3a663f05f4",
         "is_verified": false,
-        "line_number": 264
+        "line_number": 301
       }
     ],
     "tests/test-fence-config.yaml": [
@@ -350,5 +368,5 @@
       }
     ]
   },
-  "generated_at": "2023-01-23T20:39:16Z"
+  "generated_at": "2023-01-26T20:09:03Z"
 }

README.md

@@ -552,6 +552,14 @@ Add `--append` argument to add new callback urls or allowed scopes to existing c
 fence-create client-modify --client CLIENT_NAME --urls http://localhost/api/v0/new/oauth2/authorize --append (--expires-in 30)
 ```
 
+#### Rotate client credentials
+
+Use the `client-rotate` command to receive a new set of credentials for a client. The old credentials are NOT deactivated and must be deleted or expired separately. This allows for a rotation without downtime.
+
+```bash
+fence-create client-rotate --client CLIENT_NAME (--expires-in 30)
+```
+
 #### Delete OAuth Client
 
 ```bash

bin/fence_create.py

@@ -17,6 +17,7 @@
     create_sample_data,
     delete_client_action,
     delete_expired_clients_action,
+    rotate_client_action,
     delete_users,
     delete_expired_google_access,
     cleanup_expired_ga4gh_information,
@@ -163,6 +164,14 @@ def parse_arguments():
         default=7,
     )
 
+    client_rotate = subparsers.add_parser("client-rotate")
+    client_rotate.add_argument("--client", required=True)
+    client_rotate.add_argument(
+        "--expires-in",
+        help="days until the new client credentials expire",
+        required=False,
+    )
+
     user_delete = subparsers.add_parser("user-delete")
     user_delete.add_argument("--users", required=True, nargs="+")
 
@@ -477,6 +486,8 @@ def main():
         list_client_action(DB)
     elif args.action == "client-delete-expired":
         delete_expired_clients_action(DB, args.slack_webhook, args.warning_days)
+    elif args.action == "client-rotate":
+        rotate_client_action(DB, args.client, args.expires_in)
     elif args.action == "user-delete":
         delete_users(DB, args.users)
     elif args.action == "expired-service-account-delete":

deployment/uwsgi/uwsgi.ini

@@ -22,7 +22,6 @@ plugins = python3
 vacuum = true
 pythonpath = /var/www/fence/
 pythonpath = /fence/
-pythonpath = /usr/local/lib/python3.6/site-packages/
 # poetry installs git dependencies at /usr/local/src
 pythonpath = /usr/local/src/*
 

fence/config.py

@@ -51,10 +51,14 @@ def post_process(self):
 
         # allow setting DB connection string via env var
         if os.environ.get("DB"):
-            logger.info("Found environment variable 'DB': overriding 'DB' field from config file")
+            logger.info(
+                "Found environment variable 'DB': overriding 'DB' field from config file"
+            )
             self["DB"] = os.environ["DB"]
         else:
-            logger.info("Environment variable 'DB' empty or not set: using 'DB' field from config file")
+            logger.info(
+                "Environment variable 'DB' empty or not set: using 'DB' field from config file"
+            )
 
         if "ROOT_URL" not in self._configs and "BASE_URL" in self._configs:
             url = urllib.parse.urlparse(self._configs["BASE_URL"])

fence/models.py

@@ -193,7 +193,7 @@ class Client(Base, OAuth2ClientMixin):
     client_secret = Column(String(60), unique=True, index=True, nullable=True)
 
     # human readable name
-    name = Column(String(40), unique=True, nullable=False)
+    name = Column(String(40), nullable=False)
 
     # human readable description, not required
     description = Column(String(400))

fence/resources/openid/idp_oauth2.py

@@ -188,7 +188,7 @@ def get_access_token(self, user, token_endpoint, db_session=None):
         expires = None
 
         # get refresh_token and expiration from db
-        for row in sorted(user.upstream_refresh_tokens, key=lambda row:row.expires):
+        for row in sorted(user.upstream_refresh_tokens, key=lambda row: row.expires):
             refresh_token = row.refresh_token
             expires = row.expires
 

fence/scripting/fence_create.py

@@ -57,7 +57,7 @@
 from fence.scripting.google_monitor import email_users_without_access, validation_check
 from fence.config import config
 from fence.sync.sync_users import UserSyncer
-from fence.utils import create_client, get_valid_expiration
+from fence.utils import create_client, get_valid_expiration, generate_client_credentials
 
 from gen3authz.client.arborist.client import ArboristClient
 
@@ -92,43 +92,44 @@ def modify_client_action(
     driver = SQLAlchemyDriver(DB)
     with driver.session as s:
         client_name = client
-        client = s.query(Client).filter(Client.name == client_name).first()
-        if not client:
+        clients = s.query(Client).filter(Client.name == client_name).all()
+        if not clients:
             raise Exception("client {} does not exist".format(client_name))
-        if urls:
-            if append:
-                client.redirect_uris += urls
-                logger.info("Adding {} to urls".format(urls))
-            else:
-                client.redirect_uris = urls
-                logger.info("Changing urls to {}".format(urls))
-        if delete_urls:
-            client.redirect_uris = []
-            logger.info("Deleting urls")
-        if set_auto_approve:
-            client.auto_approve = True
-            logger.info("Auto approve set to True")
-        if unset_auto_approve:
-            client.auto_approve = False
-            logger.info("Auto approve set to False")
-        if name:
-            client.name = name
-            logger.info("Updating name to {}".format(name))
-        if description:
-            client.description = description
-            logger.info("Updating description to {}".format(description))
-        if allowed_scopes:
-            if append:
-                new_scopes = client._allowed_scopes.split() + allowed_scopes
-                client._allowed_scopes = " ".join(new_scopes)
-                logger.info("Adding {} to allowed_scopes".format(allowed_scopes))
-            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
-            )
+        for client in clients:
+            if urls:
+                if append:
+                    client.redirect_uris += urls
+                    logger.info("Adding {} to urls".format(urls))
+                else:
+                    client.redirect_uris = urls
+                    logger.info("Changing urls to {}".format(urls))
+            if delete_urls:
+                client.redirect_uris = []
+                logger.info("Deleting urls")
+            if set_auto_approve:
+                client.auto_approve = True
+                logger.info("Auto approve set to True")
+            if unset_auto_approve:
+                client.auto_approve = False
+                logger.info("Auto approve set to False")
+            if name:
+                client.name = name
+                logger.info("Updating name to {}".format(name))
+            if description:
+                client.description = description
+                logger.info("Updating description to {}".format(description))
+            if allowed_scopes:
+                if append:
+                    new_scopes = client._allowed_scopes.split() + allowed_scopes
+                    client._allowed_scopes = " ".join(new_scopes)
+                    logger.info("Adding {} to allowed_scopes".format(allowed_scopes))
+                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)
@@ -276,6 +277,54 @@ def split_uris(uris):
             logger.info(nothing_to_do_msg)
 
 
+def rotate_client_action(DB, client_name, expires_in=None):
+    """
+    Rorate a client's credentials (client ID and secret). The old credentials are
+    NOT deactivated and must be deleted or expired separately. This allows for a
+    rotation without downtime.
+
+    Args:
+        DB (str): database connection string
+        client_name (str): name of the client to rotate credentials for
+        expires_in (optional): number of days until this client expires (by default, no expiration)
+
+    Returns:
+        This functions does not return anything, but it prints the new set of credentials.
+    """
+    driver = SQLAlchemyDriver(DB)
+    with driver.session as s:
+        client = s.query(Client).filter(Client.name == client_name).first()
+        if not client:
+            raise Exception("client {} does not exist".format(client_name))
+
+        # create a new row in the DB for the same client, with a new ID, secret and expiration
+        client_id, client_secret, hashed_secret = generate_client_credentials(
+            client.is_confidential
+        )
+        client = Client(
+            client_id=client_id,
+            client_secret=hashed_secret,
+            expires_in=expires_in,
+            # the rest is identical to the client being rotated
+            user=client.user,
+            redirect_uris=client.redirect_uris,
+            _allowed_scopes=client._allowed_scopes,
+            description=client.description,
+            name=client.name,
+            auto_approve=client.auto_approve,
+            grant_types=client.grant_types,
+            is_confidential=client.is_confidential,
+            token_endpoint_auth_method=client.token_endpoint_auth_method,
+        )
+        s.add(client)
+        s.commit()
+
+    res = (client_id, client_secret)
+    print(
+        f"\nSave these credentials! Fence will not save the unhashed client secret.\nclient id, client secret:\n{res}"
+    )
+
+
 def _remove_client_service_accounts(db_session, client):
     client_service_accounts = (
         db_session.query(GoogleServiceAccount)

fence/utils.py

@@ -34,6 +34,29 @@ def json_res(data):
     return flask.Response(json.dumps(data), mimetype="application/json")
 
 
+def generate_client_credentials(confidential):
+    """
+    Generate a new client ID. If the client is confidential, also generate a new client secret.
+    The unhashed secret should be returned to the user and the hashed secret should be stored
+    in the database for later use.
+
+    Args:
+        confidential (bool): true if the client is confidential, false if it is public
+
+    Returns:
+        tuple: (client ID, unhashed client secret or None, hashed client secret or None)
+    """
+    client_id = random_str(40)
+    client_secret = None
+    hashed_secret = None
+    if confidential:
+        client_secret = random_str(55)
+        hashed_secret = bcrypt.hashpw(
+            client_secret.encode("utf-8"), bcrypt.gensalt()
+        ).decode("utf-8")
+    return client_id, client_secret, hashed_secret
+
+
 def create_client(
     DB,
     username=None,
@@ -49,17 +72,10 @@ def create_client(
     allowed_scopes=None,
     expires_in=None,
 ):
-    client_id = random_str(40)
+    client_id, client_secret, hashed_secret = generate_client_credentials(confidential)
     if arborist is not None:
         arborist.create_client(client_id, policies)
     driver = SQLAlchemyDriver(DB)
-    client_secret = None
-    hashed_secret = None
-    if confidential:
-        client_secret = random_str(55)
-        hashed_secret = bcrypt.hashpw(
-            client_secret.encode("utf-8"), bcrypt.gensalt()
-        ).decode("utf-8")
     auth_method = "client_secret_basic" if confidential else "none"
 
     allowed_scopes = allowed_scopes or config["CLIENT_ALLOWED_SCOPES"]
@@ -86,6 +102,7 @@ def create_client(
             if arborist is not None:
                 arborist.delete_client(client_id)
             raise Exception("client {} already exists".format(name))
+
         client = Client(
             client_id=client_id,
             client_secret=hashed_secret,