Skip to content
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

Creds serialization and deserialization #45

Merged
merged 8 commits into from
Jul 15, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion src/krb5/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -172,12 +172,19 @@


try:
from krb5._creds_mit import get_etype_info, get_validated_creds
from krb5._creds_mit import (
get_etype_info,
get_validated_creds,
serialize_creds,
unserialize_creds,
)
except ImportError:
pass
else:
__all__.append("get_etype_info")
__all__.append("get_validated_creds")
__all__.append("serialize_creds")
__all__.append("unserialize_creds")


try:
Expand Down
22 changes: 22 additions & 0 deletions src/krb5/_creds_mit.pyi
Original file line number Diff line number Diff line change
Expand Up @@ -48,3 +48,25 @@ def get_etype_info(

If there are no s2kparams in the provided etype-info, s2kparams is None.
"""

def serialize_creds(context: Context, creds: Creds) -> bytes:
"""Serialize creds in the format used by the FILE ccache format
(vesion 4) and KCM ccache protocol.

Args:
creds: Credentials to serialize.
zarganum marked this conversation as resolved.
Show resolved Hide resolved

Returns:
bytes: The serialized credentials.
"""

def unserialize_creds(context: Context, data: bytes) -> Creds:
"""Deserialize creds from the format used by the FILE ccache format
(vesion 4) and KCM ccache protocol.

Args:
context: Krb5 context.
data: serialized credentials.
Returns:
Creds: The unserialized credentials.
"""
85 changes: 84 additions & 1 deletion src/krb5/_creds_mit.pyx
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@ import typing

from krb5._exceptions import Krb5Error

from libc.stdlib cimport free
from libc.string cimport memcpy

from krb5._ccache cimport CCache
from krb5._context cimport Context
from krb5._creds cimport Creds
Expand All @@ -17,7 +20,19 @@ from krb5._principal cimport Principal
cdef extern from "python_krb5.h":
"""
#if defined(HEIMDAL_XFREE)
#error "Heimdal implementation of krb5_get_validated_creds() does not work"
#error "Heimdal implementation does not support MIT-specific calls:"
#error " krb5_get_validated_creds()"
#error " krb5_get_etype_info()"
#error " krb5_marshal_credentials()"
#error " krb5_unmarshal_credentials()"
#else
void pykrb5_free_data(
zarganum marked this conversation as resolved.
Show resolved Hide resolved
krb5_context context,
krb5_data *val
)
{
krb5_free_data(context, val);
}
#endif
"""

Expand All @@ -38,6 +53,22 @@ cdef extern from "python_krb5.h":
krb5_data *s2kparams_ou,
) nogil

krb5_error_code krb5_marshal_credentials(
krb5_context context,
krb5_creds *creds,
krb5_data **data
) nogil

krb5_error_code krb5_unmarshal_credentials(
krb5_context context,
krb5_data *data,
krb5_creds **creds,
) nogil

void pykrb5_free_data(
krb5_context context,
krb5_data *val,
) nogil

def get_validated_creds(
Context context not None,
Expand Down Expand Up @@ -120,3 +151,55 @@ EtypeInfo = collections.namedtuple('EtypeInfo', [
'salt',
's2kparams',
])

def serialize_creds(Context context not None, Creds self not None) -> bytes:
zarganum marked this conversation as resolved.
Show resolved Hide resolved
cdef krb5_error_code err = 0
cdef krb5_data *data = NULL
cdef size_t length
cdef char *value

try:
err = krb5_marshal_credentials(context.raw, &self.raw, &data)

if err:
raise Krb5Error(context, err)

pykrb5_get_krb5_data(data, &length, &value)

if length == 0:
data_bytes = b""
else:
data_bytes = value[:length]

return data_bytes

finally:
if NULL != data:
pykrb5_free_data(context.raw, data)

def unserialize_creds(Context context not None, const unsigned char[:] data not None) -> Creds:
zarganum marked this conversation as resolved.
Show resolved Hide resolved
cdef krb5_error_code err = 0
cdef krb5_creds *creds_raw = NULL
cdef krb5_data data_raw

creds = Creds(context)

if len(data) == 0:
pykrb5_set_krb5_data(&data_raw, 0, "")
else:
pykrb5_set_krb5_data(&data_raw, len(data), <char *>&data[0])

err = krb5_unmarshal_credentials(context.raw, &data_raw, &creds_raw)

if creds_raw:
# creds_raw was calloc'ed for krb5_creds structure
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm somewhat weary about using free here explicitly. The docs for krb5_unmarshal_credentials state that

Use krb5_free_creds() to free creds_out when it is no longer needed.

I can see the actual implementation essentially calls krb5_free_creds_content and then calls free. https://github.com/krb5/krb5/blob/354f176ba6d6cc544e1c15712a13f9c006ca605d/src/lib/krb5/krb/kfree.c#L213-L219

While this replicates that same logic that's more of an implementation detail and not a public API. We should try to avoid freeing data using free if we don't control how it was allocated.

Instead I'm wondering if we should add another cdef field on Creds that can be used to control how the struct is deallocated to work with this new scenario. For example (the field it actually defined in the pxd file, the comment is more for documentation)

cdef class Creds:
    # cdef Context ctx
    # cdef krb5_creds raw
    # cdef int needs_free
    # cdef int free_struct

    def __dealloc__(Creds self):
        if self.needs_free:
            if self.free_struct:
                krb5_free_creds(self.ctx.raw, &self.raw)
            else:
                krb5_free_cred_contents(self.ctx.raw, &self.raw)
            self.needs_free = 0

The krb5_free_creds would also need to be defined in the file but seems like Heimdal has it so it shouldn't be a problem.

The other benefit of this is we don't need to copy the data we can just do something like

creds = Creds(context)

...

err = krb5_unmarshal_credentials(context.raw, &data_raw, &creds.raw)
if err:
    raise Krb5Error(context, err)

creds.free_struct = 1
creds.needs_free = 1

return creds

Do you see any problem with this approach?

Copy link
Contributor Author

@zarganum zarganum Jul 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree I stepped onto thin ice here... The point is a pointer-to-pointer used by krb5_unmarshal_credentials(), it unconditioanaly allocates the structure using its k5alloc() wrapper and writes the allocated pointer to creds_out. There is just no way to not allocate some heap memory, so it has to be free'd.

https://github.com/krb5/krb5/blob/354f176ba6d6cc544e1c15712a13f9c006ca605d/src/lib/krb5/ccache/ccmarshal.c#L557

There are might be two ways to avoid shortcuts like mine, right after krb5_unmarshal_credentials()

  1. Deep copy the new creds to our self.raw and deallocate everything just returned with krb5_free_creds() as original API prescribes. Our existing raw will contain a deep copy to be free'd with krb5_free_cred_contents() in __deallocate__.
  2. Shallow copy the new creds to our self.raw, save the alloc'ed pointer as _raw_ptr and indeed call krb5_free_creds instead of krb5_free_cred_contents in __deallocate__. Our existing raw will remain the shallow copy to keep other things working.

The problem with (1) is that deep copy function krb5_copy_creds() behaves the same as krb5_unmarshal_credentials() and allocates the new structure.

Are you ok with having additional _raw_ptr member in Creds class?
Or any better ideas?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see, sorry I didn't see it was a krb5_creds** argument. I thought it was just krb5_creds*, now the API makes more sense.

# "shallow copy" the structure and free it
memcpy(&creds.raw, creds_raw, sizeof(creds.raw))
free(creds_raw)
# mark "deep copy" for future deallocation
creds.needs_free = 1

if err:
raise Krb5Error(context, err)

return creds
26 changes: 26 additions & 0 deletions tests/test_creds.py
Original file line number Diff line number Diff line change
Expand Up @@ -292,3 +292,29 @@ def test_get_etype_info(realm: k5test.K5Realm, tmp_path: pathlib.Path) -> None:
# Verify the credentials
assert creds.client.name == realm.user_princ.encode()
assert creds.server.name == b"krbtgt/KRBTEST.COM@KRBTEST.COM"


@pytest.mark.requires_api("serialize_creds")
def test_creds_serialization(realm: k5test.K5Realm) -> None:
ctx = krb5.init_context()
princ = krb5.parse_name_flags(ctx, realm.user_princ.encode())
opt = krb5.get_init_creds_opt_alloc(ctx)
creds = krb5.get_init_creds_password(ctx, princ, opt, realm.password("user").encode())
assert isinstance(creds, krb5.Creds)

with pytest.raises(krb5.Krb5Error):
krb5.unserialize_creds(ctx, b"invalid")

with pytest.raises(krb5.Krb5Error):
krb5.unserialize_creds(ctx, b"")

bytes = krb5.serialize_creds(ctx, creds)
assert len(bytes) > 0
zarganum marked this conversation as resolved.
Show resolved Hide resolved

uncreds = krb5.unserialize_creds(ctx, bytes)
assert str(uncreds) == "Creds"
zarganum marked this conversation as resolved.
Show resolved Hide resolved
assert id(creds) != id(uncreds)
assert creds.client.name == uncreds.client.name
assert creds.ticket == uncreds.ticket
assert creds.keyblock.data == uncreds.keyblock.data
assert creds.times.endtime == uncreds.times.endtime