From 33638de180a8157e369ad6c61f9e3406d9e85404 Mon Sep 17 00:00:00 2001 From: Stanislav Levin Date: Tue, 23 Jan 2024 19:12:53 +0300 Subject: [PATCH 1/3] ipapython: Clean up krb5_error `krb5_error` has different definition in MIT krb. https://web.mit.edu/kerberos/krb5-latest/doc/appdev/refs/types/krb5_error.html > Error message structure. > > Declaration: > typedef struct _krb5_error krb5_error While `krb5_error_code` https://web.mit.edu/kerberos/www/krb5-latest/doc/appdev/refs/types/krb5_error_code.html#c.krb5_error_code > krb5_error_code > Used to convey an operation status. > > The value 0 indicates success; any other values are com_err codes. Use krb5_get_error_message() to obtain a string describing the error. > > Declaration > typedef krb5_int32 krb5_error_code And this is what was actually used. To prevent confusion of types `krb5_error` was replaced with `krb5_error_code`. Fixes: https://pagure.io/freeipa/issue/9519 Signed-off-by: Stanislav Levin Reviewed-By: Alexander Bokovoy --- ipapython/session_storage.py | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/ipapython/session_storage.py b/ipapython/session_storage.py index c43ef7d4e..371cf1524 100644 --- a/ipapython/session_storage.py +++ b/ipapython/session_storage.py @@ -111,7 +111,7 @@ class KRB5Error(Exception): def krb5_errcheck(result, func, arguments): - """Error checker for krb5_error return value""" + """Error checker for krb5_error_code return value""" if result != 0: raise KRB5Error(result, func.__name__, arguments) @@ -119,14 +119,13 @@ def krb5_errcheck(result, func, arguments): krb5_context = ctypes.POINTER(_krb5_context) krb5_ccache = ctypes.POINTER(_krb5_ccache) krb5_data_p = ctypes.POINTER(_krb5_data) -krb5_error = ctypes.c_int32 krb5_creds = _krb5_creds krb5_pointer = ctypes.c_void_p krb5_cc_cursor = krb5_pointer krb5_init_context = LIBKRB5.krb5_init_context krb5_init_context.argtypes = (ctypes.POINTER(krb5_context), ) -krb5_init_context.restype = krb5_error +krb5_init_context.restype = krb5_error_code krb5_init_context.errcheck = krb5_errcheck krb5_free_context = LIBKRB5.krb5_free_context @@ -143,30 +142,30 @@ krb5_free_data_contents.restype = None krb5_cc_default = LIBKRB5.krb5_cc_default krb5_cc_default.argtypes = (krb5_context, ctypes.POINTER(krb5_ccache), ) -krb5_cc_default.restype = krb5_error +krb5_cc_default.restype = krb5_error_code krb5_cc_default.errcheck = krb5_errcheck krb5_cc_close = LIBKRB5.krb5_cc_close krb5_cc_close.argtypes = (krb5_context, krb5_ccache, ) -krb5_cc_close.restype = krb5_error +krb5_cc_close.restype = krb5_error_code krb5_cc_close.errcheck = krb5_errcheck krb5_parse_name = LIBKRB5.krb5_parse_name krb5_parse_name.argtypes = (krb5_context, ctypes.c_char_p, ctypes.POINTER(krb5_principal), ) -krb5_parse_name.restype = krb5_error +krb5_parse_name.restype = krb5_error_code krb5_parse_name.errcheck = krb5_errcheck krb5_cc_set_config = LIBKRB5.krb5_cc_set_config krb5_cc_set_config.argtypes = (krb5_context, krb5_ccache, krb5_principal, ctypes.c_char_p, krb5_data_p, ) -krb5_cc_set_config.restype = krb5_error +krb5_cc_set_config.restype = krb5_error_code krb5_cc_set_config.errcheck = krb5_errcheck krb5_cc_get_principal = LIBKRB5.krb5_cc_get_principal krb5_cc_get_principal.argtypes = (krb5_context, krb5_ccache, ctypes.POINTER(krb5_principal), ) -krb5_cc_get_principal.restype = krb5_error +krb5_cc_get_principal.restype = krb5_error_code krb5_cc_get_principal.errcheck = krb5_errcheck # krb5_build_principal is a variadic function but that can't be expressed @@ -177,26 +176,26 @@ krb5_build_principal.argtypes = (krb5_context, ctypes.POINTER(krb5_principal), ctypes.c_uint, ctypes.c_char_p, ctypes.c_char_p, ctypes.c_char_p, ctypes.c_char_p, ctypes.c_char_p, ) -krb5_build_principal.restype = krb5_error +krb5_build_principal.restype = krb5_error_code krb5_build_principal.errcheck = krb5_errcheck krb5_cc_start_seq_get = LIBKRB5.krb5_cc_start_seq_get krb5_cc_start_seq_get.argtypes = (krb5_context, krb5_ccache, ctypes.POINTER(krb5_cc_cursor), ) -krb5_cc_start_seq_get.restype = krb5_error +krb5_cc_start_seq_get.restype = krb5_error_code krb5_cc_start_seq_get.errcheck = krb5_errcheck krb5_cc_next_cred = LIBKRB5.krb5_cc_next_cred krb5_cc_next_cred.argtypes = (krb5_context, krb5_ccache, ctypes.POINTER(krb5_cc_cursor), ctypes.POINTER(krb5_creds), ) -krb5_cc_next_cred.restype = krb5_error +krb5_cc_next_cred.restype = krb5_error_code krb5_cc_next_cred.errcheck = krb5_errcheck krb5_cc_end_seq_get = LIBKRB5.krb5_cc_end_seq_get krb5_cc_end_seq_get.argtypes = (krb5_context, krb5_ccache, ctypes.POINTER(krb5_cc_cursor), ) -krb5_cc_end_seq_get.restype = krb5_error +krb5_cc_end_seq_get.restype = krb5_error_code krb5_cc_end_seq_get.errcheck = krb5_errcheck krb5_free_cred_contents = LIBKRB5.krb5_free_cred_contents @@ -212,7 +211,7 @@ krb5_principal_compare.restype = krb5_boolean krb5_unparse_name = LIBKRB5.krb5_unparse_name krb5_unparse_name.argtypes = (krb5_context, krb5_principal, ctypes.POINTER(ctypes.c_char_p), ) -krb5_unparse_name.restype = krb5_error +krb5_unparse_name.restype = krb5_error_code krb5_unparse_name.errcheck = krb5_errcheck krb5_free_unparsed_name = LIBKRB5.krb5_free_unparsed_name -- 2.43.0 From f8a616dc6196324145372713da772fe9b2352e53 Mon Sep 17 00:00:00 2001 From: Stanislav Levin Date: Tue, 23 Jan 2024 19:19:43 +0300 Subject: [PATCH 2/3] ipapython: Correct return type of krb5_free_cred_contents According to https://web.mit.edu/kerberos/krb5-latest/doc/appdev/refs/api/krb5_free_cred_contents.html > krb5_free_cred_contents - Free the contents of a krb5_creds structure. > > void krb5_free_cred_contents(krb5_context context, krb5_creds * val) > param: > [in] context - Library context > > [in] val - Credential structure to free contents of > > This function frees the contents of val , but not the structure itself. https://github.com/krb5/krb5/blob/5b00197227231943bd2305328c8260dd0b0dbcf0/src/lib/krb5/krb/kfree.c#L166 This leads to undefined behavior and `krb5_free_cred_contents` can raise KRB5Error (because of garbage data) while actually its foreign function doesn't. Fixes: https://pagure.io/freeipa/issue/9519 Signed-off-by: Stanislav Levin Reviewed-By: Alexander Bokovoy --- ipapython/session_storage.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/ipapython/session_storage.py b/ipapython/session_storage.py index 371cf1524..dc36f5493 100644 --- a/ipapython/session_storage.py +++ b/ipapython/session_storage.py @@ -200,8 +200,7 @@ krb5_cc_end_seq_get.errcheck = krb5_errcheck krb5_free_cred_contents = LIBKRB5.krb5_free_cred_contents krb5_free_cred_contents.argtypes = (krb5_context, ctypes.POINTER(krb5_creds)) -krb5_free_cred_contents.restype = krb5_error -krb5_free_cred_contents.errcheck = krb5_errcheck +krb5_free_cred_contents.restype = None krb5_principal_compare = LIBKRB5.krb5_principal_compare krb5_principal_compare.argtypes = (krb5_context, krb5_principal, -- 2.43.0 From 59b8a9fb7169561c7ba9168fe84f47ae94e5ce23 Mon Sep 17 00:00:00 2001 From: Stanislav Levin Date: Tue, 23 Jan 2024 19:52:34 +0300 Subject: [PATCH 3/3] ipapython: Propagate KRB5Error exceptions on iterating ccache `ipapython.session_storage.get_data` iterates over credentials in a credential cache till `krb5_cc_next_cred` returns an error. This function doesn't expect any error on calling other kerberos foreign functions during iteration. But that can actually happen and KRB5Error exceptions stop an iteration while they should be propagated. With this change iteration will exactly stop on `krb5_cc_next_cred` error as it was supposed to be. Fixes: https://pagure.io/freeipa/issue/9519 Signed-off-by: Stanislav Levin Reviewed-By: Alexander Bokovoy --- ipapython/session_storage.py | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/ipapython/session_storage.py b/ipapython/session_storage.py index dc36f5493..e890dc9b1 100644 --- a/ipapython/session_storage.py +++ b/ipapython/session_storage.py @@ -312,8 +312,12 @@ def get_data(princ_name, key): checkcreds = krb5_creds() # the next function will throw an error and break out of the # while loop when we try to access past the last cred - krb5_cc_next_cred(context, ccache, ctypes.byref(cursor), - ctypes.byref(checkcreds)) + try: + krb5_cc_next_cred(context, ccache, ctypes.byref(cursor), + ctypes.byref(checkcreds)) + except KRB5Error: + break + if (krb5_principal_compare(context, principal, checkcreds.client) == 1 and krb5_principal_compare(context, srv_princ, @@ -328,8 +332,6 @@ def get_data(princ_name, key): else: krb5_free_cred_contents(context, ctypes.byref(checkcreds)) - except KRB5Error: - pass finally: krb5_cc_end_seq_get(context, ccache, ctypes.byref(cursor)) -- 2.43.0