Edgewall Software
Modify

Opened 6 years ago

Closed 6 years ago

Last modified 3 weeks ago

#9206 closed enhancement (worksforme)

patch: authorize using Remote-User: header

Reported by: srl@… Owned by:
Priority: normal Milestone:
Component: web frontend Version: 0.11-stable
Severity: normal Keywords:
Cc: Ryan J Ollos
Release Notes:
API Changes:

Description

for obscure reasons, my tracd-behind-proxypass wasn't able to authorize the usual way.

This patch will make trac trust the 'Remote-User:' header, even if the adaptor (CGI/etc) didn't otherwise set the remote_user field.

This probably should be a configuration option as it would otherwise have security implications but is perfect for our use - apache LDAP-based authentication. I'm aware of LdapPlugin but this seemed simpler and with broader implications (could work for other authentication types, kerberos etc).

Attachments (2)

auth-from-header.patch (1.1 KB) - added by srl@… 6 years ago.
auth-from-header.patch
9206-remoteuser.patch (911 bytes) - added by srl@… 6 years ago.
better patch with option

Download all attachments as: .zip

Change History (19)

Changed 6 years ago by srl@…

Attachment: auth-from-header.patch added

auth-from-header.patch

comment:1 Changed 6 years ago by Christian Boos

Milestone: next-minor-0.12.x

Thanks for the patch. Would you mind adding that configuration option? This would be also the place to briefly document why you would need to set up such option.

The test could also be written if not remote_user and req.get_header('Remote-User'):, I suppose that would be slightly more efficient.

Changed 6 years ago by srl@…

Attachment: 9206-remoteuser.patch added

better patch with option

comment:2 Changed 6 years ago by Steven R. Loomis <srl@…>

The better patch here automatically logs you in if present (instead of requiring you to click login), and has a configuration option.

comment:3 Changed 6 years ago by Carsten Klein <carsten.klein@…>

Why not make this an implementation of the IAuthenticator extension point interface?

E.g.

class MyRemoteUserAuthenticator(IAuthenticator):

    obey_remote_user_header = BoolOption('trac', 'obey_remote_user_header', 'false', 
 	       """Whether the 'Remote-User:' HTTP header is to be trusted for user logins 
 	        (''since ??.??').""") 

    def authenticate(self, req):
        if self.obey_remote_user_header and req.get_header('Remote-User'): 
 	    return req.get_header('Remote-User') 
        return None

That way we can keep trac free of the change that would likely raise security issues, and you are free to deploy your plugin to all the trac installations that you have.

comment:4 Changed 6 years ago by Steven R. Loomis <srl@…>

Carsten,

That's a great idea. I'll do that and post a link here, thanks.

-Steven

comment:5 Changed 6 years ago by Christian Boos

Milestone: next-minor-0.12.x
Resolution: worksforme
Status: newclosed

I suppose the code in comment:3 is enough then (could be placed in a single file plugin).

comment:6 Changed 5 years ago by lkraav <leho@…>

i'm trying to do the same thing: basic auth on <Location> mod_proxy forwarded to tracd. unfortunately tracd doesn't seem to take this auth info no matter which way i try. tried:

  • with project/plugins/remote-user-auth.py
    from trac.config import BoolOption
    from trac.web.api import IAuthenticator
    
    class MyRemoteUserAuthenticator(IAuthenticator):
    
        obey_remote_user_header = BoolOption('trac', 'obey_remote_user_header', 'false', 
                   """Whether the 'Remote-User:' HTTP header is to be trusted for user logins 
                    (''since ??.??').""") 
    
        def authenticate(self, req):
            if self.obey_remote_user_header and req.get_header('Remote-User'): 
                return req.get_header('Remote-User') 
            return None
    
  • without project/plugins/remote-user-auth.py
  • tracd with —basic-auth="*,htpasswd,My Proxy Realm"
  • tracd without —basic-auth
  • with rewriterule
RewriteEngine On
RewriteCond %{LA-U:REMOTE_USER} (.+)
RewriteRule . - [E=RU:%1]
RequestHeader add X-Forwarded-User %{RU}e
  • without rewriterule

what am i missing?

Last edited 3 weeks ago by Ryan J Ollos (previous) (diff)

comment:7 Changed 5 years ago by lkraav <leho@…>

Solved, written up at TracStandalone@90.

