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

Implemented RFC 4178 extension #176

Merged
merged 1 commit into from
Jul 8, 2019

Conversation

tengcm2015
Copy link
Contributor

reference issue #50

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.

  • Please squash all commits into a single one (I recommend using git rebase -i if you're unfamiliar with this workflow).
  • For writing good commit messages, please see other similar commits as well as https://chris.beams.io/posts/git-commit/


def set_neg_mechs(Creds cred_handle not None, mechs not None):
"""
set_neg_mechs(Creds cred_handle not None, mechs=None)
Copy link
Member

Choose a reason for hiding this comment

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

This isn't right - it should just have the names. Also, mechs can't have default value None because it's explicitly not None.

const gss_OID_set mechs) nogil


def set_neg_mechs(Creds cred_handle not None, mechs not None):
Copy link
Member

Choose a reason for hiding this comment

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

Should be named "mech_set" in keeping with 4178, krb5, and Heimdal.

"""
set_neg_mechs(Creds cred_handle not None, mechs=None)

This allows callers to specify the set of security mechanisms that
Copy link
Member

Choose a reason for hiding this comment

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

We don't typically use the RFC text for descriptions - it tends to be overly lengthy and include a lot of details we don't care about. See if you can make a shorter description in your own words.

relative preference.

Args:
cred_handle (Creds): the Credentials to query
Copy link
Member

Choose a reason for hiding this comment

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

Technically not a query - that would be more true of gss_get_neg_mechs() (which no one implements).


Args:
cred_handle (Creds): the Credentials to query
mechs ([MechType]): the desired set of mechanisms that
Copy link
Member

Choose a reason for hiding this comment

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

Maybe shorten this too

mechs ([MechType]): the desired set of mechanisms that
may be negotiated with the credentials
Returns:
None (If the operation completed successfully)
Copy link
Member

Choose a reason for hiding this comment

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

The parenthetical is unnecessary.

if maj_stat == GSS_S_COMPLETE:
return None
else:
print("error: %d:%d", maj_stat, min_stat)
Copy link
Member

Choose a reason for hiding this comment

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

Leftover debugging line?

gssapi/tests/test_raw.py Show resolved Hide resolved
@frozencemetery
Copy link
Member

Extension detection doesn't seem to have worked on Heimdal because our Debian's Heimdal is too old. (It was added this year in 735039dbdc3aa58d06afdefd214efe3f5e421244.) However, it'll work whenever that updates - they just put it in gssapi.h.

@DirectXMan12 PTAL

@tengcm2015 tengcm2015 force-pushed the ext_rfc4178 branch 2 times, most recently from 8f77bf7 to ef0f04a Compare June 6, 2019 20:38
@DirectXMan12
Copy link
Member

my context for this is mostly lost to time. Which part did you want me to take a look at? The extension negotiation?

@frozencemetery
Copy link
Member

I was just hoping for code review :)

@frozencemetery
Copy link
Member

@tengcm2015 Hey, this is still waiting on you. Any updates?

@tengcm2015
Copy link
Contributor Author

@frozencemetery I fixed what is mentioned in the code review, but have no idea why the test is failing only on centOS

@frozencemetery
Copy link
Member

Does it work for you locally on centos?

@tengcm2015 tengcm2015 force-pushed the ext_rfc4178 branch 3 times, most recently from 5cfebdb to c55631b Compare June 24, 2019 18:13
RFC 4178 provides two support API calls that enable the caller to
manipulate the set of acceptable security mechanisms used in SPNEGO
protocol; for the given credentials, the gss_get_neg_mechs call is
used to indicate the current set of security mechanisms available
for negotiation, and the gss_set_neg_mechs call is used to specify
the set of security mechanisms avaiable for negotiation. Since
gss_get_neg_mechs is not implemented by MIT krb5, we are only
implementing the raw interface for the latter call. Note that
although RFC 4178 did not specify that the mech_set argument cannot
be an empty set, we are forcing it to be non empty in the low-level
API here since passing an empty set will always trigger an error in
MIT krb5 implementation.
@tengcm2015
Copy link
Contributor Author

Well, I guess the problem was that centOS had no ntmlssp, but since I created a credential with all_mech, it just created a credential with some other mech rather than skipping the test.

@frozencemetery frozencemetery merged commit 2347e3f into pythongssapi:master Jul 8, 2019
@tengcm2015 tengcm2015 deleted the ext_rfc4178 branch July 8, 2019 19:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants