From d05102de6557f8e1cd66426d073187446abe73f1 Mon Sep 17 00:00:00 2001 From: Sylvia McLaughlin <85905333+sylviamclaughlin@users.noreply.github.com> Date: Wed, 22 Nov 2023 09:53:53 -0800 Subject: [PATCH] Use Notify API endpoint to automatically revoke a key (#316) * Changing logic to use Notify's endpoint * Adding new notify integration * Addng notify tests * Adding additional unti tests * Writing additional unit tests * Adding additional secrets to the github actions * Adding a timeout to the post request * Fixing up a typo * Removing the api key in the message * Adding service_id to the message plus posting to the right notifications channel * Adding the Notify ops channel id to the unit tests env.varialbes --- .github/workflows/ci_code.yml | 5 +- app/integrations/notify.py | 104 ++++++++++++++++ app/requirements.txt | 1 + app/requirements_dev.txt | 1 + app/server/event_handlers/aws.py | 96 +++++++-------- app/tests/intergrations/test_notify.py | 81 ++++++++++++ .../server/event_handlers/test_aws_handler.py | 115 +++++++++++------- 7 files changed, 306 insertions(+), 97 deletions(-) create mode 100644 app/integrations/notify.py create mode 100644 app/tests/intergrations/test_notify.py diff --git a/.github/workflows/ci_code.yml b/.github/workflows/ci_code.yml index 32e8661f..68363e50 100644 --- a/.github/workflows/ci_code.yml +++ b/.github/workflows/ci_code.yml @@ -52,4 +52,7 @@ jobs: GOOGLE_CLIENT_ID: ${{ secrets.GOOGLE_CLIENT_ID }} GOOGLE_CLIENT_SECRET: ${{ secrets.GOOGLE_CLIENT_SECRET }} SESSION_SECRET_KEY: ${{ secrets.SESSION_SECRET_KEY }} - NOTIFY_TEST_KEY: ${{ secrets.NOTIFY_TEST_KEY }} \ No newline at end of file + NOTIFY_TEST_KEY: ${{ secrets.NOTIFY_TEST_KEY }} + NOTIFY_SRE_USER_NAME: ${{ secrets.NOTIFY_SRE_USER_NAME }} + NOTIFY_SRE_CLIENT_SECRET: ${{ secrets.NOTIFY_SRE_CLIENT_SECRET }} + NOTIFY_OPS_CHANNEL_ID: ${{ secrets.NOTIFY_OPS_CHANNEL_ID }} \ No newline at end of file diff --git a/app/integrations/notify.py b/app/integrations/notify.py new file mode 100644 index 00000000..45476139 --- /dev/null +++ b/app/integrations/notify.py @@ -0,0 +1,104 @@ +import os +import jwt +import time +import calendar +import logging +import requests +import json + + +# generate the epoch seconds for the jwt token +def epoch_seconds(): + return calendar.timegm(time.gmtime()) + + +def create_jwt_token(secret, client_id): + """ + Generate a JWT Token for the Notify API + + Tokens have a header consisting of: + { + "typ": "JWT", + "alg": "HS256" + } + + Parameters: + secret: Application signing secret + client_id: Identifier for the client + + Claims are: + iss: identifier for the client + iat: epoch seconds for the token (UTC) + + Returns a JWT token for this request + """ + assert secret, "Missing secret key" + assert client_id, "Missing client id" + + headers = {"typ": "JWT", "alg": "HS256"} + + claims = {"iss": client_id, "iat": epoch_seconds()} + t = jwt.encode(payload=claims, key=secret, headers=headers) + if isinstance(t, str): + return t + else: + return t.decode() + + +# Function to create the authorization header for the Notify API +def create_authorization_header(): + # get the client_id and secret from the environment variables + client_id = os.getenv("NOTIFY_SRE_USER_NAME") + secret = os.getenv("NOTIFY_SRE_CLIENT_SECRET") + + # If the client_id or secret is missing, raise an assertion error + assert client_id, "NOTIFY_SRE_USER_NAME is missing" + assert secret, "NOTIFY_SRE_CLIENT_SECRET is missing" + + # Create the jwt token and return the authorization header + token = create_jwt_token(secret=secret, client_id=client_id) + return "Authorization", "Bearer {}".format(token) + + +# Function to post an api call to Notify +def post_event(url, payload): + # Create the authorization headers + header_key, header_value = create_authorization_header() + header = {header_key: header_value, "Content-Type": "application/json"} + + # Post the response + response = requests.post(url, data=json.dumps(payload), headers=header, timeout=60) + return response + + +# Function to revoke an api key by calling Notify's revoke api endpoint +def revoke_api_key(api_key, api_type, github_repo, source): + # get the url and jwt_token + url = os.getenv("NOTIFY_API_URL") + + if url is None: + logging.error("NOTIFY_API_URL is missing") + return False + + # append the revoke-endpoint to the url + url = url + "/sre-tools/api-key-revoke" + + # generate the payload + payload = { + "token": api_key, + "type": api_type, + "url": github_repo, + "source": source, + } + + # post the event (ie call the api) + response = post_event(url, payload) + # A successful response has a status code of 201 + if response.status_code == 201: + logging.info(f"API key {api_key} has been successfully revoked") + return True + else: + logging.error( + f"API key {api_key} could not be revoked. Response code: {response.status_code}" + ) + return False diff --git a/app/requirements.txt b/app/requirements.txt index a5d43b7d..227c32d6 100644 --- a/app/requirements.txt +++ b/app/requirements.txt @@ -12,6 +12,7 @@ google-auth-oauthlib==0.8.0 httpx==0.25.1 itsdangerous==2.1.2 Jinja2==3.1.2 +PyJWT==2.8.0 PyYAML!=6.0.0,!=5.4.0,!=5.4.1 python-dotenv==0.21.1 python-i18n==0.3.9 diff --git a/app/requirements_dev.txt b/app/requirements_dev.txt index 6705cf2d..7c39f26f 100644 --- a/app/requirements_dev.txt +++ b/app/requirements_dev.txt @@ -1,6 +1,7 @@ black==22.12.0 coverage==6.5.0 flake8==6.1.0 +freezegun==1.2.2 pytest==7.4.3 pytest-asyncio==0.21.1 pytest-env==0.8.2 diff --git a/app/server/event_handlers/aws.py b/app/server/event_handlers/aws.py index b8aa18b0..46e5c294 100644 --- a/app/server/event_handlers/aws.py +++ b/app/server/event_handlers/aws.py @@ -1,8 +1,9 @@ import json import re +import os import urllib.parse from commands.utils import log_ops_message -from integrations import google_drive, opsgenie +from integrations import notify def parse(payload, client): @@ -41,47 +42,6 @@ def nested_get(dictionary, keys): return dictionary -def alert_on_call(product, client, api_key_name, github_repo): - # get the list of folders - folders = google_drive.list_folders() - # get the folder id for the Product - for folder in folders: - if folder["name"] == product: - folder = folder["id"] - break - # Get folder metadata - folder_metadata = google_drive.list_metadata(folder).get("appProperties", {}) - oncall = [] - message = "" - private_message = "" - - # Generate the opsgenie message. We don't want to expose the api key value exposed so we will just provide the name of the key. - opsgenie_message = f"Notify key with name {api_key_name} has been leaked and needs to be revoked. Please check Slack in #internal-sre-alerts or on-call staff can check your private messages for more detailed information. " - - # Get OpsGenie users on call and construct string - if "genie_schedule" in folder_metadata: - for email in opsgenie.get_on_call_users(folder_metadata["genie_schedule"]): - r = client.users_lookupByEmail(email=email) - if r.get("ok"): - oncall.append(r["user"]) - message = f"{product} on-call staff " - for user in oncall: - # send a private message to the people on call. - message += f"<@{user['id']}> " - private_message = f"Hello {user['profile']['first_name']}!\nA Notify API key has been leaked and needs to be revoked. 🙀 \nThe key name is *{api_key_name}* and it is exposed in file {github_repo}. You can see the message in #internal-sre-alerts to start an incident." - # send the private message - client.chat_postMessage( - channel=user["id"], text=private_message, as_user=True - ) - message += "have been notified." - - # create an alert in OpsGenie - result = opsgenie.create_alert(opsgenie_message) - message += f"\nAn alert has been created in OpsGenie with result: {result}." - - return message - - def format_abuse_notification(payload, msg): regex = r"arn:aws:sns:\w.*:(\d.*):\w.*" account = re.search(regex, payload.TopicArn).groups()[0] @@ -244,45 +204,73 @@ def format_cloudwatch_alarm(msg): return blocks -# If the message contains an api key it will be parsed by the format_api_key_detected function. +# Function to send the message to the Notify ops channel fo alerting. Right now it is set to #notification-ops channel +def send_message_to_notify_chanel(client, blocks): + NOTIFY_OPS_CHANNEL_ID = os.environ.get("NOTIFY_OPS_CHANNEL_ID") + + # Raise an exception if the NOTIFY_OPS_CHANNEL_ID is not set + assert NOTIFY_OPS_CHANNEL_ID, "NOTIFY_OPS_CHANNEL_ID is not set in the environment" + + # post the message to the notification channel + client.chat_postMessage(channel=NOTIFY_OPS_CHANNEL_ID, blocks=blocks) +# If the message contains an api key it will be parsed by the format_api_key_detected function. def format_api_key_detected(payload, client): msg = payload.Message - regex = r"API Key with value token='(\w.+)' has been detected in url='(\w.+)'" + regex = r"API Key with value token='(\w.+)', type='(\w.+)' and source='(\w.+)' has been detected in url='(\w.+)'!" # extract the api key and the github repo from the message api_key = re.search(regex, msg).groups()[0] - github_repo = re.search(regex, msg).groups()[1] + type = re.search(regex, msg).groups()[1] + source = re.search(regex, msg).groups()[2] + github_repo = re.search(regex, msg).groups()[3] + + # Extract the service id so that we can include it in the message + api_regex = r"(?Pgcntfy-)(?P.*)(?P[-A-Za-z0-9]{36})-(?P[-A-Za-z0-9]{36})" + pattern = re.compile(api_regex) + match = pattern.search(api_key) + if match: + service_id = match.group("service_id") # We don't want to send the actual api-key through Slack, but we do want the name to be given, # so therefore extract the api key name by following the format of a Notify api key api_key_name = api_key[7 : len(api_key) - 74] - # send a private message with the api-key and github repo to the people on call. - on_call_message = alert_on_call("Notify", client, api_key_name, github_repo) + # call the revoke api endpoint to revoke the api key + if notify.revoke_api_key(api_key, type, github_repo, source): + revoke_api_key_message = ( + f"API key {api_key_name} has been successfully revoked." + ) + header_text = "🙀 Notify API Key has been exposed and revoked! 😌" + else: + revoke_api_key_message = ( + f"API key {api_key_name} could not be revoked due to an error." + ) + header_text = "🙀 Notify API Key has been exposed but could not be revoked! 😱" # Format the message displayed in Slack - return [ + blocks = [ {"type": "section", "text": {"type": "mrkdwn", "text": " "}}, { "type": "header", - "text": { - "type": "plain_text", - "text": "🙀 Notify API Key has been compromised! 🔑", - }, + "text": {"type": "plain_text", "text": f"{header_text}"}, }, { "type": "section", "text": { "type": "mrkdwn", - "text": f"Notify API Key Name *{api_key_name}* has been committed in github file {github_repo}. The key needs to be revoked!", + "text": f"Notify API Key Name {api_key_name} from service id {service_id} was committed in github file {github_repo}.\n", }, }, { "type": "section", "text": { "type": "mrkdwn", - "text": f"{on_call_message}", + "text": f"*{revoke_api_key_message}*", }, }, ] + # send the message to the notify ops channel + send_message_to_notify_chanel(client, blocks) + + return blocks diff --git a/app/tests/intergrations/test_notify.py b/app/tests/intergrations/test_notify.py new file mode 100644 index 00000000..4cee23a0 --- /dev/null +++ b/app/tests/intergrations/test_notify.py @@ -0,0 +1,81 @@ +import jwt +import os +import pytest +from integrations import notify +from unittest.mock import patch +from freezegun import freeze_time + + +# helper function to decode the token for testing +def decode_token(token, secret): + return jwt.decode( + token, key=secret, options={"verify_signature": True}, algorithms=["HS256"] + ) + + +# Test that an exception is raised if the secret is missing +def test_create_jwt_token_secret_missing(): + with pytest.raises(AssertionError) as err: + notify.create_jwt_token(None, "client_id") + assert str(err.value) == "Missing secret key" + + +# Test that an exception is raised if the client_id is missing +def test_create_jwt_token_client_id_missing(): + with pytest.raises(AssertionError) as err: + notify.create_jwt_token("secret", None) + assert str(err.value) == "Missing client id" + + +# Test that the token is created correctly and the type and alg headers are set correctly +def test_create_jwt_token_contains_correct_headers(): + token = notify.create_jwt_token("secret", "client_id") + headers = jwt.get_unverified_header(token) + assert headers["typ"] == "JWT" + assert headers["alg"] == "HS256" + + +# Test that the claims headers are set correctly +def test_create_jwt_token_contains_correct_claims_headers(): + token = notify.create_jwt_token("secret", "client_id") + decoded_token = decode_token(token, "secret") + assert decoded_token["iss"] == "client_id" + assert "iat" in decoded_token + assert "req" not in decoded_token + assert "pay" not in decoded_token + + +# Test that the correct iat time in epoch seconds is set correctly +@freeze_time("2020-01-01 00:00:00") +def test_token_contains_correct_iat(): + token = notify.create_jwt_token("secret", "client_id") + decoded_token = decode_token(token, "secret") + assert decoded_token["iat"] == 1577836800 + + +# Test that an assertion error is raised if the NOTIFY_SRE_USER_NAME is missing +@patch.dict(os.environ, {"NOTIFY_SRE_USER_NAME": "", "NOTIFY_SRE_CLIENT_SECRET": "foo"}) +@patch("integrations.notify.create_jwt_token") +def test_authorization_header_missing_client_id(jwt_token_mock): + with pytest.raises(AssertionError) as err: + notify.create_authorization_header() + assert str(err.value) == "NOTIFY_SRE_USER_NAME is missing" + + +# Test that an assertion error is raised if the NOTIFY_SRE_CLIENT_SECRET is missing +@patch.dict(os.environ, {"NOTIFY_SRE_USER_NAME": "foo", "NOTIFY_SRE_CLIENT_SECRET": ""}) +@patch("integrations.notify.create_jwt_token") +def test_authorization_header_missing_secret(jwt_token_mock): + with pytest.raises(AssertionError) as err: + notify.create_authorization_header() + assert str(err.value) == "NOTIFY_SRE_CLIENT_SECRET is missing" + + +# Test that the authorization header is created correctly and the correct header is generated +@patch("integrations.notify.create_jwt_token") +def test_successful_creation_of_header(mock_jwt_token): + mock_jwt_token.return_value = "mocked_jwt_token" + header_key, header_value = notify.create_authorization_header() + + assert header_key == "Authorization" + assert header_value == "Bearer mocked_jwt_token" diff --git a/app/tests/server/event_handlers/test_aws_handler.py b/app/tests/server/event_handlers/test_aws_handler.py index ef6595bc..4c4512fc 100644 --- a/app/tests/server/event_handlers/test_aws_handler.py +++ b/app/tests/server/event_handlers/test_aws_handler.py @@ -2,6 +2,7 @@ import json import os +import pytest from unittest.mock import MagicMock, patch @@ -190,9 +191,20 @@ def test_parse_returns_blocks_if_api_key_detected(format_api_key_detected_mock): format_api_key_detected_mock.assert_called_once_with(payload, client) -@patch("server.event_handlers.aws.alert_on_call") +@patch("server.event_handlers.aws.format_api_key_detected") +def test_parse_returns_blocks_if_api_key_compromised(format_api_key_detected_mock): + # Test that the parse function returns the blocks returned by format_new_iam_user + client = MagicMock() + aws.format_api_key_detected.return_value = ["foo", "bar"] + payload = mock_api_key_detected() + response = aws.parse(payload, client) + assert response == ["foo", "bar"] + aws.format_api_key_detected.assert_called_once_with(payload, client) + + +@patch("integrations.notify.revoke_api_key") def test_format_api_key_detected_extracts_the_api_key_and_inserts_it_into_blocks( - alert_on_call_mock, + revoke_api_key_mock, ): # Test that the format_api_key_detected function extracts the api key properly client = MagicMock() @@ -201,9 +213,9 @@ def test_format_api_key_detected_extracts_the_api_key_and_inserts_it_into_blocks assert "api-key-blah" in response[2]["text"]["text"] -@patch("server.event_handlers.aws.alert_on_call") +@patch("integrations.notify.revoke_api_key") def test_format_api_key_detected_extracts_the_url_and_inserts_it_into_blocks( - alert_on_call_mock, + revoke_api_key_mock, ): # Test that the format_api_key_detected function extracts the url properly client = MagicMock() @@ -212,54 +224,73 @@ def test_format_api_key_detected_extracts_the_url_and_inserts_it_into_blocks( assert "https://github.com/blah" in response[2]["text"]["text"] -@patch("server.event_handlers.aws.alert_on_call") -def test_format_api_key_detected_extracts_the_on_call_message_and_inserts_it_into_blocks( - alert_on_call_mock, +@patch("integrations.notify.revoke_api_key") +def test_format_api_key_detected_extracts_the_service_id_and_inserts_it_into_blocks( + revoke_api_key_mock, +): + # Test that the format_api_key_detected function extracts the url properly + client = MagicMock() + payload = mock_api_key_detected() + response = aws.format_api_key_detected(payload, client) + assert "00000000-0000-0000-0000-000000000000" in response[2]["text"]["text"] + + +# Test that the format_api_key_detected function extracts the api revoke message if it is successful +@patch("integrations.notify.revoke_api_key") +def test_format_api_key_detected_success_extracts_the_api_revoke_message_and_inserts_it_into_blocks( + revoke_api_key_mock, ): # Test that the format_api_key_detected function extracts the on call message properly client = MagicMock() - alert_on_call_mock.return_value = "test message" + revoke_api_key_mock.return_value = True payload = mock_api_key_detected() response = aws.format_api_key_detected(payload, client) - assert "test message" in response[3]["text"]["text"] - - -@patch("integrations.google_drive.get_google_service") -@patch("commands.incident.google_drive.list_folders") -@patch("commands.incident.google_drive.list_metadata") -@patch("integrations.opsgenie.create_alert") -@patch("integrations.opsgenie.get_on_call_users") -def test_alert_on_call_returns_message( - get_on_call_users_mock, - create_alert_mock, - list_metadata_mock, - google_list_folders_mock, - get_google_service_mock, + assert ( + "*API key api-key-blah has been successfully revoked.*" + in response[3]["text"]["text"] + ) + + +# Test that the format_api_key_detected function extracts the api revoke message if it is unsuccessful +@patch("integrations.notify.revoke_api_key") +def test_format_api_key_detected_failure_extracts_the_api_revoke_message_and_inserts_it_into_blocks( + revoke_api_key_mock, ): - # Test that the alert_on_call function returns the proper message client = MagicMock() - product = "test" - api_key = "test_api_key" - github_repo = "test_repo" - google_list_folders_mock.return_value = [ - { - "name": "Notify", - "id": 12345, - "appProperties": {"genie_schedule": "test_schedule"}, - } - ] - list_metadata_mock.return_value = { - "name": "Notify", - "appProperties": {"genie_schedule": "test_schedule"}, - } - create_alert_mock.return_value = "test result" - response = aws.alert_on_call(product, client, api_key, github_repo) + revoke_api_key_mock.return_value = False + payload = mock_api_key_detected() + response = aws.format_api_key_detected(payload, client) assert ( - "test on-call staff have been notified.\nAn alert has been created in OpsGenie with result: test result." - in response + "*API key api-key-blah could not be revoked due to an error.*" + in response[3]["text"]["text"] + ) + + +@patch.dict(os.environ, {"NOTIFY_OPS_CHANNEL_ID": "test_channel_id"}) +def test_successful_message_post_notify_channel_for_notify(): + # Mock the chat_postMessage method + client = MagicMock() + blocks = ["test_blocks"] + + # Call the function + aws.send_message_to_notify_chanel(client, blocks) + + # Assert that chat_postMessage was called with the correct parameters + client.chat_postMessage.assert_called_once_with( + channel="test_channel_id", blocks=["test_blocks"] ) +@patch.dict(os.environ, {"NOTIFY_OPS_CHANNEL_ID": ""}) +def test_exception_for_missing_env_variable_notify_channel_for_notify(): + # Test that an exception is reaised if the NOTIFY_POS_CHANNEL_ID is not set + with pytest.raises(AssertionError) as err: + client = MagicMock() + aws.send_message_to_notify_chanel(client, ["test_blocks"]) + # assert that the correct exception is raised + assert str(err.value) == "NOTIFY_OPS_CHANNEL_ID is not set in the environment" + + def mock_abuse_alert(): return MagicMock( Type="Notification", @@ -346,7 +377,7 @@ def mock_api_key_detected(): MessageId="1e5f5647g-adb5-5d6f-ab5e-c2e508881361", TopicArn="arn:aws:sns:ca-central-1:412578375350:test-sre-bot", Subject="API Key detected", - Message=f"API Key with value token='{NOTIFY_TEST_KEY}' has been detected in url='https://github.com/blah'! This key needs to be revoked asap.", + Message=f"API Key with value token='{NOTIFY_TEST_KEY}', type='cds_test_type' and source='source_data' has been detected in url='https://github.com/blah'!", Timestamp="2023-09-25T20:50:37.868Z", SignatureVersion="1", Signature="EXAMPLEO0OA1HN4MIHrtym3N6SWqvotsY4EcG+Ty/wrfZcxpQ3mximWM7ZfoYlzZ8NBh4s1XTPuqbl5efK64TEuPgNWBMKsm5Gc2d8H6hoDpLqAOELGl2/xlvWf2CovLH/KPj8xrSwAgOS9jL4r/EEMdXYb705YMMBudu78gooatU9EpVl+1I2MCP2AW0ZJWrcSwYMqxo9yo7H6coyBRlmTxP97PlELXoqXLfufsfFBjZ0eFycndG5A0YHeue82uLF5fIHGpcTjqNzLF0PXuJoS9xVkGx3X7p+dzmRE4rp/swGyKCqbXvgldPRycuj7GSk3r8HLSfzjqHyThnDqMECA==",