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