jborean93 / pykrb5

Python krb5 API interface
MIT License
16 stars 7 forks source link

Cleanup cred implementation #47

Closed jborean93 closed 2 months ago

jborean93 commented 2 months ago

Cleans up the credential implementation to better handle the marshalling API. This includes splitting out those APIs into a separate pyx file as it was introduced in MIT krb5 1.20 and also unified how the krb5_creds value is stored in the Python object that represents it.

jborean93 commented 2 months ago

cc @zarganum

No expectations for you to have a look over this but here is a bit of some cleanup around the krb5_creds handling to try and unify it a bit and avoid copies.

Also the reason why you were seeing implicit-function-declaration in the warnings is because these APIs were only publicly exposed in the headers in MIT 1.20 and CI is running with 1.19. This will ensure it's only exposed if the krb5 lib exports the symbol and thus has the headers. I did test this offline with Debian 12 which ships with MIT krb5 1.20 through the build_helpers/run-container.sh script.

zarganum commented 2 months ago

No expectations for you to have a look over this but here is a bit of some cleanup around the krb5_creds handling to try and unify it a bit and avoid copies.

I fully agree Creds class deserves the proper encapsulation of what used to be raw to address the "bipolar memory disorder" (that I brought with these marshaling). My initial attempt was even to put the de/serialize as the class methods (you saw the traces like self). And then I moved to standalone functions to comply with mainstream idea... but still missed things and manipulated private data externally. Anyway, with this new memory management we can also bring the deep copy function krb5_copy_creds() from the library and potentially anything that allocates memory there.

The only thing that hurts my Python nazi eyes (yeah, I sometimes putting them on) is the __c_value__(): every time I see the dunder I assume this is a Python mechanism. My personal approach to this if I really want to use double underscores, I never put them on the right, telling reader: NO, this is not a Python magic, this is just double underscores. So I vote for __c_value() or even _raw_value() to avoid confusion:)

Also the reason why you were seeing implicit-function-declaration in the warnings is because these APIs were only publicly exposed in the headers in MIT 1.20 and CI is running with 1.19. This will ensure it's only exposed if the krb5 lib exports the symbol and thus has the headers. I did test this offline with Debian 12 which ships with MIT krb5 1.20 through the build_helpers/run-container.sh script.

makes sense, thanks.

zarganum commented 2 months ago

and how about dropping Python 3.7 support which is in EOL already? Save energy:)

jborean93 commented 2 months ago

is the __c_value__()

Yea that's a good point, I took it from an old project I worked on in the past but definitely better to just use a normal name for the function. As it's a cdef function I don't need to worry about marking it as private really.

and how about dropping Python 3.7 support which is in EOL already?

Maybe in a future version but for now I still want to support Debian 10 and the default Python there. I might revisit that in the future at one point :)