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.

Sign up for GitHub

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

Jump to bottom

Implement remaining GGF extensions #151

Merged
merged 1 commit into from
Apr 6, 2018
Merged

Implement remaining GGF extensions #151

merged 1 commit into from
Apr 6, 2018

Conversation

jborean93
Copy link
Contributor

This PR implements the remaining GGF extensions #51, excluding the gss_{import,export}_cred variants as they are not implemented as per the GGF spec in both MIT krb5 and Heimdal. This PR adds in the gss_set_cred_option and gss_set_sec_context_option functions.

While gss_set_cred_option doesn't seem to be a function that was part of the GGF draft, most implementations bundle it together with the GGF gss_set_sec_context_option and so I decided to keep it in ext_ggf, please let me know if you want to split them up further.

I also have a commented out test for gss_set_sec_context_option, it requires gss-ntlmssp and unfortunately I was unable to get that working in the docker container tests. Not sure if I needed to do anything extra (apart from installing it from apt/dnf) as it was unable to find the mech when i supplied it as part of the test. For now it is commented out until someone smarter can figure it out.

@DirectXMan12
Copy link
Member

DirectXMan12 commented Mar 23, 2018 •
edited

If gss_set_cred_option could reasonably not be present while still having the rest present, then it should be in a different file. In general, the rule is that we never want a canary check (see setup.py) to pass, but the compile to fail on missing functions or types.

DirectXMan12
DirectXMan12 suggested changes Mar 23, 2018
View reviewed changes
gssapi/raw/ext_ggf.pyx Outdated
Creds: The output credential.

Raises:
GSS_ERROR
Copy link
Member

Choose a reason for hiding this comment

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

This should be the python type (GSSError)

Copy link
Member

Choose a reason for hiding this comment

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

(can you fix that up in the rest of these functions, too? Don't know how that snuck by last time).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, don't know why I wrote them this way.

gssapi/raw/ext_ggf.pyx Outdated
An example of how this can be used would be to reset the NTLM crypto engine
used in gss-ntlmssp. The OID that controls this value is
'1.3.6.1.4.1.7165.655.1.3' and it takes it a byte value that represents
an int32 where 1 reset's the verifier handle and any other int resets the
Copy link
Member

Choose a reason for hiding this comment

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

s/reset's/resets/

gssapi/tests/test_raw.py Outdated
# nothing much we can test here apart from it doesn't fail and the
# id of the return cred is the same as the input one
output_cred = gb.set_cred_option(no_ci_flags_x, creds=orig_cred)
id(orig_cred).should_be(id(output_cred))
Copy link
Member

Choose a reason for hiding this comment

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

oof, I don't like testing the id like that. Just drop that line. It's probably fine to just test that it doesn't error out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do.

gssapi/tests/test_raw.py Outdated
b"\x00")

# TODO: get these tests to detect gss-ntlmssp once it is installed
"""
Copy link
Member

Choose a reason for hiding this comment

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

gss-ntlmssp will be installed as part of #150, so we just need to figure out how to detect it, and get that merged into k5test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good.

DirectXMan12
DirectXMan12 reviewed Mar 23, 2018
View reviewed changes
gssapi/tests/test_raw.py Outdated
# because MIT krb5 doesn't implement any OID's for
# gss_set_sec_context_option, we just need to query any OID and it will
# raise an exception
gb.set_sec_context_option.should_raise(gb.GSSError,
Copy link
Member

Choose a reason for hiding this comment

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

this test feels fragile. Is there anything else we can do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Once the NTLMSSP stuff works in the tests we can call that with an invalid value instead.

@jborean93
Copy link
Contributor Author

Thanks for the review @DirectXMan12, let me know what you would like me to do about the gss_set_sec_context_option tests considering ntlmssp isn't available right now.

@frozencemetery
Copy link
Member

It's there on master now.

@jborean93
Copy link
Contributor Author

jborean93 commented Mar 26, 2018 •
edited

@frozencemetery, I think there is more that needs to be done. I did that locally before and the tests were still unable to pick up the NTLM mech when creating the credentials/context. I'll try it again as maybe I did something wrong but it seems like something needs to be done in the k5test for it to recognize it.

Must have done something wrong the first time, have updated the tests to use ntlmssp.

@jborean93 jborean93 force-pushed the ggf-set_option branch 3 times, most recently from a7489c1 to 726366d Compare March 27, 2018 01:51
DirectXMan12
DirectXMan12 approved these changes Apr 5, 2018
View reviewed changes
Copy link
Member

@DirectXMan12 DirectXMan12 left a comment

Choose a reason for hiding this comment

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

new revisions look good at a glance. Feel free to merge @frozencemetery.

@jborean93 @frozencemetery
Implement gss_set_cred_option() and gss_set_sec_context_option()
13467fb 
These parts of th GGF extensions provide extended support for managing
security contexts and credentials.  In particular, with NTLM, they can
be used to reset the crypto handles using the
GSS_NTLMSSP_RESET_CRYPTO_OID_LENGTH OID.

Draft IETF document for the gss_set_sec_context_option():
https://tools.ietf.org/html/draft-engert-ggf-gss-extensions-00

Draft IETF document for the gss_set_cred_option():
https://tools.ietf.org/html/draft-ietf-kitten-channel-bound-flag-02

Fixes: pythongssapi#51

[rharwood@redhat.com edited commit message]
@frozencemetery frozencemetery merged commit 13467fb into pythongssapi:master Apr 6, 2018
@jborean93 jborean93 deleted the ggf-set_option branch April 7, 2018 00:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DirectXMan12 DirectXMan12 DirectXMan12 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@jborean93 @DirectXMan12 @frozencemetery