Implement support for GSSAPI extension RFC 5587 #121

cipherboy · 2017-07-14T18:53:00Z

RFC 5587 provides extended mech inquiry calls to GSSAPI.
This adds the ability to indicate mechs by their
mech attrs, along with determining the attrs supported
by a mech. These calls are provided as a part of
the raw interface and are not exposed in the high-level
interface due to not having objects for mechs or attrs.

Thanks!

frozencemetery

Thanks for the patch! Some comments inline.

frozencemetery · 2017-07-14T19:35:33Z

gssapi/tests/test_raw.py

+        mechs.shouldnt_be_empty()
+        mechs.should_be_a(set)
+
+        last_mech = None


Can you just use mechs[-1] instead of repeatedly updating a variable for this?

mechs is a set; sets don't support indexing.

So it is. Please move the update call to the top of the loop in that case.

frozencemetery · 2017-07-14T19:41:20Z

gssapi/tests/test_raw.py

+
+            last_mech = mech
+
+        last_mech.shouldnt_be_none


Isn't this check redundant with the one in the loop?

frozencemetery · 2017-07-14T19:42:02Z

gssapi/tests/test_raw.py

+        last_attr = None
+
+        for mech in mechs:
+            mech.shouldnt_be_none


You need to call your functions, in this line and elsewhere

frozencemetery · 2017-07-14T19:43:03Z

gssapi/tests/test_raw.py

+            last_mech = mech
+
+        last_mech.shouldnt_be_none
+        last_attr.shouldnt_be_none


Similarly, why is this checked here?

cipherboy · 2017-07-14T20:18:54Z

@frozencemetery Updated!

frozencemetery

Will merge once we unbreak CI

DirectXMan12 · 2017-07-17T14:52:03Z

😢 please put a full description on the PR body and commit message. For example:

Implement support for GSSAPI extension RFC 5587

RFC 5587 adds extended mechanism inquiry APIs to the GSSAPI.
This allows users to query mechanisms using a number of attributes associated with those
mechanisms.   This is surfaced directly in the low-level API, but is not surfaced in the
high-level API because $REASONS.

DirectXMan12

comments inline.

DirectXMan12 · 2017-07-17T14:53:53Z

gssapi/raw/ext_rfc5587.pyx

+    if mech is not None:
+        m = &mech.raw_oid
+
+    maj_stat = gss_inquire_attrs_for_mech(&min_stat, m, &mech_attrs,


calls to the GSSAPI functions themselves should generally drop the GIL while being called with nogil:.

Ah that annotation makes sense now. :) I notice that e.g., rfc5588 has a "nogil" on the cdef -- do you want that as well?

yeah, you'd need to add that

DirectXMan12 · 2017-07-17T14:57:28Z

gssapi/raw/ext_rfc5587.pyx

+                                     &long_desc)
+
+    if maj_stat == GSS_S_COMPLETE:
+        out_name = name.value[:name.length].decode("UTF-8")


we don't decode in the low-level API -- we just return bytes.

DirectXMan12 · 2017-07-17T14:58:28Z

gssapi/tests/test_raw.py

+                mech_attr.should_be_a(gb.OID)
+
+                display_out = gb.display_mech_attr(mech_attr)
+                display_out.name.shouldnt_be_none()


can we test a specific attr to make sure that's right, too?

Done -- see here.

DirectXMan12 · 2017-07-17T17:09:35Z

Ok, last thing: you should have doc strings on the functions. Otherwise, LGTM.

cipherboy · 2017-07-17T19:48:24Z

Thanks @DirectXMan12! :)

DirectXMan12

small doc nit, and then you're good

DirectXMan12 · 2017-07-18T18:44:59Z

gssapi/raw/ext_rfc5587.pyx

+
+def inquire_attrs_for_mech(OID mech):
+    """
+    inquire_attrs_for_mech(OID mech)


now I'm nitpicking, but the method signature in the docstring should look like normal Python (so no types in the docstring signature)

Nitpicking welcome :)

DirectXMan12 · 2017-07-18T18:45:07Z

gssapi/raw/ext_rfc5587.pyx

+
+def display_mech_attr(OID attr):
+    """
+    display_mech_attrs(OID attr)


