-
Notifications
You must be signed in to change notification settings - Fork 49
Implemented RFC 4178 extension #176
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
Conversation
There was a problem hiding this 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/
gssapi/raw/ext_rfc4178.pyx
Outdated
|
||
def set_neg_mechs(Creds cred_handle not None, mechs not None): | ||
""" | ||
set_neg_mechs(Creds cred_handle not None, mechs=None) |
There was a problem hiding this comment.
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
.
gssapi/raw/ext_rfc4178.pyx
Outdated
const gss_OID_set mechs) nogil | ||
|
||
|
||
def set_neg_mechs(Creds cred_handle not None, mechs not None): |
There was a problem hiding this comment.
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.
gssapi/raw/ext_rfc4178.pyx
Outdated
""" | ||
set_neg_mechs(Creds cred_handle not None, mechs=None) | ||
|
||
This allows callers to specify the set of security mechanisms that |
There was a problem hiding this comment.
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.
gssapi/raw/ext_rfc4178.pyx
Outdated
relative preference. | ||
|
||
Args: | ||
cred_handle (Creds): the Credentials to query |
There was a problem hiding this comment.
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).
gssapi/raw/ext_rfc4178.pyx
Outdated
|
||
Args: | ||
cred_handle (Creds): the Credentials to query | ||
mechs ([MechType]): the desired set of mechanisms that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe shorten this too
gssapi/raw/ext_rfc4178.pyx
Outdated
mechs ([MechType]): the desired set of mechanisms that | ||
may be negotiated with the credentials | ||
Returns: | ||
None (If the operation completed successfully) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parenthetical is unnecessary.
gssapi/raw/ext_rfc4178.pyx
Outdated
if maj_stat == GSS_S_COMPLETE: | ||
return None | ||
else: | ||
print("error: %d:%d", maj_stat, min_stat) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leftover debugging line?
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 |
8f77bf7
to
ef0f04a
Compare
my context for this is mostly lost to time. Which part did you want me to take a look at? The extension negotiation? |
I was just hoping for code review :) |
@tengcm2015 Hey, this is still waiting on you. Any updates? |
@frozencemetery I fixed what is mentioned in the code review, but have no idea why the test is failing only on centOS |
Does it work for you locally on centos? |
5cfebdb
to
c55631b
Compare
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.
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. |
reference issue #50