Feat/secure api revocation route (#2015)

* feat(api key revoke): add new auth type to protect revocation route * feat(api key revoke): protect api key revocation route with new auth * test(api key revoke): add scenarios to test for happy path and missing data, invalid auth * cypress(api key revocation): create a key using admin auth and then revoke it using the API with SRE auth * chore: formatting * chore: remove unused var * chore: remove unused import * chore: remove unused vars * task: add SRE_CLIENT_SECRET to list of sensitive keys, add var to example * chore: add default value for new env var * test(api key revoke): ensure method cannot be called using admin auth * chore: set cypress default environment back to staging * chore: update comments and example to make them clearer * chore: fix comment --------- Co-authored-by: Jumana B <[email protected]>
cds-snc · Nov 15, 2023 · 9cf3404 · 9cf3404
1 parent c3df334
commit 9cf3404
Show file tree

Hide file tree

Showing 9 changed files with 264 additions and 52 deletions.
diff --git a/.env.example b/.env.example
@@ -1,6 +1,7 @@
 NOTIFY_ENVIRONMENT=development
 
 ADMIN_CLIENT_SECRET=dev-notify-secret-key
+SRE_CLIENT_SECRET=dev-notify-secret-key
 SECRET_KEY=dev-notify-secret-key
 DANGEROUS_SALT=dev-notify-salt
 

diff --git a/app/__init__.py b/app/__init__.py
@@ -169,11 +169,12 @@ def register_notify_blueprint(application, blueprint, auth_function, prefix=None
 
 def register_blueprint(application):
     from app.accept_invite.rest import accept_invite
-    from app.api_key.rest import api_key_blueprint
+    from app.api_key.rest import api_key_blueprint, sre_tools_blueprint
     from app.authentication.auth import (
         requires_admin_auth,
         requires_auth,
         requires_no_auth,
+        requires_sre_auth,
     )
     from app.billing.rest import billing_blueprint
     from app.complaint.complaint_rest import complaint_blueprint
@@ -233,6 +234,8 @@ def register_blueprint(application):
 
     register_notify_blueprint(application, api_key_blueprint, requires_admin_auth, "/api-key")
 
+    register_notify_blueprint(application, sre_tools_blueprint, requires_sre_auth, "/sre-tools")
+
     register_notify_blueprint(application, letter_job, requires_admin_auth)
 
     register_notify_blueprint(application, letter_callback_blueprint, requires_no_auth)

diff --git a/app/api_key/rest.py b/app/api_key/rest.py
@@ -19,6 +19,9 @@
 api_key_blueprint = Blueprint("api_key", __name__)
 register_errors(api_key_blueprint)
 
+sre_tools_blueprint = Blueprint("sre_tools", __name__)
+register_errors(sre_tools_blueprint)
+
 
 @api_key_blueprint.route("/<uuid:api_key_id>/summary-statistics", methods=["GET"])
 def get_api_key_stats(api_key_id):
@@ -103,58 +106,68 @@ def send_api_key_revokation_email(service_id, api_key_name, api_key_information)
     return
 
 
-@api_key_blueprint.route("/revoke-api-keys", methods=["POST"])
+@sre_tools_blueprint.route("/api-key-revoke", methods=["POST"])
 def revoke_api_keys():
     """
-    We take a list of api keys and revoke them. The data is of the form:
-    [
-        {
-            "token": "NMIfyYncKcRALEXAMPLE",
-            "type": "mycompany_api_token",
-            "url": "https://github.com/octocat/Hello-World/blob/12345600b9cbe38a219f39a9941c9319b600c002/foo/bar.txt",
-            "source": "content",
-        }
-    ]
-
-    The function does 3 things:
-    1. Finds the api key by the token
-    2. Revokes the api key
+    This method accepts a single api key and revokes it. The data is of the form:
+    {
+        "token": "gcntfy-key-name-uuid-uuid",
+        "type": "mycompany_api_token",
+        "url": "https://github.com/octocat/Hello-World/blob/12345600b9cbe38a219f39a9941c9319b600c002/foo/bar.txt",
+        "source": "content",
+    }
+
+    The function does 4 things:
+    1. Finds the api key by API key itself
+    2. Revokes the API key
     3. Saves the source and url into the compromised_key_info field
-    4. Sends the service owners of the api key an email notification indicating that the key has been revoked
+    4. TODO: Sends the service owners of the api key an email notification indicating that the key has been revoked
     """
     try:
-        data = request.get_json()
+        api_key_data = request.get_json()
+        # check for correct payload
+        if (
+            isinstance(api_key_data, list)
+            or api_key_data.get("token") is None
+            or api_key_data.get("type") is None
+            or api_key_data.get("url") is None
+            or api_key_data.get("source") is None
+        ):
+            raise InvalidRequest("Invalid payload", status_code=400)
     except werkzeug.exceptions.BadRequest as errors:
         raise InvalidRequest(errors, status_code=400)
 
     # Step 1
-    for api_key_data in data:
-        try:
-            # take last 36 chars of string so that it works even if the full key is provided.
-            api_key_token = api_key_data["token"][-36:]
-            api_key = get_api_key_by_secret(api_key_token)
-        except Exception:
-            current_app.logger.error(f"API key not found for token {api_key_data['type']}")
-            continue  # skip to next api key
-
-        # Step 2
-        expire_api_key(api_key.service_id, api_key.id)
-
-        current_app.logger.info("Expired api key {} for service {}".format(api_key.id, api_key.service_id))
-
-        # Step 3
-        update_compromised_api_key_info(
-            api_key.service_id,
-            api_key.id,
-            {
-                "time_of_revocation": str(datetime.utcnow()),
-                "type": api_key_data["type"],
-                "url": api_key_data["url"],
-                "source": api_key_data["source"],
-            },
+    try:
+        # take last 36 chars of string so that it works even if the full key is provided.
+        api_key_token = api_key_data["token"][-36:]
+        api_key = get_api_key_by_secret(api_key_token)
+    except Exception:
+        current_app.logger.error(
+            "Revoke api key: API key not found for token {}".format(api_key_data["token"])
+            if api_key_data.get("token")
+            else "Revoke api key: no token provided"
         )
+        raise InvalidRequest("Invalid request", status_code=400)
+
+    # Step 2
+    expire_api_key(api_key.service_id, api_key.id)
 
-        # Step 4
-        send_api_key_revokation_email(api_key.service_id, api_key.name, api_key_data)
+    current_app.logger.info("Expired api key {} for service {}".format(api_key.id, api_key.service_id))
+
+    # Step 3
+    update_compromised_api_key_info(
+        api_key.service_id,
+        api_key.id,
+        {
+            "time_of_revocation": str(datetime.utcnow()),
+            "type": api_key_data["type"],
+            "url": api_key_data["url"],
+            "source": api_key_data["source"],
+        },
+    )
+
+    # Step 4
+    send_api_key_revokation_email(api_key.service_id, api_key.name, api_key_data)
 
     return jsonify(result="ok"), 201
diff --git a/app/authentication/auth.py b/app/authentication/auth.py
@@ -93,6 +93,21 @@ def requires_admin_auth():
         raise AuthError("Unauthorized, admin authentication token required", 401)
 
 
+def requires_sre_auth():
+    request_helper.check_proxy_header_before_request()
+
+    auth_type, auth_token = get_auth_token(request)
+    if auth_type != JWT_AUTH_TYPE:
+        raise AuthError("Invalid scheme: can only use JWT for sre authentication", 401)
+    client = __get_token_issuer(auth_token)
+
+    if client == current_app.config.get("SRE_USER_NAME"):
+        g.service_id = current_app.config.get("SRE_USER_NAME")
+        return handle_admin_key(auth_token, current_app.config.get("SRE_CLIENT_SECRET"))
+    else:
+        raise AuthError("Unauthorized, sre authentication token required", 401)
+
+
 def requires_auth():
     request_helper.check_proxy_header_before_request()
 

diff --git a/app/config.py b/app/config.py
@@ -592,6 +592,10 @@ class Config(object):
     FF_EMAIL_DAILY_LIMIT = env.bool("FF_EMAIL_DAILY_LIMIT", False)
     FF_SALESFORCE_CONTACT = env.bool("FF_SALESFORCE_CONTACT", False)
 
+    # SRE Tools auth keys
+    SRE_USER_NAME = "SRE_CLIENT_USER"
+    SRE_CLIENT_SECRET = os.getenv("SRE_CLIENT_SECRET")
+
     @classmethod
     def get_sensitive_config(cls) -> list[str]:
         "List of config keys that contain sensitive information"
@@ -612,6 +616,7 @@ def get_sensitive_config(cls) -> list[str]:
             "SALESFORCE_SECURITY_TOKEN",
             "TEMPLATE_PREVIEW_API_KEY",
             "DOCUMENT_DOWNLOAD_API_KEY",
+            "SRE_CLIENT_SECRET",
         ]
 
     @classmethod
@@ -639,6 +644,7 @@ class Development(Config):
     ADMIN_CLIENT_SECRET = os.getenv("ADMIN_CLIENT_SECRET", "dev-notify-secret-key")
     SECRET_KEY = env.list("SECRET_KEY", ["dev-notify-secret-key"])
     DANGEROUS_SALT = os.getenv("DANGEROUS_SALT", "dev-notify-salt ")
+    SRE_CLIENT_SECRET = os.getenv("SRE_CLIENT_SECRET", "dev-notify-secret-key")
 
     NOTIFY_ENVIRONMENT = "development"
     NOTIFICATION_QUEUE_PREFIX = os.getenv("NOTIFICATION_QUEUE_PREFIX", "notification-canada-ca")

diff --git a/tests/__init__.py b/tests/__init__.py
@@ -34,6 +34,14 @@ def create_authorization_header(service_id=None, key_type=KEY_TYPE_NORMAL):
     return "Authorization", "Bearer {}".format(token)
 
 
+def create_sre_authorization_header():
+    client_id = current_app.config["SRE_USER_NAME"]
+    secret = current_app.config["SRE_CLIENT_SECRET"]
+
+    token = create_jwt_token(secret=secret, client_id=client_id)
+    return "Authorization", "Bearer {}".format(token)
+
+
 def unwrap_function(fn):
     """
     Given a function, returns its undecorated original.

diff --git a/tests/app/api_key/test_rest.py b/tests/app/api_key/test_rest.py
@@ -1,8 +1,12 @@
 from datetime import datetime
 
+import pytest
+from flask import url_for
+
 from app import DATETIME_FORMAT
 from app.dao.api_key_dao import get_api_key_by_secret, get_unsigned_secret
 from app.models import KEY_TYPE_NORMAL
+from tests import create_sre_authorization_header
 from tests.app.db import (
     create_api_key,
     create_notification,
@@ -79,21 +83,81 @@ def test_get_api_keys_ranked(admin_request, notify_db, notify_db_session):
 
 
 class TestApiKeyRevocation:
-    def test_revoke_api_keys(self, admin_request, notify_db, notify_db_session):
+    def test_revoke_api_keys_with_valid_auth(self, client, notify_db, notify_db_session, mocker):
         service = create_service(service_name="Service 1")
         api_key_1 = create_api_key(service, key_type=KEY_TYPE_NORMAL, key_name="Key 1")
         unsigned_secret = get_unsigned_secret(api_key_1.id)
 
-        admin_request.post(
-            "api_key.revoke_api_keys",
-            _data=[{"token": unsigned_secret, "type": "cds-tester", "url": "https://example.com", "source": "cds-tester"}],
-            _expected_status=201,
+        sre_auth_header = create_sre_authorization_header()
+        response = client.post(
+            url_for("sre_tools.revoke_api_keys"),
+            headers=[sre_auth_header],
+            json={"token": unsigned_secret, "type": "cds-tester", "url": "https://example.com", "source": "cds-tester"},
         )
 
         # Get api key from DB
         api_key_1 = get_api_key_by_secret(api_key_1.secret)
+        assert response.status_code == 201
         assert api_key_1.expiry_date is not None
         assert api_key_1.compromised_key_info["type"] == "cds-tester"
         assert api_key_1.compromised_key_info["url"] == "https://example.com"
         assert api_key_1.compromised_key_info["source"] == "cds-tester"
         assert api_key_1.compromised_key_info["time_of_revocation"]
+
+    def test_revoke_api_keys_fails_with_no_auth(self, client, notify_db, notify_db_session, mocker):
+        service = create_service(service_name="Service 1")
+        api_key_1 = create_api_key(service, key_type=KEY_TYPE_NORMAL, key_name="Key 1")
+        unsigned_secret = get_unsigned_secret(api_key_1.id)
+
+        response = client.post(
+            url_for("sre_tools.revoke_api_keys"),
+            headers=[],
+            json={"token": unsigned_secret, "type": "cds-tester", "url": "https://example.com", "source": "cds-tester"},
+        )
+
+        assert response.status_code == 401
+
+    @pytest.mark.parametrize(
+        "payload",
+        (
+            {
+                # no token
+                "type": "cds-tester",
+                "url": "https://example.com",
+                "source": "cds-tester",
+            },
+            {
+                "token": "token",
+                # no type
+                "url": "https://example.com",
+                "source": "cds-tester",
+            },
+            {
+                "token": "token",
+                "type": "cds-tester",
+                # no url
+                "source": "cds-tester",
+            },
+            {
+                "token": "token",
+                "type": "cds-tester",
+                "url": "https://example.com",
+                # no source
+            },
+            {
+                # no anything
+            },
+            {"token": "token", "type": "cds-tester", "url": "https://example.com", "source": "cds-tester"},  # invalid token
+        ),
+    )
+    def test_revoke_api_keys_fails_with_400_missing_or_invalid_payload(
+        self, client, notify_db, notify_db_session, mocker, payload
+    ):
+        sre_auth_header = create_sre_authorization_header()
+        response = client.post(
+            url_for("sre_tools.revoke_api_keys"),
+            headers=[sre_auth_header],
+            json=payload,
+        )
+
+        assert response.status_code == 400