Expose mechanisms in the high-level API #126

cipherboy · 2017-07-25T18:25:39Z

This creates a new class, Mechanism, for inquiring
information about a mechanism. This includes support
for RFCs 5587 and 5801. As Mechanism derives from OID,
it is compatible with all places that accept a mech
by OID.

Points of discussion (mechs):

Fallback in mech_name(): what mechs should we support? We could hard-code the three MIT/KRB5 OIDs (krb5/iakerb/spnego) with respective names for the fallback when RFC5801 is missing, but I'd like thoughts on that.
~~Am I missing functions that significantly depend on a OID?~~
Do we want other high-level calls to default to exposing Mechanisms vs. raw OIDs? e.g., init_sec_context?
~~Revising init() -- I wasn't able to get a call to super().__init__ to work like was done with creds. Thoughts would be appreciated.~~

Regarding mech_attrs, I'm not yet sure how I want to handle that. display_mech_attr is about the only useful function I can see that depends on mech_attrs, and querying for a list of mech_attrs isn't trivial--It'd likely involve a call to indicate_mechs+inquire_attrs_for_mechs. I'm not really seeing a use for having mech_attrs as a class. Thoughts?

DirectXMan12 · 2017-07-25T18:58:56Z

gssapi/mechs.py

@@ -0,0 +1,180 @@
+from gssapi.raw import oids as roids


DirectXMan12 · 2017-07-25T19:01:47Z

gssapi/mechs.py

+    It inherits from the low-level GSSAPI :class:`~gssapi.raw.oids.OID` class,
+    and thus can be used with both low-level and high-level API calls.
+
+    The functionality of this class is extended by RFC 5587 and RFC 5801.


don't put this here, just put it in the method definitions

DirectXMan12 · 2017-07-25T19:02:22Z

gssapi/mechs.py

+    @property
+    def sasl_name(self):
+        """
+        Get the SASL name for the mechanism; depends on RFC 5801


use requires-ext to indicate RFC and extension requirements (e.g. https://github.com/cipherboy/python-gssapi/blob/4f91022e61a1e3c948313e4e4618dc92436183d6/gssapi/creds.py#L162)

while you're doing updates, remove the "; depends on RFC 5801" -- just the requires-ext tag below is fine.

DirectXMan12 · 2017-07-25T19:05:11Z

gssapi/mechs.py

+
+class Mechanism(roids.OID):
+    """
+    GSSAPI Mechanism


nitpicky: "A GSSAPI Mechanism"

DirectXMan12 · 2017-07-25T19:28:51Z

gssapi/mechs.py

+    _sasl_name = None
+    _mech_attrs = None
+
+    def __init__(self, cpy=None, elements=None):


we definitely should fix this before merging...

because it's a C-based object, you actually need to override at least __new__ (and then __init__ if you want -- see the other objects in the high-level API).

DirectXMan12 · 2017-07-25T19:40:09Z

gssapi/mechs.py

+        from_oids(oids)
+        Converts a list of OIDs into a set of Mechanisms
+        """
+        return set(map(lambda mech: Mechanism(cpy=mech), oids))


... mech: cls(cpy=mech)...

DirectXMan12 · 2017-07-25T19:45:11Z

gssapi/mechs.py

+            raise NotImplementedError("Your GSSAPI implementation does not "
+                                      "have support for RFC 5801")
+        if self._sasl_name is None:
+            self._sasl_name = rfc5801.inquire_saslname_for_mech(self)


We've generally opted to avoid caching unless necessary, so you should add a comment on why the caching is useful.

DirectXMan12 · 2017-07-25T19:45:27Z

gssapi/mechs.py

+        """
+        Get the SASL name for the mechanism; depends on RFC 5801
+        """
+        self._query_saslname()


self._saslname.sasl_mech_name

(don't call the method and then rely on the cache directly -- just use whatever is returned from the method/property)

DirectXMan12 · 2017-07-25T19:45:44Z

gssapi/mechs.py

+        """
+        return rmisc.inquire_names_for_mech(self)
+
+    def _query_saslname(self):


