Introduce new REST API framework and refactor registrar implementation

This commit consolidates the work completed as part of pull request
keylime#1523 and contributes a new way of architecting REST APIs based loosely
on the model-view-controller (MVC) pattern. The class library in
`keylime.web.base` and `keylime.models.base` provides a number of
building blocks for structuring web service code in a consistent and
ordered fashion. These include:

- Routes: parameterised URI patterns for declaring API endpoints
- Servers: direct requests to a controller based on a list of routes
- Controllers: handle requests and produce an appropriate response
- Models: data structures which can be mutated, validated and persisted
  according to a set pattern

Additionally, this commit re-implements the registrar APIs in the new
paradigm. `keylime.registrar_common` is no longer invoked and is
effectively replaced by `keylime.web.registrar_server` and
`keylime.web.registrar.agents_controller`. The `registrarmain` database
table is now represented in memory using the `RegistrarAgent` model.
The model defines a schema for agent records and encapsulates the
functionality for mutating these records instead of overloading request
handlers with this responsibility. Certificate and key verification
checks are broken into several small methods each with a clear, minimal
purpose in an effort to ensure readability, traceability and
composability (even when the registrar is extended in the future).

The refactor of the registrar therefore acts as a good demonstration of
how the new web framework facilitates writing modular code with clear
separation of concerns. Future contributions to implement agent-driven
attestation (keylime/enhancements#103) will be done in a similar way.

Some minor features have been added or changed, e.g., request logging is
now more detailed and log messages try to be more helpful where
possible. The user now has the option of suppressing a portion of the
warnings generated when a certificate is not strictly ASN.1 DER
compliant, or even rejecting such certificates when received from an
agent. This partially fixes issue keylime#1559 (which will be further addressed
in subsequent PRs).

Other than that, this commit should be functionally equivalent to
earlier Keylime versions.

Squashes commits: 3b8119c, 796417d, a0d3cf7, 1b42ee2, c52b005, f5869aa,
3cd4c2a, 75facbd, e6ec507, 45a1362, 3c8f202, 30eb7dc, 2b9de05, b4a2df1,
1c2db6b, 705d9d4, e28baf6, 282071c, 2f7095d, f254a78, a249d28, 9fe4042,
b3eaa3e, ca3782a, 0ae1249, 9696a39, 33b7184, 7d8a4ee, 55eacb9, 2496836,
0d8a232, 4690017, ce9315a, 9c79359, 5055dc1, 9387a0b, 0db1fb8, 42fa62d,
89474ee, 7527175, fc1217e, de282c4, a28078f.
For context, refer to PR keylime#1523.

Signed-off-by: Jean Snyman <[email protected]>

Author: stringlytyped

.github/workflows/test.yml

@@ -42,7 +42,7 @@ jobs:
     - name: Install Python dependencies
       run: sudo dnf -y install python3.6 tox python3-pip
     - name: Run lints
-      run: tox -vv -e 'pylint,pylint36'
+      run: tox -vv -e 'pylint'
     - name: Run mypy
       run: tox -vv -e 'mypy'
     - name: Run pyright

.packit.yaml

@@ -4,5 +4,5 @@ jobs:
   metadata:
     targets:
     - fedora-all
-    - centos-stream-9-x86_64
+    - centos-stream-10-x86_64
     skip_build: true

.pre-commit-config.yaml

@@ -6,6 +6,7 @@ repos:
       - id: isort
         name: isort (python)
   - repo: https://github.com/psf/black
-    rev: 23.1.0
+    rev: 23.10.0
     hooks:
       - id: black
+        args: ["--target-version", "py310"]

keylime.conf

@@ -674,6 +674,15 @@ signed_attributes = ek_tpm,aik_tpm,ekcert
 # "iak_idevid": only allow agents with an IAK and IDevID to register
 tpm_identity = default
 
+# The below option controls what Keylime does when it encounters a certificate which is not parse-able when strict
+# ASN.1 Distinguished Encoding Rules (DER) are enforced. The default behaviour ("warn") is to log a warning but still
+# accept the certificate, so long as it can be interpreted by a fallback parser.
+# The following values are accepted:
+# "warn": log a warning and re-encode the certificate with the more-forgiving fallback parser (the default)
+# "reject": log an error and refuse to accept the certificate
+# "ignore": silently re-encode the certificate without logging a message
+malformed_cert_action = warn
+
 
 #=============================================================================
 [ca]
@@ -723,7 +732,7 @@ keys = consoleHandler
 keys = formatter
 
 [formatter_formatter]
-format = %(asctime)s.%(msecs)03d - %(name)s - %(levelname)s - %(message)s
+format = %(asctime)s.%(msecs)03d - %(name)s - %(levelname)s - %(message)s %(reqidf)s
 datefmt = %Y-%m-%d %H:%M:%S
 
 [logger_root]

keylime/cmd/registrar.py

@@ -1,21 +1,55 @@
-from keylime import config, keylime_logging, registrar_common
+import sys
+
+import cryptography
+
+from keylime import config, keylime_logging
 from keylime.common.migrations import apply