cipherboy · 2017-07-18T19:12:28Z

Are we looking for higher level changes at this time to include mechs / mech_attrs as a class of their own? It seems like they'd be a storage class with only a few helper functions, so I'm not sure there's a need.

RFC 5587 provides extended mech inquiry calls to GSSAPI. This adds the ability to indicate mechs by their mech attrs, along with determining the attrs supported by a mech. These calls are provided as a part of the raw interface and are not exposed in the high-level interface due to not having objects for mechs or attrs. Signed-off-by: Alexander Scheel <ascheel@redhat.com>

cipherboy · 2017-07-19T13:21:26Z

I've added comments to the tests and added RFC 5587 support to the README.

DirectXMan12 · 2017-07-21T17:47:18Z

LGTM. We may have to revisit the exact message checking test in the future, but I'm fine to merge now.

cipherboy force-pushed the rfc5587 branch 2 times, most recently from 6569bce to 8871dc8 Compare July 14, 2017 19:08

frozencemetery reviewed Jul 14, 2017

View reviewed changes

cipherboy force-pushed the rfc5587 branch 2 times, most recently from f7c51b5 to 0b53bc2 Compare July 14, 2017 20:09

frozencemetery approved these changes Jul 14, 2017

View reviewed changes

DirectXMan12 suggested changes Jul 17, 2017

View reviewed changes

cipherboy force-pushed the rfc5587 branch 2 times, most recently from 590d016 to 11180ad Compare July 17, 2017 15:59

cipherboy force-pushed the rfc5587 branch 3 times, most recently from f726863 to 9e60f3d Compare July 17, 2017 19:41

DirectXMan12 suggested changes Jul 18, 2017

View reviewed changes

cipherboy force-pushed the rfc5587 branch from 9e60f3d to 345fb85 Compare July 18, 2017 19:09

cipherboy force-pushed the rfc5587 branch from 345fb85 to 66a7107 Compare July 19, 2017 13:15

frozencemetery assigned DirectXMan12 Jul 20, 2017

DirectXMan12 approved these changes Jul 21, 2017

View reviewed changes

DirectXMan12 merged commit ae99d0c into pythongssapi:master Jul 21, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement support for GSSAPI extension RFC 5587 #121

Implement support for GSSAPI extension RFC 5587 #121

cipherboy commented Jul 14, 2017 •

edited

Loading

frozencemetery left a comment

frozencemetery Jul 14, 2017

cipherboy Jul 14, 2017

frozencemetery Jul 14, 2017

frozencemetery Jul 14, 2017

frozencemetery Jul 14, 2017

frozencemetery Jul 14, 2017

cipherboy commented Jul 14, 2017

frozencemetery left a comment

DirectXMan12 commented Jul 17, 2017

DirectXMan12 left a comment

DirectXMan12 Jul 17, 2017

cipherboy Jul 17, 2017

DirectXMan12 Jul 17, 2017

DirectXMan12 Jul 17, 2017

DirectXMan12 Jul 17, 2017

cipherboy Jul 20, 2017

DirectXMan12 commented Jul 17, 2017

cipherboy commented Jul 17, 2017

DirectXMan12 left a comment

DirectXMan12 Jul 18, 2017

cipherboy Jul 18, 2017

DirectXMan12 Jul 18, 2017

cipherboy commented Jul 18, 2017 •

edited

Loading

cipherboy commented Jul 19, 2017

DirectXMan12 commented Jul 21, 2017

Implement support for GSSAPI extension RFC 5587 #121

Implement support for GSSAPI extension RFC 5587 #121

Conversation

cipherboy commented Jul 14, 2017 • edited Loading

frozencemetery left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cipherboy commented Jul 14, 2017

frozencemetery left a comment

Choose a reason for hiding this comment

DirectXMan12 commented Jul 17, 2017

DirectXMan12 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DirectXMan12 commented Jul 17, 2017

cipherboy commented Jul 17, 2017

DirectXMan12 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cipherboy commented Jul 18, 2017 • edited Loading

cipherboy commented Jul 19, 2017

DirectXMan12 commented Jul 21, 2017

cipherboy commented Jul 14, 2017 •

edited

Loading

cipherboy commented Jul 18, 2017 •

edited

Loading