make this a property, and then use it as such.

DirectXMan12 · 2017-07-25T19:50:46Z

gssapi/mechs.py

+
+        m = rfc5801.inquire_mech_for_saslname(n)
+
+        return Mechanism(m)


cipherboy · 2017-07-26T13:44:38Z

Thanks for the detailed review and the help tracking those issues down yesterday. :)

I added inquire_mechs_for_name() support as a missing mechanism-related function.

TODO:

~~Python 2/3 str handling.~~
~~Fallback mech names for pretty printing~~
mech_attrs

frozencemetery · 2017-07-26T14:59:31Z

Hardcoding anything with a standard is fine in principle, but kind of defeats the point of calling into the GSSAPI there at all... I'm not decided on that.

cipherboy · 2017-07-26T17:14:11Z

On second thought, I don't know if there is value to this. Both MIT and Heimdal support the RFC 5801 extensions, so mech_name will be available under both. Instead, I'd just make this PR conditional on RFC 5801 landing.

DirectXMan12 · 2017-07-31T17:11:53Z

gssapi/mechs.py

+        :requires-ext:`rfc5801`
+        """
+        self._query_saslname()
+        return self._query_saslname.mech_name.decode('UTF-8')


remove this call entirely. I don't think we want to fall back to hard-coded stuff anyway.

DirectXMan12 · 2017-07-31T17:13:35Z

gssapi/mechs.py

+        """
+        base = self.dotted_form
+        if rfc5801 is not None:
+            base = "%s %s" % (self._query_saslname.mech_name.decode('UTF-8'),


It should look like <Mechanism Foo (1.2.3.4)> or just <Mechanism (1.2.3.4)> query_saslname is unavailable

DirectXMan12 · 2017-07-31T18:20:30Z

gssapi/mechs.py

+
+        :requires-ext:`rfc5801`
+        """
+        self._query_saslname()


redundant call

DirectXMan12 · 2017-07-31T18:21:22Z

gssapi/mechs.py

+        Get a list of all mechanisms supported by GSSAPI
+        """
+        mechs = rmisc.indicate_mechs()
+        return [cls(mech) for mech in mechs]


collapse to (cls(mech) for mech in metchs)

DirectXMan12 · 2017-07-31T18:22:21Z

gssapi/mechs.py

+            raise NotImplementedError("Your GSSAPI implementation does not "
+                                      "have support for RFC 5801")
+        n = name
+        if type(n) is not bytes:


use isinstance

cipherboy · 2017-08-01T18:36:27Z

gssapi/mechs.py

+        if issubclass(str, six.text_type):
+            base = bytes(base, _utils._get_encoding())
+        else:
+            base = bytes(base)


Since dotted_form returns a string, this needs to be cast to bytes for python2 compatibility. However, python3 forces two arguments to bytes, one of which is the encoding. Suggestions on how to make this better?

it should be fine as

if isinstance(base, six.text_type): base = base.encode(_utils._get_encoding())

DirectXMan12

comments on the __bytes__ changes, otherwise seems good.

DirectXMan12 · 2017-08-10T19:28:53Z

gssapi/raw/oids.pyx

@@ -120,11 +132,15 @@ cdef class OID:
        if self._free_on_dealloc:
            free(self.raw_oid.elements)

-    def __bytes__(self):
+    @property
+    def byte_form(self):


err... why is this necessary? Why not just leave it as __bytes__?

DirectXMan12 · 2017-08-10T19:29:49Z

gssapi/mechs.py

+    def __unicode__(self):
+        return self.__bytes__().decode(_utils._get_encoding())
+
+    def __bytes__(self):


hmm... I'd just leave __bytes__ as-is. Let in inherit from the low-level API for compatibility with OIDs -- some things might expect OID bytes when calling __bytes__ -- we shouldn't change that part of the API just because you're working Mechanisms.

then put the "string form as bytes" in a separate private method (like _byte_desc).

cipherboy · 2017-08-11T12:38:40Z

Okie dokie, this is updated as well. In retrospect, not changing the __bytes__ interface was probably a good idea. :)

