From 9ab1e5cc4ef23c3f1bc9a3aca51f5f170bceedcd Mon Sep 17 00:00:00 2001 From: Jeremy Frasier Date: Mon, 1 Nov 2021 16:03:57 -0400 Subject: [PATCH 01/92] Bare minimum functionality --- .coveragerc | 2 +- setup.py | 31 +- src/example/example.py | 103 ------ src/{example => guacscanner}/__init__.py | 7 +- src/{example => guacscanner}/__main__.py | 2 +- src/{example => guacscanner}/_version.py | 0 src/{example => guacscanner}/data/secret.txt | 0 src/guacscanner/guacscanner.py | 367 +++++++++++++++++++ tests/test_example.py | 144 -------- tests/test_guacscanner.py | 161 ++++++++ 10 files changed, 556 insertions(+), 261 deletions(-) delete mode 100644 src/example/example.py rename src/{example => guacscanner}/__init__.py (68%) rename src/{example => guacscanner}/__main__.py (70%) rename src/{example => guacscanner}/_version.py (100%) rename src/{example => guacscanner}/data/secret.txt (100%) create mode 100644 src/guacscanner/guacscanner.py delete mode 100644 tests/test_example.py create mode 100644 tests/test_guacscanner.py diff --git a/.coveragerc b/.coveragerc index d315b87a..ec233bb2 100644 --- a/.coveragerc +++ b/.coveragerc @@ -2,7 +2,7 @@ # https://coverage.readthedocs.io/en/latest/config.html [run] -source = src/example +source = src/guacscanner omit = branch = true diff --git a/setup.py b/setup.py index 3291c4dd..c271100d 100644 --- a/setup.py +++ b/setup.py @@ -1,5 +1,5 @@ """ -This is the setup module for the example project. +This is the setup module for the guacscanner project. Based on: @@ -42,10 +42,10 @@ def get_version(version_file): setup( - name="example", + name="guacscanner", # Versions should comply with PEP440 - version=get_version("src/example/_version.py"), - description="Example Python library", + version=get_version("src/guacscanner/_version.py"), + description="Scan for EC2 instances added (removed) from a VPC and create (destroy) the corresponding Guacamole connections.", long_description=readme(), long_description_content_type="text/markdown", # Landing page for CISA's cybersecurity mission @@ -81,13 +81,20 @@ def get_version(version_file): ], python_requires=">=3.6", # What does your project relate to? - keywords="skeleton", + keywords="aws, guacamole, vpc", packages=find_packages(where="src"), package_dir={"": "src"}, - package_data={"example": ["data/*.txt"]}, + # package_data={"guacamole-connection-scanner": ["data/*.txt"]}, py_modules=[splitext(basename(path))[0] for path in glob("src/*.py")], include_package_data=True, - install_requires=["docopt", "schema", "setuptools >= 24.2.0"], + install_requires=[ + "boto3 == 1.19.6", + "docopt == 0.6.2", + "ec2-metadata == 2.5.0", + "psycopg == 3.0.1", + "schema == 0.7.4", + "setuptools >= 24.2.0", + ], extras_require={ "test": [ "coverage", @@ -98,11 +105,17 @@ def get_version(version_file): # 1.11.1 fixed this issue, but to ensure expected behavior we'll pin # to never grab the regression version. "coveralls != 1.11.0", + "moto", "pre-commit", "pytest-cov", "pytest", ] }, - # Conveniently allows one to run the CLI tool as `example` - entry_points={"console_scripts": ["example = example.example:main"]}, + # Conveniently allows one to run the CLI tool as + # `guacscanner` + entry_points={ + "console_scripts": [ + "guacscanner = guacscanner.guacscanner:main", + ], + }, ) diff --git a/src/example/example.py b/src/example/example.py deleted file mode 100644 index d3eda196..00000000 --- a/src/example/example.py +++ /dev/null @@ -1,103 +0,0 @@ -"""example is an example Python library and tool. - -Divide one integer by another and log the result. Also log some information -from an environment variable and a package resource. - -EXIT STATUS - This utility exits with one of the following values: - 0 Calculation completed successfully. - >0 An error occurred. - -Usage: - example [--log-level=LEVEL] - example (-h | --help) - -Options: - -h --help Show this message. - --log-level=LEVEL If specified, then the log level will be set to - the specified value. Valid values are "debug", "info", - "warning", "error", and "critical". [default: info] -""" - -# Standard Python Libraries -import logging -import os -import sys -from typing import Any, Dict - -# Third-Party Libraries -import docopt -import pkg_resources -from schema import And, Schema, SchemaError, Use - -from ._version import __version__ - -DEFAULT_ECHO_MESSAGE: str = "Hello World from the example default!" - - -def example_div(dividend: int, divisor: int) -> float: - """Print some logging messages.""" - logging.debug("This is a debug message") - logging.info("This is an info message") - logging.warning("This is a warning message") - logging.error("This is an error message") - logging.critical("This is a critical message") - return dividend / divisor - - -def main() -> None: - """Set up logging and call the example function.""" - args: Dict[str, str] = docopt.docopt(__doc__, version=__version__) - # Validate and convert arguments as needed - schema: Schema = Schema( - { - "--log-level": And( - str, - Use(str.lower), - lambda n: n in ("debug", "info", "warning", "error", "critical"), - error="Possible values for --log-level are " - + "debug, info, warning, error, and critical.", - ), - "": Use(int, error=" must be an integer."), - "": And( - Use(int), - lambda n: n != 0, - error=" must be an integer that is not 0.", - ), - str: object, # Don't care about other keys, if any - } - ) - - try: - validated_args: Dict[str, Any] = schema.validate(args) - except SchemaError as err: - # Exit because one or more of the arguments were invalid - print(err, file=sys.stderr) - sys.exit(1) - - # Assign validated arguments to variables - dividend: int = validated_args[""] - divisor: int = validated_args[""] - log_level: str = validated_args["--log-level"] - - # Set up logging - logging.basicConfig( - format="%(asctime)-15s %(levelname)s %(message)s", level=log_level.upper() - ) - - logging.info("%d / %d == %f", dividend, divisor, example_div(dividend, divisor)) - - # Access some data from an environment variable - message: str = os.getenv("ECHO_MESSAGE", DEFAULT_ECHO_MESSAGE) - logging.info('ECHO_MESSAGE="%s"', message) - - # Access some data from our package data (see the setup.py) - secret_message: str = ( - pkg_resources.resource_string("example", "data/secret.txt") - .decode("utf-8") - .strip() - ) - logging.info('Secret="%s"', secret_message) - - # Stop logging and clean up - logging.shutdown() diff --git a/src/example/__init__.py b/src/guacscanner/__init__.py similarity index 68% rename from src/example/__init__.py rename to src/guacscanner/__init__.py index 98b5e041..ac9d730c 100644 --- a/src/example/__init__.py +++ b/src/guacscanner/__init__.py @@ -1,9 +1,10 @@ -"""The example library.""" +"""The guacscanner library.""" # We disable a Flake8 check for "Module imported but unused (F401)" here because # although this import is not directly used, it populates the value # package_name.__version__, which is used to get version information about this # Python package. +# from .guacscanner import example_div +from . import guacscanner # noqa: F401 from ._version import __version__ # noqa: F401 -from .example import example_div -__all__ = ["example_div"] +# __all__ = ["example_div"] diff --git a/src/example/__main__.py b/src/guacscanner/__main__.py similarity index 70% rename from src/example/__main__.py rename to src/guacscanner/__main__.py index 11a3238f..c438269b 100644 --- a/src/example/__main__.py +++ b/src/guacscanner/__main__.py @@ -1,5 +1,5 @@ """Code to run if this package is used as a Python module.""" -from .example import main +from .guacscanner import main main() diff --git a/src/example/_version.py b/src/guacscanner/_version.py similarity index 100% rename from src/example/_version.py rename to src/guacscanner/_version.py diff --git a/src/example/data/secret.txt b/src/guacscanner/data/secret.txt similarity index 100% rename from src/example/data/secret.txt rename to src/guacscanner/data/secret.txt diff --git a/src/guacscanner/guacscanner.py b/src/guacscanner/guacscanner.py new file mode 100644 index 00000000..0c19b738 --- /dev/null +++ b/src/guacscanner/guacscanner.py @@ -0,0 +1,367 @@ +"""Query AWS for new (destroyed) instances and add (remove) Guacamole connections for them. + +Also check for instances that have been destroyed and remove their +corresponding connections. + +EXIT STATUS + 0 Update was successful. + >0 An error occurred. + +Usage: + guacscanner [--log-level=LEVEL] [--postgres-password=PASSWORD] [--postgres-password-file=FILENAME] [--postgres-username=USERNAME] [--postgres-username-file=FILENAME] [--vpc-id=VPC_ID] + guacscanner (-h | --help) + +Options: + -h --help Show this message. + --log-level=LEVEL If specified, then the log level will be set to + the specified value. Valid values are "debug", "info", + "warning", "error", and "critical". [default: info] + --postgres-password=PASSWORD If specified then the specified value will be used as the password when connecting to the PostgreSQL database. Otherwise, the password is read from a local file. + --postgres-password-file=FILENAME The file from which the PostgreSQL password will be read. [default: /run/secrets/postgres-password] + --postgres-username=USERNAME If specified then the specified value will be used when connecting to the PostgreSQL database. Otherwise, the username is read from a local file. + --postgres-username-file=FILENAME The file from which the PostgreSQL username will be read. [default: /run/secrets/postgres-username] + --vpc-id=VPC_ID If specified then query for EC2 instances created + or destroyed in the specified VPC ID. If not + specified then the ID of the VPC in which the host + resides will be used. +""" + + +# Standard Python Libraries +import logging +import re +import sys + +# Third-Party Libraries +import boto3 +import docopt +from ec2_metadata import ec2_metadata +import psycopg +from psycopg import sql +from schema import And, Schema, SchemaError, Use + +from ._version import __version__ + +DEFAULT_ADD_INSTANCE_STATES = [ + "running", +] +DEFAULT_POSTGRES_DB_NAME = "guacamole_db" +DEFAULT_POSTGRES_HOSTNAME = "postgres" +DEFAULT_POSTGRES_PORT = 5432 +DEFAULT_REMOVE_INSTANCE_STATES = [ + "terminated", +] +COUNT_QUERY = sql.SQL( + "SELECT COUNT({id_field}) FROM {table} WHERE {name_field} = %s" +).format( + id_field=sql.Identifier("connection_id"), + table=sql.Identifier("guacamole_connection"), + name_field=sql.Identifier("connection_name"), +) +IDS_QUERY = sql.SQL("SELECT {id_field} FROM {table} WHERE {name_field} = %s").format( + id_field=sql.Identifier("connection_id"), + table=sql.Identifier("guacamole_connection"), + name_field=sql.Identifier("connection_name"), +) +NAMES_QUERY = sql.SQL("SELECT {id_field}, {name_field} FROM {table}").format( + id_field=sql.Identifier("connection_id"), + name_field=sql.Identifier("connection_name"), + table=sql.Identifier("guacamole_connection"), +) +INSERT_CONNECTION_QUERY = sql.SQL( + """INSERT INTO {table} ( + {name_field}, {protocol_field}, {max_connections_field}, + {max_connections_per_user_field}, {proxy_port_field}, {proxy_hostname_field}, + {proxy_encryption_method_field}) + VALUES (%s, %s, %s, %s, %s, %s, %s) RETURNING id;""" +).format( + table=sql.Identifier("guacamole_connection"), + name_field=sql.Identifier("connection_name"), + protocol_field=sql.Identifier("protocol"), + max_connections_field=sql.Identifier("max_connections"), + max_connections_per_user_field=sql.Identifier("max_connections_per_user"), + proxy_port_field=sql.Identifier("proxy_port"), + proxy_hostname_field=sql.Identifier("proxy_hostname"), + proxy_encryption_method_field=sql.Identifier("proxy_encryption_method"), +) +INSERT_CONNECTION_PARAMETER_QUERY = sql.SQL( + """INSERT INTO {table} + ({id_field}, {parameter_name_field}, {parameter_value_field}) + VALUES (%s, %s, %s);""" +).format( + table=sql.Identifier("guacamole_connection_parameter"), + id_field=sql.Identifier("connection_id"), + parameter_name_field=sql.Identifier("parameter_name"), + parameter_value_field=sql.Identifier("parameter_value"), +) +DELETE_CONNECTIONS_QUERY = sql.SQL( + """DELETE FROM {table} WHERE {id_field} = %s;""" +).format(table=sql.Identifier("guacamole_connection"), id_field=sql.Identifier("id")) +DELETE_CONNECTION_PARAMETERS_QUERY = sql.SQL( + """DELETE FROM {table} WHERE {id_field} = %s;""" +).format( + table=sql.Identifier("guacamole_connection_parameter"), + id_field=sql.Identifier("id"), +) + + +def instance_connection_exists(db_connection, connection_name): + """Return a boolean indicating whether a connection with the specified name exists.""" + with db_connection.cursor() as cursor: + logging.debug( + "Checking to see if a connection named %s exists in the database.", + connection_name, + ) + cursor.execute(COUNT_QUERY, (connection_name,)) + count = cursor.fetchone()["count"] != 0 + if count != 0: + logging.debug( + "A connection named %s exists in the database.", connection_name + ) + else: + logging.debug( + "No connection named %s exists in the database.", connection_name + ) + + return count != 0 + + +def add_instance_connection(db_connection, instance): + """Add a connection for the EC2 isntance.""" + logging.debug("Adding connection entry for %s.", instance.id) + hostname = instance.private_dns_name + connection_name = get_connection_name(instance) + with db_connection.cursor() as cursor: + cursor.execute( + INSERT_CONNECTION_QUERY, + ( + connection_name, + "vnc", + 10, + 10, + 4822, + "guacd", + "NONE", + ), + ) + connection_id = cursor.fetchone()["id"] + + logging.debug( + "Adding connection parameter entries for connection named %s.", + connection_name, + ) + cursor.executemany( + INSERT_CONNECTION_PARAMETER_QUERY, + ( + ( + connection_id, + "cursor", + "local", + ), + ( + connection_id, + "sftp-directory", + "/home/{{ vnc_username }}/Documents", + ), + ( + connection_id, + "sftp-username", + "{{ vnc_username }}", + ), + ( + connection_id, + "sftp-private-key", + "{{ vnc_user_private_ssh_key }}", + ), + ( + connection_id, + "sftp-server-alive-interval", + 60, + ), + ( + connection_id, + "sftp-root-directory", + "/home/{{ vnc_username }}/", + ), + ( + connection_id, + "enable-sftp", + True, + ), + ( + connection_id, + "color-depth", + 24, + ), + ( + connection_id, + "hostname", + hostname, + ), + ( + connection_id, + "password", + "{{ vnc_password }}", + ), + ( + connection_id, + "port", + 5901, + ), + ), + ) + + +def remove_instance_connections(db_connection, instance): + """Remove all connections corresponding to the EC2 isntance.""" + logging.debug("Removing connections for %s.", instance.id) + connection_name = get_connection_name(instance) + with db_connection.cursor() as cursor: + logging.debug( + "Checking to see if any connections named %s exist in the database.", + connection_name, + ) + cursor.execute(IDS_QUERY, (connection_name,)) + for record in cursor: + logging.info("Removing entries for connections named %s.", connection_name) + connection_id = record["connection_id"] + logging.debug("Removing connection entries for %s.", connection_id) + cursor.execute(DELETE_CONNECTIONS_QUERY, (connection_id,)) + + logging.debug( + "Removing connection parameter entries for %s.", connection_id + ) + cursor.execute(DELETE_CONNECTION_PARAMETERS_QUERY, (connection_id,)) + + +def get_connection_name(instance): + """Return the unique connection name for an EC2 instance.""" + return f"{instance.private_dns_name} ({instance.id})" + + +def process_instance( + db_connection, instance, add_instance_states, remove_instance_states +): + """Add/remove connections for the specified EC2 instance.""" + logging.debug("Examining instance %s.", instance.id) + state = instance.state.name + connection_name = get_connection_name(instance) + logging.debug("Connection name is %s.", connection_name) + if state in add_instance_states: + logging.info( + "Instance %s is in state %s and will be added if not already present.", + instance.id, + state, + ) + if not instance_connection_exists(db_connection, connection_name): + logging.info("Adding a connection for %s.", instance.id) + add_instance_connection(db_connection, instance) + else: + logging.debug( + "Connection for %s already exists in the database.", instance.id + ) + elif state in remove_instance_states: + logging.info( + "Instance %s is in state %s and will be removed if present.", + instance.id, + state, + ) + remove_instance_connections(db_connection, instance) + else: + logging.debug( + "Instance %s is in state %s and WILL NOT be added or removed.", + instance.id, + state, + ) + + +def main() -> None: + """Add/remove connections to Guacamole DB as necessary.""" + # Parse command line arguments + args = docopt.docopt(__doc__, version=__version__) + # Validate and convert arguments as needed + schema = Schema( + { + "--log-level": And( + str, + Use(str.lower), + lambda n: n in ("debug", "info", "warning", "error", "critical"), + error="Possible values for --log-level are " + + "debug, info, warning, error, and critical.", + ), + "--vpc-id": And( + str, + Use(str.lower), + lambda x: re.fullmatch(r"^vpc-[0-9a-f]{17}$", x) is not None, + error="Possible values for --vpc-id are the characters vpc- followed by 17 hexadecimal digits.", + ), + str: object, # Don't care about other keys, if any + } + ) + try: + validated_args = schema.validate(args) + except SchemaError as err: + # Exit because one or more of the arguments were invalid + print(err, file=sys.stderr) + sys.exit(1) + + # Set up logging + log_level = validated_args["--log-level"] + logging.basicConfig( + format="%(asctime)-15s %(levelname)s %(message)s", level=log_level.upper() + ) + + add_instance_states = DEFAULT_ADD_INSTANCE_STATES + postgres_db_name = DEFAULT_POSTGRES_DB_NAME + postgres_hostname = DEFAULT_POSTGRES_HOSTNAME + postgres_port = DEFAULT_POSTGRES_PORT + remove_instance_states = DEFAULT_REMOVE_INSTANCE_STATES + + postgres_password = None + if "--postgres-password" in validated_args: + postgres_password = validated_args["--postgres-password"] + else: + with open(validated_args["--postgres-password-file"], "r") as file: + postgres_password = file.read() + + postgres_username = None + if "--postgres-username" in validated_args: + postgres_username = validated_args["--postgres-username"] + else: + with open(validated_args["--postgres-username-file"], "r") as file: + postgres_username = file.read() + + db_connection_string = f"postgresql://{postgres_username}:{postgres_password}@{postgres_hostname}:{postgres_port}/{postgres_db_name}" + + vpc_id = None + if "--vpc-id" in validated_args: + vpc_id = validated_args["--vpc-id"] + else: + vpc_id = ec2_metadata.vpc_id + logging.info("Examining instances in VPC %s.", vpc_id) + + ec2 = boto3.resource("ec2") + + with psycopg.connect(db_connection_string) as db_connection: + for instance in ec2.Vpc(vpc_id).instances.all(): + process_instance( + db_connection, instance, add_instance_states, remove_instance_states + ) + + # logging.debug( + # "Checking to see if any connections belonging to nonexistent instances are in the database." + # ) + # cursor.execute(NAMES_QUERY) + # for record in cursor: + # connection_id = record["connection_id"] + # connection_name = record["connection_name"] + # m = re.match(r"^.* \((?Pi-\d{17})\)$", connection_name) + # instance_id = None + # if m: + # instance_id = m.group("id") + # ec2.Instance(instance_id) + + # Commit all pending transactions to the database + db_connection.commit() + + logging.shutdown() diff --git a/tests/test_example.py b/tests/test_example.py deleted file mode 100644 index f8dea673..00000000 --- a/tests/test_example.py +++ /dev/null @@ -1,144 +0,0 @@ -#!/usr/bin/env pytest -vs -"""Tests for example.""" - -# Standard Python Libraries -import logging -import os -import sys -from unittest.mock import patch - -# Third-Party Libraries -import pytest - -# cisagov Libraries -import example - -div_params = [ - (1, 1, 1), - (2, 2, 1), - (0, 1, 0), - (8, 2, 4), -] - -log_levels = ( - "debug", - "info", - "warning", - "error", - "critical", -) - -# define sources of version strings -RELEASE_TAG = os.getenv("RELEASE_TAG") -PROJECT_VERSION = example.__version__ - - -def test_stdout_version(capsys): - """Verify that version string sent to stdout agrees with the module version.""" - with pytest.raises(SystemExit): - with patch.object(sys, "argv", ["bogus", "--version"]): - example.example.main() - captured = capsys.readouterr() - assert ( - captured.out == f"{PROJECT_VERSION}\n" - ), "standard output by '--version' should agree with module.__version__" - - -def test_running_as_module(capsys): - """Verify that the __main__.py file loads correctly.""" - with pytest.raises(SystemExit): - with patch.object(sys, "argv", ["bogus", "--version"]): - # F401 is a "Module imported but unused" warning. This import - # emulates how this project would be run as a module. The only thing - # being done by __main__ is importing the main entrypoint of the - # package and running it, so there is nothing to use from this - # import. As a result, we can safely ignore this warning. - # cisagov Libraries - import example.__main__ # noqa: F401 - captured = capsys.readouterr() - assert ( - captured.out == f"{PROJECT_VERSION}\n" - ), "standard output by '--version' should agree with module.__version__" - - -@pytest.mark.skipif( - RELEASE_TAG in [None, ""], reason="this is not a release (RELEASE_TAG not set)" -) -def test_release_version(): - """Verify that release tag version agrees with the module version.""" - assert ( - RELEASE_TAG == f"v{PROJECT_VERSION}" - ), "RELEASE_TAG does not match the project version" - - -@pytest.mark.parametrize("level", log_levels) -def test_log_levels(level): - """Validate commandline log-level arguments.""" - with patch.object(sys, "argv", ["bogus", f"--log-level={level}", "1", "1"]): - with patch.object(logging.root, "handlers", []): - assert ( - logging.root.hasHandlers() is False - ), "root logger should not have handlers yet" - return_code = None - try: - example.example.main() - except SystemExit as sys_exit: - return_code = sys_exit.code - assert return_code is None, "main() should return success" - assert ( - logging.root.hasHandlers() is True - ), "root logger should now have a handler" - assert ( - logging.getLevelName(logging.root.getEffectiveLevel()) == level.upper() - ), f"root logger level should be set to {level.upper()}" - assert return_code is None, "main() should return success" - - -def test_bad_log_level(): - """Validate bad log-level argument returns error.""" - with patch.object(sys, "argv", ["bogus", "--log-level=emergency", "1", "1"]): - return_code = None - try: - example.example.main() - except SystemExit as sys_exit: - return_code = sys_exit.code - assert return_code == 1, "main() should exit with error" - - -@pytest.mark.parametrize("dividend, divisor, quotient", div_params) -def test_division(dividend, divisor, quotient): - """Verify division results.""" - result = example.example_div(dividend, divisor) - assert result == quotient, "result should equal quotient" - - -@pytest.mark.slow -def test_slow_division(): - """Example of using a custom marker. - - This test will only be run if --runslow is passed to pytest. - Look in conftest.py to see how this is implemented. - """ - # Standard Python Libraries - import time - - result = example.example_div(256, 16) - time.sleep(4) - assert result == 16, "result should equal be 16" - - -def test_zero_division(): - """Verify that division by zero throws the correct exception.""" - with pytest.raises(ZeroDivisionError): - example.example_div(1, 0) - - -def test_zero_divisor_argument(): - """Verify that a divisor of zero is handled as expected.""" - with patch.object(sys, "argv", ["bogus", "1", "0"]): - return_code = None - try: - example.example.main() - except SystemExit as sys_exit: - return_code = sys_exit.code - assert return_code == 1, "main() should exit with error" diff --git a/tests/test_guacscanner.py b/tests/test_guacscanner.py new file mode 100644 index 00000000..0f4afe58 --- /dev/null +++ b/tests/test_guacscanner.py @@ -0,0 +1,161 @@ +#!/usr/bin/env pytest -vs +"""Tests for guacscanner.""" + +# Standard Python Libraries +import logging +import os +import sys +from unittest.mock import MagicMock, patch + +# Third-Party Libraries +from moto import mock_ec2 +import psycopg +import pytest + +# cisagov Libraries +import guacscanner + +log_levels = ( + "debug", + "info", + "warning", + "error", + "critical", +) + +# define sources of version strings +RELEASE_TAG = os.getenv("RELEASE_TAG") +PROJECT_VERSION = guacscanner.__version__ + +DUMMY_VPC_ID = "vpc-0123456789abcdef0" + + +def test_stdout_version(capsys): + """Verify that version string sent to stdout agrees with the module version.""" + with pytest.raises(SystemExit): + with patch.object(sys, "argv", ["bogus", "--version"]): + guacscanner.guacscanner.main() + captured = capsys.readouterr() + assert ( + captured.out == f"{PROJECT_VERSION}\n" + ), "standard output by '--version' should agree with module.__version__" + + +def test_running_as_module(capsys): + """Verify that the __main__.py file loads correctly.""" + with pytest.raises(SystemExit): + with patch.object(sys, "argv", ["bogus", "--version"]): + # F401 is a "Module imported but unused" warning. This import + # emulates how this project would be run as a module. The only thing + # being done by __main__ is importing the main entrypoint of the + # package and running it, so there is nothing to use from this + # import. As a result, we can safely ignore this warning. + # cisagov Libraries + import guacscanner.__main__ # noqa: F401 + captured = capsys.readouterr() + assert ( + captured.out == f"{PROJECT_VERSION}\n" + ), "standard output by '--version' should agree with module.__version__" + + +@pytest.mark.skipif( + RELEASE_TAG in [None, ""], reason="this is not a release (RELEASE_TAG not set)" +) +def test_release_version(): + """Verify that release tag version agrees with the module version.""" + assert ( + RELEASE_TAG == f"v{PROJECT_VERSION}" + ), "RELEASE_TAG does not match the project version" + + +@mock_ec2 +@pytest.mark.parametrize("level", log_levels) +def test_log_levels(level): + """Validate commandline log-level arguments.""" + with patch.object( + sys, + "argv", + [ + f"--log-level={level}", + "--postgres-password=dummy_password", + "--postgres-username=dummy_username", + f"--vpc-id={DUMMY_VPC_ID}", + ], + ): + with patch.object(logging.root, "handlers", []): + with patch.object(psycopg, "connect", return_value=MagicMock()): + assert ( + logging.root.hasHandlers() is False + ), "root logger should not have handlers yet" + return_code = None + try: + guacscanner.guacscanner.main() + except SystemExit as sys_exit: + return_code = sys_exit.code + assert return_code is None, "main() should return success" + assert ( + logging.root.hasHandlers() is True + ), "root logger should now have a handler" + assert ( + logging.getLevelName(logging.root.getEffectiveLevel()) + == level.upper() + ), f"root logger level should be set to {level.upper()}" + assert return_code is None, "main() should return success" + + +def test_bad_log_level(): + """Validate bad log-level argument returns error.""" + with patch.object(sys, "argv", ["bogus", "--log-level=emergency"]): + return_code = None + try: + guacscanner.guacscanner.main() + except SystemExit as sys_exit: + return_code = sys_exit.code + assert return_code == 1, "main() should exit with error" + + +# def test_new_instance(): +# with patch('builtins.open', mock_open(read_data='test')) +# mock_connect = MagicMock() +# mock_cursor = MagicMock() +# mock_cursor.fetchall.return_value = expected +# mock_connect.cursor.return_value = mock_cursor + + +# @pytest.mark.parametrize("dividend, divisor, quotient", div_params) +# def test_division(dividend, divisor, quotient): +# """Verify division results.""" +# result = guacscanner.guacscanner_div(dividend, divisor) +# assert result == quotient, "result should equal quotient" + + +# @pytest.mark.slow +# def test_slow_division(): +# """Example of using a custom marker. + +# This test will only be run if --runslow is passed to pytest. +# Look in conftest.py to see how this is implemented. +# """ +# # Standard Python Libraries +# import time + +# result = guacscanner.guacscanner_div(256, 16) +# time.sleep(4) +# assert result == 16, "result should equal be 16" + + +# def test_zero_division(): +# """Verify that division by zero throws the correct exception.""" +# with pytest.raises(ZeroDivisionError): +# guacscanner.guacscanner_div(1, 0) + + +# def test_zero_divisor_argument(): +# """Verify that a divisor of zero is handled as expected.""" +# with patch.object(sys, "argv", ["bogus", "1", "0"]): +# return_code = None +# try: +# guacscanner.guacscanner.main() +# except SystemExit as sys_exit: +# return_code = sys_exit.code +# assert return_code == 1, "main() should exit with error" From 7734d7d4f9a079036234caf5b9cde9689727ba9a Mon Sep 17 00:00:00 2001 From: Jeremy Frasier Date: Tue, 2 Nov 2021 12:13:01 -0400 Subject: [PATCH 02/92] Allow VNC username, password, and private ssh key to be specified directly --- src/guacscanner/guacscanner.py | 78 +++++++++++++++++++++++++++------- 1 file changed, 62 insertions(+), 16 deletions(-) diff --git a/src/guacscanner/guacscanner.py b/src/guacscanner/guacscanner.py index 0c19b738..c8442040 100644 --- a/src/guacscanner/guacscanner.py +++ b/src/guacscanner/guacscanner.py @@ -8,7 +8,7 @@ >0 An error occurred. Usage: - guacscanner [--log-level=LEVEL] [--postgres-password=PASSWORD] [--postgres-password-file=FILENAME] [--postgres-username=USERNAME] [--postgres-username-file=FILENAME] [--vpc-id=VPC_ID] + guacscanner [--log-level=LEVEL] [--postgres-password=PASSWORD] [--postgres-password-file=FILENAME] [--private-ssh-key=KEY] [--private-ssh-key-file=FILENAME] [--postgres-username=USERNAME] [--postgres-username-file=FILENAME] [--vnc-password=PASSWORD] [--vnc-password-file=FILENAME] [--vnc-username=USERNAME] [--vnc-username-file=FILENAME] [--vpc-id=VPC_ID] guacscanner (-h | --help) Options: @@ -16,10 +16,16 @@ --log-level=LEVEL If specified, then the log level will be set to the specified value. Valid values are "debug", "info", "warning", "error", and "critical". [default: info] - --postgres-password=PASSWORD If specified then the specified value will be used as the password when connecting to the PostgreSQL database. Otherwise, the password is read from a local file. + --postgres-password=PASSWORD If specified then the specified value will be used as the password when connecting to the PostgreSQL database. Otherwise, the password will be read from a local file. --postgres-password-file=FILENAME The file from which the PostgreSQL password will be read. [default: /run/secrets/postgres-password] - --postgres-username=USERNAME If specified then the specified value will be used when connecting to the PostgreSQL database. Otherwise, the username is read from a local file. + --postgres-username=USERNAME If specified then the specified value will be used when connecting to the PostgreSQL database. Otherwise, the username will be read from a local file. --postgres-username-file=FILENAME The file from which the PostgreSQL username will be read. [default: /run/secrets/postgres-username] + --private-ssh-key=KEY If specified then the specified value will be used for the private ssh key. Otherwise, the ssh key will be read from a local file. + --private-ssh-key-file=FILENAME The file from which the private ssh key will be read. [default: /run/secrets/private-ssh-key] + --vnc-password=PASSWORD If specified then the specified value will be used for the VNC password. Otherwise, the password will be read from a local file. + --vnc-password-file=FILENAME The file from which the VNC password will be read. [default: /run/secrets/vnc-password] + --vnc-username=USERNAME If specified then the specified value will be used for the VNC username. Otherwise, the username will be read from a local file. + --vnc-username-file=FILENAME The file from which the VNC username will be read. [default: /run/secrets/vnc-username] --vpc-id=VPC_ID If specified then query for EC2 instances created or destroyed in the specified VPC ID. If not specified then the ID of the VPC in which the host @@ -126,8 +132,10 @@ def instance_connection_exists(db_connection, connection_name): return count != 0 -def add_instance_connection(db_connection, instance): - """Add a connection for the EC2 isntance.""" +def add_instance_connection( + db_connection, instance, vnc_username, vnc_password, private_ssh_key +): + """Add a connection for the EC2 instance.""" logging.debug("Adding connection entry for %s.", instance.id) hostname = instance.private_dns_name connection_name = get_connection_name(instance) @@ -161,17 +169,17 @@ def add_instance_connection(db_connection, instance): ( connection_id, "sftp-directory", - "/home/{{ vnc_username }}/Documents", + f"/home/{vnc_username}/Documents", ), ( connection_id, "sftp-username", - "{{ vnc_username }}", + vnc_username, ), ( connection_id, "sftp-private-key", - "{{ vnc_user_private_ssh_key }}", + private_ssh_key, ), ( connection_id, @@ -181,7 +189,7 @@ def add_instance_connection(db_connection, instance): ( connection_id, "sftp-root-directory", - "/home/{{ vnc_username }}/", + f"/home/{vnc_username}/", ), ( connection_id, @@ -201,7 +209,7 @@ def add_instance_connection(db_connection, instance): ( connection_id, "password", - "{{ vnc_password }}", + vnc_password, ), ( connection_id, @@ -211,6 +219,9 @@ def add_instance_connection(db_connection, instance): ), ) + # Commit all pending transactions to the database + db_connection.commit() + def remove_instance_connections(db_connection, instance): """Remove all connections corresponding to the EC2 isntance.""" @@ -233,6 +244,9 @@ def remove_instance_connections(db_connection, instance): ) cursor.execute(DELETE_CONNECTION_PARAMETERS_QUERY, (connection_id,)) + # Commit all pending transactions to the database + db_connection.commit() + def get_connection_name(instance): """Return the unique connection name for an EC2 instance.""" @@ -240,7 +254,13 @@ def get_connection_name(instance): def process_instance( - db_connection, instance, add_instance_states, remove_instance_states + db_connection, + instance, + add_instance_states, + remove_instance_states, + vnc_username, + vnc_password, + private_ssh_key, ): """Add/remove connections for the specified EC2 instance.""" logging.debug("Examining instance %s.", instance.id) @@ -255,7 +275,9 @@ def process_instance( ) if not instance_connection_exists(db_connection, connection_name): logging.info("Adding a connection for %s.", instance.id) - add_instance_connection(db_connection, instance) + add_instance_connection( + db_connection, instance, vnc_username, vnc_password, private_ssh_key + ) else: logging.debug( "Connection for %s already exists in the database.", instance.id @@ -331,6 +353,27 @@ def main() -> None: with open(validated_args["--postgres-username-file"], "r") as file: postgres_username = file.read() + vnc_password = None + if "--vnc-password" in validated_args: + vnc_password = validated_args["--vnc-password"] + else: + with open(validated_args["--vnc-password-file"], "r") as file: + vnc_password = file.read() + + vnc_username = None + if "--vnc-username" in validated_args: + vnc_username = validated_args["--vnc-username"] + else: + with open(validated_args["--vnc-username-file"], "r") as file: + vnc_username = file.read() + + private_ssh_key = None + if "--private-ssh-key" in validated_args: + private_ssh_key = validated_args["--private-ssh-key"] + else: + with open(validated_args["--private-ssh-key-file"], "r") as file: + private_ssh_key = file.read() + db_connection_string = f"postgresql://{postgres_username}:{postgres_password}@{postgres_hostname}:{postgres_port}/{postgres_db_name}" vpc_id = None @@ -345,7 +388,13 @@ def main() -> None: with psycopg.connect(db_connection_string) as db_connection: for instance in ec2.Vpc(vpc_id).instances.all(): process_instance( - db_connection, instance, add_instance_states, remove_instance_states + db_connection, + instance, + add_instance_states, + remove_instance_states, + vnc_username, + vnc_password, + private_ssh_key, ) # logging.debug( @@ -361,7 +410,4 @@ def main() -> None: # instance_id = m.group("id") # ec2.Instance(instance_id) - # Commit all pending transactions to the database - db_connection.commit() - logging.shutdown() From ad2a7fba5a0a077ccb438a3102abf4e816b5292a Mon Sep 17 00:00:00 2001 From: Jeremy Frasier Date: Tue, 2 Nov 2021 12:17:47 -0400 Subject: [PATCH 03/92] Specify a region when using boto3 --- src/guacscanner/guacscanner.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/guacscanner/guacscanner.py b/src/guacscanner/guacscanner.py index c8442040..7b0a7980 100644 --- a/src/guacscanner/guacscanner.py +++ b/src/guacscanner/guacscanner.py @@ -383,7 +383,7 @@ def main() -> None: vpc_id = ec2_metadata.vpc_id logging.info("Examining instances in VPC %s.", vpc_id) - ec2 = boto3.resource("ec2") + ec2 = boto3.resource("ec2", region_name="us-east-1") with psycopg.connect(db_connection_string) as db_connection: for instance in ec2.Vpc(vpc_id).instances.all(): From 7c0e1d8a4809cd6b2cadab4689c99fecbd93a369 Mon Sep 17 00:00:00 2001 From: Jeremy Frasier Date: Tue, 2 Nov 2021 12:32:21 -0400 Subject: [PATCH 04/92] Refine docopt/schema validation for VPC ID --- src/guacscanner/guacscanner.py | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/src/guacscanner/guacscanner.py b/src/guacscanner/guacscanner.py index 7b0a7980..db6be4f0 100644 --- a/src/guacscanner/guacscanner.py +++ b/src/guacscanner/guacscanner.py @@ -44,7 +44,7 @@ from ec2_metadata import ec2_metadata import psycopg from psycopg import sql -from schema import And, Schema, SchemaError, Use +from schema import And, Optional, Or, Schema, SchemaError, Use from ._version import __version__ @@ -311,11 +311,14 @@ def main() -> None: error="Possible values for --log-level are " + "debug, info, warning, error, and critical.", ), - "--vpc-id": And( - str, - Use(str.lower), - lambda x: re.fullmatch(r"^vpc-[0-9a-f]{17}$", x) is not None, - error="Possible values for --vpc-id are the characters vpc- followed by 17 hexadecimal digits.", + Optional("--vpc-id"): Or( + None, + And( + str, + Use(str.lower), + lambda x: re.fullmatch(r"^vpc-[0-9a-f]{17}$", x) is not None, + error="Possible values for --vpc-id are the characters vpc- followed by 17 hexadecimal digits.", + ), ), str: object, # Don't care about other keys, if any } From 6403ae3d3c25119ded62f10f91aa26e1a943ba6b Mon Sep 17 00:00:00 2001 From: Jeremy Frasier Date: Tue, 2 Nov 2021 16:50:49 -0400 Subject: [PATCH 05/92] Add code to correctly add instances depending on OS type (Windows vs Linux) --- src/guacscanner/guacscanner.py | 178 +++++++++++++++++++++++---------- tests/test_guacscanner.py | 160 +++++++++++++++++++++-------- 2 files changed, 242 insertions(+), 96 deletions(-) diff --git a/src/guacscanner/guacscanner.py b/src/guacscanner/guacscanner.py index db6be4f0..00e69748 100644 --- a/src/guacscanner/guacscanner.py +++ b/src/guacscanner/guacscanner.py @@ -8,7 +8,7 @@ >0 An error occurred. Usage: - guacscanner [--log-level=LEVEL] [--postgres-password=PASSWORD] [--postgres-password-file=FILENAME] [--private-ssh-key=KEY] [--private-ssh-key-file=FILENAME] [--postgres-username=USERNAME] [--postgres-username-file=FILENAME] [--vnc-password=PASSWORD] [--vnc-password-file=FILENAME] [--vnc-username=USERNAME] [--vnc-username-file=FILENAME] [--vpc-id=VPC_ID] + guacscanner [--log-level=LEVEL] [--postgres-password=PASSWORD|--postgres-password-file=FILENAME] [--private-ssh-key=KEY|--private-ssh-key-file=FILENAME] [--postgres-username=USERNAME|--postgres-username-file=FILENAME] [--rdp-password=PASSWORD|--rdp-password-file=FILENAME] [--rdp-username=USERNAME|--rdp-username-file=FILENAME] [--vnc-password=PASSWORD|--vnc-password-file=FILENAME] [--vnc-username=USERNAME|--vnc-username-file=FILENAME] [--vpc-id=VPC_ID] guacscanner (-h | --help) Options: @@ -22,6 +22,10 @@ --postgres-username-file=FILENAME The file from which the PostgreSQL username will be read. [default: /run/secrets/postgres-username] --private-ssh-key=KEY If specified then the specified value will be used for the private ssh key. Otherwise, the ssh key will be read from a local file. --private-ssh-key-file=FILENAME The file from which the private ssh key will be read. [default: /run/secrets/private-ssh-key] + --rdp-password=PASSWORD If specified then the specified value will be used for the RDP password. Otherwise, the password will be read from a local file. + --rdp-password-file=FILENAME The file from which the RDP password will be read. [default: /run/secrets/rdp-password] + --rdp-username=USERNAME If specified then the specified value will be used for the RDP username. Otherwise, the username will be read from a local file. + --rdp-username-file=FILENAME The file from which the RDP username will be read. [default: /run/secrets/rdp-username] --vnc-password=PASSWORD If specified then the specified value will be used for the VNC password. Otherwise, the password will be read from a local file. --vnc-password-file=FILENAME The file from which the VNC password will be read. [default: /run/secrets/vnc-password] --vnc-username=USERNAME If specified then the specified value will be used for the VNC username. Otherwise, the username will be read from a local file. @@ -119,7 +123,7 @@ def instance_connection_exists(db_connection, connection_name): connection_name, ) cursor.execute(COUNT_QUERY, (connection_name,)) - count = cursor.fetchone()["count"] != 0 + count = cursor.fetchone()[0]["count"] if count != 0: logging.debug( "A connection named %s exists in the database.", connection_name @@ -133,18 +137,33 @@ def instance_connection_exists(db_connection, connection_name): def add_instance_connection( - db_connection, instance, vnc_username, vnc_password, private_ssh_key + db_connection, + instance, + vnc_username, + vnc_password, + private_ssh_key, + rdp_username, + rdp_password, ): """Add a connection for the EC2 instance.""" logging.debug("Adding connection entry for %s.", instance.id) hostname = instance.private_dns_name connection_name = get_connection_name(instance) + is_windows = False + connection_protocol = "vnc" + connection_port = 5901 + if instance.platform and instance.platform.lower() == "windows": + logging.debug("Instance %s is Windows and therefore uses RDP.", instance.id) + is_windows = True + connection_protocol = "rdp" + connection_port = 3389 + with db_connection.cursor() as cursor: cursor.execute( INSERT_CONNECTION_QUERY, ( connection_name, - "vnc", + connection_protocol, 10, 10, 4822, @@ -152,55 +171,72 @@ def add_instance_connection( "NONE", ), ) - connection_id = cursor.fetchone()["id"] + connection_id = cursor.fetchone()[0]["connection_id"] - logging.debug( - "Adding connection parameter entries for connection named %s.", - connection_name, - ) - cursor.executemany( - INSERT_CONNECTION_PARAMETER_QUERY, + guac_conn_params = ( ( + connection_id, + "cursor", + "local", + ), + ( + connection_id, + "sftp-directory", + f"/home/{vnc_username}/Documents", + ), + ( + connection_id, + "sftp-username", + vnc_username, + ), + ( + connection_id, + "sftp-private-key", + private_ssh_key, + ), + ( + connection_id, + "sftp-server-alive-interval", + 60, + ), + ( + connection_id, + "sftp-root-directory", + f"/home/{vnc_username}/", + ), + ( + connection_id, + "enable-sftp", + True, + ), + ( + connection_id, + "color-depth", + 24, + ), + ( + connection_id, + "hostname", + hostname, + ), + ( + connection_id, + "password", + vnc_password, + ), + ( + connection_id, + "port", + connection_port, + ), + ) + if is_windows: + guac_conn_params = ( ( connection_id, - "cursor", - "local", - ), - ( - connection_id, - "sftp-directory", - f"/home/{vnc_username}/Documents", - ), - ( - connection_id, - "sftp-username", - vnc_username, - ), - ( - connection_id, - "sftp-private-key", - private_ssh_key, - ), - ( - connection_id, - "sftp-server-alive-interval", - 60, - ), - ( - connection_id, - "sftp-root-directory", - f"/home/{vnc_username}/", - ), - ( - connection_id, - "enable-sftp", + "ignore-cert", True, ), - ( - connection_id, - "color-depth", - 24, - ), ( connection_id, "hostname", @@ -209,15 +245,25 @@ def add_instance_connection( ( connection_id, "password", - vnc_password, + rdp_password, ), ( connection_id, "port", - 5901, + connection_port, ), - ), + ( + connection_id, + "username", + rdp_username, + ), + ) + + logging.debug( + "Adding connection parameter entries for connection named %s.", + connection_name, ) + cursor.executemany(INSERT_CONNECTION_PARAMETER_QUERY, guac_conn_params) # Commit all pending transactions to the database db_connection.commit() @@ -261,10 +307,12 @@ def process_instance( vnc_username, vnc_password, private_ssh_key, + rdp_username, + rdp_password, ): """Add/remove connections for the specified EC2 instance.""" logging.debug("Examining instance %s.", instance.id) - state = instance.state.name + state = instance.state["Name"] connection_name = get_connection_name(instance) logging.debug("Connection name is %s.", connection_name) if state in add_instance_states: @@ -273,10 +321,17 @@ def process_instance( instance.id, state, ) + db_connection.cursor() if not instance_connection_exists(db_connection, connection_name): logging.info("Adding a connection for %s.", instance.id) add_instance_connection( - db_connection, instance, vnc_username, vnc_password, private_ssh_key + db_connection, + instance, + vnc_username, + vnc_password, + private_ssh_key, + rdp_username, + rdp_password, ) else: logging.debug( @@ -316,7 +371,8 @@ def main() -> None: And( str, Use(str.lower), - lambda x: re.fullmatch(r"^vpc-[0-9a-f]{17}$", x) is not None, + lambda x: re.fullmatch(r"^vpc-([0-9a-f]{8}|[0-9a-f]{17})$", x) + is not None, error="Possible values for --vpc-id are the characters vpc- followed by 17 hexadecimal digits.", ), ), @@ -356,6 +412,20 @@ def main() -> None: with open(validated_args["--postgres-username-file"], "r") as file: postgres_username = file.read() + rdp_password = None + if "--rdp-password" in validated_args: + rdp_password = validated_args["--rdp-password"] + else: + with open(validated_args["--rdp-password-file"], "r") as file: + rdp_password = file.read() + + rdp_username = None + if "--rdp-username" in validated_args: + rdp_username = validated_args["--rdp-username"] + else: + with open(validated_args["--rdp-username-file"], "r") as file: + rdp_username = file.read() + vnc_password = None if "--vnc-password" in validated_args: vnc_password = validated_args["--vnc-password"] @@ -398,6 +468,8 @@ def main() -> None: vnc_username, vnc_password, private_ssh_key, + rdp_username, + rdp_password, ) # logging.debug( diff --git a/tests/test_guacscanner.py b/tests/test_guacscanner.py index 0f4afe58..f53ac697 100644 --- a/tests/test_guacscanner.py +++ b/tests/test_guacscanner.py @@ -8,6 +8,7 @@ from unittest.mock import MagicMock, patch # Third-Party Libraries +import boto3 from moto import mock_ec2 import psycopg import pytest @@ -77,8 +78,13 @@ def test_log_levels(level): "argv", [ f"--log-level={level}", - "--postgres-password=dummy_password", - "--postgres-username=dummy_username", + "--postgres-password=dummy_db_password", + "--postgres-username=dummy_db_username", + "--private-ssh-key=dummy_key", + "--rdp-password=dummy_rdp_password", + "--rdp-username=dummy_rdp_username", + "--vnc-password=dummy_vnc_password", + "--vnc-username=dummy_vnc_username", f"--vpc-id={DUMMY_VPC_ID}", ], ): @@ -114,48 +120,116 @@ def test_bad_log_level(): assert return_code == 1, "main() should exit with error" -# def test_new_instance(): -# with patch('builtins.open', mock_open(read_data='test')) -# mock_connect = MagicMock() -# mock_cursor = MagicMock() -# mock_cursor.fetchall.return_value = expected -# mock_connect.cursor.return_value = mock_cursor - - -# @pytest.mark.parametrize("dividend, divisor, quotient", div_params) -# def test_division(dividend, divisor, quotient): -# """Verify division results.""" -# result = guacscanner.guacscanner_div(dividend, divisor) -# assert result == quotient, "result should equal quotient" - - -# @pytest.mark.slow -# def test_slow_division(): -# """Example of using a custom marker. - -# This test will only be run if --runslow is passed to pytest. -# Look in conftest.py to see how this is implemented. -# """ -# # Standard Python Libraries -# import time - -# result = guacscanner.guacscanner_div(256, 16) -# time.sleep(4) -# assert result == 16, "result should equal be 16" +@mock_ec2 +def test_new_linux_instance(): + """Verify that adding a new Linux instance works as expected.""" + # Create and populate a VPC with an EC2 instance + ec2 = boto3.client("ec2", "us-east-1") + vpc = ec2.create_vpc(CidrBlock="10.19.74.0/24") + vpc_id = vpc["Vpc"]["VpcId"] + subnet = ec2.create_subnet(CidrBlock="10.19.74.0/24", VpcId=vpc_id) + subnet_id = subnet["Subnet"]["SubnetId"] + amis = ec2.describe_images( + Filters=[ + {"Name": "Name", "Values": ["amzn-ami-hvm-2017.09.1.20171103-x86_64-gp2"]} + ] + ) + ami = amis["Images"][0] + ami_id = ami["ImageId"] + ec2.run_instances(ImageId=ami_id, SubnetId=subnet_id, MaxCount=1, MinCount=1) + + # Mock the PostgreSQL database connection + mock_connection = MagicMock(name="Mock PostgreSQL connection") + mock_cursor = MagicMock(name="Mock PostgreSQL cursor") + mock_cursor.__enter__.return_value = mock_cursor + mock_cursor.fetchone.side_effect = [[{"count": 0}], [{"connection_id": 1}]] + mock_connection.__enter__.return_value = mock_connection + mock_connection.cursor.return_value = mock_cursor + with patch.object( + sys, + "argv", + [ + "--log-level=debug", + "--postgres-password=dummy_db_password", + "--postgres-username=dummy_db_username", + "--private-ssh-key=dummy_key", + "--rdp-password=dummy_rdp_password", + "--rdp-username=dummy_rdp_username", + "--vnc-password=dummy_vnc_password", + "--vnc-username=dummy_vnc_username", + f"--vpc-id={vpc_id}", + ], + ): + with patch.object( + psycopg, "connect", return_value=mock_connection + ) as mock_connect: + guacscanner.guacscanner.main() + mock_connect.assert_called_once() + mock_connection.cursor.assert_called() + mock_connection.commit.assert_called() + mock_cursor.fetchone.assert_called() + mock_cursor.execute.assert_called() + mock_cursor.executemany.assert_called() + # Two executes, two fetchones, and one executemany + mock_cursor.call_count = 5 -# def test_zero_division(): -# """Verify that division by zero throws the correct exception.""" -# with pytest.raises(ZeroDivisionError): -# guacscanner.guacscanner_div(1, 0) +@mock_ec2 +def test_new_windows_instance(): + """Verify that adding a new Windows instance works as expected.""" + # Create and populate a VPC with an EC2 instance + ec2 = boto3.client("ec2", "us-east-1") + vpc = ec2.create_vpc(CidrBlock="10.19.74.0/24") + vpc_id = vpc["Vpc"]["VpcId"] + subnet = ec2.create_subnet(CidrBlock="10.19.74.0/24", VpcId=vpc_id) + subnet_id = subnet["Subnet"]["SubnetId"] + amis = ec2.describe_images( + Filters=[ + { + "Name": "Name", + "Values": [ + "Windows_Server-2016-English-Full-SQL_2017_Enterprise-2017.10.13" + ], + } + ] + ) + ami = amis["Images"][0] + ami_id = ami["ImageId"] + ec2.run_instances(ImageId=ami_id, SubnetId=subnet_id, MaxCount=1, MinCount=1) + + # Mock the PostgreSQL database connection + mock_connection = MagicMock(name="Mock PostgreSQL connection") + mock_cursor = MagicMock(name="Mock PostgreSQL cursor") + mock_cursor.__enter__.return_value = mock_cursor + mock_cursor.fetchone.side_effect = [[{"count": 0}], [{"connection_id": 1}]] + mock_connection.__enter__.return_value = mock_connection + mock_connection.cursor.return_value = mock_cursor -# def test_zero_divisor_argument(): -# """Verify that a divisor of zero is handled as expected.""" -# with patch.object(sys, "argv", ["bogus", "1", "0"]): -# return_code = None -# try: -# guacscanner.guacscanner.main() -# except SystemExit as sys_exit: -# return_code = sys_exit.code -# assert return_code == 1, "main() should exit with error" + with patch.object( + sys, + "argv", + [ + "--log-level=debug", + "--postgres-password=dummy_db_password", + "--postgres-username=dummy_db_username", + "--private-ssh-key=dummy_key", + "--rdp-password=dummy_rdp_password", + "--rdp-username=dummy_rdp_username", + "--vnc-password=dummy_vnc_password", + "--vnc-username=dummy_vnc_username", + f"--vpc-id={vpc_id}", + ], + ): + with patch.object( + psycopg, "connect", return_value=mock_connection + ) as mock_connect: + guacscanner.guacscanner.main() + mock_connect.assert_called_once() + mock_connection.cursor.assert_called() + mock_connection.commit.assert_called() + mock_cursor.fetchone.assert_called() + mock_cursor.execute.assert_called() + mock_cursor.executemany.assert_called() + # Two executes, two fetchones, and one executemany + mock_cursor.call_count = 5 From 6b9b5a2bc7dfce18b626287164038ec6629e26b4 Mon Sep 17 00:00:00 2001 From: Jeremy Frasier Date: Tue, 2 Nov 2021 17:02:32 -0400 Subject: [PATCH 06/92] Add tests for stale (terminated) instances --- tests/test_guacscanner.py | 123 +++++++++++++++++++++++++++++++++++++- 1 file changed, 121 insertions(+), 2 deletions(-) diff --git a/tests/test_guacscanner.py b/tests/test_guacscanner.py index f53ac697..18c4bf79 100644 --- a/tests/test_guacscanner.py +++ b/tests/test_guacscanner.py @@ -172,7 +172,64 @@ def test_new_linux_instance(): mock_cursor.execute.assert_called() mock_cursor.executemany.assert_called() # Two executes, two fetchones, and one executemany - mock_cursor.call_count = 5 + mock_cursor.call_count == 5 + + +@mock_ec2 +def test_stale_linux_instance(): + """Verify that adding a stale Linux instance works as expected.""" + # Create and populate a VPC with a stale EC2 instance + ec2 = boto3.client("ec2", "us-east-1") + vpc = ec2.create_vpc(CidrBlock="10.19.74.0/24") + vpc_id = vpc["Vpc"]["VpcId"] + subnet = ec2.create_subnet(CidrBlock="10.19.74.0/24", VpcId=vpc_id) + subnet_id = subnet["Subnet"]["SubnetId"] + amis = ec2.describe_images( + Filters=[ + {"Name": "Name", "Values": ["amzn-ami-hvm-2017.09.1.20171103-x86_64-gp2"]} + ] + ) + ami = amis["Images"][0] + ami_id = ami["ImageId"] + instances = ec2.run_instances( + ImageId=ami_id, SubnetId=subnet_id, MaxCount=1, MinCount=1 + ) + instance_id = instances["Instances"][0]["InstanceId"] + ec2.terminate_instances(InstanceIds=[instance_id]) + + # Mock the PostgreSQL database connection + mock_connection = MagicMock(name="Mock PostgreSQL connection") + mock_cursor = MagicMock(name="Mock PostgreSQL cursor") + mock_cursor.__enter__.return_value = mock_cursor + mock_cursor.fetchone.side_effect = [[{"count": 0}], [{"connection_id": 1}]] + mock_connection.__enter__.return_value = mock_connection + mock_connection.cursor.return_value = mock_cursor + + with patch.object( + sys, + "argv", + [ + "--log-level=debug", + "--postgres-password=dummy_db_password", + "--postgres-username=dummy_db_username", + "--private-ssh-key=dummy_key", + "--rdp-password=dummy_rdp_password", + "--rdp-username=dummy_rdp_username", + "--vnc-password=dummy_vnc_password", + "--vnc-username=dummy_vnc_username", + f"--vpc-id={vpc_id}", + ], + ): + with patch.object( + psycopg, "connect", return_value=mock_connection + ) as mock_connect: + guacscanner.guacscanner.main() + mock_connect.assert_called_once() + mock_connection.cursor.assert_called() + mock_connection.commit.assert_called() + mock_cursor.execute.assert_called() + # Two executes + mock_cursor.call_count == 2 @mock_ec2 @@ -232,4 +289,66 @@ def test_new_windows_instance(): mock_cursor.execute.assert_called() mock_cursor.executemany.assert_called() # Two executes, two fetchones, and one executemany - mock_cursor.call_count = 5 + mock_cursor.call_count == 5 + + +@mock_ec2 +def test_stale_windows_instance(): + """Verify that adding a stale Windows instance works as expected.""" + # Create and populate a VPC with a stale EC2 instance + ec2 = boto3.client("ec2", "us-east-1") + vpc = ec2.create_vpc(CidrBlock="10.19.74.0/24") + vpc_id = vpc["Vpc"]["VpcId"] + subnet = ec2.create_subnet(CidrBlock="10.19.74.0/24", VpcId=vpc_id) + subnet_id = subnet["Subnet"]["SubnetId"] + amis = ec2.describe_images( + Filters=[ + { + "Name": "Name", + "Values": [ + "Windows_Server-2016-English-Full-SQL_2017_Enterprise-2017.10.13" + ], + } + ] + ) + ami = amis["Images"][0] + ami_id = ami["ImageId"] + instances = ec2.run_instances( + ImageId=ami_id, SubnetId=subnet_id, MaxCount=1, MinCount=1 + ) + instance_id = instances["Instances"][0]["InstanceId"] + ec2.terminate_instances(InstanceIds=[instance_id]) + + # Mock the PostgreSQL database connection + mock_connection = MagicMock(name="Mock PostgreSQL connection") + mock_cursor = MagicMock(name="Mock PostgreSQL cursor") + mock_cursor.__enter__.return_value = mock_cursor + mock_cursor.fetchone.side_effect = [[{"count": 0}], [{"connection_id": 1}]] + mock_connection.__enter__.return_value = mock_connection + mock_connection.cursor.return_value = mock_cursor + + with patch.object( + sys, + "argv", + [ + "--log-level=debug", + "--postgres-password=dummy_db_password", + "--postgres-username=dummy_db_username", + "--private-ssh-key=dummy_key", + "--rdp-password=dummy_rdp_password", + "--rdp-username=dummy_rdp_username", + "--vnc-password=dummy_vnc_password", + "--vnc-username=dummy_vnc_username", + f"--vpc-id={vpc_id}", + ], + ): + with patch.object( + psycopg, "connect", return_value=mock_connection + ) as mock_connect: + guacscanner.guacscanner.main() + mock_connect.assert_called_once() + mock_connection.cursor.assert_called() + mock_connection.commit.assert_called() + mock_cursor.execute.assert_called() + # Two executes + mock_cursor.call_count == 2 From 736c92c55623ce8530c5447b03e35b1a921e846c Mon Sep 17 00:00:00 2001 From: Jeremy Frasier Date: Tue, 2 Nov 2021 17:04:17 -0400 Subject: [PATCH 07/92] Remove two unnecessary lines from test code --- tests/test_guacscanner.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/tests/test_guacscanner.py b/tests/test_guacscanner.py index 18c4bf79..2b395fff 100644 --- a/tests/test_guacscanner.py +++ b/tests/test_guacscanner.py @@ -201,7 +201,6 @@ def test_stale_linux_instance(): mock_connection = MagicMock(name="Mock PostgreSQL connection") mock_cursor = MagicMock(name="Mock PostgreSQL cursor") mock_cursor.__enter__.return_value = mock_cursor - mock_cursor.fetchone.side_effect = [[{"count": 0}], [{"connection_id": 1}]] mock_connection.__enter__.return_value = mock_connection mock_connection.cursor.return_value = mock_cursor @@ -323,7 +322,6 @@ def test_stale_windows_instance(): mock_connection = MagicMock(name="Mock PostgreSQL connection") mock_cursor = MagicMock(name="Mock PostgreSQL cursor") mock_cursor.__enter__.return_value = mock_cursor - mock_cursor.fetchone.side_effect = [[{"count": 0}], [{"connection_id": 1}]] mock_connection.__enter__.return_value = mock_connection mock_connection.cursor.return_value = mock_cursor From e6fe8c862930e9843a172a9602de409c8bfeabd4 Mon Sep 17 00:00:00 2001 From: Jeremy Frasier Date: Tue, 2 Nov 2021 17:06:16 -0400 Subject: [PATCH 08/92] Rename two test methods for clarity --- tests/test_guacscanner.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/test_guacscanner.py b/tests/test_guacscanner.py index 2b395fff..adca450e 100644 --- a/tests/test_guacscanner.py +++ b/tests/test_guacscanner.py @@ -176,8 +176,8 @@ def test_new_linux_instance(): @mock_ec2 -def test_stale_linux_instance(): - """Verify that adding a stale Linux instance works as expected.""" +def test_terminated_linux_instance(): + """Verify that adding a terminated Linux instance works as expected.""" # Create and populate a VPC with a stale EC2 instance ec2 = boto3.client("ec2", "us-east-1") vpc = ec2.create_vpc(CidrBlock="10.19.74.0/24") @@ -292,8 +292,8 @@ def test_new_windows_instance(): @mock_ec2 -def test_stale_windows_instance(): - """Verify that adding a stale Windows instance works as expected.""" +def test_terminated_windows_instance(): + """Verify that adding a terminated Windows instance works as expected.""" # Create and populate a VPC with a stale EC2 instance ec2 = boto3.client("ec2", "us-east-1") vpc = ec2.create_vpc(CidrBlock="10.19.74.0/24") From cdcdeb3bdc55ea2360c8c97001a8d4a6e035cb0f Mon Sep 17 00:00:00 2001 From: Jeremy Frasier Date: Tue, 2 Nov 2021 17:10:15 -0400 Subject: [PATCH 09/92] Add a couple of assert_not_called() assertions to test code --- tests/test_guacscanner.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/test_guacscanner.py b/tests/test_guacscanner.py index adca450e..f9b92758 100644 --- a/tests/test_guacscanner.py +++ b/tests/test_guacscanner.py @@ -226,7 +226,9 @@ def test_terminated_linux_instance(): mock_connect.assert_called_once() mock_connection.cursor.assert_called() mock_connection.commit.assert_called() + mock_cursor.fetchone.assert_not_called() mock_cursor.execute.assert_called() + mock_cursor.fetchone.executemany.assert_not_called() # Two executes mock_cursor.call_count == 2 @@ -347,6 +349,8 @@ def test_terminated_windows_instance(): mock_connect.assert_called_once() mock_connection.cursor.assert_called() mock_connection.commit.assert_called() + mock_cursor.fetchone.assert_not_called() mock_cursor.execute.assert_called() + mock_cursor.fetchone.executemany.assert_not_called() # Two executes mock_cursor.call_count == 2 From 5258579a732d89eb39a0558f6890f61663740cd0 Mon Sep 17 00:00:00 2001 From: Jeremy Frasier Date: Tue, 2 Nov 2021 17:11:02 -0400 Subject: [PATCH 10/92] Add a test for stopped instances --- tests/test_guacscanner.py | 53 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/tests/test_guacscanner.py b/tests/test_guacscanner.py index f9b92758..f1937ea6 100644 --- a/tests/test_guacscanner.py +++ b/tests/test_guacscanner.py @@ -233,6 +233,59 @@ def test_terminated_linux_instance(): mock_cursor.call_count == 2 +@mock_ec2 +def test_stopped_instance(): + """Verify that adding a stopped instance works as expected.""" + # Create and populate a VPC with a stopped EC2 instance + ec2 = boto3.client("ec2", "us-east-1") + vpc = ec2.create_vpc(CidrBlock="10.19.74.0/24") + vpc_id = vpc["Vpc"]["VpcId"] + subnet = ec2.create_subnet(CidrBlock="10.19.74.0/24", VpcId=vpc_id) + subnet_id = subnet["Subnet"]["SubnetId"] + amis = ec2.describe_images( + Filters=[ + {"Name": "Name", "Values": ["amzn-ami-hvm-2017.09.1.20171103-x86_64-gp2"]} + ] + ) + ami = amis["Images"][0] + ami_id = ami["ImageId"] + instances = ec2.run_instances( + ImageId=ami_id, SubnetId=subnet_id, MaxCount=1, MinCount=1 + ) + instance_id = instances["Instances"][0]["InstanceId"] + ec2.stop_instances(InstanceIds=[instance_id]) + + # Mock the PostgreSQL database connection + mock_connection = MagicMock(name="Mock PostgreSQL connection") + mock_cursor = MagicMock(name="Mock PostgreSQL cursor") + mock_cursor.__enter__.return_value = mock_cursor + mock_connection.__enter__.return_value = mock_connection + mock_connection.cursor.return_value = mock_cursor + + with patch.object( + sys, + "argv", + [ + "--log-level=debug", + "--postgres-password=dummy_db_password", + "--postgres-username=dummy_db_username", + "--private-ssh-key=dummy_key", + "--rdp-password=dummy_rdp_password", + "--rdp-username=dummy_rdp_username", + "--vnc-password=dummy_vnc_password", + "--vnc-username=dummy_vnc_username", + f"--vpc-id={vpc_id}", + ], + ): + with patch.object( + psycopg, "connect", return_value=mock_connection + ) as mock_connect: + guacscanner.guacscanner.main() + mock_connect.assert_called_once() + mock_connection.cursor.assert_not_called() + mock_connection.commit.assert_not_called() + + @mock_ec2 def test_new_windows_instance(): """Verify that adding a new Windows instance works as expected.""" From b2fd9fa95ff0c1b6b021cbfee6c7fd2953d9e4e7 Mon Sep 17 00:00:00 2001 From: Jeremy Frasier Date: Tue, 2 Nov 2021 17:12:14 -0400 Subject: [PATCH 11/92] Combine the two terminated instance tests They are really doing the same thing, and the OS doesn't matter in this case. --- tests/test_guacscanner.py | 69 ++------------------------------------- 1 file changed, 3 insertions(+), 66 deletions(-) diff --git a/tests/test_guacscanner.py b/tests/test_guacscanner.py index f1937ea6..d5ae7749 100644 --- a/tests/test_guacscanner.py +++ b/tests/test_guacscanner.py @@ -176,9 +176,9 @@ def test_new_linux_instance(): @mock_ec2 -def test_terminated_linux_instance(): - """Verify that adding a terminated Linux instance works as expected.""" - # Create and populate a VPC with a stale EC2 instance +def test_terminated_instance(): + """Verify that adding a terminated instance works as expected.""" + # Create and populate a VPC with a terminated EC2 instance ec2 = boto3.client("ec2", "us-east-1") vpc = ec2.create_vpc(CidrBlock="10.19.74.0/24") vpc_id = vpc["Vpc"]["VpcId"] @@ -344,66 +344,3 @@ def test_new_windows_instance(): mock_cursor.executemany.assert_called() # Two executes, two fetchones, and one executemany mock_cursor.call_count == 5 - - -@mock_ec2 -def test_terminated_windows_instance(): - """Verify that adding a terminated Windows instance works as expected.""" - # Create and populate a VPC with a stale EC2 instance - ec2 = boto3.client("ec2", "us-east-1") - vpc = ec2.create_vpc(CidrBlock="10.19.74.0/24") - vpc_id = vpc["Vpc"]["VpcId"] - subnet = ec2.create_subnet(CidrBlock="10.19.74.0/24", VpcId=vpc_id) - subnet_id = subnet["Subnet"]["SubnetId"] - amis = ec2.describe_images( - Filters=[ - { - "Name": "Name", - "Values": [ - "Windows_Server-2016-English-Full-SQL_2017_Enterprise-2017.10.13" - ], - } - ] - ) - ami = amis["Images"][0] - ami_id = ami["ImageId"] - instances = ec2.run_instances( - ImageId=ami_id, SubnetId=subnet_id, MaxCount=1, MinCount=1 - ) - instance_id = instances["Instances"][0]["InstanceId"] - ec2.terminate_instances(InstanceIds=[instance_id]) - - # Mock the PostgreSQL database connection - mock_connection = MagicMock(name="Mock PostgreSQL connection") - mock_cursor = MagicMock(name="Mock PostgreSQL cursor") - mock_cursor.__enter__.return_value = mock_cursor - mock_connection.__enter__.return_value = mock_connection - mock_connection.cursor.return_value = mock_cursor - - with patch.object( - sys, - "argv", - [ - "--log-level=debug", - "--postgres-password=dummy_db_password", - "--postgres-username=dummy_db_username", - "--private-ssh-key=dummy_key", - "--rdp-password=dummy_rdp_password", - "--rdp-username=dummy_rdp_username", - "--vnc-password=dummy_vnc_password", - "--vnc-username=dummy_vnc_username", - f"--vpc-id={vpc_id}", - ], - ): - with patch.object( - psycopg, "connect", return_value=mock_connection - ) as mock_connect: - guacscanner.guacscanner.main() - mock_connect.assert_called_once() - mock_connection.cursor.assert_called() - mock_connection.commit.assert_called() - mock_cursor.fetchone.assert_not_called() - mock_cursor.execute.assert_called() - mock_cursor.fetchone.executemany.assert_not_called() - # Two executes - mock_cursor.call_count == 2 From 8d13874628bf48ca55b619f7881ff35b356a9345 Mon Sep 17 00:00:00 2001 From: Jeremy Frasier Date: Tue, 2 Nov 2021 17:56:35 -0400 Subject: [PATCH 12/92] Break out removal of connections by ID into its own function Also add a new function for removing ghost instances, i.e. instances in the DB that no longer exist in AWS. Such a thing can happen if instances are terminated while the Guacamole server is down. --- src/guacscanner/guacscanner.py | 59 ++++++++++++++++++++++------------ 1 file changed, 38 insertions(+), 21 deletions(-) diff --git a/src/guacscanner/guacscanner.py b/src/guacscanner/guacscanner.py index 00e69748..c7a3f474 100644 --- a/src/guacscanner/guacscanner.py +++ b/src/guacscanner/guacscanner.py @@ -269,8 +269,18 @@ def add_instance_connection( db_connection.commit() +def remove_connection(db_connection, connection_id): + """Remove all connections corresponding to the specified ID.""" + logging.debug("Removing connection entries for %s.", connection_id) + with db_connection.cursor() as cursor: + cursor.execute(DELETE_CONNECTIONS_QUERY, (connection_id,)) + + logging.debug("Removing connection parameter entries for %s.", connection_id) + cursor.execute(DELETE_CONNECTION_PARAMETERS_QUERY, (connection_id,)) + + def remove_instance_connections(db_connection, instance): - """Remove all connections corresponding to the EC2 isntance.""" + """Remove all connections corresponding to the EC2 instance.""" logging.debug("Removing connections for %s.", instance.id) connection_name = get_connection_name(instance) with db_connection.cursor() as cursor: @@ -282,13 +292,7 @@ def remove_instance_connections(db_connection, instance): for record in cursor: logging.info("Removing entries for connections named %s.", connection_name) connection_id = record["connection_id"] - logging.debug("Removing connection entries for %s.", connection_id) - cursor.execute(DELETE_CONNECTIONS_QUERY, (connection_id,)) - - logging.debug( - "Removing connection parameter entries for %s.", connection_id - ) - cursor.execute(DELETE_CONNECTION_PARAMETERS_QUERY, (connection_id,)) + remove_connection(db_connection, connection_id) # Commit all pending transactions to the database db_connection.commit() @@ -352,6 +356,28 @@ def process_instance( ) +def check_for_ghost_instances(db_connection): + """Check to see if any connections belonging to nonexistent instances are in the database.""" + with db_connection.cursor() as cursor: + cursor.execute(NAMES_QUERY) + for record in cursor: + connection_id = record["connection_id"] + connection_name = record["connection_name"] + m = re.match(r"^.* \((?Pi-\d{17})\)$", connection_name) + instance_id = None + if m: + instance_id = m.group("id") + + ec2 = boto3.resource("ec2", region_name="us-east-1") + try: + ec2.Instance(instance_id) + except Exception as e: + print(e) + remove_connection(db_connection, connection_id) + + db_connection.commit() + + def main() -> None: """Add/remove connections to Guacamole DB as necessary.""" # Parse command line arguments @@ -457,7 +483,6 @@ def main() -> None: logging.info("Examining instances in VPC %s.", vpc_id) ec2 = boto3.resource("ec2", region_name="us-east-1") - with psycopg.connect(db_connection_string) as db_connection: for instance in ec2.Vpc(vpc_id).instances.all(): process_instance( @@ -472,17 +497,9 @@ def main() -> None: rdp_password, ) - # logging.debug( - # "Checking to see if any connections belonging to nonexistent instances are in the database." - # ) - # cursor.execute(NAMES_QUERY) - # for record in cursor: - # connection_id = record["connection_id"] - # connection_name = record["connection_name"] - # m = re.match(r"^.* \((?Pi-\d{17})\)$", connection_name) - # instance_id = None - # if m: - # instance_id = m.group("id") - # ec2.Instance(instance_id) + logging.info( + "Checking to see if any connections belonging to nonexistent instances are in the database." + ) + check_for_ghost_instances(db_connection) logging.shutdown() From 4d9718fe71af42db71b6b294e3ddbcebba14e1e7 Mon Sep 17 00:00:00 2001 From: Jeremy Frasier Date: Tue, 2 Nov 2021 17:59:22 -0400 Subject: [PATCH 13/92] Remove call_count checks They do not appear to do what I thought they would. --- tests/test_guacscanner.py | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/tests/test_guacscanner.py b/tests/test_guacscanner.py index d5ae7749..3aff3974 100644 --- a/tests/test_guacscanner.py +++ b/tests/test_guacscanner.py @@ -171,8 +171,6 @@ def test_new_linux_instance(): mock_cursor.fetchone.assert_called() mock_cursor.execute.assert_called() mock_cursor.executemany.assert_called() - # Two executes, two fetchones, and one executemany - mock_cursor.call_count == 5 @mock_ec2 @@ -228,9 +226,7 @@ def test_terminated_instance(): mock_connection.commit.assert_called() mock_cursor.fetchone.assert_not_called() mock_cursor.execute.assert_called() - mock_cursor.fetchone.executemany.assert_not_called() - # Two executes - mock_cursor.call_count == 2 + mock_cursor.executemany.assert_not_called() @mock_ec2 @@ -282,8 +278,8 @@ def test_stopped_instance(): ) as mock_connect: guacscanner.guacscanner.main() mock_connect.assert_called_once() - mock_connection.cursor.assert_not_called() - mock_connection.commit.assert_not_called() + mock_connection.cursor.assert_called() + mock_connection.commit.assert_called() @mock_ec2 @@ -342,5 +338,3 @@ def test_new_windows_instance(): mock_cursor.fetchone.assert_called() mock_cursor.execute.assert_called() mock_cursor.executemany.assert_called() - # Two executes, two fetchones, and one executemany - mock_cursor.call_count == 5 From ee5532a550a6a68f3cca73367ed6097ca35976f9 Mon Sep 17 00:00:00 2001 From: Jeremy Frasier Date: Tue, 2 Nov 2021 21:48:11 -0400 Subject: [PATCH 14/92] Bump version from 0.0.1 to 0.0.1-rc.1 --- src/guacscanner/_version.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/guacscanner/_version.py b/src/guacscanner/_version.py index 33cee844..1a85b7e4 100644 --- a/src/guacscanner/_version.py +++ b/src/guacscanner/_version.py @@ -1,2 +1,2 @@ """This file defines the version of this module.""" -__version__ = "0.0.1" +__version__ = "0.0.1-rc.1" From 4b80cd0bc82669851aa8f2739d1b71b8b772f5c6 Mon Sep 17 00:00:00 2001 From: Jeremy Frasier Date: Tue, 2 Nov 2021 21:49:02 -0400 Subject: [PATCH 15/92] Fix location of version file --- bump_version.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bump_version.sh b/bump_version.sh index e1324b84..fe8d89ad 100755 --- a/bump_version.sh +++ b/bump_version.sh @@ -6,7 +6,7 @@ set -o nounset set -o errexit set -o pipefail -VERSION_FILE=src/example/_version.py +VERSION_FILE=src/guacscanner/_version.py HELP_INFORMATION="bump_version.sh (show|major|minor|patch|prerelease|build|finalize)" From 11bb595cac285db4b95fbe61e3a6548647608389 Mon Sep 17 00:00:00 2001 From: Jeremy Frasier Date: Tue, 2 Nov 2021 22:03:35 -0400 Subject: [PATCH 16/92] Improve function that checks for ghost instances --- src/guacscanner/guacscanner.py | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/src/guacscanner/guacscanner.py b/src/guacscanner/guacscanner.py index c7a3f474..19d2c573 100644 --- a/src/guacscanner/guacscanner.py +++ b/src/guacscanner/guacscanner.py @@ -356,8 +356,9 @@ def process_instance( ) -def check_for_ghost_instances(db_connection): +def check_for_ghost_instances(db_connection, instances): """Check to see if any connections belonging to nonexistent instances are in the database.""" + instance_ids = [instance.id for instance in instances] with db_connection.cursor() as cursor: cursor.execute(NAMES_QUERY) for record in cursor: @@ -367,12 +368,17 @@ def check_for_ghost_instances(db_connection): instance_id = None if m: instance_id = m.group("id") - - ec2 = boto3.resource("ec2", region_name="us-east-1") - try: - ec2.Instance(instance_id) - except Exception as e: - print(e) + else: + logging.error( + 'Connection name "%s" does not contain a valid instance ID', + connection_name, + ) + + if instance_id not in instance_ids: + logging.info( + "Connection for %s being removed since that instance no longer exists.", + instance_id, + ) remove_connection(db_connection, connection_id) db_connection.commit() @@ -483,8 +489,9 @@ def main() -> None: logging.info("Examining instances in VPC %s.", vpc_id) ec2 = boto3.resource("ec2", region_name="us-east-1") + instances = ec2.Vpc(vpc_id).instances.all() with psycopg.connect(db_connection_string) as db_connection: - for instance in ec2.Vpc(vpc_id).instances.all(): + for instance in instances: process_instance( db_connection, instance, @@ -500,6 +507,6 @@ def main() -> None: logging.info( "Checking to see if any connections belonging to nonexistent instances are in the database." ) - check_for_ghost_instances(db_connection) + check_for_ghost_instances(db_connection, instances) logging.shutdown() From 1bd2d741cc0b7db30b47a95a07b80a2a2bd3e44b Mon Sep 17 00:00:00 2001 From: Jeremy Frasier Date: Tue, 2 Nov 2021 22:12:35 -0400 Subject: [PATCH 17/92] Improve handling of input arguments --- src/guacscanner/guacscanner.py | 48 ++++++++++++---------------------- 1 file changed, 16 insertions(+), 32 deletions(-) diff --git a/src/guacscanner/guacscanner.py b/src/guacscanner/guacscanner.py index 19d2c573..1329770c 100644 --- a/src/guacscanner/guacscanner.py +++ b/src/guacscanner/guacscanner.py @@ -430,61 +430,45 @@ def main() -> None: postgres_port = DEFAULT_POSTGRES_PORT remove_instance_states = DEFAULT_REMOVE_INSTANCE_STATES - postgres_password = None - if "--postgres-password" in validated_args: - postgres_password = validated_args["--postgres-password"] - else: + postgres_password = validated_args["--postgres-password"] + if postgres_password is None: with open(validated_args["--postgres-password-file"], "r") as file: postgres_password = file.read() - postgres_username = None - if "--postgres-username" in validated_args: - postgres_username = validated_args["--postgres-username"] - else: + postgres_username = validated_args["--postgres-username"] + if postgres_username is None: with open(validated_args["--postgres-username-file"], "r") as file: postgres_username = file.read() - rdp_password = None - if "--rdp-password" in validated_args: - rdp_password = validated_args["--rdp-password"] - else: + rdp_password = validated_args["--rdp-password"] + if rdp_password is None: with open(validated_args["--rdp-password-file"], "r") as file: rdp_password = file.read() - rdp_username = None - if "--rdp-username" in validated_args: - rdp_username = validated_args["--rdp-username"] - else: + rdp_username = validated_args["--rdp-username"] + if rdp_username is None: with open(validated_args["--rdp-username-file"], "r") as file: rdp_username = file.read() - vnc_password = None - if "--vnc-password" in validated_args: - vnc_password = validated_args["--vnc-password"] - else: + vnc_password = validated_args["--vnc-password"] + if vnc_password is None: with open(validated_args["--vnc-password-file"], "r") as file: vnc_password = file.read() - vnc_username = None - if "--vnc-username" in validated_args: - vnc_username = validated_args["--vnc-username"] - else: + vnc_username = validated_args["--vnc-username"] + if vnc_username is None: with open(validated_args["--vnc-username-file"], "r") as file: vnc_username = file.read() - private_ssh_key = None - if "--private-ssh-key" in validated_args: - private_ssh_key = validated_args["--private-ssh-key"] - else: + private_ssh_key = validated_args["--private-ssh-key"] + if private_ssh_key is None: with open(validated_args["--private-ssh-key-file"], "r") as file: private_ssh_key = file.read() db_connection_string = f"postgresql://{postgres_username}:{postgres_password}@{postgres_hostname}:{postgres_port}/{postgres_db_name}" - vpc_id = None - if "--vpc-id" in validated_args: - vpc_id = validated_args["--vpc-id"] - else: + vpc_id = validated_args["--vpc-id"] + if vpc_id is None: vpc_id = ec2_metadata.vpc_id logging.info("Examining instances in VPC %s.", vpc_id) From c9f73c2bb8aecf773ffb53e03ccd72589955ddd9 Mon Sep 17 00:00:00 2001 From: Jeremy Frasier Date: Wed, 3 Nov 2021 10:47:19 -0400 Subject: [PATCH 18/92] Skip instances launched from AMIs that match any of a list of regexes Also pre-compile all regexes where possible. --- src/guacscanner/guacscanner.py | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/src/guacscanner/guacscanner.py b/src/guacscanner/guacscanner.py index 1329770c..c076a059 100644 --- a/src/guacscanner/guacscanner.py +++ b/src/guacscanner/guacscanner.py @@ -61,6 +61,11 @@ DEFAULT_REMOVE_INSTANCE_STATES = [ "terminated", ] +DEFAULT_AMI_SKIP_REGEXES = [ + re.compile(r"^guacamole-.*$"), + re.compile(r"^samba-.*$"), +] +INSTANCE_ID_REGEX = re.compile(r"^.* \((?Pi-\d{17})\)$") COUNT_QUERY = sql.SQL( "SELECT COUNT({id_field}) FROM {table} WHERE {name_field} = %s" ).format( @@ -364,7 +369,7 @@ def check_for_ghost_instances(db_connection, instances): for record in cursor: connection_id = record["connection_id"] connection_name = record["connection_name"] - m = re.match(r"^.* \((?Pi-\d{17})\)$", connection_name) + m = INSTANCE_ID_REGEX.match(connection_name) instance_id = None if m: instance_id = m.group("id") @@ -476,6 +481,12 @@ def main() -> None: instances = ec2.Vpc(vpc_id).instances.all() with psycopg.connect(db_connection_string) as db_connection: for instance in instances: + ami = ec2.Image(instance.image_id) + # Early exit if this instance is running an AMI that we + # want to avoid adding to Guacamole. + if any([regex.match(ami.name) for regex in DEFAULT_AMI_SKIP_REGEXES]): + continue + process_instance( db_connection, instance, From 1cefa6b14772c9adc8de2fd4fe1fa832dcaaf3f7 Mon Sep 17 00:00:00 2001 From: Jeremy Frasier Date: Wed, 3 Nov 2021 10:48:51 -0400 Subject: [PATCH 19/92] Bump version from 0.0.1-rc.1 to 0.0.1-rc.2 --- src/guacscanner/_version.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/guacscanner/_version.py b/src/guacscanner/_version.py index 1a85b7e4..c150520b 100644 --- a/src/guacscanner/_version.py +++ b/src/guacscanner/_version.py @@ -1,2 +1,2 @@ """This file defines the version of this module.""" -__version__ = "0.0.1-rc.1" +__version__ = "0.0.1-rc.2" From bc01b3e0ac32591908027871aa9d77146c79cc37 Mon Sep 17 00:00:00 2001 From: Jeremy Frasier Date: Wed, 3 Nov 2021 13:06:43 -0400 Subject: [PATCH 20/92] Fix a bug in the way the IF of the default VPC is retrieved via instance metadata --- src/guacscanner/guacscanner.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/guacscanner/guacscanner.py b/src/guacscanner/guacscanner.py index c076a059..f43f0648 100644 --- a/src/guacscanner/guacscanner.py +++ b/src/guacscanner/guacscanner.py @@ -473,11 +473,17 @@ def main() -> None: db_connection_string = f"postgresql://{postgres_username}:{postgres_password}@{postgres_hostname}:{postgres_port}/{postgres_db_name}" vpc_id = validated_args["--vpc-id"] + + ec2 = boto3.resource("ec2", region_name="us-east-1") + + # If no VPC ID was specified on the command line then grab the VPC + # ID where this instance resides and use that. if vpc_id is None: - vpc_id = ec2_metadata.vpc_id + instance_id = ec2_metadata.instance_id + instance = ec2.Instance(instance_id) + vpc_id = instance.vpc_id logging.info("Examining instances in VPC %s.", vpc_id) - ec2 = boto3.resource("ec2", region_name="us-east-1") instances = ec2.Vpc(vpc_id).instances.all() with psycopg.connect(db_connection_string) as db_connection: for instance in instances: From 2cfdb8b4794265437d4103d759c890c42f8b1a00 Mon Sep 17 00:00:00 2001 From: Jeremy Frasier Date: Wed, 3 Nov 2021 13:07:38 -0400 Subject: [PATCH 21/92] Bump version from 0.0.1-rc.2 to 0.0.1-rc.3 --- src/guacscanner/_version.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/guacscanner/_version.py b/src/guacscanner/_version.py index c150520b..52cb50ea 100644 --- a/src/guacscanner/_version.py +++ b/src/guacscanner/_version.py @@ -1,2 +1,2 @@ """This file defines the version of this module.""" -__version__ = "0.0.1-rc.2" +__version__ = "0.0.1-rc.3" From 1000145537a6acff80547092a0e325e24ade31b4 Mon Sep 17 00:00:00 2001 From: Jeremy Frasier Date: Wed, 3 Nov 2021 13:36:12 -0400 Subject: [PATCH 22/92] Add a --oneshot option that runs the update loop only once This is useful for testing, although it isn't the way the utility is usually run. --- src/guacscanner/guacscanner.py | 56 +++++++++++++++++++++------------- tests/test_guacscanner.py | 5 +++ 2 files changed, 39 insertions(+), 22 deletions(-) diff --git a/src/guacscanner/guacscanner.py b/src/guacscanner/guacscanner.py index f43f0648..fb31058f 100644 --- a/src/guacscanner/guacscanner.py +++ b/src/guacscanner/guacscanner.py @@ -8,7 +8,7 @@ >0 An error occurred. Usage: - guacscanner [--log-level=LEVEL] [--postgres-password=PASSWORD|--postgres-password-file=FILENAME] [--private-ssh-key=KEY|--private-ssh-key-file=FILENAME] [--postgres-username=USERNAME|--postgres-username-file=FILENAME] [--rdp-password=PASSWORD|--rdp-password-file=FILENAME] [--rdp-username=USERNAME|--rdp-username-file=FILENAME] [--vnc-password=PASSWORD|--vnc-password-file=FILENAME] [--vnc-username=USERNAME|--vnc-username-file=FILENAME] [--vpc-id=VPC_ID] + guacscanner [--log-level=LEVEL] [--oneshot] [--postgres-password=PASSWORD|--postgres-password-file=FILENAME] [--private-ssh-key=KEY|--private-ssh-key-file=FILENAME] [--postgres-username=USERNAME|--postgres-username-file=FILENAME] [--rdp-password=PASSWORD|--rdp-password-file=FILENAME] [--rdp-username=USERNAME|--rdp-username-file=FILENAME] [--vnc-password=PASSWORD|--vnc-password-file=FILENAME] [--vnc-username=USERNAME|--vnc-username-file=FILENAME] [--vpc-id=VPC_ID] guacscanner (-h | --help) Options: @@ -16,6 +16,7 @@ --log-level=LEVEL If specified, then the log level will be set to the specified value. Valid values are "debug", "info", "warning", "error", and "critical". [default: info] + --oneshot If present then the loop that adds (removes) connections for new (terminated) instances will only be run once. --postgres-password=PASSWORD If specified then the specified value will be used as the password when connecting to the PostgreSQL database. Otherwise, the password will be read from a local file. --postgres-password-file=FILENAME The file from which the PostgreSQL password will be read. [default: /run/secrets/postgres-password] --postgres-username=USERNAME If specified then the specified value will be used when connecting to the PostgreSQL database. Otherwise, the username will be read from a local file. @@ -435,6 +436,9 @@ def main() -> None: postgres_port = DEFAULT_POSTGRES_PORT remove_instance_states = DEFAULT_REMOVE_INSTANCE_STATES + oneshot = validated_args["--oneshot"] + logging.debug("oneshot is %s.", oneshot) + postgres_password = validated_args["--postgres-password"] if postgres_password is None: with open(validated_args["--postgres-password-file"], "r") as file: @@ -485,29 +489,37 @@ def main() -> None: logging.info("Examining instances in VPC %s.", vpc_id) instances = ec2.Vpc(vpc_id).instances.all() + keep_looping = True with psycopg.connect(db_connection_string) as db_connection: - for instance in instances: - ami = ec2.Image(instance.image_id) - # Early exit if this instance is running an AMI that we - # want to avoid adding to Guacamole. - if any([regex.match(ami.name) for regex in DEFAULT_AMI_SKIP_REGEXES]): - continue - - process_instance( - db_connection, - instance, - add_instance_states, - remove_instance_states, - vnc_username, - vnc_password, - private_ssh_key, - rdp_username, - rdp_password, + while keep_looping: + for instance in instances: + ami = ec2.Image(instance.image_id) + # Early exit if this instance is running an AMI that + # we want to avoid adding to Guacamole. + if any([regex.match(ami.name) for regex in DEFAULT_AMI_SKIP_REGEXES]): + continue + + process_instance( + db_connection, + instance, + add_instance_states, + remove_instance_states, + vnc_username, + vnc_password, + private_ssh_key, + rdp_username, + rdp_password, + ) + + logging.info( + "Checking to see if any connections belonging to nonexistent instances are in the database." ) + check_for_ghost_instances(db_connection, instances) - logging.info( - "Checking to see if any connections belonging to nonexistent instances are in the database." - ) - check_for_ghost_instances(db_connection, instances) + if oneshot: + logging.debug( + "Stopping Guacamole connection update loop because --oneshot is present." + ) + keep_looping = False logging.shutdown() diff --git a/tests/test_guacscanner.py b/tests/test_guacscanner.py index 3aff3974..dc37f72a 100644 --- a/tests/test_guacscanner.py +++ b/tests/test_guacscanner.py @@ -78,6 +78,7 @@ def test_log_levels(level): "argv", [ f"--log-level={level}", + "--oneshot", "--postgres-password=dummy_db_password", "--postgres-username=dummy_db_username", "--private-ssh-key=dummy_key", @@ -151,6 +152,7 @@ def test_new_linux_instance(): "argv", [ "--log-level=debug", + "--oneshot", "--postgres-password=dummy_db_password", "--postgres-username=dummy_db_username", "--private-ssh-key=dummy_key", @@ -207,6 +209,7 @@ def test_terminated_instance(): "argv", [ "--log-level=debug", + "--oneshot", "--postgres-password=dummy_db_password", "--postgres-username=dummy_db_username", "--private-ssh-key=dummy_key", @@ -263,6 +266,7 @@ def test_stopped_instance(): "argv", [ "--log-level=debug", + "--oneshot", "--postgres-password=dummy_db_password", "--postgres-username=dummy_db_username", "--private-ssh-key=dummy_key", @@ -318,6 +322,7 @@ def test_new_windows_instance(): "argv", [ "--log-level=debug", + "--oneshot", "--postgres-password=dummy_db_password", "--postgres-username=dummy_db_username", "--private-ssh-key=dummy_key", From 3ba0f33e92fcdec916047b9c61a06a2d07ddb480 Mon Sep 17 00:00:00 2001 From: Jeremy Frasier Date: Wed, 3 Nov 2021 15:16:40 -0400 Subject: [PATCH 23/92] Bump version from 0.0.1-rc.3 to 0.0.1-rc.4 --- src/guacscanner/_version.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/guacscanner/_version.py b/src/guacscanner/_version.py index 52cb50ea..61a8bac3 100644 --- a/src/guacscanner/_version.py +++ b/src/guacscanner/_version.py @@ -1,2 +1,2 @@ """This file defines the version of this module.""" -__version__ = "0.0.1-rc.3" +__version__ = "0.0.1-rc.4" From 5c43c211f3a48ed8b13b51b865ce3056e8ced9f1 Mon Sep 17 00:00:00 2001 From: Jeremy Frasier Date: Wed, 3 Nov 2021 16:06:56 -0400 Subject: [PATCH 24/92] Add an input parameter to specify the sleep between loop iterations --- src/guacscanner/guacscanner.py | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/src/guacscanner/guacscanner.py b/src/guacscanner/guacscanner.py index fb31058f..ac2d6d01 100644 --- a/src/guacscanner/guacscanner.py +++ b/src/guacscanner/guacscanner.py @@ -8,7 +8,7 @@ >0 An error occurred. Usage: - guacscanner [--log-level=LEVEL] [--oneshot] [--postgres-password=PASSWORD|--postgres-password-file=FILENAME] [--private-ssh-key=KEY|--private-ssh-key-file=FILENAME] [--postgres-username=USERNAME|--postgres-username-file=FILENAME] [--rdp-password=PASSWORD|--rdp-password-file=FILENAME] [--rdp-username=USERNAME|--rdp-username-file=FILENAME] [--vnc-password=PASSWORD|--vnc-password-file=FILENAME] [--vnc-username=USERNAME|--vnc-username-file=FILENAME] [--vpc-id=VPC_ID] + guacscanner [--log-level=LEVEL] [--oneshot] [--sleep=SECONDS] [--postgres-password=PASSWORD|--postgres-password-file=FILENAME] [--private-ssh-key=KEY|--private-ssh-key-file=FILENAME] [--postgres-username=USERNAME|--postgres-username-file=FILENAME] [--rdp-password=PASSWORD|--rdp-password-file=FILENAME] [--rdp-username=USERNAME|--rdp-username-file=FILENAME] [--vnc-password=PASSWORD|--vnc-password-file=FILENAME] [--vnc-username=USERNAME|--vnc-username-file=FILENAME] [--vpc-id=VPC_ID] guacscanner (-h | --help) Options: @@ -27,6 +27,7 @@ --rdp-password-file=FILENAME The file from which the RDP password will be read. [default: /run/secrets/rdp-password] --rdp-username=USERNAME If specified then the specified value will be used for the RDP username. Otherwise, the username will be read from a local file. --rdp-username-file=FILENAME The file from which the RDP username will be read. [default: /run/secrets/rdp-username] + --sleep=SECONDS Sleep for the specified number of seconds between executions of the Guacamole connection update loop. [default: 60] --vnc-password=PASSWORD If specified then the specified value will be used for the VNC password. Otherwise, the password will be read from a local file. --vnc-password-file=FILENAME The file from which the VNC password will be read. [default: /run/secrets/vnc-password] --vnc-username=USERNAME If specified then the specified value will be used for the VNC username. Otherwise, the username will be read from a local file. @@ -42,6 +43,7 @@ import logging import re import sys +import time # Third-Party Libraries import boto3 @@ -404,6 +406,10 @@ def main() -> None: error="Possible values for --log-level are " + "debug, info, warning, error, and critical.", ), + "--sleep": And( + Use(float), + error="Value for --sleep must be parseable as a floating point number.", + ), Optional("--vpc-id"): Or( None, And( @@ -411,7 +417,7 @@ def main() -> None: Use(str.lower), lambda x: re.fullmatch(r"^vpc-([0-9a-f]{8}|[0-9a-f]{17})$", x) is not None, - error="Possible values for --vpc-id are the characters vpc- followed by 17 hexadecimal digits.", + error="Possible values for --vpc-id are the characters vpc- followed by either 8 or 17 hexadecimal digits.", ), ), str: object, # Don't care about other keys, if any @@ -521,5 +527,8 @@ def main() -> None: "Stopping Guacamole connection update loop because --oneshot is present." ) keep_looping = False + continue + + time.sleep(validated_args["--sleep"]) logging.shutdown() From 368a0fb1ed872df52bdefb2be34a16b136cd0011 Mon Sep 17 00:00:00 2001 From: Jeremy Frasier Date: Wed, 3 Nov 2021 16:09:53 -0400 Subject: [PATCH 25/92] Precompile the VPC ID regex --- src/guacscanner/guacscanner.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/guacscanner/guacscanner.py b/src/guacscanner/guacscanner.py index ac2d6d01..35e1dffb 100644 --- a/src/guacscanner/guacscanner.py +++ b/src/guacscanner/guacscanner.py @@ -69,6 +69,7 @@ re.compile(r"^samba-.*$"), ] INSTANCE_ID_REGEX = re.compile(r"^.* \((?Pi-\d{17})\)$") +VPC_ID_REGEX = re.compile(r"^vpc-([0-9a-f]{8}|[0-9a-f]{17})$") COUNT_QUERY = sql.SQL( "SELECT COUNT({id_field}) FROM {table} WHERE {name_field} = %s" ).format( @@ -415,8 +416,7 @@ def main() -> None: And( str, Use(str.lower), - lambda x: re.fullmatch(r"^vpc-([0-9a-f]{8}|[0-9a-f]{17})$", x) - is not None, + lambda x: VPC_ID_REGEX.match(x) is not None, error="Possible values for --vpc-id are the characters vpc- followed by either 8 or 17 hexadecimal digits.", ), ), From c9c3396cea3cef54e65085646b0bbf7cddf99c08 Mon Sep 17 00:00:00 2001 From: Jeremy Frasier Date: Wed, 3 Nov 2021 16:11:12 -0400 Subject: [PATCH 26/92] Bump version from 0.0.1-rc.4 to 0.0.1-rc.5 --- src/guacscanner/_version.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/guacscanner/_version.py b/src/guacscanner/_version.py index 61a8bac3..e7e60d88 100644 --- a/src/guacscanner/_version.py +++ b/src/guacscanner/_version.py @@ -1,2 +1,2 @@ """This file defines the version of this module.""" -__version__ = "0.0.1-rc.4" +__version__ = "0.0.1-rc.5" From 21ac2ed24d3a31d3aa7f0f6dabcc93cffe899b75 Mon Sep 17 00:00:00 2001 From: Jeremy Frasier Date: Wed, 3 Nov 2021 21:50:14 -0400 Subject: [PATCH 27/92] Move DB connection attempt inside the loop This allows us to wrap it in a try block, so we can repeat the loop if the connection fails. --- src/guacscanner/guacscanner.py | 67 +++++++++++++++++++--------------- 1 file changed, 38 insertions(+), 29 deletions(-) diff --git a/src/guacscanner/guacscanner.py b/src/guacscanner/guacscanner.py index 35e1dffb..c21f6c0c 100644 --- a/src/guacscanner/guacscanner.py +++ b/src/guacscanner/guacscanner.py @@ -481,6 +481,7 @@ def main() -> None: private_ssh_key = file.read() db_connection_string = f"postgresql://{postgres_username}:{postgres_password}@{postgres_hostname}:{postgres_port}/{postgres_db_name}" + logging.debug("DB connection string is %s.", db_connection_string) vpc_id = validated_args["--vpc-id"] @@ -496,39 +497,47 @@ def main() -> None: instances = ec2.Vpc(vpc_id).instances.all() keep_looping = True - with psycopg.connect(db_connection_string) as db_connection: - while keep_looping: - for instance in instances: - ami = ec2.Image(instance.image_id) - # Early exit if this instance is running an AMI that - # we want to avoid adding to Guacamole. - if any([regex.match(ami.name) for regex in DEFAULT_AMI_SKIP_REGEXES]): - continue + while keep_looping: + try: + with psycopg.connect(db_connection_string) as db_connection: + for instance in instances: + ami = ec2.Image(instance.image_id) + # Early exit if this instance is running an AMI + # that we want to avoid adding to Guacamole. + if any( + [regex.match(ami.name) for regex in DEFAULT_AMI_SKIP_REGEXES] + ): + continue + + process_instance( + db_connection, + instance, + add_instance_states, + remove_instance_states, + vnc_username, + vnc_password, + private_ssh_key, + rdp_username, + rdp_password, + ) - process_instance( - db_connection, - instance, - add_instance_states, - remove_instance_states, - vnc_username, - vnc_password, - private_ssh_key, - rdp_username, - rdp_password, + logging.info( + "Checking to see if any connections belonging to nonexistent instances are in the database." ) + check_for_ghost_instances(db_connection, instances) - logging.info( - "Checking to see if any connections belonging to nonexistent instances are in the database." + if oneshot: + logging.debug( + "Stopping Guacamole connection update loop because --oneshot is present." + ) + keep_looping = False + continue + except psycopg.OperationalError: + logging.exception( + "Unable to connect to the PostgreSQL database backending Guacamole." ) - check_for_ghost_instances(db_connection, instances) - - if oneshot: - logging.debug( - "Stopping Guacamole connection update loop because --oneshot is present." - ) - keep_looping = False - continue + continue - time.sleep(validated_args["--sleep"]) + time.sleep(validated_args["--sleep"]) logging.shutdown() From 077359f5c01ea43767332d9146afe0d925640461 Mon Sep 17 00:00:00 2001 From: Jeremy Frasier Date: Wed, 3 Nov 2021 21:51:17 -0400 Subject: [PATCH 28/92] Bump version from 0.0.1-rc.5 to 0.0.1-rc.6 --- src/guacscanner/_version.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/guacscanner/_version.py b/src/guacscanner/_version.py index e7e60d88..bad5d195 100644 --- a/src/guacscanner/_version.py +++ b/src/guacscanner/_version.py @@ -1,2 +1,2 @@ """This file defines the version of this module.""" -__version__ = "0.0.1-rc.5" +__version__ = "0.0.1-rc.6" From 20f5e3904c006633c6312cf29776965ef51bd5c3 Mon Sep 17 00:00:00 2001 From: Jeremy Frasier Date: Wed, 3 Nov 2021 22:15:32 -0400 Subject: [PATCH 29/92] Modify the connection name to be something more meaningful --- src/guacscanner/guacscanner.py | 3 ++- tests/test_guacscanner.py | 36 ++++++++++++++++++++++++++++++---- 2 files changed, 34 insertions(+), 5 deletions(-) diff --git a/src/guacscanner/guacscanner.py b/src/guacscanner/guacscanner.py index c21f6c0c..c94d4161 100644 --- a/src/guacscanner/guacscanner.py +++ b/src/guacscanner/guacscanner.py @@ -309,7 +309,8 @@ def remove_instance_connections(db_connection, instance): def get_connection_name(instance): """Return the unique connection name for an EC2 instance.""" - return f"{instance.private_dns_name} ({instance.id})" + name = [tag["Value"] for tag in instance.tags if tag["Key"] == "Name"][0] + return f"{name} ({instance.id})" def process_instance( diff --git a/tests/test_guacscanner.py b/tests/test_guacscanner.py index dc37f72a..b6a71d6c 100644 --- a/tests/test_guacscanner.py +++ b/tests/test_guacscanner.py @@ -137,7 +137,15 @@ def test_new_linux_instance(): ) ami = amis["Images"][0] ami_id = ami["ImageId"] - ec2.run_instances(ImageId=ami_id, SubnetId=subnet_id, MaxCount=1, MinCount=1) + ec2.run_instances( + ImageId=ami_id, + SubnetId=subnet_id, + MaxCount=1, + MinCount=1, + TagSpecifications=[ + {"ResourceType": "instance", "Tags": [{"Key": "Name", "Value": "Linux"}]} + ], + ) # Mock the PostgreSQL database connection mock_connection = MagicMock(name="Mock PostgreSQL connection") @@ -192,7 +200,13 @@ def test_terminated_instance(): ami = amis["Images"][0] ami_id = ami["ImageId"] instances = ec2.run_instances( - ImageId=ami_id, SubnetId=subnet_id, MaxCount=1, MinCount=1 + ImageId=ami_id, + SubnetId=subnet_id, + MaxCount=1, + MinCount=1, + TagSpecifications=[ + {"ResourceType": "instance", "Tags": [{"Key": "Name", "Value": "Linux"}]} + ], ) instance_id = instances["Instances"][0]["InstanceId"] ec2.terminate_instances(InstanceIds=[instance_id]) @@ -249,7 +263,13 @@ def test_stopped_instance(): ami = amis["Images"][0] ami_id = ami["ImageId"] instances = ec2.run_instances( - ImageId=ami_id, SubnetId=subnet_id, MaxCount=1, MinCount=1 + ImageId=ami_id, + SubnetId=subnet_id, + MaxCount=1, + MinCount=1, + TagSpecifications=[ + {"ResourceType": "instance", "Tags": [{"Key": "Name", "Value": "Linux"}]} + ], ) instance_id = instances["Instances"][0]["InstanceId"] ec2.stop_instances(InstanceIds=[instance_id]) @@ -307,7 +327,15 @@ def test_new_windows_instance(): ) ami = amis["Images"][0] ami_id = ami["ImageId"] - ec2.run_instances(ImageId=ami_id, SubnetId=subnet_id, MaxCount=1, MinCount=1) + ec2.run_instances( + ImageId=ami_id, + SubnetId=subnet_id, + MaxCount=1, + MinCount=1, + TagSpecifications=[ + {"ResourceType": "instance", "Tags": [{"Key": "Name", "Value": "Windows"}]} + ], + ) # Mock the PostgreSQL database connection mock_connection = MagicMock(name="Mock PostgreSQL connection") From 0229d0f9215ae524cff7b6e6ae89567acf39a0c8 Mon Sep 17 00:00:00 2001 From: Jeremy Frasier Date: Wed, 3 Nov 2021 22:16:16 -0400 Subject: [PATCH 30/92] Bump version from 0.0.1-rc.6 to 0.0.1-rc.7 --- src/guacscanner/_version.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/guacscanner/_version.py b/src/guacscanner/_version.py index bad5d195..d10142b9 100644 --- a/src/guacscanner/_version.py +++ b/src/guacscanner/_version.py @@ -1,2 +1,2 @@ """This file defines the version of this module.""" -__version__ = "0.0.1-rc.6" +__version__ = "0.0.1-rc.7" From 5c8ee8468adee4d444f98e4d9105be2c82a7047d Mon Sep 17 00:00:00 2001 From: Jeremy Frasier Date: Thu, 4 Nov 2021 09:27:31 -0400 Subject: [PATCH 31/92] Modify DB connectin string to use key/value pairs instead of a URI This avoids the need to URI escape the password, for example, in the event that it contains special characters. --- src/guacscanner/guacscanner.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/guacscanner/guacscanner.py b/src/guacscanner/guacscanner.py index c94d4161..55e63000 100644 --- a/src/guacscanner/guacscanner.py +++ b/src/guacscanner/guacscanner.py @@ -481,7 +481,7 @@ def main() -> None: with open(validated_args["--private-ssh-key-file"], "r") as file: private_ssh_key = file.read() - db_connection_string = f"postgresql://{postgres_username}:{postgres_password}@{postgres_hostname}:{postgres_port}/{postgres_db_name}" + db_connection_string = f"user={postgres_username} password={postgres_password} host={postgres_hostname} port={postgres_port} dbname={postgres_db_name}" logging.debug("DB connection string is %s.", db_connection_string) vpc_id = validated_args["--vpc-id"] From 30b220617042f56c9542d5cfa4e6807329e18c06 Mon Sep 17 00:00:00 2001 From: Jeremy Frasier Date: Thu, 4 Nov 2021 09:32:04 -0400 Subject: [PATCH 32/92] Remove continue statement This way the sleep will take place before the next DB connection attempt is made. --- src/guacscanner/guacscanner.py | 1 - 1 file changed, 1 deletion(-) diff --git a/src/guacscanner/guacscanner.py b/src/guacscanner/guacscanner.py index 55e63000..77366f0a 100644 --- a/src/guacscanner/guacscanner.py +++ b/src/guacscanner/guacscanner.py @@ -537,7 +537,6 @@ def main() -> None: logging.exception( "Unable to connect to the PostgreSQL database backending Guacamole." ) - continue time.sleep(validated_args["--sleep"]) From db1c891126a34d7c372738dcff725958fff9ee83 Mon Sep 17 00:00:00 2001 From: Jeremy Frasier Date: Thu, 4 Nov 2021 09:32:53 -0400 Subject: [PATCH 33/92] Bump version from 0.0.1-rc.7 to 0.0.1-rc.8 --- src/guacscanner/_version.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/guacscanner/_version.py b/src/guacscanner/_version.py index d10142b9..587a83f7 100644 --- a/src/guacscanner/_version.py +++ b/src/guacscanner/_version.py @@ -1,2 +1,2 @@ """This file defines the version of this module.""" -__version__ = "0.0.1-rc.7" +__version__ = "0.0.1-rc.8" From b461185aa43aa4867164e1b14a3273ba02ced771 Mon Sep 17 00:00:00 2001 From: Jeremy Frasier Date: Thu, 4 Nov 2021 11:00:09 -0400 Subject: [PATCH 34/92] Remove unnecessary line --- src/guacscanner/guacscanner.py | 1 - 1 file changed, 1 deletion(-) diff --git a/src/guacscanner/guacscanner.py b/src/guacscanner/guacscanner.py index 77366f0a..0fa1e841 100644 --- a/src/guacscanner/guacscanner.py +++ b/src/guacscanner/guacscanner.py @@ -335,7 +335,6 @@ def process_instance( instance.id, state, ) - db_connection.cursor() if not instance_connection_exists(db_connection, connection_name): logging.info("Adding a connection for %s.", instance.id) add_instance_connection( From 811847bcffb80dc8bc56c8e9dfa11cc2ab6ed54e Mon Sep 17 00:00:00 2001 From: Jeremy Frasier Date: Thu, 4 Nov 2021 11:07:40 -0400 Subject: [PATCH 35/92] Use a dictionary row factory for the DB connection Otherwise the results are returned as tuples, which is not what I was expecting. It's also inconvenient to have to figure out the index of the field one wants, so a dictionary is preferred. --- src/guacscanner/guacscanner.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/guacscanner/guacscanner.py b/src/guacscanner/guacscanner.py index 0fa1e841..2d3ce455 100644 --- a/src/guacscanner/guacscanner.py +++ b/src/guacscanner/guacscanner.py @@ -499,7 +499,9 @@ def main() -> None: keep_looping = True while keep_looping: try: - with psycopg.connect(db_connection_string) as db_connection: + with psycopg.connect( + db_connection_string, row_factory=psycopg.rows.dict_row + ) as db_connection: for instance in instances: ami = ec2.Image(instance.image_id) # Early exit if this instance is running an AMI From a6d931fcad3127937774ccaa9dc06ddb84026ff6 Mon Sep 17 00:00:00 2001 From: Jeremy Frasier Date: Thu, 4 Nov 2021 11:11:58 -0400 Subject: [PATCH 36/92] Bump version from 0.0.1-rc.8 to 0.0.1-rc.9 --- src/guacscanner/_version.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/guacscanner/_version.py b/src/guacscanner/_version.py index 587a83f7..bc7a8021 100644 --- a/src/guacscanner/_version.py +++ b/src/guacscanner/_version.py @@ -1,2 +1,2 @@ """This file defines the version of this module.""" -__version__ = "0.0.1-rc.8" +__version__ = "0.0.1-rc.9" From faafc6038619c0308cb9faa673e07a8ed293749b Mon Sep 17 00:00:00 2001 From: Jeremy Frasier Date: Thu, 4 Nov 2021 12:24:06 -0400 Subject: [PATCH 37/92] Fix handling of return values from fetchone() fetchone() returns a dictionary, not a list containing a single dictionary. --- src/guacscanner/guacscanner.py | 4 ++-- tests/test_guacscanner.py | 28 ++++++++++++++++++---------- 2 files changed, 20 insertions(+), 12 deletions(-) diff --git a/src/guacscanner/guacscanner.py b/src/guacscanner/guacscanner.py index 2d3ce455..dfb8e4b5 100644 --- a/src/guacscanner/guacscanner.py +++ b/src/guacscanner/guacscanner.py @@ -132,7 +132,7 @@ def instance_connection_exists(db_connection, connection_name): connection_name, ) cursor.execute(COUNT_QUERY, (connection_name,)) - count = cursor.fetchone()[0]["count"] + count = cursor.fetchone()["count"] if count != 0: logging.debug( "A connection named %s exists in the database.", connection_name @@ -180,7 +180,7 @@ def add_instance_connection( "NONE", ), ) - connection_id = cursor.fetchone()[0]["connection_id"] + connection_id = cursor.fetchone()["connection_id"] guac_conn_params = ( ( diff --git a/tests/test_guacscanner.py b/tests/test_guacscanner.py index b6a71d6c..29c82ded 100644 --- a/tests/test_guacscanner.py +++ b/tests/test_guacscanner.py @@ -148,10 +148,12 @@ def test_new_linux_instance(): ) # Mock the PostgreSQL database connection - mock_connection = MagicMock(name="Mock PostgreSQL connection") - mock_cursor = MagicMock(name="Mock PostgreSQL cursor") + mock_connection = MagicMock( + name="Mock PostgreSQL connection", spec_set=psycopg.Connection + ) + mock_cursor = MagicMock(name="Mock PostgreSQL cursor", spec_set=psycopg.Cursor) mock_cursor.__enter__.return_value = mock_cursor - mock_cursor.fetchone.side_effect = [[{"count": 0}], [{"connection_id": 1}]] + mock_cursor.fetchone.side_effect = [{"count": 0}, {"connection_id": 1}] mock_connection.__enter__.return_value = mock_connection mock_connection.cursor.return_value = mock_cursor @@ -212,8 +214,10 @@ def test_terminated_instance(): ec2.terminate_instances(InstanceIds=[instance_id]) # Mock the PostgreSQL database connection - mock_connection = MagicMock(name="Mock PostgreSQL connection") - mock_cursor = MagicMock(name="Mock PostgreSQL cursor") + mock_connection = MagicMock( + name="Mock PostgreSQL connection", spec_set=psycopg.Connection + ) + mock_cursor = MagicMock(name="Mock PostgreSQL cursor", spec_set=psycopg.Cursor) mock_cursor.__enter__.return_value = mock_cursor mock_connection.__enter__.return_value = mock_connection mock_connection.cursor.return_value = mock_cursor @@ -275,8 +279,10 @@ def test_stopped_instance(): ec2.stop_instances(InstanceIds=[instance_id]) # Mock the PostgreSQL database connection - mock_connection = MagicMock(name="Mock PostgreSQL connection") - mock_cursor = MagicMock(name="Mock PostgreSQL cursor") + mock_connection = MagicMock( + name="Mock PostgreSQL connection", spec_set=psycopg.Connection + ) + mock_cursor = MagicMock(name="Mock PostgreSQL cursor", spec_set=psycopg.Cursor) mock_cursor.__enter__.return_value = mock_cursor mock_connection.__enter__.return_value = mock_connection mock_connection.cursor.return_value = mock_cursor @@ -338,10 +344,12 @@ def test_new_windows_instance(): ) # Mock the PostgreSQL database connection - mock_connection = MagicMock(name="Mock PostgreSQL connection") - mock_cursor = MagicMock(name="Mock PostgreSQL cursor") + mock_connection = MagicMock( + name="Mock PostgreSQL connection", spec_set=psycopg.Connection + ) + mock_cursor = MagicMock(name="Mock PostgreSQL cursor", spec_set=psycopg.Cursor) mock_cursor.__enter__.return_value = mock_cursor - mock_cursor.fetchone.side_effect = [[{"count": 0}], [{"connection_id": 1}]] + mock_cursor.fetchone.side_effect = [{"count": 0}, {"connection_id": 1}] mock_connection.__enter__.return_value = mock_connection mock_connection.cursor.return_value = mock_cursor From 5e17187cae393640a2b1416782d664dfc3c3aeaf Mon Sep 17 00:00:00 2001 From: Jeremy Frasier Date: Thu, 4 Nov 2021 12:24:36 -0400 Subject: [PATCH 38/92] Bump version from 0.0.1-rc.9 to 0.0.1-rc.10 --- src/guacscanner/_version.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/guacscanner/_version.py b/src/guacscanner/_version.py index bc7a8021..822bf841 100644 --- a/src/guacscanner/_version.py +++ b/src/guacscanner/_version.py @@ -1,2 +1,2 @@ """This file defines the version of this module.""" -__version__ = "0.0.1-rc.9" +__version__ = "0.0.1-rc.10" From 186707e9f8260641b82d7e5313f501a3b3970e6a Mon Sep 17 00:00:00 2001 From: Jeremy Frasier Date: Thu, 4 Nov 2021 13:20:11 -0400 Subject: [PATCH 39/92] Fix a typo in a DB query --- src/guacscanner/guacscanner.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/guacscanner/guacscanner.py b/src/guacscanner/guacscanner.py index dfb8e4b5..aca460cd 100644 --- a/src/guacscanner/guacscanner.py +++ b/src/guacscanner/guacscanner.py @@ -92,7 +92,7 @@ {name_field}, {protocol_field}, {max_connections_field}, {max_connections_per_user_field}, {proxy_port_field}, {proxy_hostname_field}, {proxy_encryption_method_field}) - VALUES (%s, %s, %s, %s, %s, %s, %s) RETURNING id;""" + VALUES (%s, %s, %s, %s, %s, %s, %s) RETURNING connection_id;""" ).format( table=sql.Identifier("guacamole_connection"), name_field=sql.Identifier("connection_name"), From acf8dfc00841984ecfe2a5d5017400fa50e2899e Mon Sep 17 00:00:00 2001 From: Jeremy Frasier Date: Thu, 4 Nov 2021 13:20:30 -0400 Subject: [PATCH 40/92] Bump version from 0.0.1-rc.10 to 0.0.1-rc.11 --- src/guacscanner/_version.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/guacscanner/_version.py b/src/guacscanner/_version.py index 822bf841..ffb28e61 100644 --- a/src/guacscanner/_version.py +++ b/src/guacscanner/_version.py @@ -1,2 +1,2 @@ """This file defines the version of this module.""" -__version__ = "0.0.1-rc.10" +__version__ = "0.0.1-rc.11" From 201b42fea2d7e7bea3d41c07c264afed6195805f Mon Sep 17 00:00:00 2001 From: Jeremy Frasier Date: Thu, 4 Nov 2021 15:42:56 -0400 Subject: [PATCH 41/92] Fix error in instance ID regex --- src/guacscanner/guacscanner.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/guacscanner/guacscanner.py b/src/guacscanner/guacscanner.py index aca460cd..59481e98 100644 --- a/src/guacscanner/guacscanner.py +++ b/src/guacscanner/guacscanner.py @@ -68,7 +68,7 @@ re.compile(r"^guacamole-.*$"), re.compile(r"^samba-.*$"), ] -INSTANCE_ID_REGEX = re.compile(r"^.* \((?Pi-\d{17})\)$") +INSTANCE_ID_REGEX = re.compile(r"^.* \((?Pi-[0-9a-f]{17})\)$") VPC_ID_REGEX = re.compile(r"^vpc-([0-9a-f]{8}|[0-9a-f]{17})$") COUNT_QUERY = sql.SQL( "SELECT COUNT({id_field}) FROM {table} WHERE {name_field} = %s" From 52c7013e638d658f356b4ee23045e3a7214aee17 Mon Sep 17 00:00:00 2001 From: Jeremy Frasier Date: Thu, 4 Nov 2021 15:43:32 -0400 Subject: [PATCH 42/92] Use sql.Identifier for a name field I use it for all the others but somehow didn't use it for this one. --- src/guacscanner/guacscanner.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/guacscanner/guacscanner.py b/src/guacscanner/guacscanner.py index 59481e98..e193d01f 100644 --- a/src/guacscanner/guacscanner.py +++ b/src/guacscanner/guacscanner.py @@ -92,7 +92,7 @@ {name_field}, {protocol_field}, {max_connections_field}, {max_connections_per_user_field}, {proxy_port_field}, {proxy_hostname_field}, {proxy_encryption_method_field}) - VALUES (%s, %s, %s, %s, %s, %s, %s) RETURNING connection_id;""" + VALUES (%s, %s, %s, %s, %s, %s, %s) RETURNING {id_field};""" ).format( table=sql.Identifier("guacamole_connection"), name_field=sql.Identifier("connection_name"), @@ -102,6 +102,7 @@ proxy_port_field=sql.Identifier("proxy_port"), proxy_hostname_field=sql.Identifier("proxy_hostname"), proxy_encryption_method_field=sql.Identifier("proxy_encryption_method"), + id_field=sql.Identifier("connection_id"), ) INSERT_CONNECTION_PARAMETER_QUERY = sql.SQL( """INSERT INTO {table} From 4aefae594d2d4b8f256057787b9f0ab61db7efb8 Mon Sep 17 00:00:00 2001 From: Jeremy Frasier Date: Thu, 4 Nov 2021 15:44:38 -0400 Subject: [PATCH 43/92] Fix typo in field name --- src/guacscanner/guacscanner.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/guacscanner/guacscanner.py b/src/guacscanner/guacscanner.py index e193d01f..93408f0e 100644 --- a/src/guacscanner/guacscanner.py +++ b/src/guacscanner/guacscanner.py @@ -121,7 +121,7 @@ """DELETE FROM {table} WHERE {id_field} = %s;""" ).format( table=sql.Identifier("guacamole_connection_parameter"), - id_field=sql.Identifier("id"), + id_field=sql.Identifier("connection_id"), ) From 821b220d74646efc03fb7494e6a7db86ca7a01f3 Mon Sep 17 00:00:00 2001 From: Jeremy Frasier Date: Thu, 4 Nov 2021 15:46:02 -0400 Subject: [PATCH 44/92] Bump version from 0.0.1-rc.11 to 0.0.1-rc.12 --- src/guacscanner/_version.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/guacscanner/_version.py b/src/guacscanner/_version.py index ffb28e61..e4f729d4 100644 --- a/src/guacscanner/_version.py +++ b/src/guacscanner/_version.py @@ -1,2 +1,2 @@ """This file defines the version of this module.""" -__version__ = "0.0.1-rc.11" +__version__ = "0.0.1-rc.12" From f61410fa9eded1309428a21f185d4fb05b0e380b Mon Sep 17 00:00:00 2001 From: Jeremy Frasier Date: Thu, 4 Nov 2021 22:12:54 -0400 Subject: [PATCH 45/92] Fix typo in field name --- src/guacscanner/guacscanner.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/guacscanner/guacscanner.py b/src/guacscanner/guacscanner.py index 93408f0e..80fd2422 100644 --- a/src/guacscanner/guacscanner.py +++ b/src/guacscanner/guacscanner.py @@ -116,7 +116,10 @@ ) DELETE_CONNECTIONS_QUERY = sql.SQL( """DELETE FROM {table} WHERE {id_field} = %s;""" -).format(table=sql.Identifier("guacamole_connection"), id_field=sql.Identifier("id")) +).format( + table=sql.Identifier("guacamole_connection"), + id_field=sql.Identifier("connection_id"), +) DELETE_CONNECTION_PARAMETERS_QUERY = sql.SQL( """DELETE FROM {table} WHERE {id_field} = %s;""" ).format( From c80822c579ba5cfb56ec3a6fb34a7ab639639f28 Mon Sep 17 00:00:00 2001 From: Jeremy Frasier Date: Thu, 4 Nov 2021 22:13:28 -0400 Subject: [PATCH 46/92] Bump version from 0.0.1-rc.12 to 0.0.1-rc.13 --- src/guacscanner/_version.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/guacscanner/_version.py b/src/guacscanner/_version.py index e4f729d4..3a89af20 100644 --- a/src/guacscanner/_version.py +++ b/src/guacscanner/_version.py @@ -1,2 +1,2 @@ """This file defines the version of this module.""" -__version__ = "0.0.1-rc.12" +__version__ = "0.0.1-rc.13" From 75f0c6d1991b628f8e66a5fe1fa0dfbca5e28532 Mon Sep 17 00:00:00 2001 From: Jeremy Frasier Date: Thu, 4 Nov 2021 22:41:18 -0400 Subject: [PATCH 47/92] Update README from skeleton content --- README.md | 25 +++++++++++-------------- 1 file changed, 11 insertions(+), 14 deletions(-) diff --git a/README.md b/README.md index 8b1647a3..5253bfdc 100644 --- a/README.md +++ b/README.md @@ -6,20 +6,17 @@ [![Language grade: Python](https://img.shields.io/lgtm/grade/python/g/cisagov/guacscanner.svg?logo=lgtm&logoWidth=18)](https://lgtm.com/projects/g/cisagov/guacscanner/context:python) [![Known Vulnerabilities](https://snyk.io/test/github/cisagov/guacscanner/develop/badge.svg)](https://snyk.io/test/github/cisagov/guacscanner) -This is a generic skeleton project that can be used to quickly get a -new [cisagov](https://github.com/cisagov) Python library GitHub -project started. This skeleton project contains [licensing -information](LICENSE), as well as -[pre-commit hooks](https://pre-commit.com) and -[GitHub Actions](https://github.com/features/actions) configurations -appropriate for a Python library project. - -## New Repositories from a Skeleton ## - -Please see our [Project Setup guide](https://github.com/cisagov/development-guide/tree/develop/project_setup) -for step-by-step instructions on how to start a new repository from -a skeleton. This will save you time and effort when configuring a -new repository! +This project is a Python utility that continually scans the instances +in an AWS VPC and adds/removes Guacamole connections in the underlying +PostgreSQL database accordingly. + +This utility is [Dockerized](https://docker.com) in +[cisagov/guacscanner-docker](https://github.com/cisagov/guacscanner-docker), +and the resulting Docker container is intended to run as a part of +[cisagov/guacamole-composition](https://github.com/cisagov/guacamole-composition), +although it could - probably uselessly - run in a [Docker +composition](https://docs.docker.com/compose/) alongside only the +[official PostgreSQL Docker image](https://hub.docker.com/_/postgres). ## Contributing ## From 4419fa9bfe1233b7cb3b6627add576e6ddf69e12 Mon Sep 17 00:00:00 2001 From: Jeremy Frasier Date: Fri, 5 Nov 2021 11:16:08 -0400 Subject: [PATCH 48/92] Add code to create guacuser and giver it permission to use connections --- src/guacscanner/guacscanner.py | 164 +++++++++++++++++++++++++++++++++ 1 file changed, 164 insertions(+) diff --git a/src/guacscanner/guacscanner.py b/src/guacscanner/guacscanner.py index 80fd2422..dae94c02 100644 --- a/src/guacscanner/guacscanner.py +++ b/src/guacscanner/guacscanner.py @@ -40,6 +40,7 @@ # Standard Python Libraries +import datetime import logging import re import sys @@ -55,6 +56,8 @@ from ._version import __version__ +# TODO: Create command line options with defaults for these variables. +# See cisagov/guacscanner#2 for more details. DEFAULT_ADD_INSTANCE_STATES = [ "running", ] @@ -68,8 +71,17 @@ re.compile(r"^guacamole-.*$"), re.compile(r"^samba-.*$"), ] + +# Some precompiled regexes INSTANCE_ID_REGEX = re.compile(r"^.* \((?Pi-[0-9a-f]{17})\)$") VPC_ID_REGEX = re.compile(r"^vpc-([0-9a-f]{8}|[0-9a-f]{17})$") + +# Determine if we can use f-strings instead of .format() for these +# queries. Also define the sql.Identifier() variables separately so +# that they can be reused where that is possible. See +# cisagov/guacscanner#3 for more details. + +# The PostgreSQL queries used for adding and removing connections COUNT_QUERY = sql.SQL( "SELECT COUNT({id_field}) FROM {table} WHERE {name_field} = %s" ).format( @@ -127,6 +139,128 @@ id_field=sql.Identifier("connection_id"), ) +# The PostgreSQL queries used for adding and removing users +ENTITY_COUNT_QUERY = sql.SQL( + "SELECT COUNT({id_field}) FROM {table} WHERE {name_field} = %s AND {type_field} = %s" +).format( + id_field=sql.Identifier("entity_id"), + table=sql.Identifier("guacamole_entity"), + name_field=sql.Identifier("name"), + type_field=sql.Identifier("type"), +) +ENTITY_ID_QUERY = sql.SQL( + "SELECT {id_field} FROM {table} WHERE {name_field} = %s AND {type_field} = %s" +).format( + id_field=sql.Identifier("entity_id"), + table=sql.Identifier("guacamole_entity"), + name_field=sql.Identifier("name"), + type_field=sql.Identifier("type"), +) +INSERT_ENTITY_QUERY = sql.SQL( + """INSERT INTO {table} ( + {name_field}, {type_field}) + VALUES (%s, %s) RETURNING {id_field};""" +).format( + table=sql.Identifier("guacamole_entity"), + name_field=sql.Identifier("name"), + type_field=sql.Identifier("type"), + id_field=sql.Identifier("entity_id"), +) +INSERT_USER_QUERY = sql.SQL( + """INSERT INTO {table} ( + {id_field}, {hash_field}, {salt_field}, {date_field}) + VALUES (%s, %s, %s, %s);""" +).format( + table=sql.Identifier("guacamole_user"), + id_field=sql.Identifier("entity_id"), + hash_field=sql.Identifier("password_hash"), + salt_field=sql.Identifier("password_salt"), + date_field=sql.Identifier("password_date"), +) +# The PostgreSQL queries used to add and remove connection +# permissions +INSERT_CONNECTION_PERMISSION_QUERY = sql.SQL( + """INSERT INTO {table} ( + {entity_id_field}, {connection_id_field}, {permission_field}) + VALUES (%s, %s, %s);""" +).format( + table=sql.Identifier("guacamole_connection_permission"), + entity_id_field=sql.Identifier("entity_id"), + connection_id_field=sql.Identifier("connection_id"), + permission_field=sql.Identifier("permission"), +) +DELETE_CONNECTION_PERMISSIONS_QUERY = sql.SQL( + """DELETE FROM {table} WHERE {connection_id_field} = %s;""" +).format( + table=sql.Identifier("guacamole_connection_permission"), + connection_id_field=sql.Identifier("connection_id"), +) + + +def entity_exists(db_connection, entity_name): + """Return a boolean indicating whether an entity with the specified name exists.""" + with db_connection.cursor() as cursor: + logging.debug( + "Checking to see if an entity named %s exists in the database.", + entity_name, + ) + cursor.execute(ENTITY_COUNT_QUERY, (entity_name,)) + count = cursor.fetchone()["count"] + if count != 0: + logging.debug("An entity named %s exists in the database.", entity_name) + else: + logging.debug("No entity named %s exists in the database.", entity_name) + + return count != 0 + + +def get_entity_id(db_connection, entity_name): + """Remove all connections corresponding to the EC2 instance.""" + logging.debug("Looking for entity ID for %s.", entity_name) + with db_connection.cursor() as cursor: + logging.debug( + "Checking to see if any entities named %s exist in the database.", + entity_name, + ) + cursor.execute(ENTITY_ID_QUERY, (entity_name,)) + + # Note that we are assuming there is only a single match. + return cursor.fetchone()["entity_id"] + + +def add_user(db_connection, username): + """Add a user, returning its corresponding entity ID.""" + logging.debug("Adding user entry for %s.", username) + entity_id = None + with db_connection.cursor() as cursor: + cursor.execute( + INSERT_ENTITY_QUERY, + ( + username, + "USER", + ), + ) + entity_id = cursor.fetchone()["entity_id"] + cursor.execute( + INSERT_USER_QUERY, + ( + entity_id, + # guacadmin + bytes.fromhex( + "CA458A7D494E3BE824F5E1E175A1556C0F8EEF2C2D7DF3633BEC4A29C4411960" + ), + bytes.fromhex( + "FE24ADC5E11E2B25288D1704ABE67A79E342ECC26064CE69C5B3177795A82264" + ), + datetime.datetime.now(), + ), + ) + + # Commit all pending transactions to the database + db_connection.commit() + + return entity_id + def instance_connection_exists(db_connection, connection_name): """Return a boolean indicating whether a connection with the specified name exists.""" @@ -157,6 +291,7 @@ def add_instance_connection( private_ssh_key, rdp_username, rdp_password, + entity_id, ): """Add a connection for the EC2 instance.""" logging.debug("Adding connection entry for %s.", instance.id) @@ -278,6 +413,19 @@ def add_instance_connection( ) cursor.executemany(INSERT_CONNECTION_PARAMETER_QUERY, guac_conn_params) + logging.debug( + "Adding connection permission entries for connection named %s.", + connection_name, + ) + cursor.execute( + INSERT_CONNECTION_PERMISSION_QUERY, + ( + entity_id, + connection_id, + "READ", + ), + ) + # Commit all pending transactions to the database db_connection.commit() @@ -291,6 +439,9 @@ def remove_connection(db_connection, connection_id): logging.debug("Removing connection parameter entries for %s.", connection_id) cursor.execute(DELETE_CONNECTION_PARAMETERS_QUERY, (connection_id,)) + logging.debug("Removing connection permission entries for %s.", connection_id) + cursor.execute(DELETE_CONNECTION_PERMISSIONS_QUERY, (connection_id,)) + def remove_instance_connections(db_connection, instance): """Remove all connections corresponding to the EC2 instance.""" @@ -327,6 +478,7 @@ def process_instance( private_ssh_key, rdp_username, rdp_password, + entity_id, ): """Add/remove connections for the specified EC2 instance.""" logging.debug("Examining instance %s.", instance.id) @@ -349,6 +501,7 @@ def process_instance( private_ssh_key, rdp_username, rdp_password, + entity_id, ) else: logging.debug( @@ -489,6 +642,16 @@ def main() -> None: vpc_id = validated_args["--vpc-id"] + # Create guac user if it doesn't already exist + guacuser_id = None + with psycopg.connect( + db_connection_string, row_factory=psycopg.rows.dict_row + ) as db_connection: + if not entity_exists(db_connection, "guacuser"): + guacuser_id = add_user(db_connection, "guacuser") + else: + guacuser_id = get_entity_id(db_connection, "guacuser") + ec2 = boto3.resource("ec2", region_name="us-east-1") # If no VPC ID was specified on the command line then grab the VPC @@ -525,6 +688,7 @@ def main() -> None: private_ssh_key, rdp_username, rdp_password, + guacuser_id, ) logging.info( From befafec85653e8a7af705573d7a4e599417359b2 Mon Sep 17 00:00:00 2001 From: Jeremy Frasier Date: Fri, 5 Nov 2021 11:16:50 -0400 Subject: [PATCH 49/92] Bump version from 0.0.1-rc.13 to 0.0.1-rc.14 --- src/guacscanner/_version.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/guacscanner/_version.py b/src/guacscanner/_version.py index 3a89af20..6cc9a8a4 100644 --- a/src/guacscanner/_version.py +++ b/src/guacscanner/_version.py @@ -1,2 +1,2 @@ """This file defines the version of this module.""" -__version__ = "0.0.1-rc.13" +__version__ = "0.0.1-rc.14" From c4b997baf5be6bb84492542521ddc40ba08d6be3 Mon Sep 17 00:00:00 2001 From: Jeremy Frasier Date: Fri, 5 Nov 2021 11:30:23 -0400 Subject: [PATCH 50/92] Move creation of guacuser inside the connection creation loop This is to allow for the PostgreSQL database to come up before trying to query it. --- src/guacscanner/guacscanner.py | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/src/guacscanner/guacscanner.py b/src/guacscanner/guacscanner.py index dae94c02..5cf2df1a 100644 --- a/src/guacscanner/guacscanner.py +++ b/src/guacscanner/guacscanner.py @@ -642,16 +642,6 @@ def main() -> None: vpc_id = validated_args["--vpc-id"] - # Create guac user if it doesn't already exist - guacuser_id = None - with psycopg.connect( - db_connection_string, row_factory=psycopg.rows.dict_row - ) as db_connection: - if not entity_exists(db_connection, "guacuser"): - guacuser_id = add_user(db_connection, "guacuser") - else: - guacuser_id = get_entity_id(db_connection, "guacuser") - ec2 = boto3.resource("ec2", region_name="us-east-1") # If no VPC ID was specified on the command line then grab the VPC @@ -669,6 +659,13 @@ def main() -> None: with psycopg.connect( db_connection_string, row_factory=psycopg.rows.dict_row ) as db_connection: + # Create guacuser if it doesn't already exist + guacuser_id = None + if not entity_exists(db_connection, "guacuser"): + guacuser_id = add_user(db_connection, "guacuser") + else: + guacuser_id = get_entity_id(db_connection, "guacuser") + for instance in instances: ami = ec2.Image(instance.image_id) # Early exit if this instance is running an AMI From b20b3dcfb58361174fcc897e89e7c3f0448e0ab0 Mon Sep 17 00:00:00 2001 From: Jeremy Frasier Date: Fri, 5 Nov 2021 11:30:39 -0400 Subject: [PATCH 51/92] Bump version from 0.0.1-rc.14 to 0.0.1-rc.15 --- src/guacscanner/_version.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/guacscanner/_version.py b/src/guacscanner/_version.py index 6cc9a8a4..6d6254c1 100644 --- a/src/guacscanner/_version.py +++ b/src/guacscanner/_version.py @@ -1,2 +1,2 @@ """This file defines the version of this module.""" -__version__ = "0.0.1-rc.14" +__version__ = "0.0.1-rc.15" From 08d2f52286b36e36c1c60c1f8089bda41db6b8f2 Mon Sep 17 00:00:00 2001 From: Jeremy Frasier Date: Fri, 5 Nov 2021 11:35:20 -0400 Subject: [PATCH 52/92] Add some code so that the guacuser_id is only looked up once --- src/guacscanner/guacscanner.py | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/guacscanner/guacscanner.py b/src/guacscanner/guacscanner.py index 5cf2df1a..ea205211 100644 --- a/src/guacscanner/guacscanner.py +++ b/src/guacscanner/guacscanner.py @@ -654,17 +654,20 @@ def main() -> None: instances = ec2.Vpc(vpc_id).instances.all() keep_looping = True + guacuser_id = None while keep_looping: try: with psycopg.connect( db_connection_string, row_factory=psycopg.rows.dict_row ) as db_connection: # Create guacuser if it doesn't already exist - guacuser_id = None - if not entity_exists(db_connection, "guacuser"): - guacuser_id = add_user(db_connection, "guacuser") - else: - guacuser_id = get_entity_id(db_connection, "guacuser") + if guacuser_id is None: + # We haven't initialized guacuser_id yet, so let's + # do it now. + if not entity_exists(db_connection, "guacuser"): + guacuser_id = add_user(db_connection, "guacuser") + else: + guacuser_id = get_entity_id(db_connection, "guacuser") for instance in instances: ami = ec2.Image(instance.image_id) From 76e36b84934a26217148822e1e948a1ae4e529ad Mon Sep 17 00:00:00 2001 From: Jeremy Frasier Date: Fri, 5 Nov 2021 11:35:31 -0400 Subject: [PATCH 53/92] Bump version from 0.0.1-rc.15 to 0.0.1-rc.16 --- src/guacscanner/_version.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/guacscanner/_version.py b/src/guacscanner/_version.py index 6d6254c1..fd0c51b0 100644 --- a/src/guacscanner/_version.py +++ b/src/guacscanner/_version.py @@ -1,2 +1,2 @@ """This file defines the version of this module.""" -__version__ = "0.0.1-rc.15" +__version__ = "0.0.1-rc.16" From ef47a62079b34666bd63f1472350fdf3ff2754b7 Mon Sep 17 00:00:00 2001 From: Jeremy Frasier Date: Fri, 5 Nov 2021 12:35:16 -0400 Subject: [PATCH 54/92] Modify entity_exists() and get_entity_id() to take the entity type as a parameter --- src/guacscanner/guacscanner.py | 36 ++++++++++++++++++++++------------ 1 file changed, 23 insertions(+), 13 deletions(-) diff --git a/src/guacscanner/guacscanner.py b/src/guacscanner/guacscanner.py index ea205211..b080e973 100644 --- a/src/guacscanner/guacscanner.py +++ b/src/guacscanner/guacscanner.py @@ -197,32 +197,42 @@ ) -def entity_exists(db_connection, entity_name): - """Return a boolean indicating whether an entity with the specified name exists.""" +def entity_exists(db_connection, entity_name, entity_type): + """Return a boolean indicating whether an entity with the specified name and type exists.""" with db_connection.cursor() as cursor: logging.debug( - "Checking to see if an entity named %s exists in the database.", + "Checking to see if an entity named %s of type %s exists in the database.", entity_name, + entity_type, ) - cursor.execute(ENTITY_COUNT_QUERY, (entity_name,)) + cursor.execute(ENTITY_COUNT_QUERY, (entity_name, entity_type)) count = cursor.fetchone()["count"] if count != 0: - logging.debug("An entity named %s exists in the database.", entity_name) + logging.debug( + "An entity named %s of type %s exists in the database.", + entity_name, + entity_type, + ) else: - logging.debug("No entity named %s exists in the database.", entity_name) + logging.debug( + "No entity named %s of type %s exists in the database.", + entity_name, + entity_type, + ) return count != 0 -def get_entity_id(db_connection, entity_name): - """Remove all connections corresponding to the EC2 instance.""" - logging.debug("Looking for entity ID for %s.", entity_name) +def get_entity_id(db_connection, entity_name, entity_type): + """Return the ID corresponding to the entity with the specified name and type.""" + logging.debug("Looking for entity ID for %s of type %s.", entity_name, entity_type) with db_connection.cursor() as cursor: logging.debug( - "Checking to see if any entities named %s exist in the database.", + "Checking to see if any entity named %s of type %s exists in the database.", entity_name, + entity_type, ) - cursor.execute(ENTITY_ID_QUERY, (entity_name,)) + cursor.execute(ENTITY_ID_QUERY, (entity_name, entity_type)) # Note that we are assuming there is only a single match. return cursor.fetchone()["entity_id"] @@ -664,10 +674,10 @@ def main() -> None: if guacuser_id is None: # We haven't initialized guacuser_id yet, so let's # do it now. - if not entity_exists(db_connection, "guacuser"): + if not entity_exists(db_connection, "guacuser", "USER"): guacuser_id = add_user(db_connection, "guacuser") else: - guacuser_id = get_entity_id(db_connection, "guacuser") + guacuser_id = get_entity_id(db_connection, "guacuser", "USER") for instance in instances: ami = ec2.Image(instance.image_id) From d7fd3fa4706fb9e04e02603a56a03d3e64226462 Mon Sep 17 00:00:00 2001 From: Jeremy Frasier Date: Fri, 5 Nov 2021 12:35:32 -0400 Subject: [PATCH 55/92] Bump version from 0.0.1-rc.16 to 0.0.1-rc.17 --- src/guacscanner/_version.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/guacscanner/_version.py b/src/guacscanner/_version.py index fd0c51b0..c795d3e5 100644 --- a/src/guacscanner/_version.py +++ b/src/guacscanner/_version.py @@ -1,2 +1,2 @@ """This file defines the version of this module.""" -__version__ = "0.0.1-rc.16" +__version__ = "0.0.1-rc.17" From 808c91e8cc1fa957cfcb22fa3d28f3875b85cd28 Mon Sep 17 00:00:00 2001 From: Jeremy Frasier Date: Fri, 5 Nov 2021 12:56:59 -0400 Subject: [PATCH 56/92] Update tests now that the guacuser code has been added --- tests/test_guacscanner.py | 123 +++++++++++++++++++++++++++++++++++++- 1 file changed, 120 insertions(+), 3 deletions(-) diff --git a/tests/test_guacscanner.py b/tests/test_guacscanner.py index 29c82ded..5455e029 100644 --- a/tests/test_guacscanner.py +++ b/tests/test_guacscanner.py @@ -121,6 +121,104 @@ def test_bad_log_level(): assert return_code == 1, "main() should exit with error" +@mock_ec2 +def test_addition_of_guacuser(): + """Verify that adding the guacuser works as expected.""" + # Create and VPC + ec2 = boto3.client("ec2", "us-east-1") + vpc = ec2.create_vpc(CidrBlock="10.19.74.0/24") + vpc_id = vpc["Vpc"]["VpcId"] + + # Mock the PostgreSQL database connection + mock_connection = MagicMock( + name="Mock PostgreSQL connection", spec_set=psycopg.Connection + ) + mock_cursor = MagicMock(name="Mock PostgreSQL cursor", spec_set=psycopg.Cursor) + mock_cursor.__enter__.return_value = mock_cursor + mock_cursor.fetchone.side_effect = [ + # Checking to see if guacuser exists and then adding it + {"count": 0}, + {"entity_id": 1}, + ] + mock_connection.__enter__.return_value = mock_connection + mock_connection.cursor.return_value = mock_cursor + + with patch.object( + sys, + "argv", + [ + "--log-level=debug", + "--oneshot", + "--postgres-password=dummy_db_password", + "--postgres-username=dummy_db_username", + "--private-ssh-key=dummy_key", + "--rdp-password=dummy_rdp_password", + "--rdp-username=dummy_rdp_username", + "--vnc-password=dummy_vnc_password", + "--vnc-username=dummy_vnc_username", + f"--vpc-id={vpc_id}", + ], + ): + with patch.object( + psycopg, "connect", return_value=mock_connection + ) as mock_connect: + guacscanner.guacscanner.main() + mock_connect.assert_called_once() + mock_connection.cursor.assert_called() + mock_connection.commit.assert_called() + mock_cursor.fetchone.assert_called() + mock_cursor.execute.assert_called() + + +@mock_ec2 +def test_guacuser_already_exists(): + """Verify that the case where the guacuser already exists works as expected.""" + # Create and VPC + ec2 = boto3.client("ec2", "us-east-1") + vpc = ec2.create_vpc(CidrBlock="10.19.74.0/24") + vpc_id = vpc["Vpc"]["VpcId"] + + # Mock the PostgreSQL database connection + mock_connection = MagicMock( + name="Mock PostgreSQL connection", spec_set=psycopg.Connection + ) + mock_cursor = MagicMock(name="Mock PostgreSQL cursor", spec_set=psycopg.Cursor) + mock_cursor.__enter__.return_value = mock_cursor + mock_cursor.fetchone.side_effect = [ + # Checking to see if guacuser exists and then fetching its ID + {"count": 1}, + {"entity_id": 1}, + ] + mock_connection.__enter__.return_value = mock_connection + mock_connection.cursor.return_value = mock_cursor + + with patch.object( + sys, + "argv", + [ + "--log-level=debug", + "--oneshot", + "--postgres-password=dummy_db_password", + "--postgres-username=dummy_db_username", + "--private-ssh-key=dummy_key", + "--rdp-password=dummy_rdp_password", + "--rdp-username=dummy_rdp_username", + "--vnc-password=dummy_vnc_password", + "--vnc-username=dummy_vnc_username", + f"--vpc-id={vpc_id}", + ], + ): + with patch.object( + psycopg, "connect", return_value=mock_connection + ) as mock_connect: + guacscanner.guacscanner.main() + mock_connect.assert_called_once() + mock_connection.cursor.assert_called() + mock_connection.commit.assert_called() + mock_cursor.fetchone.assert_called() + mock_cursor.execute.assert_called() + + @mock_ec2 def test_new_linux_instance(): """Verify that adding a new Linux instance works as expected.""" @@ -153,7 +251,14 @@ def test_new_linux_instance(): ) mock_cursor = MagicMock(name="Mock PostgreSQL cursor", spec_set=psycopg.Cursor) mock_cursor.__enter__.return_value = mock_cursor - mock_cursor.fetchone.side_effect = [{"count": 0}, {"connection_id": 1}] + mock_cursor.fetchone.side_effect = [ + # Checking to see if guacuser exists and then adding it + {"count": 0}, + {"entity_id": 1}, + # Checking to see if the connection exists and then adding it + {"count": 0}, + {"connection_id": 1}, + ] mock_connection.__enter__.return_value = mock_connection mock_connection.cursor.return_value = mock_cursor @@ -219,6 +324,11 @@ def test_terminated_instance(): ) mock_cursor = MagicMock(name="Mock PostgreSQL cursor", spec_set=psycopg.Cursor) mock_cursor.__enter__.return_value = mock_cursor + mock_cursor.fetchone.side_effect = [ + # Checking to see if guacuser exists and then adding it + {"count": 0}, + {"entity_id": 1}, + ] mock_connection.__enter__.return_value = mock_connection mock_connection.cursor.return_value = mock_cursor @@ -245,7 +355,7 @@ def test_terminated_instance(): mock_connect.assert_called_once() mock_connection.cursor.assert_called() mock_connection.commit.assert_called() - mock_cursor.fetchone.assert_not_called() + mock_cursor.fetchone.assert_called() mock_cursor.execute.assert_called() mock_cursor.executemany.assert_not_called() @@ -349,7 +459,14 @@ def test_new_windows_instance(): ) mock_cursor = MagicMock(name="Mock PostgreSQL cursor", spec_set=psycopg.Cursor) mock_cursor.__enter__.return_value = mock_cursor - mock_cursor.fetchone.side_effect = [{"count": 0}, {"connection_id": 1}] + mock_cursor.fetchone.side_effect = [ + # Checking to see if guacuser exists and then adding it + {"count": 0}, + {"entity_id": 1}, + # Checking to see if the connection exists and then adding it + {"count": 0}, + {"connection_id": 1}, + ] mock_connection.__enter__.return_value = mock_connection mock_connection.cursor.return_value = mock_cursor From bbc0cf941149bbc6a47f9793bd2c218a7b06f50d Mon Sep 17 00:00:00 2001 From: Jeremy Frasier Date: Fri, 5 Nov 2021 13:11:42 -0400 Subject: [PATCH 57/92] Add a todo comment to improve the way the Guacamole user is created --- src/guacscanner/guacscanner.py | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/src/guacscanner/guacscanner.py b/src/guacscanner/guacscanner.py index b080e973..8e59e91e 100644 --- a/src/guacscanner/guacscanner.py +++ b/src/guacscanner/guacscanner.py @@ -76,9 +76,9 @@ INSTANCE_ID_REGEX = re.compile(r"^.* \((?Pi-[0-9a-f]{17})\)$") VPC_ID_REGEX = re.compile(r"^vpc-([0-9a-f]{8}|[0-9a-f]{17})$") -# Determine if we can use f-strings instead of .format() for these -# queries. Also define the sql.Identifier() variables separately so -# that they can be reused where that is possible. See +# TODO: Determine if we can use f-strings instead of .format() for +# these queries. Also define the sql.Identifier() variables +# separately so that they can be reused where that is possible. See # cisagov/guacscanner#3 for more details. # The PostgreSQL queries used for adding and removing connections @@ -671,6 +671,14 @@ def main() -> None: db_connection_string, row_factory=psycopg.rows.dict_row ) as db_connection: # Create guacuser if it doesn't already exist + # + # TODO: Figure out a way to make this cleaner. We + # don't want to hardcode the guacuser name, and we + # want to allow the user to specify a list of users + # that should be created if they don't exist and given + # access to use the connections created by + # guacscanner. See cisagov/guacscanner#4 for more + # details. if guacuser_id is None: # We haven't initialized guacuser_id yet, so let's # do it now. From 60448e023672d48766f539c9f5b22ba57db40cee Mon Sep 17 00:00:00 2001 From: Jeremy Frasier Date: Fri, 5 Nov 2021 13:17:22 -0400 Subject: [PATCH 58/92] Add a TODO comment for improvement of exception handling --- src/guacscanner/guacscanner.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/guacscanner/guacscanner.py b/src/guacscanner/guacscanner.py index 8e59e91e..c8e0d100 100644 --- a/src/guacscanner/guacscanner.py +++ b/src/guacscanner/guacscanner.py @@ -56,6 +56,12 @@ from ._version import __version__ +# TODO: Add exception handling for all the database accesses and +# wherever else it is appropriate. guacscanner currently just bombs +# out if an exception is thrown, but it would probably make more sense +# to print an error message and keep looping, keepin' the train +# a-chooglin'. See cisagov/guacscanner#5 for more details. + # TODO: Create command line options with defaults for these variables. # See cisagov/guacscanner#2 for more details. DEFAULT_ADD_INSTANCE_STATES = [ From 914eaf85ab0059680d5fbbc97330f02dbe2470dd Mon Sep 17 00:00:00 2001 From: Shane Frasier Date: Fri, 5 Nov 2021 14:39:13 -0400 Subject: [PATCH 59/92] Improve capitalization Co-authored-by: dav3r --- src/guacscanner/guacscanner.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/guacscanner/guacscanner.py b/src/guacscanner/guacscanner.py index c8e0d100..cb62fcd4 100644 --- a/src/guacscanner/guacscanner.py +++ b/src/guacscanner/guacscanner.py @@ -21,8 +21,8 @@ --postgres-password-file=FILENAME The file from which the PostgreSQL password will be read. [default: /run/secrets/postgres-password] --postgres-username=USERNAME If specified then the specified value will be used when connecting to the PostgreSQL database. Otherwise, the username will be read from a local file. --postgres-username-file=FILENAME The file from which the PostgreSQL username will be read. [default: /run/secrets/postgres-username] - --private-ssh-key=KEY If specified then the specified value will be used for the private ssh key. Otherwise, the ssh key will be read from a local file. - --private-ssh-key-file=FILENAME The file from which the private ssh key will be read. [default: /run/secrets/private-ssh-key] + --private-ssh-key=KEY If specified then the specified value will be used for the private SSH key. Otherwise, the SSH key will be read from a local file. + --private-ssh-key-file=FILENAME The file from which the private SSH key will be read. [default: /run/secrets/private-ssh-key] --rdp-password=PASSWORD If specified then the specified value will be used for the RDP password. Otherwise, the password will be read from a local file. --rdp-password-file=FILENAME The file from which the RDP password will be read. [default: /run/secrets/rdp-password] --rdp-username=USERNAME If specified then the specified value will be used for the RDP username. Otherwise, the username will be read from a local file. From 2e7c8d9b2b52192abb6efbd4e334e6ddeffde861 Mon Sep 17 00:00:00 2001 From: Shane Frasier Date: Fri, 5 Nov 2021 14:39:33 -0400 Subject: [PATCH 60/92] Improve comment Co-authored-by: dav3r --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 5253bfdc..220769cc 100644 --- a/README.md +++ b/README.md @@ -6,7 +6,7 @@ [![Language grade: Python](https://img.shields.io/lgtm/grade/python/g/cisagov/guacscanner.svg?logo=lgtm&logoWidth=18)](https://lgtm.com/projects/g/cisagov/guacscanner/context:python) [![Known Vulnerabilities](https://snyk.io/test/github/cisagov/guacscanner/develop/badge.svg)](https://snyk.io/test/github/cisagov/guacscanner) -This project is a Python utility that continually scans the instances +This project is a Python utility that continually scans the EC2 instances in an AWS VPC and adds/removes Guacamole connections in the underlying PostgreSQL database accordingly. From 6de761f0ab3472b56798b7afa593ff8fd8228202 Mon Sep 17 00:00:00 2001 From: Shane Frasier Date: Fri, 5 Nov 2021 14:40:56 -0400 Subject: [PATCH 61/92] Correctly alphabetize CLI arguments Co-authored-by: dav3r --- src/guacscanner/guacscanner.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/guacscanner/guacscanner.py b/src/guacscanner/guacscanner.py index cb62fcd4..77be99ce 100644 --- a/src/guacscanner/guacscanner.py +++ b/src/guacscanner/guacscanner.py @@ -8,7 +8,7 @@ >0 An error occurred. Usage: - guacscanner [--log-level=LEVEL] [--oneshot] [--sleep=SECONDS] [--postgres-password=PASSWORD|--postgres-password-file=FILENAME] [--private-ssh-key=KEY|--private-ssh-key-file=FILENAME] [--postgres-username=USERNAME|--postgres-username-file=FILENAME] [--rdp-password=PASSWORD|--rdp-password-file=FILENAME] [--rdp-username=USERNAME|--rdp-username-file=FILENAME] [--vnc-password=PASSWORD|--vnc-password-file=FILENAME] [--vnc-username=USERNAME|--vnc-username-file=FILENAME] [--vpc-id=VPC_ID] + guacscanner [--log-level=LEVEL] [--oneshot] [--sleep=SECONDS] [--postgres-password=PASSWORD|--postgres-password-file=FILENAME] [--postgres-username=USERNAME|--postgres-username-file=FILENAME] [--private-ssh-key=KEY|--private-ssh-key-file=FILENAME] [--rdp-password=PASSWORD|--rdp-password-file=FILENAME] [--rdp-username=USERNAME|--rdp-username-file=FILENAME] [--vnc-password=PASSWORD|--vnc-password-file=FILENAME] [--vnc-username=USERNAME|--vnc-username-file=FILENAME] [--vpc-id=VPC_ID] guacscanner (-h | --help) Options: From 92bc51b57ad285789f148a786f9739fe98bda6ef Mon Sep 17 00:00:00 2001 From: Shane Frasier Date: Fri, 5 Nov 2021 14:45:29 -0400 Subject: [PATCH 62/92] Also ignore instances running Nessus AMIs Such instances are currently only accessible via a browser on another instance (Kali or Teamserver). Co-authored-by: dav3r --- src/guacscanner/guacscanner.py | 1 + 1 file changed, 1 insertion(+) diff --git a/src/guacscanner/guacscanner.py b/src/guacscanner/guacscanner.py index 77be99ce..c1f48e2e 100644 --- a/src/guacscanner/guacscanner.py +++ b/src/guacscanner/guacscanner.py @@ -75,6 +75,7 @@ ] DEFAULT_AMI_SKIP_REGEXES = [ re.compile(r"^guacamole-.*$"), + re.compile(r"^nessus-.*$"), re.compile(r"^samba-.*$"), ] From 6a2c4e06d04219c5f2637939b7758c9e9a1388e1 Mon Sep 17 00:00:00 2001 From: Jeremy Frasier Date: Fri, 5 Nov 2021 14:51:44 -0400 Subject: [PATCH 63/92] Add a comment explaining why a line is commented out Co-authored-by: dav3r --- setup.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/setup.py b/setup.py index c271100d..cfda3707 100644 --- a/setup.py +++ b/setup.py @@ -84,7 +84,8 @@ def get_version(version_file): keywords="aws, guacamole, vpc", packages=find_packages(where="src"), package_dir={"": "src"}, - # package_data={"guacamole-connection-scanner": ["data/*.txt"]}, + # This package contains no data + # package_data={"guacscanner": ["data/*.txt"]}, py_modules=[splitext(basename(path))[0] for path in glob("src/*.py")], include_package_data=True, install_requires=[ From 890461bbb3eb2b1879d876c1fc48e03f096475ee Mon Sep 17 00:00:00 2001 From: Jeremy Frasier Date: Fri, 5 Nov 2021 14:57:26 -0400 Subject: [PATCH 64/92] List functions to be exported These may be useful to others. Co-authored-by: dav3r --- src/guacscanner/__init__.py | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/src/guacscanner/__init__.py b/src/guacscanner/__init__.py index ac9d730c..18f10013 100644 --- a/src/guacscanner/__init__.py +++ b/src/guacscanner/__init__.py @@ -7,4 +7,16 @@ from . import guacscanner # noqa: F401 from ._version import __version__ # noqa: F401 -# __all__ = ["example_div"] +__all__ = [ + "add_instance_connection", + "add_user", + "check_for_ghost_instances", + "entity_exists", + "get_connection_name", + "get_entity_id", + "instance_connection_exists", + "main", + "process_instance", + "remove_connection", + "remove_instance_connection", +] From 1985e7849b8823bb537fb8faf27d886ba94ef99d Mon Sep 17 00:00:00 2001 From: Jeremy Frasier Date: Fri, 5 Nov 2021 15:13:16 -0400 Subject: [PATCH 65/92] Add some exception handling for the AMI regex matching An AttributeError is thrown sometimes, but I'm not sure why. I believe it is a temporary problem that only happens while instances are in flux. This exception handling code will still log the error, but keep things moving instead of bombing out the entire process. --- src/guacscanner/guacscanner.py | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/src/guacscanner/guacscanner.py b/src/guacscanner/guacscanner.py index c1f48e2e..cabf4f9b 100644 --- a/src/guacscanner/guacscanner.py +++ b/src/guacscanner/guacscanner.py @@ -698,9 +698,21 @@ def main() -> None: ami = ec2.Image(instance.image_id) # Early exit if this instance is running an AMI # that we want to avoid adding to Guacamole. - if any( - [regex.match(ami.name) for regex in DEFAULT_AMI_SKIP_REGEXES] - ): + try: + ami_matches = [ + regex.match(ami.name) for regex in DEFAULT_AMI_SKIP_REGEXES + ] + except AttributeError: + # I'm not sure why this happens, but this + # should keep things moving when it does. It + # is a temporary problem that eventually sorts + # itself out. I think it only happens while + # instances are being created or destroyed. + logging.exception( + "Unable to determine if instance is running an AMI that would cause it to be skipped." + ) + continue + if any(ami_matches): continue process_instance( From ea88bb14b18c52b9f11004a3ac12a5c203900876 Mon Sep 17 00:00:00 2001 From: Jeremy Frasier Date: Fri, 5 Nov 2021 15:16:20 -0400 Subject: [PATCH 66/92] Bump version from 0.0.1-rc.17 to 0.0.1-rc.18 --- src/guacscanner/_version.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/guacscanner/_version.py b/src/guacscanner/_version.py index c795d3e5..12382f3e 100644 --- a/src/guacscanner/_version.py +++ b/src/guacscanner/_version.py @@ -1,2 +1,2 @@ """This file defines the version of this module.""" -__version__ = "0.0.1-rc.17" +__version__ = "0.0.1-rc.18" From 3f421fbfa0b04d7998e8314586f78f04ca053dea Mon Sep 17 00:00:00 2001 From: Shane Frasier Date: Fri, 5 Nov 2021 16:05:04 -0400 Subject: [PATCH 67/92] Remove unnecessary import Co-authored-by: dav3r --- src/guacscanner/__init__.py | 1 - 1 file changed, 1 deletion(-) diff --git a/src/guacscanner/__init__.py b/src/guacscanner/__init__.py index 18f10013..5e500b44 100644 --- a/src/guacscanner/__init__.py +++ b/src/guacscanner/__init__.py @@ -3,7 +3,6 @@ # although this import is not directly used, it populates the value # package_name.__version__, which is used to get version information about this # Python package. -# from .guacscanner import example_div from . import guacscanner # noqa: F401 from ._version import __version__ # noqa: F401 From 916531e8f90d9bf2986750e5ebf370407e1686a0 Mon Sep 17 00:00:00 2001 From: Jeremy Frasier Date: Mon, 8 Nov 2021 13:03:39 -0500 Subject: [PATCH 68/92] Improve comment about exception thrown by ami.name access I have been able to determine the exact cause since first encountering the exception, so it makes sense to update the comment. --- src/guacscanner/guacscanner.py | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/src/guacscanner/guacscanner.py b/src/guacscanner/guacscanner.py index cabf4f9b..34dcbf86 100644 --- a/src/guacscanner/guacscanner.py +++ b/src/guacscanner/guacscanner.py @@ -703,11 +703,20 @@ def main() -> None: regex.match(ami.name) for regex in DEFAULT_AMI_SKIP_REGEXES ] except AttributeError: - # I'm not sure why this happens, but this - # should keep things moving when it does. It - # is a temporary problem that eventually sorts - # itself out. I think it only happens while - # instances are being created or destroyed. + # This exception can be thrown when an + # instance is running an AMI to which the + # account no longer has access; for example, + # between the time when a new AMI of the same + # type is built and terraform-post-packer is + # run and the new AMI is applied to the + # account. In this situation we can't take + # any action because we can't access the AMI's + # name and hence can't know if the instance + # AMI is of a type whose Guacamole connections + # are being controlled by guacscanner. + # + # In any event, this continue statement should + # keep things moving when it does. logging.exception( "Unable to determine if instance is running an AMI that would cause it to be skipped." ) From af9e26e555cdc6e1f05d702f711cda3526ef5c3f Mon Sep 17 00:00:00 2001 From: Jeremy Frasier Date: Mon, 8 Nov 2021 13:31:20 -0500 Subject: [PATCH 69/92] Bump version from 0.0.1-rc.18 to 0.0.1 --- src/guacscanner/_version.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/guacscanner/_version.py b/src/guacscanner/_version.py index 12382f3e..33cee844 100644 --- a/src/guacscanner/_version.py +++ b/src/guacscanner/_version.py @@ -1,2 +1,2 @@ """This file defines the version of this module.""" -__version__ = "0.0.1-rc.18" +__version__ = "0.0.1" From 3e05d61d38d71406c12a32038705b48f7fa86435 Mon Sep 17 00:00:00 2001 From: Shane Frasier Date: Tue, 9 Nov 2021 15:45:40 -0500 Subject: [PATCH 70/92] Fix typos in comments Co-authored-by: dav3r --- tests/test_guacscanner.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/test_guacscanner.py b/tests/test_guacscanner.py index 5455e029..c0918efd 100644 --- a/tests/test_guacscanner.py +++ b/tests/test_guacscanner.py @@ -124,7 +124,7 @@ def test_bad_log_level(): @mock_ec2 def test_addition_of_guacuser(): """Verify that adding the guacuser works as expected.""" - # Create and VPC + # Create a VPC ec2 = boto3.client("ec2", "us-east-1") vpc = ec2.create_vpc(CidrBlock="10.19.74.0/24") vpc_id = vpc["Vpc"]["VpcId"] @@ -173,7 +173,7 @@ def test_addition_of_guacuser(): @mock_ec2 def test_guacuser_already_exists(): """Verify that the case where the guacuser already exists works as expected.""" - # Create and VPC + # Create a VPC ec2 = boto3.client("ec2", "us-east-1") vpc = ec2.create_vpc(CidrBlock="10.19.74.0/24") vpc_id = vpc["Vpc"]["VpcId"] From ec52ab480fd6cf6883284d3918599317831806ba Mon Sep 17 00:00:00 2001 From: Jeremy Frasier Date: Tue, 9 Nov 2021 21:37:11 -0500 Subject: [PATCH 71/92] Add a --region CLI optional, defaulting to us-east-1 This option is used in the case where --vpc-id is specified. It specifies the AWS region in which the VPC with the given ID exists. If --vpc-id is not specified then the region where the current instances exists is read from EC2 metadata. Co-authored-by: dav3r --- src/guacscanner/guacscanner.py | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/guacscanner/guacscanner.py b/src/guacscanner/guacscanner.py index 34dcbf86..d6e9a73a 100644 --- a/src/guacscanner/guacscanner.py +++ b/src/guacscanner/guacscanner.py @@ -8,7 +8,7 @@ >0 An error occurred. Usage: - guacscanner [--log-level=LEVEL] [--oneshot] [--sleep=SECONDS] [--postgres-password=PASSWORD|--postgres-password-file=FILENAME] [--postgres-username=USERNAME|--postgres-username-file=FILENAME] [--private-ssh-key=KEY|--private-ssh-key-file=FILENAME] [--rdp-password=PASSWORD|--rdp-password-file=FILENAME] [--rdp-username=USERNAME|--rdp-username-file=FILENAME] [--vnc-password=PASSWORD|--vnc-password-file=FILENAME] [--vnc-username=USERNAME|--vnc-username-file=FILENAME] [--vpc-id=VPC_ID] + guacscanner [--log-level=LEVEL] [--oneshot] [--sleep=SECONDS] [--postgres-password=PASSWORD|--postgres-password-file=FILENAME] [--postgres-username=USERNAME|--postgres-username-file=FILENAME] [--private-ssh-key=KEY|--private-ssh-key-file=FILENAME] [--rdp-password=PASSWORD|--rdp-password-file=FILENAME] [--rdp-username=USERNAME|--rdp-username-file=FILENAME] [--region=REGION] [--vnc-password=PASSWORD|--vnc-password-file=FILENAME] [--vnc-username=USERNAME|--vnc-username-file=FILENAME] [--vpc-id=VPC_ID] guacscanner (-h | --help) Options: @@ -27,6 +27,7 @@ --rdp-password-file=FILENAME The file from which the RDP password will be read. [default: /run/secrets/rdp-password] --rdp-username=USERNAME If specified then the specified value will be used for the RDP username. Otherwise, the username will be read from a local file. --rdp-username-file=FILENAME The file from which the RDP username will be read. [default: /run/secrets/rdp-username] + --region=REGION The AWS region in which the VPC specified by --vpc-id exists. Unused if --vpc-id is not specified. [default: us-east-1] --sleep=SECONDS Sleep for the specified number of seconds between executions of the Guacamole connection update loop. [default: 60] --vnc-password=PASSWORD If specified then the specified value will be used for the VNC password. Otherwise, the password will be read from a local file. --vnc-password-file=FILENAME The file from which the VNC password will be read. [default: /run/secrets/vnc-password] @@ -658,15 +659,20 @@ def main() -> None: logging.debug("DB connection string is %s.", db_connection_string) vpc_id = validated_args["--vpc-id"] - - ec2 = boto3.resource("ec2", region_name="us-east-1") + region = validated_args["--region"] # If no VPC ID was specified on the command line then grab the VPC # ID where this instance resides and use that. + ec2 = None if vpc_id is None: instance_id = ec2_metadata.instance_id + region = ec2_metadata.region + ec2 = boto3.resource("ec2", region_name=region) instance = ec2.Instance(instance_id) vpc_id = instance.vpc_id + else: + ec2 = boto3.resource("ec2", region_name=region) + logging.info("Examining instances in VPC %s.", vpc_id) instances = ec2.Vpc(vpc_id).instances.all() From de02b0671ddf1693ce80f251a8b5aeea8e95eda4 Mon Sep 17 00:00:00 2001 From: Jeremy Frasier Date: Wed, 10 Nov 2021 11:58:11 -0500 Subject: [PATCH 72/92] Add code to correctly handle generation of Guacamole passwords I also added type hints to the add_user() function, since the types of the password and salt input variables are important to note. Thanks to @mcdonnnj for figuring out exactly how the salted password hash should be generated! --- src/guacscanner/guacscanner.py | 51 ++++++++++++++++++++++++++++------ 1 file changed, 42 insertions(+), 9 deletions(-) diff --git a/src/guacscanner/guacscanner.py b/src/guacscanner/guacscanner.py index d6e9a73a..da236cbb 100644 --- a/src/guacscanner/guacscanner.py +++ b/src/guacscanner/guacscanner.py @@ -42,8 +42,11 @@ # Standard Python Libraries import datetime +import hashlib import logging import re +import secrets +import string import sys import time @@ -68,6 +71,8 @@ DEFAULT_ADD_INSTANCE_STATES = [ "running", ] +DEFAULT_PASSWORD_LENGTH = 32 +DEFAULT_SALT_LENGTH = 32 DEFAULT_POSTGRES_DB_NAME = "guacamole_db" DEFAULT_POSTGRES_HOSTNAME = "postgres" DEFAULT_POSTGRES_PORT = 5432 @@ -246,9 +251,42 @@ def get_entity_id(db_connection, entity_name, entity_type): return cursor.fetchone()["entity_id"] -def add_user(db_connection, username): - """Add a user, returning its corresponding entity ID.""" +def add_user( + db_connection: psycopg.Connection, + username: str, + password: str = None, + salt: bytes = None, +) -> int: + """Add a user, returning its corresponding entity ID. + + If password (salt) is None (the default) then a random password + (salt) will be generated for the user. + + Note that the salt should be an array of bytes, while the password + should be an ASCII string. + + """ logging.debug("Adding user entry for %s.", username) + + if password is None: + # Generate a random password consisting of ASCII letters and + # digits + alphabet = string.ascii_letters + string.digits + password = "".join( + secrets.choice(alphabet) for i in range(DEFAULT_PASSWORD_LENGTH) + ) + if salt is None: + # Generate a random byte array + salt = secrets.token_bytes(DEFAULT_SALT_LENGTH) + + # Compute the salted password hash that is to be saved to the + # database + hexed_salt = salt.hex() + hasher = hashlib.sha256() + hasher.update(password.encode()) + hasher.update(hexed_salt.encode()) + salted_password_hash = hasher.hexdigest() + entity_id = None with db_connection.cursor() as cursor: cursor.execute( @@ -263,13 +301,8 @@ def add_user(db_connection, username): INSERT_USER_QUERY, ( entity_id, - # guacadmin - bytes.fromhex( - "CA458A7D494E3BE824F5E1E175A1556C0F8EEF2C2D7DF3633BEC4A29C4411960" - ), - bytes.fromhex( - "FE24ADC5E11E2B25288D1704ABE67A79E342ECC26064CE69C5B3177795A82264" - ), + bytes.fromhex(salted_password_hash), + salt, datetime.datetime.now(), ), ) From e595bb3462acbe6708a5fce876729df21e7fe796 Mon Sep 17 00:00:00 2001 From: Jeremy Frasier Date: Wed, 10 Nov 2021 12:20:01 -0500 Subject: [PATCH 73/92] Uppercase values that are saved to the DB before converting to byte arrays This is necessary to match the output of the Guacamole DB initialization script. --- src/guacscanner/guacscanner.py | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/guacscanner/guacscanner.py b/src/guacscanner/guacscanner.py index da236cbb..66d1af85 100644 --- a/src/guacscanner/guacscanner.py +++ b/src/guacscanner/guacscanner.py @@ -280,12 +280,17 @@ def add_user( salt = secrets.token_bytes(DEFAULT_SALT_LENGTH) # Compute the salted password hash that is to be saved to the - # database - hexed_salt = salt.hex() + # database. + # + # Note that we convert the hexed salt and the salted password hash + # to uppercase, since that must be done to match the corresponding + # values in the database that are generated for the default + # guacadmin password by the database initialization script. + hexed_salt = salt.hex().upper() hasher = hashlib.sha256() hasher.update(password.encode()) hasher.update(hexed_salt.encode()) - salted_password_hash = hasher.hexdigest() + salted_password_hash = hasher.hexdigest().upper() entity_id = None with db_connection.cursor() as cursor: From cd170b4183c0b0838d85ad4ad5f9aeb53dba2c20 Mon Sep 17 00:00:00 2001 From: Jeremy Frasier Date: Wed, 10 Nov 2021 12:41:56 -0500 Subject: [PATCH 74/92] Get rid of need for noqa in one line --- src/guacscanner/__init__.py | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/src/guacscanner/__init__.py b/src/guacscanner/__init__.py index 5e500b44..090f9814 100644 --- a/src/guacscanner/__init__.py +++ b/src/guacscanner/__init__.py @@ -1,10 +1,22 @@ """The guacscanner library.""" -# We disable a Flake8 check for "Module imported but unused (F401)" here because -# although this import is not directly used, it populates the value -# package_name.__version__, which is used to get version information about this -# Python package. -from . import guacscanner # noqa: F401 +# We disable a Flake8 check for "Module imported but unused (F401)" +# here because, although this import is not directly used, it +# populates the value package_name.__version__, which is used to get +# version information about this Python package. from ._version import __version__ # noqa: F401 +from .guacscanner import ( + add_instance_connection, + add_user, + check_for_ghost_instances, + entity_exists, + get_connection_name, + get_entity_id, + instance_connection_exists, + main, + process_instance, + remove_connection, + remove_instance_connections, +) __all__ = [ "add_instance_connection", @@ -17,5 +29,5 @@ "main", "process_instance", "remove_connection", - "remove_instance_connection", + "remove_instance_connections", ] From c69e7756e9f9ddbc3155bd321009dacc2e7799aa Mon Sep 17 00:00:00 2001 From: Jeremy Frasier Date: Wed, 10 Nov 2021 12:44:51 -0500 Subject: [PATCH 75/92] Remove package_data code that is commented out anyway Also remove the src/guacscanner/data directory. --- setup.py | 2 -- src/guacscanner/data/secret.txt | 1 - 2 files changed, 3 deletions(-) delete mode 100644 src/guacscanner/data/secret.txt diff --git a/setup.py b/setup.py index cfda3707..fb9a0d6b 100644 --- a/setup.py +++ b/setup.py @@ -84,8 +84,6 @@ def get_version(version_file): keywords="aws, guacamole, vpc", packages=find_packages(where="src"), package_dir={"": "src"}, - # This package contains no data - # package_data={"guacscanner": ["data/*.txt"]}, py_modules=[splitext(basename(path))[0] for path in glob("src/*.py")], include_package_data=True, install_requires=[ diff --git a/src/guacscanner/data/secret.txt b/src/guacscanner/data/secret.txt deleted file mode 100644 index c40a49b5..00000000 --- a/src/guacscanner/data/secret.txt +++ /dev/null @@ -1 +0,0 @@ -Three may keep a secret, if two of them are dead. From a6278b86fbb2d7023dbb90e895dc4db74f0ce27e Mon Sep 17 00:00:00 2001 From: Jeremy Frasier Date: Wed, 10 Nov 2021 13:06:48 -0500 Subject: [PATCH 76/92] Add todo comment re validating AWS region names passed in on the command line --- src/guacscanner/guacscanner.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/guacscanner/guacscanner.py b/src/guacscanner/guacscanner.py index 66d1af85..eeac9642 100644 --- a/src/guacscanner/guacscanner.py +++ b/src/guacscanner/guacscanner.py @@ -697,6 +697,8 @@ def main() -> None: logging.debug("DB connection string is %s.", db_connection_string) vpc_id = validated_args["--vpc-id"] + # TODO: Verify that the region specified is indeed a valid AWS + # region. See cisagov/guacscanner#6 for more details. region = validated_args["--region"] # If no VPC ID was specified on the command line then grab the VPC From c5f57d16b05a7480cf6330e97ecceb96af4fe3ca Mon Sep 17 00:00:00 2001 From: Jeremy Frasier Date: Wed, 10 Nov 2021 13:12:12 -0500 Subject: [PATCH 77/92] Add a comment explaining the use of a named capture group --- src/guacscanner/guacscanner.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/guacscanner/guacscanner.py b/src/guacscanner/guacscanner.py index eeac9642..61059b91 100644 --- a/src/guacscanner/guacscanner.py +++ b/src/guacscanner/guacscanner.py @@ -86,6 +86,10 @@ ] # Some precompiled regexes +# +# Note the use of a named capture group here via the (?P...) +# syntax, as described here: +# https://docs.python.org/3/library/re.html#regular-expression-syntax INSTANCE_ID_REGEX = re.compile(r"^.* \((?Pi-[0-9a-f]{17})\)$") VPC_ID_REGEX = re.compile(r"^vpc-([0-9a-f]{8}|[0-9a-f]{17})$") From 5c45b4a842da73bd7525722553d0ab6f80bd7c61 Mon Sep 17 00:00:00 2001 From: Jeremy Frasier Date: Wed, 10 Nov 2021 13:25:21 -0500 Subject: [PATCH 78/92] Add a TODO comment to reduce duplication of mock EC2 setup code This comment references #7. --- tests/test_guacscanner.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/test_guacscanner.py b/tests/test_guacscanner.py index c0918efd..cc661f74 100644 --- a/tests/test_guacscanner.py +++ b/tests/test_guacscanner.py @@ -223,6 +223,9 @@ def test_guacuser_already_exists(): def test_new_linux_instance(): """Verify that adding a new Linux instance works as expected.""" # Create and populate a VPC with an EC2 instance + # + # TODO: Create a test fixture to reduce duplicatin of this EC2 + # setup code. See cisagov/guacscanner#7 for more details. ec2 = boto3.client("ec2", "us-east-1") vpc = ec2.create_vpc(CidrBlock="10.19.74.0/24") vpc_id = vpc["Vpc"]["VpcId"] From 478e1a377c55afff353bb5cb86acd88e6e6c13c7 Mon Sep 17 00:00:00 2001 From: Jeremy Frasier Date: Wed, 10 Nov 2021 13:34:20 -0500 Subject: [PATCH 79/92] Add TODO comment for loosening version pins --- setup.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/setup.py b/setup.py index fb9a0d6b..6252bd6f 100644 --- a/setup.py +++ b/setup.py @@ -86,6 +86,8 @@ def get_version(version_file): package_dir={"": "src"}, py_modules=[splitext(basename(path))[0] for path in glob("src/*.py")], include_package_data=True, + # TODO: Loosen these requirements. See cisagov/guacscanner#9 for + # more details. install_requires=[ "boto3 == 1.19.6", "docopt == 0.6.2", From 6b2db012e68f11f1f66d4a31e64ab094bf2eade9 Mon Sep 17 00:00:00 2001 From: Jeremy Frasier Date: Wed, 10 Nov 2021 13:38:25 -0500 Subject: [PATCH 80/92] Remove debug logging statement that could contain sensitive information --- src/guacscanner/guacscanner.py | 1 - 1 file changed, 1 deletion(-) diff --git a/src/guacscanner/guacscanner.py b/src/guacscanner/guacscanner.py index 61059b91..9aae8366 100644 --- a/src/guacscanner/guacscanner.py +++ b/src/guacscanner/guacscanner.py @@ -698,7 +698,6 @@ def main() -> None: private_ssh_key = file.read() db_connection_string = f"user={postgres_username} password={postgres_password} host={postgres_hostname} port={postgres_port} dbname={postgres_db_name}" - logging.debug("DB connection string is %s.", db_connection_string) vpc_id = validated_args["--vpc-id"] # TODO: Verify that the region specified is indeed a valid AWS From d1eb68f94e250f6bafc0d30d5b0a8019d26edb6d Mon Sep 17 00:00:00 2001 From: Jeremy Frasier Date: Wed, 10 Nov 2021 13:45:06 -0500 Subject: [PATCH 81/92] Avoid importing from psycopg in two different ways LGTM complains about this on the grounds that it is confusing. --- src/guacscanner/guacscanner.py | 129 +++++++++++++++++---------------- 1 file changed, 65 insertions(+), 64 deletions(-) diff --git a/src/guacscanner/guacscanner.py b/src/guacscanner/guacscanner.py index 9aae8366..3a3c4c1f 100644 --- a/src/guacscanner/guacscanner.py +++ b/src/guacscanner/guacscanner.py @@ -55,7 +55,6 @@ import docopt from ec2_metadata import ec2_metadata import psycopg -from psycopg import sql from schema import And, Optional, Or, Schema, SchemaError, Use from ._version import __version__ @@ -94,123 +93,125 @@ VPC_ID_REGEX = re.compile(r"^vpc-([0-9a-f]{8}|[0-9a-f]{17})$") # TODO: Determine if we can use f-strings instead of .format() for -# these queries. Also define the sql.Identifier() variables +# these queries. Also define the psycopg.sql.Identifier() variables # separately so that they can be reused where that is possible. See # cisagov/guacscanner#3 for more details. # The PostgreSQL queries used for adding and removing connections -COUNT_QUERY = sql.SQL( +COUNT_QUERY = psycopg.sql.SQL( "SELECT COUNT({id_field}) FROM {table} WHERE {name_field} = %s" ).format( - id_field=sql.Identifier("connection_id"), - table=sql.Identifier("guacamole_connection"), - name_field=sql.Identifier("connection_name"), + id_field=psycopg.sql.Identifier("connection_id"), + table=psycopg.sql.Identifier("guacamole_connection"), + name_field=psycopg.sql.Identifier("connection_name"), ) -IDS_QUERY = sql.SQL("SELECT {id_field} FROM {table} WHERE {name_field} = %s").format( - id_field=sql.Identifier("connection_id"), - table=sql.Identifier("guacamole_connection"), - name_field=sql.Identifier("connection_name"), +IDS_QUERY = psycopg.sql.SQL( + "SELECT {id_field} FROM {table} WHERE {name_field} = %s" +).format( + id_field=psycopg.sql.Identifier("connection_id"), + table=psycopg.sql.Identifier("guacamole_connection"), + name_field=psycopg.sql.Identifier("connection_name"), ) -NAMES_QUERY = sql.SQL("SELECT {id_field}, {name_field} FROM {table}").format( - id_field=sql.Identifier("connection_id"), - name_field=sql.Identifier("connection_name"), - table=sql.Identifier("guacamole_connection"), +NAMES_QUERY = psycopg.sql.SQL("SELECT {id_field}, {name_field} FROM {table}").format( + id_field=psycopg.sql.Identifier("connection_id"), + name_field=psycopg.sql.Identifier("connection_name"), + table=psycopg.sql.Identifier("guacamole_connection"), ) -INSERT_CONNECTION_QUERY = sql.SQL( +INSERT_CONNECTION_QUERY = psycopg.sql.SQL( """INSERT INTO {table} ( {name_field}, {protocol_field}, {max_connections_field}, {max_connections_per_user_field}, {proxy_port_field}, {proxy_hostname_field}, {proxy_encryption_method_field}) VALUES (%s, %s, %s, %s, %s, %s, %s) RETURNING {id_field};""" ).format( - table=sql.Identifier("guacamole_connection"), - name_field=sql.Identifier("connection_name"), - protocol_field=sql.Identifier("protocol"), - max_connections_field=sql.Identifier("max_connections"), - max_connections_per_user_field=sql.Identifier("max_connections_per_user"), - proxy_port_field=sql.Identifier("proxy_port"), - proxy_hostname_field=sql.Identifier("proxy_hostname"), - proxy_encryption_method_field=sql.Identifier("proxy_encryption_method"), - id_field=sql.Identifier("connection_id"), + table=psycopg.sql.Identifier("guacamole_connection"), + name_field=psycopg.sql.Identifier("connection_name"), + protocol_field=psycopg.sql.Identifier("protocol"), + max_connections_field=psycopg.sql.Identifier("max_connections"), + max_connections_per_user_field=psycopg.sql.Identifier("max_connections_per_user"), + proxy_port_field=psycopg.sql.Identifier("proxy_port"), + proxy_hostname_field=psycopg.sql.Identifier("proxy_hostname"), + proxy_encryption_method_field=psycopg.sql.Identifier("proxy_encryption_method"), + id_field=psycopg.sql.Identifier("connection_id"), ) -INSERT_CONNECTION_PARAMETER_QUERY = sql.SQL( +INSERT_CONNECTION_PARAMETER_QUERY = psycopg.sql.SQL( """INSERT INTO {table} ({id_field}, {parameter_name_field}, {parameter_value_field}) VALUES (%s, %s, %s);""" ).format( - table=sql.Identifier("guacamole_connection_parameter"), - id_field=sql.Identifier("connection_id"), - parameter_name_field=sql.Identifier("parameter_name"), - parameter_value_field=sql.Identifier("parameter_value"), + table=psycopg.sql.Identifier("guacamole_connection_parameter"), + id_field=psycopg.sql.Identifier("connection_id"), + parameter_name_field=psycopg.sql.Identifier("parameter_name"), + parameter_value_field=psycopg.sql.Identifier("parameter_value"), ) -DELETE_CONNECTIONS_QUERY = sql.SQL( +DELETE_CONNECTIONS_QUERY = psycopg.sql.SQL( """DELETE FROM {table} WHERE {id_field} = %s;""" ).format( - table=sql.Identifier("guacamole_connection"), - id_field=sql.Identifier("connection_id"), + table=psycopg.sql.Identifier("guacamole_connection"), + id_field=psycopg.sql.Identifier("connection_id"), ) -DELETE_CONNECTION_PARAMETERS_QUERY = sql.SQL( +DELETE_CONNECTION_PARAMETERS_QUERY = psycopg.sql.SQL( """DELETE FROM {table} WHERE {id_field} = %s;""" ).format( - table=sql.Identifier("guacamole_connection_parameter"), - id_field=sql.Identifier("connection_id"), + table=psycopg.sql.Identifier("guacamole_connection_parameter"), + id_field=psycopg.sql.Identifier("connection_id"), ) # The PostgreSQL queries used for adding and removing users -ENTITY_COUNT_QUERY = sql.SQL( +ENTITY_COUNT_QUERY = psycopg.sql.SQL( "SELECT COUNT({id_field}) FROM {table} WHERE {name_field} = %s AND {type_field} = %s" ).format( - id_field=sql.Identifier("entity_id"), - table=sql.Identifier("guacamole_entity"), - name_field=sql.Identifier("name"), - type_field=sql.Identifier("type"), + id_field=psycopg.sql.Identifier("entity_id"), + table=psycopg.sql.Identifier("guacamole_entity"), + name_field=psycopg.sql.Identifier("name"), + type_field=psycopg.sql.Identifier("type"), ) -ENTITY_ID_QUERY = sql.SQL( +ENTITY_ID_QUERY = psycopg.sql.SQL( "SELECT {id_field} FROM {table} WHERE {name_field} = %s AND {type_field} = %s" ).format( - id_field=sql.Identifier("entity_id"), - table=sql.Identifier("guacamole_entity"), - name_field=sql.Identifier("name"), - type_field=sql.Identifier("type"), + id_field=psycopg.sql.Identifier("entity_id"), + table=psycopg.sql.Identifier("guacamole_entity"), + name_field=psycopg.sql.Identifier("name"), + type_field=psycopg.sql.Identifier("type"), ) -INSERT_ENTITY_QUERY = sql.SQL( +INSERT_ENTITY_QUERY = psycopg.sql.SQL( """INSERT INTO {table} ( {name_field}, {type_field}) VALUES (%s, %s) RETURNING {id_field};""" ).format( - table=sql.Identifier("guacamole_entity"), - name_field=sql.Identifier("name"), - type_field=sql.Identifier("type"), - id_field=sql.Identifier("entity_id"), + table=psycopg.sql.Identifier("guacamole_entity"), + name_field=psycopg.sql.Identifier("name"), + type_field=psycopg.sql.Identifier("type"), + id_field=psycopg.sql.Identifier("entity_id"), ) -INSERT_USER_QUERY = sql.SQL( +INSERT_USER_QUERY = psycopg.sql.SQL( """INSERT INTO {table} ( {id_field}, {hash_field}, {salt_field}, {date_field}) VALUES (%s, %s, %s, %s);""" ).format( - table=sql.Identifier("guacamole_user"), - id_field=sql.Identifier("entity_id"), - hash_field=sql.Identifier("password_hash"), - salt_field=sql.Identifier("password_salt"), - date_field=sql.Identifier("password_date"), + table=psycopg.sql.Identifier("guacamole_user"), + id_field=psycopg.sql.Identifier("entity_id"), + hash_field=psycopg.sql.Identifier("password_hash"), + salt_field=psycopg.sql.Identifier("password_salt"), + date_field=psycopg.sql.Identifier("password_date"), ) # The PostgreSQL queries used to add and remove connection # permissions -INSERT_CONNECTION_PERMISSION_QUERY = sql.SQL( +INSERT_CONNECTION_PERMISSION_QUERY = psycopg.sql.SQL( """INSERT INTO {table} ( {entity_id_field}, {connection_id_field}, {permission_field}) VALUES (%s, %s, %s);""" ).format( - table=sql.Identifier("guacamole_connection_permission"), - entity_id_field=sql.Identifier("entity_id"), - connection_id_field=sql.Identifier("connection_id"), - permission_field=sql.Identifier("permission"), + table=psycopg.sql.Identifier("guacamole_connection_permission"), + entity_id_field=psycopg.sql.Identifier("entity_id"), + connection_id_field=psycopg.sql.Identifier("connection_id"), + permission_field=psycopg.sql.Identifier("permission"), ) -DELETE_CONNECTION_PERMISSIONS_QUERY = sql.SQL( +DELETE_CONNECTION_PERMISSIONS_QUERY = psycopg.sql.SQL( """DELETE FROM {table} WHERE {connection_id_field} = %s;""" ).format( - table=sql.Identifier("guacamole_connection_permission"), - connection_id_field=sql.Identifier("connection_id"), + table=psycopg.sql.Identifier("guacamole_connection_permission"), + connection_id_field=psycopg.sql.Identifier("connection_id"), ) From 08427575408d3d8542fee260fa5441b2eee4b748 Mon Sep 17 00:00:00 2001 From: Jeremy Frasier Date: Wed, 10 Nov 2021 13:51:20 -0500 Subject: [PATCH 82/92] Add an LGTM comment string to disable a warning We must use the same password hashing algorithm as is used in the Guacamole source code, so we cannot avoid the LGTM warning. --- src/guacscanner/guacscanner.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/guacscanner/guacscanner.py b/src/guacscanner/guacscanner.py index 3a3c4c1f..c278772c 100644 --- a/src/guacscanner/guacscanner.py +++ b/src/guacscanner/guacscanner.py @@ -293,7 +293,10 @@ def add_user( # guacadmin password by the database initialization script. hexed_salt = salt.hex().upper() hasher = hashlib.sha256() - hasher.update(password.encode()) + # We must use the same password hashing algorithm as is used in + # the Guacamole source code, so we cannot avoid the LGTM warning + # here. + hasher.update(password.encode()) # lgtm[py/weak-sensitive-data-hashing] hasher.update(hexed_salt.encode()) salted_password_hash = hasher.hexdigest().upper() From da2df6b4af3287f9206c06767222af3c54a2cdcf Mon Sep 17 00:00:00 2001 From: Jeremy Frasier Date: Wed, 10 Nov 2021 13:52:35 -0500 Subject: [PATCH 83/92] Do not include username in logging statement Our linters think this is logging of sensitive information, which is reasonable. --- src/guacscanner/guacscanner.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/guacscanner/guacscanner.py b/src/guacscanner/guacscanner.py index c278772c..886a49a0 100644 --- a/src/guacscanner/guacscanner.py +++ b/src/guacscanner/guacscanner.py @@ -271,7 +271,7 @@ def add_user( should be an ASCII string. """ - logging.debug("Adding user entry for %s.", username) + logging.debug("Adding user entry.") if password is None: # Generate a random password consisting of ASCII letters and From 40269cae2c44011cbe6e55585e4d0714b5e3f03f Mon Sep 17 00:00:00 2001 From: Jeremy Frasier Date: Wed, 10 Nov 2021 14:20:57 -0500 Subject: [PATCH 84/92] Bump version from 0.0.1 to 0.0.2 --- src/guacscanner/_version.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/guacscanner/_version.py b/src/guacscanner/_version.py index 33cee844..5a5a7325 100644 --- a/src/guacscanner/_version.py +++ b/src/guacscanner/_version.py @@ -1,2 +1,2 @@ """This file defines the version of this module.""" -__version__ = "0.0.1" +__version__ = "0.0.2" From 70e36169298192327c6737a69972a22fa43c315c Mon Sep 17 00:00:00 2001 From: Jeremy Frasier Date: Wed, 10 Nov 2021 14:21:17 -0500 Subject: [PATCH 85/92] Bump version from 0.0.2 to 0.0.2-rc.1 --- src/guacscanner/_version.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/guacscanner/_version.py b/src/guacscanner/_version.py index 5a5a7325..d154cb19 100644 --- a/src/guacscanner/_version.py +++ b/src/guacscanner/_version.py @@ -1,2 +1,2 @@ """This file defines the version of this module.""" -__version__ = "0.0.2" +__version__ = "0.0.2-rc.1" From 6dd384b8dd728da8c759cf405c1a70b1891e1526 Mon Sep 17 00:00:00 2001 From: Shane Frasier Date: Wed, 10 Nov 2021 15:09:32 -0500 Subject: [PATCH 86/92] Rename a variable to preserve alphabetical order and yet keep related variables together Co-authored-by: dav3r --- src/guacscanner/guacscanner.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/guacscanner/guacscanner.py b/src/guacscanner/guacscanner.py index 886a49a0..4a7c2782 100644 --- a/src/guacscanner/guacscanner.py +++ b/src/guacscanner/guacscanner.py @@ -71,7 +71,7 @@ "running", ] DEFAULT_PASSWORD_LENGTH = 32 -DEFAULT_SALT_LENGTH = 32 +DEFAULT_PASSWORD_SALT_LENGTH = 32 DEFAULT_POSTGRES_DB_NAME = "guacamole_db" DEFAULT_POSTGRES_HOSTNAME = "postgres" DEFAULT_POSTGRES_PORT = 5432 @@ -282,7 +282,7 @@ def add_user( ) if salt is None: # Generate a random byte array - salt = secrets.token_bytes(DEFAULT_SALT_LENGTH) + salt = secrets.token_bytes(DEFAULT_PASSWORD_SALT_LENGTH) # Compute the salted password hash that is to be saved to the # database. From 36025de8aeaaa51a3682e6484964728c01b7b2e1 Mon Sep 17 00:00:00 2001 From: Shane Frasier Date: Wed, 10 Nov 2021 15:10:24 -0500 Subject: [PATCH 87/92] Fix a typo Co-authored-by: dav3r --- tests/test_guacscanner.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_guacscanner.py b/tests/test_guacscanner.py index cc661f74..da1b41a8 100644 --- a/tests/test_guacscanner.py +++ b/tests/test_guacscanner.py @@ -224,7 +224,7 @@ def test_new_linux_instance(): """Verify that adding a new Linux instance works as expected.""" # Create and populate a VPC with an EC2 instance # - # TODO: Create a test fixture to reduce duplicatin of this EC2 + # TODO: Create a test fixture to reduce duplication of this EC2 # setup code. See cisagov/guacscanner#7 for more details. ec2 = boto3.client("ec2", "us-east-1") vpc = ec2.create_vpc(CidrBlock="10.19.74.0/24") From 95de41315f7ec46c8c9b58f3f68436b1f6d9b4df Mon Sep 17 00:00:00 2001 From: Jeremy Frasier Date: Wed, 10 Nov 2021 23:13:27 -0500 Subject: [PATCH 88/92] Use a dataclass for passing Guacamole connection parameters as function inputs This simplifies function signatures and makes modifying the list of connection parameters less error-prone. Co-authored-by: Nick M <50747025+mcdonnnj@users.noreply.github.com> --- src/guacscanner/ConnectionParameters.py | 34 +++++++++++++++++ src/guacscanner/__init__.py | 2 + src/guacscanner/guacscanner.py | 50 +++++++++++-------------- 3 files changed, 58 insertions(+), 28 deletions(-) create mode 100644 src/guacscanner/ConnectionParameters.py diff --git a/src/guacscanner/ConnectionParameters.py b/src/guacscanner/ConnectionParameters.py new file mode 100644 index 00000000..01b81b36 --- /dev/null +++ b/src/guacscanner/ConnectionParameters.py @@ -0,0 +1,34 @@ +"""A dataclass container for Guacamole connection parameters.""" + + +# Standard Python Libraries +from dataclasses import dataclass + + +@dataclass +class ConnectionParameters: + """A dataclass container for Guacamole connection parameters.""" + + """The slots for this dataclass.""" + __slots__ = ( + "private_ssh_key", + "rdp_password", + "rdp_username", + "vnc_password", + "vnc_username", + ) + + """The private SSH key to use when transferring data via VNC.""" + private_ssh_key: str + + """The password to use when Guacamole establishes an RDP connection.""" + rdp_password: str + + """The user name to use when Guacamole establishes an RDP connection.""" + rdp_username: str + + """The password to use when Guacamole establishes a VNC connection.""" + vnc_password: str + + """The user name to use when Guacamole establishes a VNC connection.""" + vnc_username: str diff --git a/src/guacscanner/__init__.py b/src/guacscanner/__init__.py index 090f9814..3f9d58ac 100644 --- a/src/guacscanner/__init__.py +++ b/src/guacscanner/__init__.py @@ -5,6 +5,7 @@ # version information about this Python package. from ._version import __version__ # noqa: F401 from .guacscanner import ( + ConnectionParameters, add_instance_connection, add_user, check_for_ghost_instances, @@ -19,6 +20,7 @@ ) __all__ = [ + "ConnectionParameters", "add_instance_connection", "add_user", "check_for_ghost_instances", diff --git a/src/guacscanner/guacscanner.py b/src/guacscanner/guacscanner.py index 4a7c2782..270e8e83 100644 --- a/src/guacscanner/guacscanner.py +++ b/src/guacscanner/guacscanner.py @@ -57,6 +57,7 @@ import psycopg from schema import And, Optional, Or, Schema, SchemaError, Use +from .ConnectionParameters import ConnectionParameters from ._version import __version__ # TODO: Add exception handling for all the database accesses and @@ -350,11 +351,7 @@ def instance_connection_exists(db_connection, connection_name): def add_instance_connection( db_connection, instance, - vnc_username, - vnc_password, - private_ssh_key, - rdp_username, - rdp_password, + connection_parameters: ConnectionParameters, entity_id, ): """Add a connection for the EC2 instance.""" @@ -394,17 +391,17 @@ def add_instance_connection( ( connection_id, "sftp-directory", - f"/home/{vnc_username}/Documents", + f"/home/{connection_parameters.vnc_username}/Documents", ), ( connection_id, "sftp-username", - vnc_username, + connection_parameters.vnc_username, ), ( connection_id, "sftp-private-key", - private_ssh_key, + connection_parameters.private_ssh_key, ), ( connection_id, @@ -414,7 +411,7 @@ def add_instance_connection( ( connection_id, "sftp-root-directory", - f"/home/{vnc_username}/", + f"/home/{connection_parameters.vnc_username}/", ), ( connection_id, @@ -434,7 +431,7 @@ def add_instance_connection( ( connection_id, "password", - vnc_password, + connection_parameters.vnc_password, ), ( connection_id, @@ -443,7 +440,10 @@ def add_instance_connection( ), ) if is_windows: - guac_conn_params = ( + # mypy gives a warning on this line because we are + # re-assigning the variable with a tuple of a different + # length, but we know this is safe to do here. + guac_conn_params = ( # type: ignore ( connection_id, "ignore-cert", @@ -457,7 +457,7 @@ def add_instance_connection( ( connection_id, "password", - rdp_password, + connection_parameters.rdp_password, ), ( connection_id, @@ -467,7 +467,7 @@ def add_instance_connection( ( connection_id, "username", - rdp_username, + connection_parameters.rdp_username, ), ) @@ -537,11 +537,7 @@ def process_instance( instance, add_instance_states, remove_instance_states, - vnc_username, - vnc_password, - private_ssh_key, - rdp_username, - rdp_password, + connection_parameters: ConnectionParameters, entity_id, ): """Add/remove connections for the specified EC2 instance.""" @@ -560,11 +556,7 @@ def process_instance( add_instance_connection( db_connection, instance, - vnc_username, - vnc_password, - private_ssh_key, - rdp_username, - rdp_password, + connection_parameters, entity_id, ) else: @@ -782,11 +774,13 @@ def main() -> None: instance, add_instance_states, remove_instance_states, - vnc_username, - vnc_password, - private_ssh_key, - rdp_username, - rdp_password, + ConnectionParameters( + private_ssh_key=private_ssh_key, + rdp_password=rdp_password, + rdp_username=rdp_username, + vnc_password=vnc_password, + vnc_username=vnc_username, + ), guacuser_id, ) From 270ac59e3fc204292c8552d5c867ef47253dd64a Mon Sep 17 00:00:00 2001 From: Jeremy Frasier Date: Fri, 12 Nov 2021 13:55:16 -0500 Subject: [PATCH 89/92] Do not create DB connection in a context manager @mcdonnnj pointed out that psycopg.connect() does not entirely act as expected in that it does not automatically close the connection when you exit the context. It only rolls back transactions in the event that one fails. Since we are managing transactions in this code via explicit calls to psycopg.Connection.commit(), the context manager isn't actually buying us anything; therefore, I have gotten rid of it. I added an explicit psycopg.Connection.close() at the bottom of the loop. I don't want to create the connection outside the loop and leave it open because the guacscanner may be up and running for months at a time, and who knows what could go wrong. It seems safer to just recreate the connection at the top of the loop. --- src/guacscanner/guacscanner.py | 154 +++++++++++++++++---------------- 1 file changed, 78 insertions(+), 76 deletions(-) diff --git a/src/guacscanner/guacscanner.py b/src/guacscanner/guacscanner.py index 270e8e83..a85cfe5a 100644 --- a/src/guacscanner/guacscanner.py +++ b/src/guacscanner/guacscanner.py @@ -718,88 +718,90 @@ def main() -> None: keep_looping = True guacuser_id = None while keep_looping: + time.sleep(validated_args["--sleep"]) + try: - with psycopg.connect( + db_connection = psycopg.connect( db_connection_string, row_factory=psycopg.rows.dict_row - ) as db_connection: - # Create guacuser if it doesn't already exist - # - # TODO: Figure out a way to make this cleaner. We - # don't want to hardcode the guacuser name, and we - # want to allow the user to specify a list of users - # that should be created if they don't exist and given - # access to use the connections created by - # guacscanner. See cisagov/guacscanner#4 for more - # details. - if guacuser_id is None: - # We haven't initialized guacuser_id yet, so let's - # do it now. - if not entity_exists(db_connection, "guacuser", "USER"): - guacuser_id = add_user(db_connection, "guacuser") - else: - guacuser_id = get_entity_id(db_connection, "guacuser", "USER") - - for instance in instances: - ami = ec2.Image(instance.image_id) - # Early exit if this instance is running an AMI - # that we want to avoid adding to Guacamole. - try: - ami_matches = [ - regex.match(ami.name) for regex in DEFAULT_AMI_SKIP_REGEXES - ] - except AttributeError: - # This exception can be thrown when an - # instance is running an AMI to which the - # account no longer has access; for example, - # between the time when a new AMI of the same - # type is built and terraform-post-packer is - # run and the new AMI is applied to the - # account. In this situation we can't take - # any action because we can't access the AMI's - # name and hence can't know if the instance - # AMI is of a type whose Guacamole connections - # are being controlled by guacscanner. - # - # In any event, this continue statement should - # keep things moving when it does. - logging.exception( - "Unable to determine if instance is running an AMI that would cause it to be skipped." - ) - continue - if any(ami_matches): - continue - - process_instance( - db_connection, - instance, - add_instance_states, - remove_instance_states, - ConnectionParameters( - private_ssh_key=private_ssh_key, - rdp_password=rdp_password, - rdp_username=rdp_username, - vnc_password=vnc_password, - vnc_username=vnc_username, - ), - guacuser_id, - ) - - logging.info( - "Checking to see if any connections belonging to nonexistent instances are in the database." - ) - check_for_ghost_instances(db_connection, instances) - - if oneshot: - logging.debug( - "Stopping Guacamole connection update loop because --oneshot is present." - ) - keep_looping = False - continue + ) except psycopg.OperationalError: logging.exception( "Unable to connect to the PostgreSQL database backending Guacamole." ) + continue + + # Create guacuser if it doesn't already exist + # + # TODO: Figure out a way to make this cleaner. We don't want + # to hardcode the guacuser name, and we want to allow the user + # to specify a list of users that should be created if they + # don't exist and given access to use the connections created + # by guacscanner. See cisagov/guacscanner#4 for more details. + if guacuser_id is None: + # We haven't initialized guacuser_id yet, so let's do it + # now. + if not entity_exists(db_connection, "guacuser", "USER"): + guacuser_id = add_user(db_connection, "guacuser") + else: + guacuser_id = get_entity_id(db_connection, "guacuser", "USER") + + for instance in instances: + ami = ec2.Image(instance.image_id) + # Early exit if this instance is running an AMI that we + # want to avoid adding to Guacamole. + try: + ami_matches = [ + regex.match(ami.name) for regex in DEFAULT_AMI_SKIP_REGEXES + ] + except AttributeError: + # This exception can be thrown when an instance is + # running an AMI to which the account no longer has + # access; for example, between the time when a new AMI + # of the same type is built and terraform-post-packer + # is run and the new AMI is applied to the account. + # In this situation we can't take any action because + # we can't access the AMI's name and hence can't know + # if the instance AMI is of a type whose Guacamole + # connections are being controlled by guacscanner. + # + # In any event, this continue statement should keep + # things moving when it does. + logging.exception( + "Unable to determine if instance is running an AMI that would cause it to be skipped." + ) + continue + if any(ami_matches): + continue - time.sleep(validated_args["--sleep"]) + process_instance( + db_connection, + instance, + add_instance_states, + remove_instance_states, + ConnectionParameters( + private_ssh_key=private_ssh_key, + rdp_password=rdp_password, + rdp_username=rdp_username, + vnc_password=vnc_password, + vnc_username=vnc_username, + ), + guacuser_id, + ) + + logging.info( + "Checking to see if any connections belonging to nonexistent instances are in the database." + ) + check_for_ghost_instances(db_connection, instances) + + if oneshot: + logging.debug( + "Stopping Guacamole connection update loop because --oneshot is present." + ) + keep_looping = False + + # pycopg.connect() can act as a context manager, but the + # connection is not closed when you leave the context; + # therefore, we still have to close the connection manually. + db_connection.close() logging.shutdown() From 51bf9d2507be348deef25e561aac563fb6658183 Mon Sep 17 00:00:00 2001 From: Jeremy Frasier Date: Fri, 12 Nov 2021 14:02:23 -0500 Subject: [PATCH 90/92] Bump version from 0.0.2-rc.1 to 0.0.2-rc.2 --- src/guacscanner/_version.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/guacscanner/_version.py b/src/guacscanner/_version.py index d154cb19..07f61334 100644 --- a/src/guacscanner/_version.py +++ b/src/guacscanner/_version.py @@ -1,2 +1,2 @@ """This file defines the version of this module.""" -__version__ = "0.0.2-rc.1" +__version__ = "0.0.2-rc.2" From 73e625f965f9cdf2bf48446358a9ca9f90d3e8ff Mon Sep 17 00:00:00 2001 From: Jeremy Frasier Date: Fri, 12 Nov 2021 15:49:21 -0500 Subject: [PATCH 91/92] Bump version from 0.0.2-rc.2 to 0.0.2 --- src/guacscanner/_version.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/guacscanner/_version.py b/src/guacscanner/_version.py index 07f61334..5a5a7325 100644 --- a/src/guacscanner/_version.py +++ b/src/guacscanner/_version.py @@ -1,2 +1,2 @@ """This file defines the version of this module.""" -__version__ = "0.0.2-rc.2" +__version__ = "0.0.2" From d3887b2735e8213b8b3908edccfb78957979b94d Mon Sep 17 00:00:00 2001 From: Jeremy Frasier Date: Fri, 12 Nov 2021 16:10:52 -0500 Subject: [PATCH 92/92] Bump version from 0.0.2 to 1.0.0 --- src/guacscanner/_version.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/guacscanner/_version.py b/src/guacscanner/_version.py index 5a5a7325..de155d77 100644 --- a/src/guacscanner/_version.py +++ b/src/guacscanner/_version.py @@ -1,2 +1,2 @@ """This file defines the version of this module.""" -__version__ = "0.0.2" +__version__ = "1.0.0"