#96 add ssl cert auth support
Merged 6 years ago by mjia. Opened 6 years ago by mjia.
mjia/waiverdb issue76  into  master

file modified
+5
@@ -53,3 +53,8 @@ 

  @pytest.fixture()

  def enable_kerberos(app, monkeypatch):

      monkeypatch.setitem(app.config, 'AUTH_METHOD', 'Kerberos')

+ 

+ 

+ @pytest.fixture()

+ def enable_ssl(app, monkeypatch):

+     monkeypatch.setitem(app.config, 'AUTH_METHOD', 'SSL')

file modified
+27
@@ -90,3 +90,30 @@ 

          request.headers.__contains__.side_effect = headers.__contains__

          user, header = waiverdb.auth.get_user(request)

          assert user == name

+ 

+ 

+ @pytest.mark.usefixtures('enable_ssl')

+ class TestSSLAuthentication(object):

+     def test_SSL_CLIENT_VERIFY_is_not_set_should_raise_error(self):

+         with pytest.raises(Unauthorized) as excinfo:

+             request = mock.MagicMock()

+             waiverdb.auth.get_user(request)

+         assert 'Cannot verify client' in excinfo.value.get_description()

+ 

+     def test_SSL_CLIENT_S_DN_is_not_set_should_raise_error(self):

+         with pytest.raises(Unauthorized) as excinfo:

+             request = mock.MagicMock(environ={'SSL_CLIENT_VERIFY': 'SUCCESS'})

+             waiverdb.auth.get_user(request)

+         assert 'Unable to get user information (DN) from the client certificate' \

+                in excinfo.value.get_description()

+ 

+     def test_good_ssl_cert(self):

+         # http://vsbattles.wikia.com/wiki/Son_Goku

+         name = 'Son Goku'

+         ssl = {

+             'SSL_CLIENT_VERIFY': 'SUCCESS',

+             'SSL_CLIENT_S_DN': name

+         }

+         request = mock.MagicMock(environ=ssl)

+         user, header = waiverdb.auth.get_user(request)

+         assert user == name

file modified
+9
@@ -112,6 +112,15 @@ 

          user = user.split("@")[0]

          if kerberos_token is not None:

              headers = {'WWW-Authenticate': ' '.join(['negotiate', kerberos_token])}

+     elif current_app.config['AUTH_METHOD'] == 'SSL':

+         # Nginx sets SSL_CLIENT_VERIFY and SSL_CLIENT_S_DN in request.environ

+         # when doing SSL authentication.

+         ssl_client_verify = request.environ.get('SSL_CLIENT_VERIFY')

+         if ssl_client_verify != 'SUCCESS':

+             raise Unauthorized('Cannot verify client: %s' % ssl_client_verify)

+         if not request.environ.get('SSL_CLIENT_S_DN'):

+             raise Unauthorized('Unable to get user information (DN) from the client certificate')

+         user = request.environ.get('SSL_CLIENT_S_DN')

      elif current_app.config['AUTH_METHOD'] == 'dummy':

          # Blindly accept any username. For testing purposes only of course!

          if not request.authorization:

file modified
+1 -1
@@ -22,7 +22,7 @@ 

      # need to explicitly turn this off

      # https://github.com/flask-restful/flask-restful/issues/449

      ERROR_404_HELP = False

-     AUTH_METHOD = 'OIDC'  # Specify OIDC or Kerberos for authentication

+     AUTH_METHOD = 'OIDC'  # Specify OIDC, Kerberos or SSL for authentication

      # Change it if the Kerberos service is not running on which the waiverdb is run.

      KERBEROS_HTTP_HOST = None

      # Set this to True or False to enable publishing to a message bus

This is using the same approach used by ODCS in https://pagure.io/odcs/pull-request/132.
Presumably we could deploy WaiverDB in OpenShift as in https://developers.redhat.com/blog/2017/01/24/end-to-end-encryption-with-openshift-part-1-two-way-ssl/ and everything should work.

So the question is - will this work on OpenShift? I.e will the client certificate get all the way to waiverdb or will it be stopped by proxies?

It should work if the tls passthrough route configuration in OpenShift works as expected.

@mjia, do you figure we'll keep the gunicorn container and front it with a reverse proxy nginx container? I guess that wouldn't set anything in request.environ though.

For this to work, we'd have to replace gunicorn with nginx directly in our main webapp container. Correct?

:+1: to the gist. I guess we can work out how the containers will be arranged in a separate PR.

Huh, we could use two containers for this( nginx reverse proxy + gunicorn). You are right, nginx will not set anything in request.environ. Instead, it will set X-SSL-Client-Verify and X-SSL-Client-S-DN in the headers with a config like this

server {
# ...
ssl_client_certificate /srv/ssl/self.crt;
ssl_verify_depth 1;
ssl_verify_client on;
# ...
location @app
# ...
proxy_set_header X-SSL-Client-Verify $ssl_client_verify;
proxy_set_header X-SSL-Client-S-DN $ssl_client_s_dn;
# ...
}

It turns out that only Apache sets them in request.environ and we are not using it for deployment.

rebased onto 588c1f0ca9a79227cdf5deeefc96c757f141aa55

6 years ago

Rebased to get ssl info from request.headers.

Nginx may set those ssl info into request.environ as well. It would be interesting to figure it out when deploying.

Note that we'll have to be very careful that the gunicorn container doesn't respond to request from outside the cluster -- only from the nginx tls-terminating container.

Otherwise, @ralph will be able to make a request directly to the gunicorn container and simply pass his own X-SSL-* headers, and impersonate anybody.

