Authorization token being wiped out by basic auth even though no username/password set.

[GrahamDumpleton]

If you are setting KG_AUTH_TOKEN with the aim to be able to have token based authentication used, the Authorization header for token is being wiped out by a basic one.

This is because the code has:

KG_HTTP_USER = os.getenv('KG_HTTP_USER', '')
KG_HTTP_PASS = os.getenv('KG_HTTP_PASS', '')

...

def load_connection_args(**kwargs):
    ....
    kwargs['auth_username'] = kwargs.get('auth_username', KG_HTTP_USER)
    kwargs['auth_password'] = kwargs.get('auth_password', KG_HTTP_PASS)
    return kwargs

@gen.coroutine
def fetch_kg(endpoint, **kwargs):
    """Make an async request to kernel gateway endpoint."""
    client = AsyncHTTPClient()
    url = url_path_join(KG_URL, endpoint)

    kwargs = load_connection_args(**kwargs)

    response = yield client.fetch(url, **kwargs)
    raise gen.Return(response)

The auth_username and auth_password values end up being passed as empty strings rather than None when not set, meaning that although token is set and kwargs looks like:

{'method': 'GET', 'validate_cert': True, 'connect_timeout': 20.0, 'request_timeout': 20.0, 'headers': {'Authorization': 'token colonels'}, 'auth_username': '', 'auth_password': ''}

with Authorization header, the AsyncHTTPClient().fetch() method replaces Authorization header passed in with one generated with empty basic authentication username and password.

The code should be changed to use:

KG_HTTP_USER = os.getenv('KG_HTTP_USER', None)
KG_HTTP_PASS = os.getenv('KG_HTTP_PASS', None)

if want to still allow empty username and password to be supplied by environment variables, or better still, should use:

    kwargs['auth_username'] = kwargs.get('auth_username', KG_HTTP_USER) or None
    kwargs['auth_password'] = kwargs.get('auth_password', KG_HTTP_PASS) or None

That way empty username and password are ignored by AsyncHTTPClient().fetch() as they will be None values if not set (or empty if go with latter change).

With either change, means that token based Authorization header will not be overridden and things will work if want to use that method.

Is late at night for me, but if you suggest your preference (I prefer ignoring username and password all together even if set as empty strings), then I can create a PR my tomorrow.

[kevin-bates]

@GrahamDumpleton - thank you for raising this issue. I wanted to first touch base with this code's originator, and we're both in agreement that None is more correct. (I suppose we could remove the default parameter since None is the default, but listing it adds clarity and intention - IMHO.)

Regarding whether to use the or clause to prevent intentional empty string values, I prefer to take the give 'em enough rope approach and am inclined to not add or None to each statement. If others disagree, however, I won't push back.

[GrahamDumpleton]

My current proposed set of changes can be seen at:

• https://github.com/jupyter-on-openshift/nb2kg/commit/ab8fa379ba1e0041fa7c90f7a934cbd51a28bc33

This gets things working for me. Now on to next problem of dealing with env whitelisting.

[kevin-bates]

@GrahamDumpleton - these look good. Please update the os.getenv() statements and then submit a PR.

Since the preceeding os.getenv() statements don't set an explicit default, then I suppose these updates should go ahead and not set None explicitly as the default. That is, consistency trumps clarity and intention (in reference to my previous comment). :smile:

[GrahamDumpleton]

Bit confused what you are asking to change. Are you saying, just to make it more explicit, also change:

KG_HTTP_USER = os.getenv('KG_HTTP_USER', '')
KG_HTTP_PASS = os.getenv('KG_HTTP_PASS', '')

to:

KG_HTTP_USER = os.getenv('KG_HTTP_USER', None)
KG_HTTP_PASS = os.getenv('KG_HTTP_PASS', None)

The change already made wouldn't strictly require that as neither None or empty string would be True.

Unless you also wanted to have later be:

        if KG_HTTP_USER is not None:
            parameters["auth_username"] = KG_HTTP_USER
        if KG_HTTP_PASS is not None:
            parameters["auth_password"] = KG_HTTP_PASS

so people can still have empty username and password.

I preferred not to allow empty values because then it makes it impossible to unset them by using empty value. For example, Docker image may bake in settings for KG_HTTP_USER and KG_HTTP_PASS but you might want to override with empty values and set KG_AUTH_TOKEN instead. You can't remove the environment variable and can only set them to empty value by overriding baked in defaults.

[kevin-bates]

My last comment was suggesting to use your branch, but, to those changes, remove the defaults from os.getenv(). Once the PR is submitted, we can iterate further as necessary. Thanks.

[GrahamDumpleton]

Okay, os.getenv() returns None anyway when not set. Understand now.