From 95d083fca1093f4f4029f95272e248c063d0b4fc Mon Sep 17 00:00:00 2001 From: Max Erenberg Date: Sat, 21 Aug 2021 06:27:33 +0000 Subject: [PATCH] use our own SPNEGO implementation --- .drone.yml | 9 +-- .drone/auth1-setup.sh | 13 +++-- .drone/phosphoric-acid-setup.sh | 15 ++--- ceo_common/model/HTTPClient.py | 14 ++--- ceod/api/app_factory.py | 8 +-- ceod/api/error_handlers.py | 3 + ceod/api/spnego.py | 55 +++++++++++++++++++ ceod/api/utils.py | 2 +- requirements.txt | 1 - tests/ceo_common/model/test_remote_mailman.py | 2 +- tests/conftest.py | 15 ++++- tests/conftest_ceod_api.py | 4 +- 12 files changed, 104 insertions(+), 37 deletions(-) create mode 100644 ceod/api/spnego.py diff --git a/.drone.yml b/.drone.yml index f56d99f..835c5bd 100644 --- a/.drone.yml +++ b/.drone.yml @@ -1,19 +1,20 @@ kind: pipeline type: docker -name: phosphoric-acid +name: default steps: -- name: run tests + # use the step name to mock out the gethostname() call in our tests +- name: phosphoric-acid image: python:3.7-buster # unfortunately we have to do everything in one step because there's no # way to share system packages between steps commands: - # install dependencies + # install dependencies - apt update && apt install -y libkrb5-dev libsasl2-dev python3-dev - python3 -m venv venv - . venv/bin/activate - - pip install -r requirements.txt - pip install -r dev-requirements.txt + - pip install -r requirements.txt - cd ceo_common/krb5 && python krb5_build.py && cd ../.. # lint diff --git a/.drone/auth1-setup.sh b/.drone/auth1-setup.sh index 962ce0d..5314f8b 100755 --- a/.drone/auth1-setup.sh +++ b/.drone/auth1-setup.sh @@ -2,17 +2,18 @@ set -ex -# sanity check -test $(hostname) = auth1 - # don't resolve container names to *real* CSC machines sed -E '/^(domain|search)[[:space:]]+csclub.uwaterloo.ca/d' /etc/resolv.conf > /tmp/resolv.conf cat /tmp/resolv.conf > /etc/resolv.conf rm /tmp/resolv.conf +get_ip_addr() { + getent hosts $1 | cut -d' ' -f1 +} + add_fqdn_to_hosts() { - hostname=$1 - ip_addr=$(getent hosts $hostname | cut -d' ' -f1) + ip_addr=$1 + hostname=$2 sed -E "/${ip_addr}.*\\b${hostname}\\b/d" /etc/hosts > /tmp/hosts cat /tmp/hosts > /etc/hosts rm /tmp/hosts @@ -20,7 +21,7 @@ add_fqdn_to_hosts() { } # set FQDN in /etc/hosts -add_fqdn_to_hosts auth1 +add_fqdn_to_hosts $(get_ip_addr $(hostname)) auth1 export DEBIAN_FRONTEND=noninteractive apt update diff --git a/.drone/phosphoric-acid-setup.sh b/.drone/phosphoric-acid-setup.sh index c48b2eb..f1d0969 100755 --- a/.drone/phosphoric-acid-setup.sh +++ b/.drone/phosphoric-acid-setup.sh @@ -2,17 +2,18 @@ set -ex -# sanity check -test $(hostname) = phosphoric-acid - # don't resolve container names to *real* CSC machines sed -E '/^(domain|search)[[:space:]]+csclub.uwaterloo.ca/d' /etc/resolv.conf > /tmp/resolv.conf cat /tmp/resolv.conf > /etc/resolv.conf rm /tmp/resolv.conf +get_ip_addr() { + getent hosts $1 | cut -d' ' -f1 +} + add_fqdn_to_hosts() { - hostname=$1 - ip_addr=$(getent hosts $hostname | cut -d' ' -f1) + ip_addr=$1 + hostname=$2 sed -E "/${ip_addr}.*\\b${hostname}\\b/d" /etc/hosts > /tmp/hosts cat /tmp/hosts > /etc/hosts rm /tmp/hosts @@ -20,8 +21,8 @@ add_fqdn_to_hosts() { } # set FQDN in /etc/hosts -add_fqdn_to_hosts phosphoric-acid -add_fqdn_to_hosts auth1 +add_fqdn_to_hosts $(get_ip_addr $(hostname)) phosphoric-acid +add_fqdn_to_hosts $(get_ip_addr auth1) auth1 export DEBIAN_FRONTEND=noninteractive apt update diff --git a/ceo_common/model/HTTPClient.py b/ceo_common/model/HTTPClient.py index 98ac8e8..81d4bc5 100644 --- a/ceo_common/model/HTTPClient.py +++ b/ceo_common/model/HTTPClient.py @@ -24,18 +24,16 @@ class HTTPClient: def request(self, host: str, api_path: str, method='GET', **kwargs): principal = g.sasl_user - if '@' not in principal: - principal = principal + '@' + self.krb_realm gssapi_name = gssapi.Name(principal) creds = gssapi.Credentials(name=gssapi_name, usage='initiate') - auth = HTTPSPNEGOAuth( - opportunistic_auth=True, - target_name='ceod', - creds=creds, - ) - # always use the FQDN, for HTTPS purposes + # always use the FQDN if '.' not in host: host = host + '.' + self.base_domain + auth = HTTPSPNEGOAuth( + opportunistic_auth=True, + target_name=gssapi.Name('ceod/' + host), + creds=creds, + ) return requests.request( method, f'{self.scheme}://{host}:{self.ceod_port}{api_path}', diff --git a/ceod/api/app_factory.py b/ceod/api/app_factory.py index a7bc66d..b8de6c4 100644 --- a/ceod/api/app_factory.py +++ b/ceod/api/app_factory.py @@ -3,7 +3,6 @@ import os import socket from flask import Flask -from flask_kerberos import init_kerberos from zope import component from .error_handlers import register_error_handlers @@ -11,6 +10,7 @@ from .krb5_cred_handlers import before_request, teardown_request from ceo_common.interfaces import IConfig, IKerberosService, ILDAPService, IFileService, \ IMailmanService, IMailService, IUWLDAPService, IHTTPClient from ceo_common.model import Config, HTTPClient, RemoteMailmanService +from ceod.api.spnego import init_spnego from ceod.model import KerberosService, LDAPService, FileService, \ MailmanService, MailService, UWLDAPService @@ -23,9 +23,7 @@ def create_app(flask_config={}): register_services(app) cfg = component.getUtility(IConfig) - fqdn = socket.getfqdn() - os.environ['KRB5_KTNAME'] = '/etc/krb5.keytab' - init_kerberos(app, service='ceod', hostname=fqdn) + init_spnego('ceod') hostname = socket.gethostname() # Only ceod_admin_host should serve the /api/members endpoints because @@ -68,8 +66,6 @@ def register_services(app): component.provideUtility(cfg, IConfig) # KerberosService - if 'KRB5_KTNAME' not in os.environ: - os.environ['KRB5_KTNAME'] = '/etc/krb5.keytab' hostname = socket.gethostname() fqdn = socket.getfqdn() # Only ceod_admin_host has the ceod/admin key in its keytab diff --git a/ceod/api/error_handlers.py b/ceod/api/error_handlers.py index 54d44f2..4183d80 100644 --- a/ceod/api/error_handlers.py +++ b/ceod/api/error_handlers.py @@ -1,6 +1,7 @@ import traceback from flask.app import Flask +import ldap3 from werkzeug.exceptions import HTTPException from ceo_common.errors import UserNotFoundError, GroupNotFoundError @@ -21,6 +22,8 @@ def generic_error_handler(err: Exception): status_code = err.code elif isinstance(err, UserNotFoundError) or isinstance(err, GroupNotFoundError): status_code = 404 + elif isinstance(err, ldap3.core.exceptions.LDAPStrongerAuthRequiredResult): + status_code = 403 else: status_code = 500 logger.error(traceback.format_exc()) diff --git a/ceod/api/spnego.py b/ceod/api/spnego.py new file mode 100644 index 0000000..bc1878f --- /dev/null +++ b/ceod/api/spnego.py @@ -0,0 +1,55 @@ +from base64 import b64decode, b64encode +import functools +import socket +from typing import Union + +from flask import request, Response, make_response +import gssapi + +_server_name = None + + +def init_spnego(service_name: str, fqdn: Union[str, None] = None): + """Set the server principal which will be used for SPNEGO.""" + global _server_name + if fqdn is None: + fqdn = socket.getfqdn() + _server_name = gssapi.Name('ceod/' + fqdn) + + # make sure that we're actually capable of acquiring credentials + gssapi.Credentials(usage='accept', name=_server_name) + + +def requires_authentication(f): + """ + Requires that all requests to f have a GSSAPI initiator token. + The initiator principal will be passed to the first argument of f + in the form user@REALM. + """ + @functools.wraps(f) + def wrapper(*args, **kwargs): + if 'authorization' not in request.headers: + return Response('Unauthorized', 401, {'WWW-Authenticate': 'Negotiate'}) + header = request.headers['authorization'] + client_token = b64decode(header.split()[1]) + creds = gssapi.Credentials(usage='accept', name=_server_name) + ctx = gssapi.SecurityContext(creds=creds, usage='accept') + server_token = ctx.step(client_token) + + # OK so we're going to cheat a bit here by assuming that Kerberos is the + # mechanism being used (which we know will be true). We know that Kerberos + # only requires one round-trip for the service handshake, so we don't need + # to store state between requests. Just to be sure, we assert that this is + # indeed the case. + # (This isn't compliant with the GSSAPI spec, but why write more code than + # necessary?) + assert ctx.complete, 'only one round trip expected' + + resp = make_response(f(str(ctx.initiator_name), *args, **kwargs)) + # RFC 2744, section 5.1: + # "If no token need be sent, gss_accept_sec_context will indicate this + # by setting the length field of the output_token argument to zero." + if server_token is not None: + resp.headers['WWW-Authenticate'] = 'Negotiate ' + b64encode(server_token).decode() + return resp + return wrapper diff --git a/ceod/api/utils.py b/ceod/api/utils.py index c0068eb..42f900e 100644 --- a/ceod/api/utils.py +++ b/ceod/api/utils.py @@ -7,8 +7,8 @@ import traceback from typing import Callable, List from flask import current_app, stream_with_context -from flask_kerberos import requires_authentication +from .spnego import requires_authentication from ceo_common.logger_factory import logger_factory from ceod.transactions import AbstractTransaction diff --git a/requirements.txt b/requirements.txt index 402fafc..7023ac8 100644 --- a/requirements.txt +++ b/requirements.txt @@ -1,5 +1,4 @@ Flask==2.0.1 -Flask-Kerberos==1.0.4 gssapi==1.6.14 Jinja2==3.0.1 ldap3==2.9.1 diff --git a/tests/ceo_common/model/test_remote_mailman.py b/tests/ceo_common/model/test_remote_mailman.py index cc2fada..44b61fb 100644 --- a/tests/ceo_common/model/test_remote_mailman.py +++ b/tests/ceo_common/model/test_remote_mailman.py @@ -24,7 +24,7 @@ def test_remote_mailman(cfg, http_client, app, mock_mailman_server, g_syscom): try: http_client.get(hostname, '/ping') except requests.exceptions.ConnectionError: - time.sleep(0.5) + time.sleep(1) continue break diff --git a/tests/conftest.py b/tests/conftest.py index 3ff3c54..6422f46 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -7,7 +7,7 @@ import shutil import subprocess from subprocess import DEVNULL import tempfile -from unittest.mock import patch +from unittest.mock import patch, Mock import flask import ldap3 @@ -27,8 +27,19 @@ from .MockMailmanServer import MockMailmanServer from .conftest_ceod_api import client # noqa: F401 +@pytest.fixture(scope='session', autouse=True) +def _drone_hostname_mock(): + # Drone doesn't appear to set the hostname of the container. + # Mock it instead. + if 'DRONE_STEP_NAME' in os.environ: + hostname = os.environ['DRONE_STEP_NAME'] + fqdn = hostname + '.csclub.internal' + socket.gethostname = Mock(return_value=hostname) + socket.getfqdn = Mock(return_value=fqdn) + + @pytest.fixture(scope='session') -def cfg(): +def cfg(_drone_hostname_mock): with importlib.resources.path('tests', 'ceod_test_local.ini') as p: config_file = p.__fspath__() _cfg = Config(config_file) diff --git a/tests/conftest_ceod_api.py b/tests/conftest_ceod_api.py index b2c94fc..4dbeb95 100644 --- a/tests/conftest_ceod_api.py +++ b/tests/conftest_ceod_api.py @@ -36,6 +36,8 @@ class CeodTestClient: self.principal_ccaches = {} # this is where we'll store the credentials for each principal self.cache_dir = cache_dir + # for SPNEGO + self.target_name = gssapi.Name('ceod/' + socket.getfqdn()) @contextlib.contextmanager def krb5ccname_env(self, principal): @@ -55,7 +57,7 @@ class CeodTestClient: creds = gssapi.Credentials(name=name, usage='initiate') auth = HTTPSPNEGOAuth( opportunistic_auth=True, - target_name='ceod', + target_name=self.target_name, creds=creds, ) return auth