From 95e167578f39e28a9612623eb5ff7eb2dcffed04 Mon Sep 17 00:00:00 2001 From: Max Erenberg Date: Tue, 24 Aug 2021 20:50:34 +0000 Subject: [PATCH 1/4] remove libsasl2-dev dependency --- .drone.yml | 2 +- README.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.drone.yml b/.drone.yml index 8ab7961..615bb73 100644 --- a/.drone.yml +++ b/.drone.yml @@ -10,7 +10,7 @@ steps: # way to share system packages between steps commands: # install dependencies - - apt update && apt install -y libkrb5-dev libsasl2-dev python3-dev + - apt update && apt install -y libkrb5-dev python3-dev - python3 -m venv venv - . venv/bin/activate - pip install -r dev-requirements.txt diff --git a/README.md b/README.md index 64fd585..1650e15 100644 --- a/README.md +++ b/README.md @@ -54,7 +54,7 @@ messages get accepted (by default they get held). #### Dependencies Next, install and activate a virtualenv: ```sh -sudo apt install libkrb5-dev libsasl2-dev python3-dev +sudo apt install libkrb5-dev python3-dev python3 -m venv venv . venv/bin/activate pip install -r requirements.txt From e011e98026f36c77643c9596218f950c0d26f3e4 Mon Sep 17 00:00:00 2001 From: Max Erenberg Date: Thu, 26 Aug 2021 02:19:18 +0000 Subject: [PATCH 2/4] use GSSAPI delegation --- README.md | 11 -- architecture.md | 6 +- ceo/utils.py | 7 +- ceo_common/interfaces/IHTTPClient.py | 23 ++- ceo_common/krb5/__init__.py | 0 ceo_common/krb5/krb5_build.py | 97 ----------- ceo_common/krb5/utils.py | 202 ----------------------- ceo_common/model/HTTPClient.py | 59 +++---- ceo_common/model/RemoteMailmanService.py | 5 +- ceod/api/app_factory.py | 3 - ceod/api/krb5_cred_handlers.py | 31 ---- ceod/api/spnego.py | 14 +- ceod/model/KerberosService.py | 35 +--- ceod/model/LDAPService.py | 5 +- ceod/model/MailService.py | 3 +- dev-requirements.txt | 1 - tests/ceo_common/krb5/__init__.py | 0 tests/ceo_common/krb5/test_krb5.py | 45 ----- tests/ceod/api/test_members.py | 2 +- tests/ceod_dev.ini | 1 - tests/ceod_test_local.ini | 1 - tests/conftest.py | 43 ++--- tests/conftest_ceo.py | 4 +- tests/conftest_ceod_api.py | 67 ++++---- tests/utils.py | 33 ++-- 25 files changed, 132 insertions(+), 566 deletions(-) delete mode 100644 ceo_common/krb5/__init__.py delete mode 100644 ceo_common/krb5/krb5_build.py delete mode 100644 ceo_common/krb5/utils.py delete mode 100644 ceod/api/krb5_cred_handlers.py delete mode 100644 tests/ceo_common/krb5/__init__.py delete mode 100644 tests/ceo_common/krb5/test_krb5.py diff --git a/README.md b/README.md index 1650e15..b14eae7 100644 --- a/README.md +++ b/README.md @@ -61,17 +61,6 @@ pip install -r requirements.txt pip install -r dev-requirements.txt ``` -#### C bindings -Due to the lack of a decent Python library for Kerberos we ended up -writing our own C bindings using [cffi](https://cffi.readthedocs.io). -Make sure you compile the bindings: -```sh -cd ceo_common/krb5 -python krb5_build.py -``` -This should create a file named '_krb5.cpython-37m-x86_64-linux-gnu.so'. -This will be imported by other modules in ceo. - ## Running the application ceod is a distributed application, with instances on different hosts offering different services. diff --git a/architecture.md b/architecture.md index 7fb2cb4..8b632ec 100644 --- a/architecture.md +++ b/architecture.md @@ -40,8 +40,8 @@ not worth it if ceo is the only app which will use it. Therefore, we will use unconstrained delegation. The client essentially forwards their TGT to ceod, which uses it to access other services over GSSAPI -on the client's behalf. The TGT is formatted as a KRB-CRED message, -base64-encoded, and placed in an HTTP header named 'X-KRB5-CRED'. +on the client's behalf. We accomplish this using GSSAPI delegation (i.e. set +the GSS_C_DELEG_FLAG when creating a security context). Since the client's credentials are used when interacting with LDAP, this means that most LDAP-related endpoints can actually be accessed from any host. @@ -57,7 +57,7 @@ to protect the KRB-CRED message, which is unencrypted.) SPNEGO is pretty awkward, to be honest, as it completely breaks the stateless nature of HTTP. If we decide that SPNEGO is too much trouble, we should switch -to plain HTTP cookies instead, and cache them somewhere in the client's home +to plain HTTP cookies instead, and cache them somewhere in the client's home directory. ## Web UI diff --git a/ceo/utils.py b/ceo/utils.py index fc84265..5673d8c 100644 --- a/ceo/utils.py +++ b/ceo/utils.py @@ -11,14 +11,13 @@ def http_request(method: str, path: str, **kwargs) -> requests.Response: cfg = component.getUtility(IConfig) if path.startswith('/api/db'): host = cfg.get('ceod_db_host') - need_cred = False + delegate = False else: host = cfg.get('ceod_admin_host') # The forwarded TGT is only needed for endpoints which write to LDAP - need_cred = method != 'GET' + delegate = method != 'GET' return client.request( - host, path, method, principal=None, need_cred=need_cred, - stream=True, **kwargs) + host, path, method, delegate=delegate, stream=True, **kwargs) def http_get(path: str, **kwargs) -> requests.Response: diff --git a/ceo_common/interfaces/IHTTPClient.py b/ceo_common/interfaces/IHTTPClient.py index 61c9599..b65eec6 100644 --- a/ceo_common/interfaces/IHTTPClient.py +++ b/ceo_common/interfaces/IHTTPClient.py @@ -1,27 +1,24 @@ -from typing import Union - from zope.interface import Interface class IHTTPClient(Interface): """A helper class for HTTP requests to ceod.""" - def request(host: str, api_path: str, method: str, principal: str, - need_cred: bool, **kwargs): - """Make an HTTP request.""" + def request(host: str, api_path: str, method: str, delegate: bool, **kwargs): + """ + Make an HTTP request. + If `delegate` is True, GSSAPI credentials will be forwarded to the + remote. + """ - def get(host: str, api_path: str, principal: Union[str, None] = None, - need_cred: bool = True, **kwargs): + def get(host: str, api_path: str, delegate: bool = True, **kwargs): """Make a GET request.""" - def post(host: str, api_path: str, principal: Union[str, None] = None, - need_cred: bool = True, **kwargs): + def post(host: str, api_path: str, delegate: bool = True, **kwargs): """Make a POST request.""" - def patch(host: str, api_path: str, principal: Union[str, None] = None, - need_cred: bool = True, **kwargs): + def patch(host: str, api_path: str, delegate: bool = True, **kwargs): """Make a PATCH request.""" - def delete(host: str, api_path: str, principal: Union[str, None] = None, - need_cred: bool = True, **kwargs): + def delete(host: str, api_path: str, delegate: bool = True, **kwargs): """Make a DELETE request.""" diff --git a/ceo_common/krb5/__init__.py b/ceo_common/krb5/__init__.py deleted file mode 100644 index e69de29..0000000 diff --git a/ceo_common/krb5/krb5_build.py b/ceo_common/krb5/krb5_build.py deleted file mode 100644 index 3c67721..0000000 --- a/ceo_common/krb5/krb5_build.py +++ /dev/null @@ -1,97 +0,0 @@ -from cffi import FFI - -ffibuilder = FFI() - -# Definitions selectively copied from . -# Add more if necessary. -ffibuilder.cdef(r""" - # define KV5M_DATA ... - typedef int32_t krb5_int32; - typedef krb5_int32 krb5_error_code; - typedef krb5_error_code krb5_magic; - struct _krb5_context; - typedef struct _krb5_context* krb5_context; - struct _krb5_auth_context; - typedef struct _krb5_auth_context * krb5_auth_context; - struct _krb5_ccache; - typedef struct _krb5_ccache *krb5_ccache; - typedef struct krb5_principal_data { - ...; - } krb5_principal_data; - typedef krb5_principal_data * krb5_principal; - typedef const krb5_principal_data *krb5_const_principal; - typedef struct _krb5_creds { - krb5_principal client; - krb5_principal server; - ...; - } krb5_creds; - typedef struct _krb5_data { - krb5_magic magic; - unsigned int length; - char *data; - } krb5_data; - typedef struct krb5_replay_data { - ...; - } krb5_replay_data; - - krb5_error_code krb5_init_context(krb5_context * context); - void krb5_free_context(krb5_context context); - krb5_error_code krb5_auth_con_init( - krb5_context context, krb5_auth_context *auth_context); - krb5_error_code krb5_auth_con_setflags( - krb5_context context, krb5_auth_context auth_context, krb5_int32 flags); - krb5_error_code krb5_auth_con_free( - krb5_context context, krb5_auth_context auth_context); - krb5_error_code - krb5_cc_new_unique(krb5_context context, const char *type, const char *hint, - krb5_ccache *id); - krb5_error_code krb5_cc_default(krb5_context context, krb5_ccache *ccache); - krb5_error_code krb5_cc_resolve( - krb5_context context, const char * name, krb5_ccache * cache); - krb5_error_code - krb5_cc_initialize(krb5_context context, krb5_ccache cache, - krb5_principal principal); - krb5_error_code - krb5_cc_get_principal(krb5_context context, krb5_ccache cache, - krb5_principal *principal); - krb5_error_code krb5_cc_store_cred( - krb5_context context, krb5_ccache cache, krb5_creds *creds); - krb5_error_code krb5_cc_close(krb5_context context, krb5_ccache cache); - krb5_error_code krb5_cc_destroy(krb5_context context, krb5_ccache cache); - krb5_error_code - krb5_build_principal(krb5_context context, - krb5_principal * princ, - unsigned int rlen, - const char * realm, ...); - krb5_error_code - krb5_parse_name(krb5_context context, const char *name, - krb5_principal *principal_out); - void krb5_free_principal(krb5_context context, krb5_principal val); - krb5_error_code - krb5_rd_cred(krb5_context context, krb5_auth_context auth_context, - krb5_data *pcreddata, krb5_creds ***pppcreds, - krb5_replay_data *outdata); - krb5_error_code krb5_fwd_tgt_creds( - krb5_context context, krb5_auth_context auth_context, - const char * rhost, krb5_principal client, krb5_principal server, - krb5_ccache cc, int forwardable, krb5_data * outbuf); - void krb5_free_tgt_creds(krb5_context context, krb5_creds ** tgts); - void krb5_free_data_contents(krb5_context context, krb5_data * val); - krb5_error_code krb5_unparse_name( - krb5_context context, krb5_const_principal principal, char **name); - void krb5_free_unparsed_name(krb5_context context, char *val); - const char * krb5_get_error_message(krb5_context ctx, krb5_error_code code); - void krb5_free_error_message(krb5_context ctx, const char * msg); -""") - -ffibuilder.set_source( - "_krb5", - """ - #include - """, - libraries=['krb5'], - extra_link_args=['-fsanitize=address', '-static-libasan'], -) - -if __name__ == '__main__': - ffibuilder.compile(verbose=True) diff --git a/ceo_common/krb5/utils.py b/ceo_common/krb5/utils.py deleted file mode 100644 index 413a036..0000000 --- a/ceo_common/krb5/utils.py +++ /dev/null @@ -1,202 +0,0 @@ -import contextlib -import functools -from typing import Union - -from ._krb5 import ffi, lib -from ceo_common.errors import KerberosError - - -def check_rc(k_ctx, rc: int): - """ - Check the return code of a krb5 function. - An exception is raised if rc != 0. - """ - if rc == 0: - return - c_msg = lib.krb5_get_error_message(k_ctx, rc) - msg = ffi.string(c_msg).decode() - lib.krb5_free_error_message(k_ctx, c_msg) - raise KerberosError(msg) - - -@contextlib.contextmanager -def get_krb5_context(): - """Yields a krb5_context.""" - k_ctx = None - try: - p_k_ctx = ffi.new('krb5_context *') - rc = lib.krb5_init_context(p_k_ctx) - k_ctx = p_k_ctx[0] # dereference the pointer - check_rc(k_ctx, rc) - yield k_ctx - finally: - if k_ctx is not None: - lib.krb5_free_context(k_ctx) - - -@contextlib.contextmanager -def get_krb5_auth_context(k_ctx): - """Yields a krb5_auth_context.""" - a_ctx = None - try: - p_a_ctx = ffi.new('krb5_auth_context *') - rc = lib.krb5_auth_con_init(k_ctx, p_a_ctx) - a_ctx = p_a_ctx[0] - check_rc(k_ctx, rc) - # clear the flags which enable the replay cache - rc = lib.krb5_auth_con_setflags(k_ctx, a_ctx, 0) - check_rc(k_ctx, rc) - yield a_ctx - finally: - if a_ctx is not None: - lib.krb5_auth_con_free(k_ctx, a_ctx) - - -@contextlib.contextmanager -def get_krb5_cc_default(k_ctx): - """Yields the default krb5_ccache.""" - cache = None - try: - p_cache = ffi.new('krb5_ccache *') - rc = lib.krb5_cc_default(k_ctx, p_cache) - check_rc(k_ctx, rc) - cache = p_cache[0] - yield cache - finally: - if cache is not None: - lib.krb5_cc_close(k_ctx, cache) - - -@contextlib.contextmanager -def get_krb5_cc_resolve(k_ctx, name: str): - """ - Resolve a credential cache name. - `name` should have the format 'type:residual'. - """ - cache = None - try: - c_name = ffi.new('char[]', name.encode()) - p_cache = ffi.new('krb5_ccache *') - rc = lib.krb5_cc_resolve(k_ctx, c_name, p_cache) - check_rc(k_ctx, rc) - cache = p_cache[0] - yield cache - finally: - if cache is not None: - lib.krb5_cc_close(k_ctx, cache) - - -def get_fwd_tgt(server: str, cache_name: Union[str, None] = None) -> bytes: - """ - Get a forwarded TGT formatted as a KRB-CRED message. - `server` should have the format 'service/host', - e.g. 'ceod/phosphoric-acid.csclub.uwaterloo.ca'. - - If `cache_name` is None, the default cache will be used; otherwise, - the cache with that name will be used. Must have the format - 'TYPE:residual'. - """ - if cache_name is None: - cache_function = get_krb5_cc_default - else: - cache_function = functools.partial(get_krb5_cc_resolve, name=cache_name) - - with get_krb5_context() as k_ctx, \ - get_krb5_auth_context(k_ctx) as a_ctx, \ - cache_function(k_ctx) as cache: - server_princ = None - client_princ = None - outbuf = None - try: - # create a server principal from the server string - c_server = ffi.new('char[]', server.encode()) - p_server_princ = ffi.new('krb5_principal *') - rc = lib.krb5_parse_name(k_ctx, c_server, p_server_princ) - check_rc(k_ctx, rc) - server_princ = p_server_princ[0] - # get a client principal from the default cache - p_client_princ = ffi.new('krb5_principal *') - rc = lib.krb5_cc_get_principal(k_ctx, cache, p_client_princ) - check_rc(k_ctx, rc) - client_princ = p_client_princ[0] - # get the forwarded TGT - p_outbuf = ffi.new('krb5_data *') - rc = lib.krb5_fwd_tgt_creds( - k_ctx, a_ctx, ffi.NULL, client_princ, server_princ, - cache, 0, p_outbuf) - check_rc(k_ctx, rc) - outbuf = p_outbuf[0] - return ffi.unpack(outbuf.data, outbuf.length) - finally: - if outbuf is not None: - lib.krb5_free_data_contents(k_ctx, p_outbuf) - if client_princ is not None: - lib.krb5_free_principal(k_ctx, client_princ) - if server_princ is not None: - lib.krb5_free_principal(k_ctx, server_princ) - - -@contextlib.contextmanager -def store_fwd_tgt_creds(cred_data_bytes: bytes): - """ - Stores the credentials found in cred_data_bytes - in a new sub-cache in the default cache (which should be of type DIR), - yields the name of the principal in the credential, then finally removes - the sub-cache from the collection. - - The principal name will have the form 'username@REALM', e.g. - 'ctdalek@CSCLUB.UWATERLOO.CA'. - """ - with get_krb5_context() as k_ctx, get_krb5_auth_context(k_ctx) as a_ctx: - creds_out = None - cache = None - c_name = None - - try: - # fill a krb5_data struct - cred_data = ffi.new('krb5_data *') - cred_data_buf = ffi.new('char[]', cred_data_bytes) - cred_data.magic = lib.KV5M_DATA - cred_data.length = len(cred_data_bytes) - cred_data.data = cred_data_buf - - # read the KRB5-CRED message into a krb5_creds array - p_creds_out = ffi.new('krb5_creds ***') - rc = lib.krb5_rd_cred(k_ctx, a_ctx, cred_data, p_creds_out, ffi.NULL) - check_rc(k_ctx, rc) - creds_out = p_creds_out[0] - # there should only be one cred in the array - cred = creds_out[0] - - # read the name of the client principal - client_princ = cred.client - p_name = ffi.new('char **') - rc = lib.krb5_unparse_name(k_ctx, client_princ, p_name) - check_rc(k_ctx, rc) - c_name = p_name[0] - name = ffi.string(c_name).decode() - - # create a new cache inside the collection (default cache) - p_cache = ffi.new('krb5_ccache *') - cctype = ffi.new('char[]', b'DIR') - rc = lib.krb5_cc_new_unique(k_ctx, cctype, ffi.NULL, p_cache) - check_rc(k_ctx, rc) - cache = p_cache[0] - # initialize the new cache - rc = lib.krb5_cc_initialize(k_ctx, cache, client_princ) - check_rc(k_ctx, rc) - - # store the cred into the cache - rc = lib.krb5_cc_store_cred(k_ctx, cache, cred) - check_rc(k_ctx, rc) - - yield name - finally: - if cache is not None: - # We destroy the cache (instead of closing it) since we want - # to remove it from disk. - lib.krb5_cc_destroy(k_ctx, cache) - if c_name is not None: - lib.krb5_free_unparsed_name(k_ctx, c_name) - if creds_out is not None: - lib.krb5_free_tgt_creds(k_ctx, creds_out) diff --git a/ceo_common/model/HTTPClient.py b/ceo_common/model/HTTPClient.py index 4da38fc..d4cb8cf 100644 --- a/ceo_common/model/HTTPClient.py +++ b/ceo_common/model/HTTPClient.py @@ -1,13 +1,10 @@ -from base64 import b64encode -from typing import Union - +import flask import gssapi import requests from requests_gssapi import HTTPSPNEGOAuth from zope import component from zope.interface import implementer -from ceo_common.krb5.utils import get_fwd_tgt from ceo_common.interfaces import IConfig, IHTTPClient @@ -23,48 +20,40 @@ class HTTPClient: self.ceod_port = cfg.get('ceod_port') self.base_domain = cfg.get('base_domain') - def request(self, host: str, api_path: str, method: str, principal: str, - need_cred: bool, **kwargs): + def request(self, host: str, api_path: str, method: str, delegate: bool, **kwargs): # always use the FQDN if '.' not in host: host = host + '.' + self.base_domain # SPNEGO - if principal is not None: - gssapi_name = gssapi.Name(principal) - creds = gssapi.Credentials(name=gssapi_name, usage='initiate') - else: - creds = None - auth = HTTPSPNEGOAuth( - opportunistic_auth=True, - target_name=gssapi.Name('ceod/' + host), - creds=creds, - ) - - # Forwarded TGT (X-KRB5-CRED) - headers = {} - if need_cred: - b = get_fwd_tgt('ceod/' + host) - headers['X-KRB5-CRED'] = b64encode(b).decode() + spnego_kwargs = { + 'opportunistic_auth': True, + 'target_name': gssapi.Name('ceod/' + host), + } + if flask.has_request_context() and 'client_creds' in flask.g: + # This is reached when we are the server and the client has forwarded + # their credentials to us. + spnego_kwargs['creds'] = flask.g.client_creds + if delegate: + # This is reached when we are the client and we want to forward our + # credentials to the server. + spnego_kwargs['delegate'] = True + auth = HTTPSPNEGOAuth(**spnego_kwargs) return requests.request( method, f'{self.scheme}://{host}:{self.ceod_port}{api_path}', - auth=auth, headers=headers, **kwargs, + auth=auth, **kwargs, ) - def get(self, host: str, api_path: str, principal: Union[str, None] = None, - need_cred: bool = False, **kwargs): - return self.request(host, api_path, 'GET', principal, need_cred, **kwargs) + def get(self, host: str, api_path: str, delegate: bool = True, **kwargs): + return self.request(host, api_path, 'GET', delegate, **kwargs) - def post(self, host: str, api_path: str, principal: Union[str, None] = None, - need_cred: bool = False, **kwargs): - return self.request(host, api_path, 'POST', principal, need_cred, **kwargs) + def post(self, host: str, api_path: str, delegate: bool = True, **kwargs): + return self.request(host, api_path, 'POST', delegate, **kwargs) - def patch(self, host: str, api_path: str, principal: Union[str, None] = None, - need_cred: bool = False, **kwargs): - return self.request(host, api_path, 'PATCH', principal, need_cred, **kwargs) + def patch(self, host: str, api_path: str, delegate: bool = True, **kwargs): + return self.request(host, api_path, 'PATCH', delegate, **kwargs) - def delete(self, host: str, api_path: str, principal: Union[str, None] = None, - need_cred: bool = False, **kwargs): - return self.request(host, api_path, 'DELETE', principal, need_cred, **kwargs) + def delete(self, host: str, api_path: str, delegate: bool = True, **kwargs): + return self.request(host, api_path, 'DELETE', delegate, **kwargs) diff --git a/ceo_common/model/RemoteMailmanService.py b/ceo_common/model/RemoteMailmanService.py index d9e70e0..f6f23de 100644 --- a/ceo_common/model/RemoteMailmanService.py +++ b/ceo_common/model/RemoteMailmanService.py @@ -1,4 +1,3 @@ -from flask import g from zope import component from zope.interface import implementer @@ -17,7 +16,7 @@ class RemoteMailmanService: def subscribe(self, address: str, mailing_list: str): resp = self.http_client.post( self.mailman_host, f'/api/mailman/{mailing_list}/{address}', - principal=g.sasl_user) + delegate=False) if not resp.ok: if resp.status_code == 409: raise UserAlreadySubscribedError() @@ -28,7 +27,7 @@ class RemoteMailmanService: def unsubscribe(self, address: str, mailing_list: str): resp = self.http_client.delete( self.mailman_host, f'/api/mailman/{mailing_list}/{address}', - principal=g.sasl_user) + delegate=False) if not resp.ok: if resp.status_code == 404: raise UserNotSubscribedError() diff --git a/ceod/api/app_factory.py b/ceod/api/app_factory.py index f033a2b..7204f8e 100644 --- a/ceod/api/app_factory.py +++ b/ceod/api/app_factory.py @@ -6,7 +6,6 @@ from flask import Flask from zope import component from .error_handlers import register_error_handlers -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 @@ -47,8 +46,6 @@ def create_app(flask_config={}): app.register_blueprint(uwldap.bp, url_prefix='/api/uwldap') register_error_handlers(app) - app.before_request(before_request) - app.teardown_request(teardown_request) @app.route('/ping') def ping(): diff --git a/ceod/api/krb5_cred_handlers.py b/ceod/api/krb5_cred_handlers.py deleted file mode 100644 index 237e6aa..0000000 --- a/ceod/api/krb5_cred_handlers.py +++ /dev/null @@ -1,31 +0,0 @@ -from base64 import b64decode -import traceback - -from flask import g, request - -from ceo_common.logger_factory import logger_factory -from ceo_common.krb5.utils import store_fwd_tgt_creds - -logger = logger_factory(__name__) - - -def before_request(): - if 'x-krb5-cred' not in request.headers: - return - cred = b64decode(request.headers['x-krb5-cred']) - ctx = store_fwd_tgt_creds(cred) - name = ctx.__enter__() - g.stored_creds_ctx = ctx - g.sasl_user = name - - -def teardown_request(err): - if 'stored_creds_ctx' not in g: - return - try: - ctx = g.stored_creds_ctx - ctx.__exit__(None, None, None) - g.pop('sasl_user') - g.pop('stored_creds_ctx') - except Exception: - logger.error(traceback.format_exc()) diff --git a/ceod/api/spnego.py b/ceod/api/spnego.py index bc1878f..16e0c1c 100644 --- a/ceod/api/spnego.py +++ b/ceod/api/spnego.py @@ -3,8 +3,9 @@ import functools import socket from typing import Union -from flask import request, Response, make_response +from flask import request, Response, make_response, g import gssapi +from gssapi.raw import RequirementFlag _server_name = None @@ -45,7 +46,16 @@ def requires_authentication(f): # necessary?) assert ctx.complete, 'only one round trip expected' - resp = make_response(f(str(ctx.initiator_name), *args, **kwargs)) + # Store the username in flask.g + client_princ = str(ctx.initiator_name) + g.auth_user = client_princ[:client_princ.index('@')] + + # Store the delegated credentials, if they were given + if ctx.actual_flags & RequirementFlag.delegate_to_peer: + g.client_creds = ctx.delegated_creds + + # TODO: don't pass client_princ to f anymore since it's stored in flask.g + resp = make_response(f(client_princ, *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." diff --git a/ceod/model/KerberosService.py b/ceod/model/KerberosService.py index 3f67fcb..01ee2d7 100644 --- a/ceod/model/KerberosService.py +++ b/ceod/model/KerberosService.py @@ -1,5 +1,4 @@ import os -import shutil import subprocess from typing import List @@ -7,9 +6,6 @@ from zope import component from zope.interface import implementer from ceo_common.interfaces import IKerberosService, IConfig -from ceo_common.krb5._krb5 import ffi, lib -from ceo_common.krb5.utils import check_rc, get_krb5_context, \ - get_krb5_cc_default @implementer(IKerberosService) @@ -21,35 +17,10 @@ class KerberosService: cfg = component.getUtility(IConfig) self.admin_principal = admin_principal - self.cache_dir = cfg.get('ceod_krb5_cache_dir') self.realm = cfg.get('ldap_sasl_realm') - self._initialize_cache() - - def _initialize_cache(self, **kwargs): - if os.path.isdir(self.cache_dir): - shutil.rmtree(self.cache_dir) - os.makedirs(self.cache_dir) - os.environ['KRB5CCNAME'] = 'DIR:' + self.cache_dir - - with get_krb5_context() as k_ctx, get_krb5_cc_default(k_ctx) as cache: - princ = None - try: - # build a principal for 'nobody' - realm = self.realm.encode() - c_realm = ffi.new('char[]', realm) - component = ffi.new('char[]', b'nobody') - p_princ = ffi.new('krb5_principal *') - rc = lib.krb5_build_principal( - k_ctx, p_princ, len(realm), c_realm, component, ffi.NULL) - check_rc(k_ctx, rc) - princ = p_princ[0] - - # initialize the default cache with 'nobody' as the default principal - rc = lib.krb5_cc_initialize(k_ctx, cache, princ) - check_rc(k_ctx, rc) - finally: - if princ is not None: - lib.krb5_free_principal(k_ctx, princ) + # We don't need a credentials cache because the client forwards + # their credentials to us + os.environ['KRB5CCNAME'] = 'FILE:/dev/null' def _run(self, args: List[str]): subprocess.run(args, check=True) diff --git a/ceod/model/LDAPService.py b/ceod/model/LDAPService.py index 218f971..67c961e 100644 --- a/ceod/model/LDAPService.py +++ b/ceod/model/LDAPService.py @@ -34,10 +34,11 @@ class LDAPService: if 'ldap_conn' in g: return g.ldap_conn kwargs = {'auto_bind': True, 'raise_exceptions': True} - if 'sasl_user' in g: + if 'client_creds' in g: kwargs['authentication'] = ldap3.SASL kwargs['sasl_mechanism'] = ldap3.KERBEROS - kwargs['user'] = g.sasl_user + # see https://github.com/cannatag/ldap3/blob/master/ldap3/protocol/sasl/kerberos.py + kwargs['sasl_credentials'] = (None, None, g.client_creds) conn = ldap3.Connection(self.ldap_server_url, **kwargs) # cache the connection for a single request g.ldap_conn = conn diff --git a/ceod/model/MailService.py b/ceod/model/MailService.py index be61e2f..2891dd0 100644 --- a/ceod/model/MailService.py +++ b/ceod/model/MailService.py @@ -65,8 +65,7 @@ class MailService: def announce_new_user(self, user: IUser, operations: List[str]): # The person who added the new user - # TODO: store the auth_user from SPNEGO into flask.g - auth_user = g.sasl_user + auth_user = g.auth_user if '@' in auth_user: auth_user = auth_user[:auth_user.index('@')] diff --git a/dev-requirements.txt b/dev-requirements.txt index 2a5a2f5..8392fb6 100644 --- a/dev-requirements.txt +++ b/dev-requirements.txt @@ -1,7 +1,6 @@ flake8==3.9.2 setuptools==40.8.0 wheel==0.36.2 -cffi==1.14.6 pytest==6.2.4 aiosmtpd==1.4.2 aiohttp==3.7.4.post0 diff --git a/tests/ceo_common/krb5/__init__.py b/tests/ceo_common/krb5/__init__.py deleted file mode 100644 index e69de29..0000000 diff --git a/tests/ceo_common/krb5/test_krb5.py b/tests/ceo_common/krb5/test_krb5.py deleted file mode 100644 index 0e3d19d..0000000 --- a/tests/ceo_common/krb5/test_krb5.py +++ /dev/null @@ -1,45 +0,0 @@ -import os -import socket -import subprocess -from subprocess import DEVNULL -import tempfile - -import ldap3 - -from ceo_common.krb5.utils import get_fwd_tgt, store_fwd_tgt_creds - - -def test_fwd_tgt(cfg): - realm = cfg.get('ldap_sasl_realm') - ldap_server = cfg.get('ldap_server_url') - hostname = socket.gethostname() - old_krb5ccname = os.environ['KRB5CCNAME'] - f1 = tempfile.NamedTemporaryFile() - d2 = tempfile.TemporaryDirectory() - - try: - subprocess.run( - ['kinit', '-c', 'FILE:' + f1.name, 'regular1'], - text=True, input='krb5', check=True, stdout=DEVNULL) - subprocess.run( - ['kinit', '-c', 'DIR:' + d2.name, 'ctdalek'], - text=True, input='krb5', check=True, stdout=DEVNULL) - os.environ['KRB5CCNAME'] = 'FILE:' + f1.name - b = get_fwd_tgt(hostname) - os.environ['KRB5CCNAME'] = 'DIR:' + d2.name - # make sure that we can import the creds from regular1 into the - # cache collection - with store_fwd_tgt_creds(b) as name: - assert name == 'regular1@' + realm - - kwargs = { - 'server': ldap_server, 'auto_bind': True, - 'authentication': ldap3.SASL, 'sasl_mechanism': ldap3.KERBEROS, - } - conn = ldap3.Connection(**kwargs, user='regular1') - assert conn.extend.standard.who_am_i().startswith('dn:uid=regular1,') - conn.unbind() - finally: - os.environ['KRB5CCNAME'] = old_krb5ccname - f1.close() - d2.cleanup() diff --git a/tests/ceod/api/test_members.py b/tests/ceod/api/test_members.py index 79e4c6f..a2c9867 100644 --- a/tests/ceod/api/test_members.py +++ b/tests/ceod/api/test_members.py @@ -210,5 +210,5 @@ def test_authz_check(client, create_user_result): # If we're syscom but we don't pass credentials, the request should fail _, data = client.post('/api/members', json={ 'uid': 'test_1', 'cn': 'Test One', 'terms': ['s2021'], - }, principal='ctdalek', need_cred=False) + }, principal='ctdalek', delegate=False) assert data[-1]['status'] == 'aborted' diff --git a/tests/ceod_dev.ini b/tests/ceod_dev.ini index f23b130..a7bbf9f 100644 --- a/tests/ceod_dev.ini +++ b/tests/ceod_dev.ini @@ -7,7 +7,6 @@ admin_host = phosphoric-acid # this is the host with NFS no_root_squash fs_root_host = phosphoric-acid mailman_host = mail -krb5_cache_dir = /run/ceod/krb5_cache use_https = false port = 9987 diff --git a/tests/ceod_test_local.ini b/tests/ceod_test_local.ini index 6a8f7bd..f843f3c 100644 --- a/tests/ceod_test_local.ini +++ b/tests/ceod_test_local.ini @@ -7,7 +7,6 @@ uw_domain = uwaterloo.internal admin_host = phosphoric-acid fs_root_host = phosphoric-acid mailman_host = phosphoric-acid -krb5_cache_dir = /tmp/ceod_test_krb5_cache use_https = false port = 9987 diff --git a/tests/conftest.py b/tests/conftest.py index d51f230..4fefa46 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -17,7 +17,7 @@ import requests import socket from zope import component -from .utils import krb5ccname_ctx +from .utils import gssapi_creds_ctx, ccache_cleanup # noqa: F401 from ceo_common.interfaces import IConfig, IKerberosService, ILDAPService, \ IFileService, IMailmanService, IHTTPClient, IUWLDAPService, IMailService from ceo_common.model import Config, HTTPClient @@ -51,18 +51,6 @@ def cfg(_drone_hostname_mock): return _cfg -@pytest.fixture(scope='session', autouse=True) -def _delete_ccaches(): - # I've noticed when pytest finishes, the temporary files - # created by tempfile.NamedTemporaryFile() aren't destroyed. - # So, we clean them up here. - from .utils import _ccaches - yield - # forcefully decrement the reference counts, which will trigger - # the destructors - _ccaches.clear() - - def delete_test_princs(krb_srv): proc = subprocess.run([ 'kadmin', '-k', '-p', krb_srv.admin_principal, 'listprincs', 'test_*', @@ -82,14 +70,12 @@ def krb_srv(cfg): principal = 'ceod/admin' else: principal = 'ceod/' + socket.getfqdn() - cache_dir = cfg.get('ceod_krb5_cache_dir') krb = KerberosService(principal) component.provideUtility(krb, IKerberosService) delete_test_princs(krb) yield krb delete_test_princs(krb) - shutil.rmtree(cache_dir) def delete_subtree(conn: ldap3.Connection, base_dn: str): @@ -105,7 +91,7 @@ def delete_subtree(conn: ldap3.Connection, base_dn: str): @pytest.fixture def g_admin_ctx(app): """ - Store the principal for ceod/admin in flask.g. + Store the credentials for ceod/admin in flask.g, and override KBR5CCNAME. This context manager should be used any time LDAP is modified via the LDAPService, and we are not in a request context. This should NOT be active when CeodTestClient is making a request, since @@ -113,28 +99,31 @@ def g_admin_ctx(app): """ @contextlib.contextmanager def wrapper(): - with krb5ccname_ctx('ceod/admin'), app.app_context(): + with gssapi_creds_ctx('ceod/admin') as creds, app.app_context(): try: - flask.g.sasl_user = 'ceod/admin' + flask.g.auth_user = 'ceod/admin' + flask.g.client_creds = creds yield finally: - flask.g.pop('sasl_user') + flask.g.pop('client_creds') + flask.g.pop('auth_user') return wrapper @pytest.fixture def g_syscom(app): """ - Store the principal for the syscom member in flask.g, and point - KRB5CCNAME to the file where the TGT is stored. + Store the credentials for ctdalek in flask.g and override KRB5CCNAME. Use this fixture if you need syscom credentials for an HTTP request to a different process. """ - with krb5ccname_ctx('ctdalek'), app.app_context(): + with gssapi_creds_ctx('ctdalek') as creds, app.app_context(): try: flask.g.sasl_user = 'ctdalek' + flask.g.client_creds = creds yield finally: + flask.g.pop('client_creds') flask.g.pop('sasl_user') @@ -146,11 +135,11 @@ def ldap_conn(cfg) -> ldap3.Connection: server_url = cfg.get('ldap_server_url') # sanity check assert server_url == cfg.get('uwldap_server_url') - with krb5ccname_ctx('ceod/admin'): + with gssapi_creds_ctx('ceod/admin') as creds: conn = ldap3.Connection( server_url, auto_bind=True, raise_exceptions=True, authentication=ldap3.SASL, sasl_mechanism=ldap3.KERBEROS, - user='ceod/admin') + sasl_credentials=(None, None, creds)) return conn @@ -378,10 +367,12 @@ def app_process(cfg, app, http_client): proc.start() try: - with krb5ccname_ctx('ctdalek'): + # Currently the HTTPClient uses SPNEGO for all requests, + # even GETs + with gssapi_creds_ctx('ctdalek'): for i in range(5): try: - http_client.get(hostname, '/ping') + http_client.get(hostname, '/ping', delegate=False) except requests.exceptions.ConnectionError: time.sleep(1) continue diff --git a/tests/conftest_ceo.py b/tests/conftest_ceo.py index be1f63d..07a7aca 100644 --- a/tests/conftest_ceo.py +++ b/tests/conftest_ceo.py @@ -2,7 +2,7 @@ import os import pytest -from .utils import krb5ccname_ctx +from .utils import gssapi_creds_ctx @pytest.fixture(scope='module') @@ -14,5 +14,5 @@ def cli_setup(app_process): # messy because they would be sharing the same environment variables, # Kerberos cache, and registered utilities (via zope). So we're just # going to start the app in a child process intead. - with krb5ccname_ctx('ctdalek'): + with gssapi_creds_ctx('ctdalek'): yield diff --git a/tests/conftest_ceod_api.py b/tests/conftest_ceod_api.py index 1962d37..6a53ed1 100644 --- a/tests/conftest_ceod_api.py +++ b/tests/conftest_ceod_api.py @@ -1,16 +1,14 @@ -from base64 import b64encode import json import socket -from flask import g +import flask from flask.testing import FlaskClient import gssapi import pytest from requests import Request from requests_gssapi import HTTPSPNEGOAuth -from ceo_common.krb5.utils import get_fwd_tgt -from .utils import krb5ccname_ctx +from .utils import gssapi_creds_ctx __all__ = ['client'] @@ -30,39 +28,30 @@ class CeodTestClient: # for SPNEGO self.target_name = gssapi.Name('ceod/' + socket.getfqdn()) - def get_auth(self, principal): + def get_auth(self, principal: str, delegate: bool): """Acquire a HTTPSPNEGOAuth instance for the principal.""" - name = gssapi.Name(principal) - # the 'store' arg doesn't seem to work for DIR ccaches - creds = gssapi.Credentials(name=name, usage='initiate') - auth = HTTPSPNEGOAuth( - opportunistic_auth=True, - target_name=self.target_name, - creds=creds, - ) - return auth + with gssapi_creds_ctx(principal) as creds: + return HTTPSPNEGOAuth( + opportunistic_auth=True, + target_name=self.target_name, + creds=creds, + delegate=delegate, + ) - def get_headers(self, principal: str, need_cred: bool): - with krb5ccname_ctx(principal): - # Get the Authorization header (SPNEGO). - # The method doesn't matter here because we just need to extract - # the header using req.prepare(). - req = Request('GET', self.base_url, auth=self.get_auth(principal)) - headers = list(req.prepare().headers.items()) - if need_cred: - # Get the X-KRB5-CRED header (forwarded TGT). - cred = b64encode(get_fwd_tgt('ceod/' + socket.getfqdn())).decode() - headers.append(('X-KRB5-CRED', cred)) + def get_headers(self, principal: str, delegate: bool): + # Get the Authorization header (SPNEGO). + # The method doesn't matter here because we just need to extract + # the header using req.prepare(). + req = Request('GET', self.base_url, auth=self.get_auth(principal, delegate)) + headers = list(req.prepare().headers.items()) return headers - def request(self, method: str, path: str, principal: str, need_cred: bool, **kwargs): - # Make sure that we're not already in a request context, otherwise - # g will get overridden - with pytest.raises(RuntimeError): - '' in g + def request(self, method: str, path: str, principal: str, delegate: bool, **kwargs): + # make sure that we're not already in a Flask context + assert not flask.has_app_context() if principal is None: principal = self.syscom_principal - headers = self.get_headers(principal, need_cred) + headers = self.get_headers(principal, delegate) resp = self.client.open(path, method=method, headers=headers, **kwargs) status = int(resp.status.split(' ', 1)[0]) if resp.headers['content-type'] == 'application/json': @@ -71,14 +60,14 @@ class CeodTestClient: data = [json.loads(line) for line in resp.data.splitlines()] return status, data - def get(self, path, principal=None, need_cred=True, **kwargs): - return self.request('GET', path, principal, need_cred, **kwargs) + def get(self, path, principal=None, delegate=True, **kwargs): + return self.request('GET', path, principal, delegate, **kwargs) - def post(self, path, principal=None, need_cred=True, **kwargs): - return self.request('POST', path, principal, need_cred, **kwargs) + def post(self, path, principal=None, delegate=True, **kwargs): + return self.request('POST', path, principal, delegate, **kwargs) - def patch(self, path, principal=None, need_cred=True, **kwargs): - return self.request('PATCH', path, principal, need_cred, **kwargs) + def patch(self, path, principal=None, delegate=True, **kwargs): + return self.request('PATCH', path, principal, delegate, **kwargs) - def delete(self, path, principal=None, need_cred=True, **kwargs): - return self.request('DELETE', path, principal, need_cred, **kwargs) + def delete(self, path, principal=None, delegate=True, **kwargs): + return self.request('DELETE', path, principal, delegate, **kwargs) diff --git a/tests/utils.py b/tests/utils.py index 324aedd..d7a12e1 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -4,31 +4,44 @@ import subprocess from subprocess import DEVNULL import tempfile +import gssapi +import pytest -# map principals to files storing credentials -_ccaches = {} + +# map principals to GSSAPI credentials +_cache = {} @contextlib.contextmanager -def krb5ccname_ctx(principal: str): +def gssapi_creds_ctx(principal: str): """ Temporarily set KRB5CCNAME to a ccache storing credentials - for the specified user. + for the specified user, and yield the GSSAPI credentials. """ old_krb5ccname = os.environ['KRB5CCNAME'] try: - if principal not in _ccaches: + if principal not in _cache: f = tempfile.NamedTemporaryFile() os.environ['KRB5CCNAME'] = 'FILE:' + f.name args = ['kinit', principal] if principal == 'ceod/admin': args = ['kinit', '-k', principal] subprocess.run( - args, stdout=DEVNULL, text=True, input='krb5', - check=True) - _ccaches[principal] = f + args, stdout=DEVNULL, text=True, input='krb5', check=True) + creds = gssapi.Credentials(name=gssapi.Name(principal), usage='initiate') + # Keep the credential cache files around as long as the creds are + # used, otherwise we get a "Invalid credential was supplied" error + _cache[principal] = creds, f else: - os.environ['KRB5CCNAME'] = 'FILE:' + _ccaches[principal].name - yield + creds, f = _cache[principal] + os.environ['KRB5CCNAME'] = 'FILE:' + f.name + yield creds finally: os.environ['KRB5CCNAME'] = old_krb5ccname + + +@pytest.fixture(scope='session', autouse=True) +def ccache_cleanup(): + """Make sure the ccache files get deleted at the end of the tests.""" + yield + _cache.clear() From 46881f7a1f76cf77636adfde681b0aadce8ddb8b Mon Sep 17 00:00:00 2001 From: Max Erenberg Date: Thu, 26 Aug 2021 02:20:24 +0000 Subject: [PATCH 3/4] update .drone.yml --- .drone.yml | 1 - 1 file changed, 1 deletion(-) diff --git a/.drone.yml b/.drone.yml index 615bb73..7084efa 100644 --- a/.drone.yml +++ b/.drone.yml @@ -15,7 +15,6 @@ steps: - . venv/bin/activate - pip install -r dev-requirements.txt - pip install -r requirements.txt - - cd ceo_common/krb5 && python krb5_build.py && cd ../.. # lint - flake8 From d8e5b1f1d4ffe9b3fc6b17923ccc76b825ca474d Mon Sep 17 00:00:00 2001 From: Max Erenberg Date: Thu, 26 Aug 2021 02:26:56 +0000 Subject: [PATCH 4/4] update README --- .gitignore | 2 -- README.md | 12 +----------- 2 files changed, 1 insertion(+), 13 deletions(-) diff --git a/.gitignore b/.gitignore index b41a4d6..797afdc 100644 --- a/.gitignore +++ b/.gitignore @@ -2,7 +2,5 @@ __pycache__/ *.pyc /venv/ .vscode/ -/cred *.o *.so -/ceo_common/krb5/_krb5.c diff --git a/README.md b/README.md index b14eae7..69883c4 100644 --- a/README.md +++ b/README.md @@ -94,22 +94,12 @@ curl -V ``` Your should see 'SPNEGO' in the 'Features' section. -The API also uses unconstrained Kerberos delegation when interacting with -the LDAP database. This means that the client obtains a forwarded TGT, then -sends that to ceod, which then uses it to interact with LDAP on the client's -behalf. There is a script called `gen_cred.py` which can generate this -ticket for you. - - Here's an example of making a request to an endpoint which writes to LDAP: ```sh # Get a Kerberos TGT first kinit -# Obtain a forwarded TGT -./gen_cred.py phosphoric-acid # Make the request -curl --negotiate -u : --service-name ceod \ - -H "X-KRB5-CRED: $(cat cred)" \ +curl --negotiate -u : --service-name ceod --delegation always \ -d '{"uid":"test_1","cn":"Test One","program":"Math","terms":["s2021"]}' \ -X POST http://phosphoric-acid:9987/api/members ```