+from keylime.models import da_manager, db_manager
+from keylime.web import RegistrarServer
 
 logger = keylime_logging.init_logging("registrar")
 
 
+def _check_devid_requirements() -> None:
+    """Checks that the cryptography package is the version needed for DevID support (>= 38). Exits if this requirement
+    is not met and DevID is the only identity allowable by the config.
+    """
+    tpm_identity = config.get("registrar", "tpm_identity", fallback="default")
+
+    if int(cryptography.__version__.split(".", maxsplit=1)[0]) < 38:
+        if tpm_identity == "iak_idevid":
+            logger.error(
+                "DevID is REQUIRED in config ('tpm_identity = %s') but python-cryptography version < 38", tpm_identity
+            )
+            sys.exit(1)
+
+        if tpm_identity in ("default", "ek_cert_or_iak_idevid"):
+            logger.info(
+                "DevID is enabled in config ('tpm_identity = %s') but python-cryptography version < 38, so only the EK "
+                "will be used for device registration",
+                tpm_identity,
+            )
+
+
 def main() -> None:
+    logger.info("Starting Keylime registrar...")
+
     config.check_version("registrar", logger=logger)
 
     # if we are configured to auto-migrate the DB, check if there are any migrations to perform
     if config.has_option("registrar", "auto_migrate_db") and config.getboolean("registrar", "auto_migrate_db"):
         apply("registrar")
 
-    registrar_common.start(
-        config.get("registrar", "ip"),
-        config.getint("registrar", "tls_port"),
-        config.getint("registrar", "port"),
-    )
+    # Check if DevID is required in config and, if so, that the required dependencies are met
+    _check_devid_requirements()
+    # Prepare to use the registrar database
+    db_manager.make_engine("registrar")
+    # Prepare backend for durable attestation, if configured
+    da_manager.make_backend("registrar")
+
+    # Start HTTP server
+    server = RegistrarServer()
+    server.start_multi()
 
 
 if __name__ == "__main__":

keylime/common/migrations.py

@@ -19,4 +19,4 @@ def apply(db_name: Optional[str]) -> None:
 
     alembic_args.extend(["upgrade", "head"])
 
-    alembic.config.main(argv=alembic_args)
+    alembic.config.main(argv=alembic_args)  # type: ignore[no-untyped-call]

keylime/da/record.py

@@ -46,9 +46,9 @@ def __init__(self, service: str, key_tls_pub: Optional[str] = "") -> None:
     def record_create(
         self,
         agent_data: Dict[Any, Any],
-        attestation_data: Dict[Any, Any],
+        attestation_data: Optional[Dict[Any, Any]],
         mb_policy_data: Optional[str],
-        runtime_policy_data: Dict[Any, Any],
+        runtime_policy_data: Optional[Dict[Any, Any]],
         service: str = "auto",
         signed_attributes: str = "auto",
     ) -> None:

keylime/keylime_logging.py

@@ -1,16 +1,23 @@
+import contextvars
 import logging
 from logging import Logger
 from logging import config as logging_config
-from typing import Any, Callable, Dict
+from typing import TYPE_CHECKING, Any, Callable, Dict
 
 from keylime import config
 
+if TYPE_CHECKING:
+    from logging import LogRecord
+
 try:
     logging_config.fileConfig(config.get_config("logging"))
 except KeyError:
     logging.basicConfig(format="%(asctime)s %(name)-12s %(levelname)-8s %(message)s", level=logging.DEBUG)
 
 
+request_id_var: contextvars.ContextVar[str] = contextvars.ContextVar("request_id")
+
+
 def set_log_func(loglevel: int, logger: Logger) -> Callable[..., None]:
     log_func = logger.info
 
