Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

Expose mechanisms in the high-level API #126

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

Merged
DirectXMan12 merged 3 commits into pythongssapi:master from cipherboy:mechs
Aug 16, 2018

Conversation

Copy link
Contributor

@cipherboy cipherboy commented Jul 25, 2017 •
edited
Loading

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?

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

@DirectXMan12 DirectXMan12 Jul 25, 2017

Choose a reason for hiding this comment

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

💪

frozencemetery reacted with thumbs up emoji
gssapi/mechs.py Outdated
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.
Copy link
Member

@DirectXMan12 DirectXMan12 Jul 25, 2017

Choose a reason for hiding this comment

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

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

gssapi/mechs.py Outdated
@property
def sasl_name(self):
"""
Get the SASL name for the mechanism; depends on RFC 5801
Copy link
Member

@DirectXMan12 DirectXMan12 Jul 25, 2017

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@cipherboy cipherboy Jul 26, 2017

Choose a reason for hiding this comment

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

(done)

Copy link
Member

@DirectXMan12 DirectXMan12 Sep 5, 2017

Choose a reason for hiding this comment

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

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

gssapi/mechs.py Outdated

class Mechanism(roids.OID):
"""
GSSAPI Mechanism
Copy link
Member

@DirectXMan12 DirectXMan12 Jul 25, 2017

Choose a reason for hiding this comment

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

nitpicky: "A GSSAPI Mechanism"

gssapi/mechs.py Outdated
_sasl_name = None
_mech_attrs = None

def __init__(self, cpy=None, elements=None):
Copy link
Member

@DirectXMan12 DirectXMan12 Jul 25, 2017

Choose a reason for hiding this comment

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

we definitely should fix this before merging...

Copy link
Member

@DirectXMan12 DirectXMan12 Jul 25, 2017

Choose a reason for hiding this comment

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

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).

gssapi/mechs.py Outdated
from_oids(oids)
Converts a list of OIDs into a set of Mechanisms
"""
return set(map(lambda mech: Mechanism(cpy=mech), oids))
Copy link
Member

@DirectXMan12 DirectXMan12 Jul 25, 2017

Choose a reason for hiding this comment

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

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

gssapi/mechs.py Outdated
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)
Copy link
Member

@DirectXMan12 DirectXMan12 Jul 25, 2017

Choose a reason for hiding this comment

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

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

gssapi/mechs.py Outdated
"""
Get the SASL name for the mechanism; depends on RFC 5801
"""
self._query_saslname()
Copy link
Member

@DirectXMan12 DirectXMan12 Jul 25, 2017

Choose a reason for hiding this comment

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

self._saslname.sasl_mech_name

Copy link
Member

@DirectXMan12 DirectXMan12 Jul 25, 2017

Choose a reason for hiding this comment

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

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

gssapi/mechs.py Outdated
"""
return rmisc.inquire_names_for_mech(self)

def _query_saslname(self):
Copy link
Member

@DirectXMan12 DirectXMan12 Jul 25, 2017

Choose a reason for hiding this comment

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

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

gssapi/mechs.py Outdated

m = rfc5801.inquire_mech_for_saslname(n)

return Mechanism(m)
Copy link
Member

@DirectXMan12 DirectXMan12 Jul 25, 2017

Choose a reason for hiding this comment

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

cls(m)

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

cipherboy commented Jul 26, 2017 •
edited
Loading

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

Copy link
Member

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.

Copy link
Contributor Author

cipherboy commented Jul 26, 2017 •
edited
Loading

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.