Last edited 3 weeks ago by Ryan J Ollos (previous) (diff)

comment:8 Changed 4 years ago by guidod-2007-@…

Very good - it did solve my problem as well.

I have transformed the script into a setuptools Trac plugin that can be packaged and installed just like any other trac-hack. Along with polishing the documentation it is now ready - download at

https://bitbucket.org/guidod/trac-proxy-auth

comment:9 Changed 4 years ago by lkraav <leho@…>

so i'm wondering what it would take for this ticket to reach fixed?

comment:10 Changed 6 months ago by Ryan J Ollos

It seems like the configuration option is unnecessary when the authenticator is packaged as a single-file plugins since the behavior can be controlled by enabling/disabling the plugin.

The security risk seems relatively low provided there are no caveats to the statement documented on the Django site:

This warning doesn’t apply to RemoteUserMiddleware in its default configuration with header = 'REMOTE_USER', since a key that doesn’t start with HTTP_ in request.META can only be set by your WSGI server, not directly from an HTTP request header.

Any objections to applying attachment:9206-remoteuser.patch, perhaps renaming the option to trust_remote_user_header?

Alternatively, we could put the IAuthenticator in /contrib or /tracopt/web/auth.

Last edited 3 weeks ago by Ryan J Ollos (previous) (diff)

comment:11 Changed 3 weeks ago by Ryan J Ollos

We discussed this a bit more in gmessage:trac-users:5p8DjrgFHvw/atuGxm90DQAJ. I'm unsure how the login button can function in the recipe TracStandalone#Authenticationfortracdbehindaproxy. Perhaps the site described in the recipe is completely unavailable to anonymous users, and authentication occurs in the web server before hitting the Trac application. For the login button to work, it looks like req.environ['REMOTE_USER'] needs to be set.

A different solution to the issue of running TracStandalone behind a proxy might be to plugin in a different implementation of AuthenticationMiddleware. Making the middleware "pluggable" is probably related to #11585 (or at least the related work that was done in Bloodhound).

comment:12 Changed 3 weeks ago by srl@…

Yes! The site is completely unavailable to anonymous. Sorry for not making that clear.

comment:13 Changed 3 weeks ago by Ryan J Ollos

Cc: Ryan J Ollos added

Thanks for the additional information. It's good to be able to reconcile the behavior of your site with what we are seeing elsewhere.

There are very few places that req.remote_user is used:

$grep -R -in "req.remote_user" . --exclude-dir=.git --exclude-dir=build --exclude=*.pyc
./trac/web/auth.py:93:        if req.remote_user:
./trac/web/auth.py:94:            authname = req.remote_user
./trac/web/auth.py:150:        in req.remote_user will be converted to lower case before
./trac/web/auth.py:155:        if not req.remote_user:
./trac/web/auth.py:164:        remote_user = req.remote_user

It's only used in LoginModule.authenticate and LoginModule._do_login. Plugins such as AccountManagerPlugin access req.remote_user as well.

Summarizing what I see in the code, req.authname can be populated in an IAuthenticator implementation from the value of the HTTP_REMOTE_USER header, or in the case of LoginModule.authenticate, from the value of req.remote_user. req.remote_user is either set by AuthenticationMiddleware or by the WSGI application.

What I don't understand is why LoginModule needs to access req.remote_user in addition to req.authname: tags/trac-1.0.9/trac/web/auth.py@:148,157,161#L131. If trac.web.main.RequestDispatcher.authenticate is called before processing the request to the /login path (which I'm not entirely sure is the case), couldn't we just rely on the IAuthenticator implementation to set req.authname and use the value of req.authname in LoginModule._do_login rather than req.remote_user?

comment:14 in reply to:  13 Changed 3 weeks ago by Jun Omae

Summarizing what I see in the code, req.authname can be populated in an IAuthenticator implementation from the value of the HTTP_REMOTE_USER header, or in the case of LoginModule.authenticate, from the value of req.remote_user. req.remote_user is either set by AuthenticationMiddleware or by the WSGI application.

If the patch is put to Trac, I think we should be able to use any header for the remote-user's name, not only Remote-User. The HTTP_REMOTE_USER variable can be set by remote attacker, easily. The following commands can set the HTTP_REMOTE_USER variable for Apache.

