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

Add Kerberos specific GSS-API Extensions #261

Merged
merged 1 commit into from
Aug 10, 2021

Conversation

jborean93
Copy link
Contributor

@jborean93 jborean93 commented Aug 6, 2021

Adds some of the Kerberos specific GSS-API extension methods. These
methods are useful when migrating from the Kerberos API to GSS-API or
when needing Kerberos specific functionality that is not exposed in
GSS-API.

Partially fixes #75

Signed-off-by: Jordan Borean jborean93@gmail.com

@jborean93 jborean93 force-pushed the krb5-apis branch 3 times, most recently from b011ba7 to a582dcc Compare August 9, 2021 19:01
Copy link
Member

@frozencemetery frozencemetery left a comment

Choose a reason for hiding this comment

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

Some initial thoughts.

gssapi/raw/python_gssapi_krb5.h Outdated Show resolved Hide resolved
gssapi/raw/python_gssapi_krb5.h Outdated Show resolved Hide resolved
gssapi/tests/test_raw.py Outdated Show resolved Hide resolved
gssapi/tests/test_raw.py Outdated Show resolved Hide resolved
cls.KRB5_LIB_PATH = cls._get_krb5_lib_path()

@classmethod
def _get_krb5_lib_path(cls) -> str:
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, this is just giving us your "find the right krb5" problem in the test suite. This won't work right if: libgssapi is statically built, the correct krb5-config isn't first in PATH (e.g. freeBSD), etc..

At the very least, all users of this need to gracefully handle when krb5 isn't found. More generally, testing krb5_import_cred() may just be more trouble than it's worth - right now I'm more inclined to skip it with a note.

Another thought is gss_krb5_copy_ccache() - you elected not to implement this, but I think if you did, we could just treat krb5_ccache as an opaque pointer and test the two function as a unit.

Copy link
Contributor Author

@jborean93 jborean93 Aug 9, 2021

Choose a reason for hiding this comment

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

Yea I couldn't figure out a way to avoid this. I thought at least having it tested in CI is better than no tests at all but if you prefer not to have this logic and just skip that is ok.

Another thought is to just read the value from an env var and skip it if it's not defined. This can allow the test suite to run normally without these tests locally and CI can have it set as we would know the correct paths beforehand. What do you think of that?

Another thought is gss_krb5_copy_ccache() - you elected not to implement this, but I think if you did, we could just treat krb5_ccache as an opaque pointer and test the two function as a unit.

Unfortunately that doesn't help and it has the same problem. gss_krb5_copy_ccache() takes in a pointer to the ccache and it doesn't set the pointer on return so it would also need this same logic to open the ccache using something like ctypes.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, good point about gss_krb5_copy_ccache(). Will think about the rest.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll set the env var as it is definitely safer and at least keeps the tests running in CI. If you think of another way or want to do something different then let me know.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The latest commit adds this and sets the env var in the distro specific section of ci/lib-setup.sh.

gssapi/raw/ext_krb5.pyx Outdated Show resolved Hide resolved
@frozencemetery frozencemetery force-pushed the krb5-apis branch 2 times, most recently from 0378a84 to 89545bb Compare August 10, 2021 18:06
Adds most of the Kerberos specific GSS-API extension methods.  These
methods are useful when migrating from the Kerberos API to GSS-API or
when needing Kerberos specific functionality that is not exposed in
GSS-API.

Signed-off-by: Jordan Borean <jborean93@gmail.com>
[rharwood@redhat.com: style and grammar things]
@frozencemetery frozencemetery merged commit be33336 into pythongssapi:main Aug 10, 2021
@jborean93
Copy link
Contributor Author

Thanks for the review!

@jborean93 jborean93 deleted the krb5-apis branch August 10, 2021 18:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement krb5-specific extensions
2 participants