deshaw / wsgi-kerberos Goto Github PK

The current _consume_request implementation reads the Content-Length header and blindly tries to read and buffer that many bytes into memory without regard for size:
https://github.com/mkomitee/wsgi-kerberos/blob/c2199a3c1143c0a340b576c0d547442224080e53/wsgi_kerberos.py#L57

I think it would be safer to only read up to read_max bytes.

Python 3.6 is the oldest version of Python that is not past end-of-life.

Ref: https://www.python.org/downloads/

This would reduce the maintenance burden for WSGI-Kerberos (e.g. less Python versions in the CI matrix that would be needed for #21), would enable dropping the test dependency on mock, and would enable dropping e.g. string/bytes compatibility code that imposes maintenance and performance costs.

The current implementation of _consume_request assumes a 0-length body when a Content-Length header is not sent:
https://github.com/mkomitee/wsgi-kerberos/blob/c2199a3c1143c0a340b576c0d547442224080e53/wsgi_kerberos.py#L49

However, clients may stream request bodies using the Transfer-Encoding: chunked without sending a Content-Length header.

To handle this, we would just read chunks of data from the socket until hitting\r\n\r\n.

Maybe we're fine not handling this, and just giving clients "Broken Pipe" for such requests? If that's the case, the code change here could just be to update the comments around this code to document that decision.

So, I just spent a while troubleshooting a production issue where uploading a 20MB file was failing with a connection reset message.

The problem is that unless you have debug logging enabled (which is usually not the case in production), this is a very difficult to troubleshoot issue. I think the very real possibility of this causing difficult to troubleshoot production issues is just not worth the hypothetical DoS protection, especially since most production setups have a front-end server where you configure the max body size anyway. Even if we change the log level to error, it's not guaranteed to work correctly or may output spurious errors.

Thought, @jab?

Would you accept a pull request (and then make a release) adding Python 3 compatibility? From a quick look I think it's just a matter of fixing the references to basestring and Exception.message.

Thanks for releasing wsgi-kerberos.

Continuous integration should be set up to run the tests against a revision proposed by a pull request and against all supported versions of Python, with a PR status check for the results. CI should also run the tests against the latest revision of master on a periodic basis.

https://github.com/mkomitee/wsgi-kerberos/blob/c2199a3c1143c0a340b576c0d547442224080e53/wsgi_kerberos.py#L153

calls authGSSServerStep(state, client_token) even when a client_token of empty string is provided.

In this case, a kerberos.KrbError is produced that is propagated, instead of being swallowed by the

except kerberos.GSSError:
    pass

GSSError is a subclass of KrbError, so the except statement is too narrow to catch the KrbError that is raised in this case.

Google Chrome triggers this when a WSGI-Kerberos-wrapped server sends two 401 responses in a row (the first sent by WSGI-Kerberos, the second by the wrapped WSGI app). In this case, Chrome sends an Authorization header with a client token of empty string, triggering the unhandled KrbError, ultimately resulting in 500 internal server error.

Should WSGI-Kerberos have special handling of the invalid client_token == "" case without passing it through to authGSSServerStep, and/or broaden the except statement to swallow KrbError? Testing with only the latter change results in sending a 403 response.

When a client uses Kerberos to authenticate to a server it presents a token for a specific service name. The server then checks its keytab to see if it has a key for that name, and if it does then the authentication can successfully complete. The server does not need to know ahead of time what name a client might use.

Pre-specifying a service when calling authGSSServerInit needlessly constrains what service names can work and breaks any clients that attempt to access the service by another name, even if a key for that service name exists in the keytab.

The simplest solution is to simply pass the empty string ("") to authGSSServerInit(), and drop use of hostname and self.service entirely.

Also:
Attempting to get the server principal details from the keytab using a provided hostname (as is done in init) may have some value as a sanity check, but:

1. It should be clearly characterized as just an optional sanity check, and not actually required for proper operation of the service.
2. To be maximally useful, it needs to take a list of service names, since services can have multiple keys.
3. Just because a particular set of service names are or are not valid when the service starts doesn't mean that will be the case when an actual authentication is attempted. Keys can be added to (or removed from) a keytab without restarting the service.

It may be easier to simply remove that code.

It seems the library only supports rc4-hmac, at least according to the output when running example_application.py:
1.1.2.2 - - [02/Oct/2021 10:40:32] "GET / HTTP/1.1" 403 9
ERROR:wsgi_kerberos:Unhandled GSSError: (('Unspecified GSS failure. Minor code may provide more information', 851968), ('Request ticket server HTTP/HOSTNAME.FQDN@REALM kvno 1 found in keytab but not with enctype rc4-hmac', 100002))

Can this code support aes256-cts-hmac-sha1-96 or aes128-cts-hmac-sha1-96 keytab enctypes?

In addition it seems that lowercase http/service_principal keytab entries are ignored, in order to be recognized they must be HTTP/service_principal@REALM. I don't recall this being a requirement previously.

I'm currently working on an open source IT infrastructure project and
was trying to deploy a particular WSGI application with kerberos
support. I came across your project and would ideally like to use it in
the infrastructure. However, I'd just like to make some minor
improvements to it that should ideally benefit everyone.

Here are my proposals:

• Change project documentation to reflect that this is a SPNEGO (GSSAPI)
  middleware layer that supports Kerberos as the authentication backend
• Change the dependency on the relatively unmaintained and narrow python-kerberos[1] to
  the more general and well maintained gssapi[2].

I have the bandwidth to work on these changes now. I'd like to know if
these are reasonable proposals so that I can spend some time to submit a
PR to the project. Is that ok?

Thanks for the great project!

In the following line, WSGI-Kerberos incorrectly ignores the first word of the Authorization header, rather than verifying that it's "Negotiate" before attempting the self._authenticate call:
https://github.com/mkomitee/wsgi-kerberos/blob/98e1e745dbae918b8d3af143c835e4dc32d94de0/wsgi_kerberos.py#L187

Instead, WSGI-Kerberos should check if the first word is "Negotiate", and if not, call self._unauthorized().

Would you accept a PR that adds support for an optional auth_succeeded_callback argument? If supplied, WSGI-Kerberos would call the callback upon successful authentication, passing in the authenticated user.

This would enable users to pass something like auth_succeeded_callback=lamdba user: structlog.contextvars.bind_contextvars(user=user), to enable all subsequent log calls made during the lifetime of the request to automatically include the authenticated user, which can be really convenient.

We are already doing this in our fork of WSGI-Kerberos, and would love to get it upstreamed.

Thanks for your consideration!

Ref: https://www.structlog.org/en/stable/contextvars.html