$ curl -o /dev/null --header 'REMOTE-USER: xx' http://hostname/trac  # works with apache 2.2 and 2.4
$ curl -o /dev/null --header 'REMOTE!USER: xx' http://hostname/trac  # works with apache 2.2
$ curl -o /dev/null --header 'REMOTE.USER: xx' http://hostname/trac  # works with apache 2.2
$ curl -o /dev/null --header 'REMOTE=USER: xx' http://hostname/trac  # works with apache 2.2

RequestHeader unset can remove the header. However, it cannot remove in the cases except use of -.

RequestHeader unset Remote-User early

IMO, I don't recommend to use HTTP header….

comment:15 Changed 3 weeks ago by Ryan J Ollos

I misunderstood the statement in the Django documentation that I referenced in comment:10. I had hoped we could modify AuthenticationMiddleware to set the REMOTE_USER from an HTTP header, as in the flask example. If that's not secure, any other ideas on how to set REMOTE_USER in TracStandalone when the web server acts as a proxy and handles authentication? It seems like a generalized problem that we should try to support in Trac.

comment:16 in reply to:  15 Changed 3 weeks ago by Jun Omae

Replying to Ryan J Ollos:

I misunderstood the statement in the Django documentation that I referenced in comment:10. I had hoped we could modify AuthenticationMiddleware to set the REMOTE_USER from an HTTP header, as in the flask example.

That example is insecure, I think. If an HTTP header is set on the reverse proxy, the reverse proxy must remove the header from remote.

Apache 2.4:

RequestHeader unset Remote-User early

Nginx:

    server {
        listen *:3000;
        server_name localhost;

        location / {
            proxy_pass http://127.0.0.1:3001;
            proxy_redirect http://127.0.0.1:3001 /;
            proxy_set_header Remote-User "";
        }

        location /login {
            proxy_pass http://127.0.0.1:3001/login;
            proxy_redirect http://127.0.0.1:3001 /;
            proxy_set_header Remote-User $remote_user;
            auth_basic "auth";
            auth_basic_user_file "./htpasswd.txt";
        }
    }

However, non-alphanumeric characters are replaced with _ in Apache 2.2 (e.g. Remote!User: adminHTTP_REMOTE_USER: admin). It's hard to remove such headers only using configurations. Since Apache 2.4, such headers is not converted to HTTP_ variables.

This changes is introduced in http://svn.apache.org/viewvc?view=revision&revision=1053353.

If that's not secure, any other ideas on how to set REMOTE_USER in TracStandalone when the web server acts as a proxy and handles authentication? It seems like a generalized problem that we should try to support in Trac.

Another idea is adding configurable option to use secret header. If the header is unforeseeable, it would be unable to send the header from remote. Untested patch:

  • trac/web/auth.py

    diff --git a/trac/web/auth.py b/trac/web/auth.py
    index 81be36fac..64a54812a 100644
    a b class LoginModule(Component): 
    8080        base path of several Trac instances if you want them to share
    8181        the cookie.  (''since 0.12'')""")
    8282
     83    remote_user_http_header = Option('trac', 'remote_user_http_header', '',
     84        """HTTP header for the remote user.
     85
     86        The option can be used when REMOTE_USER variable is not available.
     87        The header from remote should be removed to be secure. If it's hard
     88        to remove it, keep the header secret using long enough and
     89        unforeseeable value. (''since 1.0.10'')""")
     90
    8391    # IAuthenticator methods
    8492
    8593    def authenticate(self, req):
    86         authname = None
    87         if req.remote_user:
    88             authname = req.remote_user
    89         elif 'trac_auth' in req.incookie:
     94        authname = req.remote_user
     95        if not authname:
     96            header = self.remote_user_http_header
     97            if header:
     98                authname = req.get_header(header)
     99        if not authname and 'trac_auth' in req.incookie:
    90100            authname = self._get_name_for_cookie(req,
    91101                                                 req.incookie['trac_auth'])
    92102
Last edited 3 weeks ago by Jun Omae (previous) (diff)

comment:17 Changed 3 weeks ago by Ryan J Ollos

Thanks, it sounds like a good idea to use a secret key for servers that don't allow secure web server configuration. We might run into the same issue of req.remote_user not being set in LoginModule._do_login. If that's the case, maybe it's possible to implement a similar idea of specifying the secret key as an option of TracStandalone and renaming the key in AuthenticationMiddleware. I'll do some testing in the coming days.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The ticket will remain with no owner.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from (none) to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.