Note that we'll have to be very careful that the gunicorn container doesn't respond to request from outside the cluster -- only from the nginx tls-terminating container.
Otherwise, @ralph will be able to make a request directly to the gunicorn container and simply pass his own X-SSL-* headers, and impersonate anybody.

RIght, this is the thing we need to pay attention to when doing the deployment.

You must use request.environ here and not headers. In case you would misconfigure the frontend webserver in such way it would allow non-SSL connection, anyone could set these headers and get in without any auth.

This has already happened in wild, @puiterwijk could tell you more.

I now read the whole discussion and see @ralph has already pointed out the issue with X-SSL-* headers. I will let the final decision to you, since I don't know whole deployment of waiverdb. Just wanted to point out that when we were reviewing ODCS SSL auth, fedora-infra was against request.headers. This situation might be different because of a deployment you use is different from what ODCS does.

From personal experience (i.e. CVE-2016-1000038), I would strongly suggest to use environment variables rather than headers.

I am wondering why you want to be so strict about using gunicorn or nginx rather than httpd with mod_ssl and mod_wsgi, since it could do environ injection for you?

Also, I'm not entirely convinced that you can't use environment even with nginx: Nginx sets variables with cert status (http://nginx.org/en/docs/http/ngx_http_ssl_module.html#variables), which you can probably pass into fastcgi_param (http://nginx.org/en/docs/http/ngx_http_fastcgi_module.html#fastcgi_param) or whatever else to get info to your application.

If you really, really can not use anything like that, I would have some suggestions:
- Make any request that doesn't pass these headers return an error. Since at least some of the headers should be passed even when no client cert is being passed, this should allow you to notice an incorrectly deployed setup if someone does a request without client cert (and make it very clear that people are not to disable this). Make it also fill up the web server logs. Be obnoxious.
- Document this into oblivion. It must not be ignorable for anyone setting this up with this auth module.
- Make sure this auth module does not get enabled easily.
- If you must use headers, grab the client cert and do full verification of it yourself. Though even this is really weak (since this does not include the Proof of Possession), it makes the barrier to abuse way higher.

And I had some more that I can't think of right now, but when I do I'll add them.

Basically, I think that using request headers for this is a Really Bad Idea(R), and should be avoided unless every other alternative has been exercised.

(Do note that as long as we are 100% sure this code never runs in Fedora Infrastructure, I will not block this PR, but just give my personal advice.).

You are right. I could not find a way to get nginx set those ssl info into request.environ yesterday. Now I know and I will this.

Thanks @puiterwijk for your thoughtful suggestions.

I am wondering why you want to be so strict about using gunicorn or nginx rather than httpd >with mod_ssl and mod_wsgi, since it could do environ injection for you?

I'm not strict about using nginx + gunicorn. We could use apache+gunicorn if it makes life easier.

Also, I'm not entirely convinced that you can't use environment even with nginx: Nginx sets >variables with cert status (http://nginx.org/en/docs/http/ngx_http_ssl_module.html#variables), >which you can probably pass into fastcgi_param (http://nginx.org/en/docs
/http/ngx_http_fastcgi_module.html#fastcgi_param) or whatever else to get info to your application.

Cool. This is what I was looking for. I will update patch to use environment variables.

rebased onto 57e4a8a3467d9384f3f60f996d2adc20194e4caa

6 years ago

Rebased to get ssl info from request.environ which is safe, so a nignx config would look like this:

server { # ... ssl_client_certificate /srv/ssl/self.crt; ssl_verify_depth 1; ssl_verify_client on; # ... location @app { # ... fastcgi_param SSL-Client-Verify $ssl_client_verify; fastcgi_param SSL-Client-S-DN $ssl_client_s_dn; # ... } }

Would it be an idea to use the underscore variant of the environment variable name?
That way, the same code would work with Apache mod_ssl out of the box.

rebased onto bacf8ef

6 years ago

@puiterwijk good point. I've updated the patch.

I got super confused about this, but @mjia and @dcallagh set me straight. Looks good from my PoV.

I got super confused about this, but @mjia and @dcallagh set me straight. Looks good from my PoV.

To clarify, here is a sum up of what I've figured out:

  • Nginx + Gunicorn

By using one container(nginx+gunicorn), nginx might inject SSL_CLIENT_VERIFY and SSL_CLIENT_VERIFY into request.environ. If not, both need to be explicitly set in request.headers. If we want to run Nginx reverse proxy on a different container than gunicorn, we need to tell Gunicorn to trust the X-Forwarded-* headers sent by Nginx to
prevent a malicious client from forging these headers. Something like this gunicorn --forwarded-allow-ips="10.170.3.217" waiverdb:app.

http://docs.gunicorn.org/en/stable/deploy.html#nginx-configuration

  • Nginx + FastCGI/uWSGI

Nginx supports setting parameters for both servers, so SSL_CLIENT_VERIFY and SSL_CLIENT_VERIFY can be accessed from require.environ in the backend. As I understand, uWSGI might be easier to go.
http://flask.pocoo.org/docs/0.12/deploying/uwsgi/#starting-your-app-with-uwsgi

  • Apache+mod_wsgi

This approach would work as well. See how ResultsDB uses it to run tests in OpenShift.

https://pagure.io/taskotron/resultsdb/blob/develop/f/openshift/run_app.sh#_8

@ralph does this all make sense? By looking at all of these options again, I feel like Apache+mod_wsgi might be the easiest one which we could just copy what has been done on ResultsDB side, :-P

Looks good to me now :). +1

Just a quick note that in Fedora Infra, we also definitely prefer Apache+mod_wsgi for the the time being, as that's what we are using for everything else.

Pull-Request has been merged by mjia

6 years ago