@@ -45,8 +52,33 @@ def log_http_response(logger: Logger, loglevel: int, response_body: Dict[str, An
     return True
 
 
+def annotate_logger(logger: Logger) -> None:
+    request_id_filter = RequestIDFilter()
+
+    for handler in logger.handlers:
+        handler.addFilter(request_id_filter)
+
+
 def init_logging(loggername: str) -> Logger:
     logger = logging.getLogger(f"keylime.{loggername}")
     logging.getLogger("requests").setLevel(logging.WARNING)
 
+    # Disable default Tornado logs, as we are outputting more detail to the 'keylime.web' logger
+    logging.getLogger("tornado.general").disabled = True
+    logging.getLogger("tornado.access").disabled = True
+    logging.getLogger("tornado.application").disabled = True
+
+    # Add metadata to root logger, so that it is inherited by all
+    annotate_logger(logging.getLogger())
+
     return logger
+
+
+class RequestIDFilter(logging.Filter):
+    def filter(self, record: "LogRecord") -> bool:
+        reqid = request_id_var.get("")
+
+        record.reqid = reqid
+        record.reqidf = f"(reqid={reqid})" if reqid else ""
+
+        return True

keylime/migrations/versions/330024be7bef_convert_registrar_column_types.py

@@ -0,0 +1,99 @@
+"""Convert registrar column types 
+
+Revision ID: 330024be7bef
+Revises: 9d2f6fab52b1
+Create Date: 2024-02-15 11:48:41.458971
+
+"""
+import sqlalchemy as sa
+from alembic import op
+
+# revision identifiers, used by Alembic.
+revision = "330024be7bef"
+down_revision = "9d2f6fab52b1"
+branch_labels = None
+depends_on = None
+
+
+def upgrade(engine_name):
+    globals()[f"upgrade_{engine_name}"]()
+
+
+def downgrade(engine_name):
+    globals()[f"downgrade_{engine_name}"]()
+
+
+def upgrade_registrar():
+    with op.batch_alter_table("registrarmain") as batch_op:
+        # SQLite, MySQL and MariaDB do not have a native BOOLEAN datatype but Postgres does. In the former engines, True
+        # and False are automatically translated to 1 and 0 respectively. In Postgres, attempting to set an INTEGER
+        # column to True/False results in an error. To ensure consistent behaviour across engines, convert the relevant
+        # columns to the SQLAlchemy "Boolean" datatype which will automatically use the appropriate engine-native
+        # datatype (INTEGER for SQLite, TINYINT for MySQL/MariaDB and BOOLEAN for Postgres).
+        batch_op.alter_column(
+            "active",
+            existing_type=sa.Integer,
+            type_=sa.Boolean,
+            existing_nullable=True,
+            postgresql_using="active::boolean",
+        )
+        batch_op.alter_column(
+            "virtual",
+            existing_type=sa.Integer,
+            type_=sa.Boolean,
+            existing_nullable=True,
+            postgresql_using="virtual::boolean",
+        )
+        # Certificates can easily be more than 2048 characters when Base64 encoded. SQLite does not enforce length
+        # restrictions (VARCHAR(2048) = TEXT) which may have prevented this from being a problem in the past.
+        # The other engines do enforce these restrictions, so it is better to treat certificates as TEXT columns.
+        batch_op.alter_column(
+            "ekcert",
+            existing_type=sa.String(2048),
+            type_=sa.Text,
+            existing_nullable=True,
+        )
+        batch_op.alter_column(
+            "mtls_cert",
+            existing_type=sa.String(2048),
+            type_=sa.Text,
+            existing_nullable=True,
+        )
+
+
+def downgrade_registrar():
+    with op.batch_alter_table("registrarmain") as batch_op:
+        batch_op.alter_column(
+            "active",
+            existing_type=sa.Boolean,
+            type_=sa.Integer,
+            existing_nullable=True,
+            postgresql_using="active::integer",
+        )
+        batch_op.alter_column(
+            "virtual",
+            existing_type=sa.Boolean,
+            type_=sa.Integer,
+            existing_nullable=True,
+            postgresql_using="virtual::integer",
+        )
+        batch_op.alter_column(
+            "ekcert",
+            existing_type=sa.Text,
+            type_=sa.String(2048),
+            existing_nullable=True,
+        )
+        batch_op.alter_column(
+            "mtls_cert",
+            existing_type=sa.Text,
+            type_=sa.String(2048),
+            existing_nullable=True,
+        )
+
+
+def upgrade_cloud_verifier():
+    pass
+
+
+def downgrade_cloud_verifier():
+    pass

keylime/models/__init__.py

@@ -0,0 +1,17 @@
+# Checks whether script is being invoked by tox in a virtual environment
+def is_tox_env() -> bool:
+    # Import 'os' inside function to avoid polluting the namespace of any module which imports 'keylime.models'
+    import os  # pylint: disable=import-outside-toplevel
+
+    return bool(os.environ.get("TOX_ENV_NAME"))
+
+
+# Only perform automatic imports of submodules if tox is not being used to perform static checks. This is necessary as
+# models like RegistrarAgent indirectly import package 'gpg' which is not available in a tox environment as it is
+# installed via the system package manager
+if not is_tox_env():
+    from keylime.models.base.da import da_manager
+    from keylime.models.base.db import db_manager
+    from keylime.models.registrar import *
+
+    __all__ = ["da_manager", "db_manager"]

keylime/models/base/__init__.py

@@ -0,0 +1,27 @@
+from sqlalchemy import BigInteger, Boolean, Float, Integer, LargeBinary, SmallInteger, String, Text
+
+from keylime.models.base.basic_model import BasicModel
+from keylime.models.base.da import da_manager
+from keylime.models.base.db import db_manager
+from keylime.models.base.persistable_model import PersistableModel
+from keylime.models.base.types.certificate import Certificate
+from keylime.models.base.types.dictionary import Dictionary
+from keylime.models.base.types.one_of import OneOf
+
+__all__ = [
+    "BigInteger",
+    "Boolean",
+    "Float",
+    "Integer",
+    "LargeBinary",
+    "SmallInteger",
+    "String",
+    "Text",
+    "BasicModel",
+    "da_manager",
+    "db_manager",
+    "PersistableModel",
+    "Certificate",
+    "Dictionary",
+    "OneOf",
+]