-
Notifications
You must be signed in to change notification settings - Fork 12
add krb5.aname_to_localname()
#19
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| # Copyright: (c) 2022 Jordan Borean (@jborean93) <jborean93@gmail.com> | ||
| # MIT License (see LICENSE or https://opensource.org/licenses/MIT) | ||
|
|
||
| from krb5._context import Context | ||
| from krb5._principal import Principal | ||
|
|
||
| def aname_to_localname(context: Context, principal: Principal) -> str: | ||
| """Translate a Kerberos principal to an authorization ID ("local name"). | ||
|
|
||
| Using configured rules, translate a Kerberos principal name to a | ||
| string suitable for use in authorization rules; often, this means | ||
| mapping it to a username for the host OS. See krb5.conf(5) | ||
| "localauth interface" for details. | ||
|
|
||
| Args: | ||
| context: krb5 context | ||
| principal: principal name to translate | ||
|
|
||
| Returns: | ||
| str: translation | ||
| None: principal has no translation | ||
| (krb5_aname_to_localname() returns KRB5_LNAME_NOTRANS) | ||
| """ | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,47 @@ | ||
| # Copyright: (c) 2022 Jordan Borean (@jborean93) <jborean93@gmail.com> | ||
| # MIT License (see LICENSE or https://opensource.org/licenses/MIT) | ||
|
|
||
| from libc.stdlib cimport free, malloc | ||
|
|
||
| from krb5._exceptions import Krb5Error | ||
|
|
||
| from krb5._context cimport Context | ||
| from krb5._krb5_types cimport * | ||
| from krb5._principal cimport Principal | ||
|
|
||
|
|
||
| cdef extern from "python_krb5.h": | ||
|
|
||
| krb5_error_code krb5_aname_to_localname( | ||
| krb5_context context, | ||
| krb5_const_principal aname, | ||
| int lnsize, | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looking at Heimdal, this is declared as It might require some trickery that we do in _keyblock.pyx to define a generic function but use compiler directives to cast as needed to Considering the code passes and the compiler doesn't complain, I think it will be fine but it's something to keep in mind. |
||
| char *lname_out | ||
| ) nogil | ||
|
|
||
| krb5_error_code KRB5_LNAME_NOTRANS | ||
|
|
||
| def aname_to_localname( | ||
| context: Context, | ||
| principal: Principal | ||
| ) -> str: | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See comments about bytes, but this should also be |
||
|
|
||
| cdef krb5_error_code err = 0 | ||
| limit = 256 | ||
|
|
||
| cdef char *localname = <char *>malloc(limit + 1) | ||
| if not localname: | ||
| raise MemoryError() | ||
|
|
||
| err = krb5_aname_to_localname(context.raw, principal.raw, limit, localname) | ||
| # The definition of KRB5_LNAME_NOTRANS in the Heimdal header file | ||
| # included in the pykrb5 source appears to be out of date; this is | ||
| # the value that actually gets returned in the test -- so I'm just | ||
| # adding it here for now. | ||
| if err in [KRB5_LNAME_NOTRANS, -1765328227]: | ||
|
Comment on lines
+37
to
+41
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like Just as an FYI the Heimdal headers in the repo are only used for macOS builds as they are not available on the host. If compiling against Heimdal on an actual Linux host it will use the headers from the source you are linking against. |
||
| return None | ||
| elif err != 0: | ||
| free(localname) | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You free it here but not in the good case, we should always free any memory we allocate or any memory allocated by the krb5 library. See kt_default_name which has a similar API to this call. It starts with a fixed buffer and continues to increase it until there is enough space. It also ensures the buffer is freed once it has been allocated using a try/finally and uses I would see the current code looking like cdef krb5_error_code err = KRB5_CONFIG_NOTENUFSPACE
buffer_size = 1024
buffer_length = buffer_size
cdef char *buffer = <char *>malloc(buffer_length)
if not buffer:
raise MemoryError()
try:
while err == KRB5_CONFIG_NOTENUFSPACE:
err = krb5_aname_to_localname(context.raw, principal.raw, buffer_length, buffer)
if err == KRB5_CONFIG_NOTENUFSPACE:
buffer_length += buffer_size
# Use a temp var to ensure buffer is always something valid to be freed
new_buffer = <char *>realloc(buffer, buffer_length)
if not new_buffer:
raise MemoryError()
buffer = new_buffer
elif err in [KRB5_LNAME_NOTRANS, KRB5_NO_LOCALNAME]:
return None
elif err:
raise Krb5Error(context, err)
else:
name_len = strlen(buffer)
return <bytes>buffer[:name_len]
finally:
free(buffer)I saw in the MIT krb5 and Heimdal tests that they use a buffer size of 1024 and not 256 so I've used that as a base.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the detailed review! Skimming it everything you say makes sense. I'll try to make the changes in the next few days; I won't get to it right away, I'm afraid.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One thing:
I must be missing something: we can't free it in the good case, because we're returning it to the caller. I was assuming that once it's returned it's reified as a Python Ah, maybe it's this: an expression like
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I actually thought about the keep-mallocing-until-it's-big-enough strategy, but I didn't because a) I'm suspicious of things that can cause a program to just keep allocating memory endlessly until it crashes; I prefer setting some reasonable limit instead, where it makes sense, and b) it seems pretty unlikely in practice to have such long translations. It's a bit of a tension between different aspects of correctness and safety. But I'm happy to do it this way if you prefer.
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
In that case the use of
That's a good point, we should probably have a check to see if the |
||
| raise Krb5Error(context, err) | ||
| else: | ||
| return localname.decode("utf-8") | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I try to keep these APIs agnostic to encodings as there isn't really one set encoding that the krb5 uses. It might be utf-8 in most cases but that isn't a guarantee. Because of this, char based APIs return |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,63 @@ | ||
| # Copyright: (c) 2021 Jordan Borean (@jborean93) <jborean93@gmail.com> | ||
| # MIT License (see LICENSE or https://opensource.org/licenses/MIT) | ||
|
|
||
| import os | ||
| import tempfile | ||
| from typing import Optional | ||
|
|
||
| import k5test | ||
| import pytest | ||
|
|
||
| import krb5 | ||
|
|
||
|
|
||
| def test_aname_to_localname(realm: k5test.K5Realm) -> None: | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Have a look at test_cc_cache_match which uses pytests' def test_aname_to_localname(realm: k5test.K5Realm, tmp_path: pathlib.Path, monkeypatch: pytest.MonkeyPatch) -> None:
krb5_conf = tmp_path / "krb5.conf"
with open(krb5_conf, mode="w") as fd:
fd.write(...)
monkeypatch.setenv("KRB5_CONFIG", str(krb5_conf))
... test code starts here |
||
|
|
||
| # require: translation(p) == t | ||
| def test(ctx: krb5.Context, p: str, t: Optional[str]) -> None: | ||
| assert t == krb5.aname_to_localname(ctx, krb5.parse_name_flags(ctx, p.encode())) | ||
|
|
||
| # To test, apply our own krb5.conf with a single translation | ||
| # rule. The file is read by krb5_init_context(), so set the | ||
| # environment variable before calling that. This overrides the | ||
| # default rule, so only matching principals will have a | ||
| # translation. Require a 2-instance name in the realm TEST. | ||
| # | ||
| # ... that's for MIT. Heimdal doesn't appear to have RULE | ||
| # translations built in, so use auth_to_local_names instead. | ||
| # | ||
| # Since environment variables are process-wide, this could mess up | ||
| # other tests in a multi-threaded test framework -- but that | ||
| # probably won't be an issue here. | ||
|
|
||
| with tempfile.NamedTemporaryFile() as config: | ||
| saved_conf = os.environ.get("KRB5_CONFIG") | ||
| try: | ||
| config.write( | ||
| b""" | ||
| [libdefaults] | ||
| default_realm = TEST | ||
|
|
||
| [realms] | ||
| TEST = { | ||
| auth_to_local = RULE:[2:$1:$2@$0](^.*@TEST$)s/@TEST$// | ||
| auth_to_local_names = { | ||
| heimdal = mapped | ||
| } | ||
| } | ||
| """ | ||
| ) | ||
| config.flush() | ||
| os.environ["KRB5_CONFIG"] = config.name | ||
| ctx = krb5.init_context() | ||
| if realm.provider == "mit": | ||
| test(ctx, "foo/bar@TEST", "foo:bar") | ||
| test(ctx, "foo@TEST", None) | ||
| test(ctx, "foo/bar@WRONG", None) | ||
| if realm.provider == "heimdal": | ||
| test(ctx, "heimdal", "mapped") | ||
| finally: | ||
| if saved_conf: | ||
| os.environ["KRB5_CONFIG"] = saved_conf | ||
| else: | ||
| del os.environ["KRB5_CONFIG"] | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if Google doc strings support this format. Usually I do