@cipherboy cipherboy force-pushed the mechs branch 2 times, most recently from 9dc24ac to afd68bf Compare July 27, 2017 17:29
gssapi/mechs.py Outdated
:requires-ext:`rfc5801`
"""
self._query_saslname()
return self._query_saslname.mech_name.decode('UTF-8')
Copy link
Member

@DirectXMan12 DirectXMan12 Jul 31, 2017

Choose a reason for hiding this comment

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

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

frozencemetery reacted with thumbs up emoji
gssapi/mechs.py Outdated
"""
base = self.dotted_form
if rfc5801 is not None:
base = "%s %s" % (self._query_saslname.mech_name.decode('UTF-8'),
Copy link
Member

@DirectXMan12 DirectXMan12 Jul 31, 2017

Choose a reason for hiding this comment

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

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

gssapi/mechs.py Outdated
:requires-ext:`rfc5801`
"""
self._query_saslname()
Copy link
Member

@DirectXMan12 DirectXMan12 Jul 31, 2017

Choose a reason for hiding this comment

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

redundant call

gssapi/mechs.py Outdated
Get a list of all mechanisms supported by GSSAPI
"""
mechs = rmisc.indicate_mechs()
return [cls(mech) for mech in mechs]
Copy link
Member

@DirectXMan12 DirectXMan12 Jul 31, 2017

Choose a reason for hiding this comment

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

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

gssapi/mechs.py Outdated
raise NotImplementedError("Your GSSAPI implementation does not "
"have support for RFC 5801")
n = name
if type(n) is not bytes:
Copy link
Member

@DirectXMan12 DirectXMan12 Jul 31, 2017

Choose a reason for hiding this comment

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

use isinstance

@cipherboy cipherboy force-pushed the mechs branch 4 times, most recently from 5f46d74 to 44a0071 Compare August 1, 2017 18:33
gssapi/mechs.py Outdated
if issubclass(str, six.text_type):
base = bytes(base, _utils._get_encoding())
else:
base = bytes(base)
Copy link
Contributor Author

@cipherboy cipherboy Aug 1, 2017

Choose a reason for hiding this comment

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

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?

Copy link
Member

@DirectXMan12 DirectXMan12 Sep 5, 2017

Choose a reason for hiding this comment

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

it should be fine as

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

Copy link
Member

@DirectXMan12 DirectXMan12 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

comments on the __bytes__ changes, otherwise seems good.


def __bytes__(self):
@property
def byte_form(self):
Copy link
Member

@DirectXMan12 DirectXMan12 Aug 10, 2017

Choose a reason for hiding this comment

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

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

gssapi/mechs.py Outdated
def __unicode__(self):
return self.__bytes__().decode(_utils._get_encoding())

def __bytes__(self):
Copy link
Member

@DirectXMan12 DirectXMan12 Aug 10, 2017

Choose a reason for hiding this comment

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

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.

Copy link
Member

@DirectXMan12 DirectXMan12 Aug 10, 2017

Choose a reason for hiding this comment

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

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

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

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

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.

a few more nits.

gssapi/mechs.py Outdated
if issubclass(str, six.text_type):
base = bytes(base, _utils._get_encoding())
else:
base = bytes(base)
Copy link
Member

@DirectXMan12 DirectXMan12 Sep 5, 2017

Choose a reason for hiding this comment

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

it should be fine as

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

gssapi/mechs.py Outdated
@property
def sasl_name(self):
"""
Get the SASL name for the mechanism; depends on RFC 5801
Copy link
Member

@DirectXMan12 DirectXMan12 Sep 5, 2017

Choose a reason for hiding this comment

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

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

gssapi/mechs.py Outdated
"have support for RFC 5801")
n = name
if not isinstance(n, bytes):
n = str(n).encode()
Copy link
Member

@DirectXMan12 DirectXMan12 Sep 5, 2017

Choose a reason for hiding this comment

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

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

s = str(mech)
s.shouldnt_be_empty()
s.should_be_a(str)
s[0].shouldnt_be('<')
Copy link
Member

@DirectXMan12 DirectXMan12 Sep 5, 2017

Choose a reason for hiding this comment

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

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

e = list(gssmechs.Mechanism.from_attrs(m_except=[attr]))

count = len(i) + len(e)
count.should_be(c)
Copy link
Member

@DirectXMan12 DirectXMan12 Sep 5, 2017

Choose a reason for hiding this comment

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

(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.

self._free_on_dealloc = True
return 0

def _copy_oid(self, OID other):
Copy link
Member

@DirectXMan12 DirectXMan12 Sep 5, 2017

Choose a reason for hiding this comment

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

this doesn't seem to be needed any more

cipherboy added 2 commits March 1, 2018 12:15 
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>
@cipherboy cipherboy force-pushed the mechs branch 3 times, most recently from bc994a6 to c8e80b1 Compare March 1, 2018 19:03
Copy link
Member

@DirectXMan12 DirectXMan12 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

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

gssapi/mechs.py Outdated
return super(Mechanism, cls).__new__(cls, cpy, elements)

@property
def names(self):
Copy link
Member

@DirectXMan12 DirectXMan12 Mar 2, 2018

Choose a reason for hiding this comment

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

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

gssapi/mechs.py Outdated
@property
def description(self):
"""
Get the description of the mechanism; depends on RFC 5801
Copy link
Member

@DirectXMan12 DirectXMan12 Mar 2, 2018

Choose a reason for hiding this comment

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

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

Copy link
Member

@DirectXMan12 DirectXMan12 Mar 2, 2018

Choose a reason for hiding this comment

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

ditto for the cases below

Copy link
Contributor Author

@cipherboy cipherboy Mar 5, 2018

Choose a reason for hiding this comment

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

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

gssapi/mechs.py Outdated
@property
def known_attrs(self):
"""
Get the known attributes of the mechanism; depends on RFC 5587
Copy link
Member

@DirectXMan12 DirectXMan12 Mar 2, 2018

Choose a reason for hiding this comment

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

You should document what form an "attribute" takes.

gssapi/mechs.py Outdated
@classmethod
def all_mechs(cls):
"""
all_mechs()
Copy link
Member

@DirectXMan12 DirectXMan12 Mar 2, 2018

Choose a reason for hiding this comment

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

no need for the explicit signature here.

Copy link
Member

@DirectXMan12 DirectXMan12 Mar 2, 2018

Choose a reason for hiding this comment

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

ditto below

gssapi/mechs.py Outdated
"""
from_attrs
Get a generator of mechanisms supporting the specified attributes. See
RFC 5587's indicate_mechs_by_attrs for more information.
Copy link
Member

@DirectXMan12 DirectXMan12 Mar 2, 2018

Choose a reason for hiding this comment

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

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>
@property
def attrs(self):
"""
Get the attributes of the mechanism; returns a set of OIDs ([OID])
Copy link
Contributor Author

@cipherboy cipherboy Mar 5, 2018

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@cipherboy cipherboy Mar 5, 2018

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

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

Copy link
Contributor Author

@DirectXMan12 friendly bump. 🐈

Copy link
Member

/me attempts to page this back into RAM

Copy link
Member

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 DirectXMan12 merged commit fe6d5cc into pythongssapi:master Aug 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
1 more reviewer

@DirectXMan12 DirectXMan12 DirectXMan12 approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /