cred_store should allow specifying empty values

[abbra]

When relying on KRB5_CLIENT_KTNAME to specify a keytab, it is useful to support empty client_keytab values in the cred store.

FreeIPA has a helper kinit_keytab which uses python-gssapi and when passing a None for keytab there, python-gssapi fails:

$ python3
Python 3.7.4 (default, Jul  9 2019, 16:32:37) 
[GCC 9.1.1 20190503 (Red Hat 9.1.1-1)] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from ipalib.install.kinit import kinit_keytab
>>> cred = kinit_keytab('user@EXAMPLE.COM', None, 'MEMORY:FOOBAR')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib/python3.7/site-packages/ipalib/install/kinit.py", line 47, in kinit_keytab
    cred = gssapi.Credentials(name=name, store=store, usage='initiate')
  File "/usr/lib64/python3.7/site-packages/gssapi/creds.py", line 64, in __new__
    store=store)
  File "/usr/lib64/python3.7/site-packages/gssapi/creds.py", line 148, in acquire
    usage)
  File "gssapi/raw/ext_cred_store.pyx", line 154, in gssapi.raw.ext_cred_store.acquire_cred_from
  File "gssapi/raw/ext_cred_store.pyx", line 86, in gssapi.raw.ext_cred_store.c_create_key_value_set
TypeError: expected bytes, NoneType found
>>> 

This is because of the following code: https://github.com/pythongssapi/python-gssapi/blob/master/gssapi/raw/ext_cred_store.pyx#L68-L88 where I'd suggest skip assignment of None values to avoid the problem I verified that kg_value_from_cred_store() will happily work with NULL values:

    for (i, (k, v)) in enumerate(values.items()):
        res.elements[i].key = k
        if v:
            res.elements[i].value = v

[simo5]

I do not really understand this and the fix looks incomplete as it will cause the cred store to have a key but not a value if I read it right ?

[simo5]

Clarifying: do not understand why you'd pass a client_keytab key at all if you have no value, just do not pass it in ?
We should probably raise an exception if a key without value is passed in as this is illegal in the C interface, afaik and iirc.

[abbra]

@simo5 for released FreeIPA versions, we cannot fix kinit_keytab to work with default client keytab which is the reason I'm asking to add this support here. In fact, C interface accepts happily NULL values for existing keys.

OM_uint32
kg_value_from_cred_store(gss_const_key_value_set_t cred_store,
                         const char *type, const char **value)
{
    OM_uint32 i;

    if (value == NULL)
        return GSS_S_CALL_INACCESSIBLE_WRITE;
    *value = NULL;

    if (cred_store == GSS_C_NO_CRED_STORE)
        return GSS_S_COMPLETE;

    for (i = 0; i < cred_store->count; i++) {
        if (strcmp(cred_store->elements[i].key, type) == 0) {
            if (*value != NULL)
                return GSS_S_DUPLICATE_ELEMENT;
            *value = cred_store->elements[i].value;
        }
    }

    return GSS_S_COMPLETE;
}

As you can see, the value in cred_store->elements[i].value is never checked and you can happily pass through NULL value.

[abbra]

To expand a bit, if this is fixed like in #183, we then can use None as a keytab name in kinit_keytab() call in ansible-freeipa to rely on the default client keytab.

[simo5]

The C code does accidentally allow this, but we should definitely not rely on it, we do not know how Heimdal will implement it for example.
In python-gssapi a None value should either cause an exception to be raised, or, if we want to tolearet is, then it should completely omit the key when calling the C interface, IMHO.

[simo5]

@frozencemetery you implemented #183 so may want to comment here

[frozencemetery]

The code as written in #183 will I believe result in a key with NULL value.

Heimdal already implemented the cred store extensions; I checked their implementation as well, and it supports NULL values.

The behavior before #183 is entirely incorrect: it throws an error about how NoneType doesn't have the required string-ish methods. This change causes None to be converted to the appropriate C object (i.e., NULL).

Lacking a standard for these extensions, it's hard for me to say what's the "right" thing to do here. It sounds like there's some (albeit unintentional) use case for passing through NULL here; while I can see an argument for throwing an (appropriate) exception instead, that puts python-gssapi in the position of adding additional restrictions to the de-facto behavior that are not imposed by other implementations.

[simo5]

As one of the originators of that API I would strongly prefer not to use the implementation detail that current implementations tolerate empty keys, but I guess the cat is out of the bag now :-(

[frozencemetery]

@simo5 Well, I haven't cut a release with it, just committed to master. We can still do something else if it'd be better somehow. In my opinion, whether to use the implementation detail is on the calling application (here, freeipa), not python-gssapi. Nothing I can find from Nico or you proposes behavior for NULL values in the cred_store API. If you and @abbra agree on a better solution to the problem for freeipa, I'm happy to do something different here?

[simo5]

let's pull @nicowilliams in the discussion and see what he thinks

[greghudson]

I agree with Simo. If I understand properly:

• The C implementations of gss_acquire_cred_from() will silently ignore cred store keys that have null values, just as if the key weren't present. This is an emergent behavior, not an explicit one.

• FreeIPA's kinit_keytab() could be changed to check for keytab is None.

Relying on the emergent behavior is inferior to solving the problem at a higher layer.

[nicowilliams]

I agree with @simo5 and @greghudson. Because we didn't specify behavior with NULL values, we'd like that to remain open for future specification changes -- we, the implementors, would like to reserve the right to treat those cases specially in ways we've not yet dreamed up. I.e., we reserve the right to break this code if you use NULL values.

[abbra]

Thanks for the discussion.

It all started when we tried to add support for the default client keytab in ansible-freeipa. We can change FreeIPA's kinit_keytab() behavior to avoid adding a client keytab entry but this would not work for already released versions. This means that ansible-freeipa will have to check if KRB5_CLIENT_KTNAME is set in the environment and force pass its value to kinit_keytab() to avoid this issue.

[greghudson]

I'm a little confused. Are old releases of python-gssapi (two layers down) less of a concern than old releases of FreeIPA (one layer down)?

[abbra]

you are right that both would be problematic. Anyway, for ansible-freeipa the choice is between explicitly passing a keytab path from KRB5_CLIENT_KTNAME and not passing anything. I think we'll just keep passing the value to avoid a difference between new/older releases. And we'll fix FreeIPA to not pass None.

[frozencemetery]

From my perspective, before #183, python-gssapi throws an unexpected exception (i.e., not mentioned in docs) when given a None value. This is a bug. However, there's no specification for these functions (this seems like a good place to point out that reviews on draft-ietf-kitten-gssapi-extensions-iana are very welcome). If passing through the NULL value to the underlying GSSAPI library isn't the preferred option, what do you propose we do instead? Throw an exception about the bad value?

[greghudson]

I think throwing a bad-value exception is better than passing on an invalid cred store.