DirectXMan12

a few more nits.

DirectXMan12 · 2017-09-05T18:06:44Z

gssapi/mechs.py

+        if issubclass(str, six.text_type):
+            base = bytes(base, _utils._get_encoding())
+        else:
+            base = bytes(base)


it should be fine as

if isinstance(base, six.text_type): base = base.encode(_utils._get_encoding())

DirectXMan12 · 2017-09-05T18:07:30Z

gssapi/mechs.py

+    @property
+    def sasl_name(self):
+        """
+        Get the SASL name for the mechanism; depends on RFC 5801


while you're doing updates, remove the "; depends on RFC 5801" -- just the requires-ext tag below is fine.

DirectXMan12 · 2017-09-05T18:14:02Z

gssapi/mechs.py

+                                      "have support for RFC 5801")
+        n = name
+        if not isinstance(n, bytes):
+            n = str(n).encode()


The pattern we generally use here is

if isinstance(name, six.text_type): name = name.encode(_utils._get_encoding())

See the gssapi/names.py (at the bottom) for example

DirectXMan12 · 2017-09-05T18:16:12Z

gssapi/tests/test_high_level.py

+            s = str(mech)
+            s.shouldnt_be_empty()
+            s.should_be_a(str)
+            s[0].shouldnt_be('<')


this is a fairly fragile line here. I'd just remove it, unless the SASL names are guaranteed to not begin with "<"...

DirectXMan12 · 2017-09-05T19:03:46Z

gssapi/tests/test_high_level.py

+                e = list(gssmechs.Mechanism.from_attrs(m_except=[attr]))
+
+                count = len(i) + len(e)
+                count.should_be(c)


(len(from_desired) + len(from_except)).should_be(c) so we get a good error message like len(from_desired) + len(from_except) should be 37.

DirectXMan12 · 2017-09-05T19:04:08Z

gssapi/raw/oids.pyx

@@ -61,6 +61,9 @@ cdef class OID:
        self._free_on_dealloc = True
        return 0

+    def _copy_oid(self, OID other):


this doesn't seem to be needed any more

When constructing via a the cpy parameter, cpy would maintain ownership; if cpy._free_on_dealloc was set to true, the new object could have memory freed while still maintaining a reference to it. This fixes the issue by having the new object take ownership. To perform a memory copy, use the elements argument to the constructor. Signed-off-by: Alexander Scheel <ascheel@redhat.com>

This introduces a new property, dotted_form, for querying the dotted form of the OID. Signed-off-by: Alexander Scheel <ascheel@redhat.com>

DirectXMan12

minor nits (mostly doc-related) inline, otherwise looks good

DirectXMan12 · 2018-03-02T20:11:25Z

gssapi/mechs.py

+        return super(Mechanism, cls).__new__(cls, cpy, elements)
+
+    @property
+    def names(self):


this should probably be name_types, to be clearer -- we use the term name_type elsewhere in the high-level API

DirectXMan12 · 2018-03-02T20:26:27Z

gssapi/mechs.py

+    @property
+    def description(self):
+        """
+        Get the description of the mechanism; depends on RFC 5801


you can remove depends on RFC 5801, since you have the requires-ext tag.

ditto for the cases below

Sorry, should've caught those sooner if I were thinking when I updated the PR.

DirectXMan12 · 2018-03-02T20:27:00Z

gssapi/mechs.py

+    @property
+    def known_attrs(self):
+        """
+        Get the known attributes of the mechanism; depends on RFC 5587


You should document what form an "attribute" takes.

DirectXMan12 · 2018-03-02T20:27:49Z

gssapi/mechs.py

+    @classmethod
+    def all_mechs(cls):
+        """
+        all_mechs()


no need for the explicit signature here.

ditto below

DirectXMan12 · 2018-03-02T20:29:56Z

gssapi/mechs.py

