Add TicketFlags class and Creds.ticket_flags attribute

[steffen-kiess]

Note that this uses the heimdal definition of the ticket flags (where flag i is represented as 1 << i) instead of the MIT one (where flag i is represented as 1 << (31 - i)) because this seems to make more sense to me. For MIT the values are converted when reading Creds.ticket_flags.

[jborean93]

I'm a bit torn here, while this library does paper over some API differences that's more around some minor API/struct changes to expose the same data through a Python interface. This case is actually changing the results we get back to some more common standard. Without thinking about it too hard I'm probably against the idea for the following reasons

• We are trying to provide an interface into the C API to call from Python rather than a high level API
• People who are used to the API might in fact be more familiar with the MIT result vs Heimdal. The same argument can be used if the behaviour was the opposite

I'm wondering whether we should keep ticket_flags (or rename to ticket_flags_raw or some variant) and have another property or method that can give the friendly enum variant. What are your thoughts around this?

[steffen-kiess]

The reason I wrote it this way was to allow checking things like "does this ticket have the initial flag set". If only the raw value is provided, this would require the user of the library to hardcode the value of TKT_FLG_INITIAL, which would mean the code would likely break when moving between MIT and Heimdal. In many other places pykrb5 (e.g. PrincipalParseFlags) provides the value from the C library to the user, but in this case

• this would not work for unknown flags, which are more likely in this case, because this is a value which comes from the KDC, not from the user, and
• Heimdal does not even provide TKT_FLG_INITIAL, instead assuming the user uses the bit-fields in struct TicketFlags.

• People who are used to the API might in fact be more familiar with the MIT result vs Heimdal.

I mostly chose the Heimdal version here because it is closer to the RFCs, and because it would in theory allow more than 32 flags. (The on-the-wire ASN1 structure allow more than 32 flags, but both the MIT and Heimdal APIs can return only 32 flags, and changing this would break the ABI.)

But I understand the point that this might be confusing to people. Another thing is that python-gssapi also returns the raw ticket flags (in krb5_get_tkt_flags()), and an application which uses both pykrb5 and python-gssapi might get confused by that.

I'm wondering whether we should keep ticket_flags (or rename to ticket_flags_raw or some variant) and have another property or method that can give the friendly enum variant. What are your thoughts around this?

I think I like the idea.

Should I update the PR to

• provide the raw value from the library as ticket_flags_raw (or ticket_flags)
• add static method from_raw(int) -> TicketFlags and a property raw to TicketFlags
• add a property to Creds which returns TicketFlags.from_raw(self.ticket_flags_raw), called e.g. ticket_flags_enum or ticket_flags or something like that?

[jborean93]

provide the raw value from the library as ticket_flags_raw (or ticket_flags)

I think this might be the easiest option to me, change the existing ticket_flags to ticket_flags_raw that returns the raw integer and have ticket_flags return the enum.IntFlags enum. I'm not seeing much benefit by adding in extra methods to convert and even if there is a demand for it we can always add it later.

I mostly chose the Heimdal version here because it is closer to the RFCs, and because it would in theory allow more than 32 flags.

Makes sense to me, I would prefer that setup as well. I think as long as we document that ticket_flags is a human friendly representation of the raw value and to use ticket_flags_raw if you really want it is the way to go.

[steffen-kiess]

I've updated the PR to do that.

[jborean93]

Would be great to add a docstring about this enum that links to the RFC where they are defined.

[jborean93]

Did you add this for a reason? AFAIK this isn't exposed on the actual Creds object.

[steffen-kiess]

Hmm. I thought it was exposed. Probably I added it at some point to the .pyx file and then removed it again.

I'll remove it from the .pyi file.

[jborean93]

Can you share an actual scenario where this is required? When you say will contain only known flags do you mean that unsetting the bit will clear out any values that aren't defined here or something else?

[steffen-kiess]

Can you share an actual scenario where this is required?

This would be if the KDC sets some flag which is not known to the pykrb5 library and the application wants to see that flag.

When you say will contain only known flags do you mean that unsetting the bit will clear out any values that aren't defined here

Yes.

import enum

class MyFlags(enum.IntFlag):
    A = 1
    B = 2

print(int(MyFlags(15)))  # Will print 15
print(int(MyFlags(15) & ~MyFlags.B))  # On python >= 3.11, will print 1, before 3.11 will print 13
print(int(~MyFlags.B))  # On python >= 3.11, will print 1, before then will print -3

That means without this on Python >= 3.11, a & ~TicketFlags.initial will remove not only the initial flag, but also all flags which are not defined in the TicketFlags enum.

[jborean93]

That's definitely surprising behaviour but if that's out of our control here :) Thanks for sharing the details.

[jborean93]

Thanks for all your fantastic PRs for this library, please let me know if you have any more features you want to add anytime soon and I'll hold off on doing some integration testing for the next release.

[steffen-kiess]

Thanks for all your fantastic PRs for this library

Thanks for the quick review and merging of the changes :-)

please let me know if you have any more features you want to add anytime soon and I'll hold off on doing some integration testing for the next release.

I opened one last PR #44, but other than that I'm not planning on any new features soon (the last PRs were mostly things I had in my local repository for the last 1-2 years).