+        """
+        from_attrs
+        Get a generator of mechanisms supporting the specified attributes. See
+        RFC 5587's indicate_mechs_by_attrs for more information.


link to indicate_mechs_by_attrs using the correct documentation tag (IIRC, it's something like :func:).

This creates a new class, Mechanism, for inquiring information about a mechanism. This includes support for RFCs 5587 and 5801. As Mechanism derives from OID, it is compatible with all places that accept a mech by OID. Signed-off-by: Alexander Scheel <ascheel@redhat.com> Signed-off-by: Alexander Scheel <alexander.m.scheel@gmail.com>

cipherboy · 2018-03-05T16:41:41Z

gssapi/mechs.py

+    @property
+    def attrs(self):
+        """
+        Get the attributes of the mechanism; returns a set of OIDs ([OID])


Leaving this open as I'm not sure if this is the desired format.

Per IRC discussion with @DirectXMan12 this appears to be in the correct format.

cipherboy · 2018-04-16T15:08:28Z

@DirectXMan12 -- any new feedback would be appreciated. :)

cipherboy · 2018-08-16T17:23:14Z

@DirectXMan12 friendly bump. 🐈

DirectXMan12 · 2018-08-16T18:43:25Z

/me attempts to page this back into RAM

DirectXMan12 · 2018-08-16T18:55:37Z

Looks like my last comments were just around docs, so assuming past me's review was fairly thorough (I recall it being so), I'm going to merge this.

DirectXMan12 suggested changes Jul 25, 2017

View reviewed changes

cipherboy force-pushed the mechs branch 3 times, most recently from 387543c to acc28ca Compare July 26, 2017 13:38

cipherboy force-pushed the mechs branch 2 times, most recently from 9dc24ac to afd68bf Compare July 27, 2017 17:29

DirectXMan12 suggested changes Jul 31, 2017

View reviewed changes

cipherboy force-pushed the mechs branch 4 times, most recently from 5f46d74 to 44a0071 Compare August 1, 2017 18:33

cipherboy commented Aug 1, 2017

View reviewed changes

cipherboy force-pushed the mechs branch from 44a0071 to ac7776e Compare August 1, 2017 18:39

DirectXMan12 suggested changes Aug 10, 2017

View reviewed changes

cipherboy force-pushed the mechs branch 2 times, most recently from e40a67f to 9566a39 Compare August 11, 2017 12:08

DirectXMan12 suggested changes Sep 5, 2017

View reviewed changes

frozencemetery added the needs revision label Dec 4, 2017

cipherboy added 2 commits March 1, 2018 12:15

OIDs: Add dotted_form properties

e0b8084

This introduces a new property, dotted_form, for querying the dotted form of the OID. Signed-off-by: Alexander Scheel <ascheel@redhat.com>

cipherboy force-pushed the mechs branch 3 times, most recently from bc994a6 to c8e80b1 Compare March 1, 2018 19:03

DirectXMan12 suggested changes Mar 2, 2018

View reviewed changes

cipherboy force-pushed the mechs branch from c8e80b1 to 535e67a Compare March 5, 2018 16:37

cipherboy commented Mar 5, 2018

View reviewed changes

DirectXMan12 approved these changes Aug 16, 2018

View reviewed changes

DirectXMan12 merged commit fe6d5cc into pythongssapi:master Aug 16, 2018

Expose mechanisms in the high-level API #126

Expose mechanisms in the high-level API #126

Conversation

cipherboy commented Jul 25, 2017 • edited Loading

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

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 26, 2017 • edited Loading

frozencemetery commented Jul 26, 2017

cipherboy commented Jul 26, 2017 • edited Loading

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 left a comment • edited Loading

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 Aug 11, 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 left a comment • edited Loading

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

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 Apr 16, 2018

cipherboy commented Aug 16, 2018

DirectXMan12 commented Aug 16, 2018

DirectXMan12 commented Aug 16, 2018

cipherboy commented Jul 25, 2017 •

edited

Loading

cipherboy commented Jul 26, 2017 •

edited

Loading

cipherboy commented Jul 26, 2017 •

edited

Loading

DirectXMan12 left a comment •

edited

Loading

DirectXMan12 left a comment •

edited

Loading