apache / incubator-pagespeed-mod

Apache module for rewriting web pages to reduce latency and bandwidth.
http://modpagespeed.com
Apache License 2.0
696 stars 157 forks source link

allow prohibiting handler access #1088

Closed jeffkaufman closed 9 years ago

jeffkaufman commented 9 years ago

On shared hosting the admins want to deny individual site owners access to things like /pagespeed_global_admin, but on Apache the only way to prevent them from mapping one in with AddHandler in a .htaccess file seems to be to not include FileInfo in AllowOverride, which is generally much too restrictive (no headers, no rewrite rules).

To fix this we could add directives:

StatisticsDomains
GlobalStatisticsDomains
MessagesDomains
ConsoleDomains
AdminDomains
GlobalAdminDomains

which would take a comma separated list of domains. They'd be set at the VHost/server level or above. Then in mod_pagespeed.cc:instaweb_handler and ngx_pagespeed.cc:ps_route_request we would check these before classifying a request as belonging to the admin handler.

(This isn't really needed on nginx because there aren't .htaccess files, but I think it's simpler to add the feature to all platforms because then we can just doc it normally. And there may be uses for it on nginx I'm not thinking of.)

ghost commented 9 years ago

Jeff, Thank you very much! I really appreciate your move.

Pawel-Panek commented 9 years ago

What do you mean by 'domain'? Do you want to use Host header value? When set to empty string would it result in blocking all domains?

jeffkaufman commented 9 years ago

@Pawel-Panek

Do you want to use Host header value?

yes

When set to empty string would it result in blocking all domains?

yes

jmarantz commented 9 years ago

Wildcards may be appropriate here. What do you think?

Also note that the restrictions imposed here would be in addition to those imposed in the block containing the SetHandler directive. On Jun 3, 2015 8:13 AM, "Jeff Kaufman" notifications@github.com wrote:

@Pawel-Panek https://github.com/Pawel-Panek

Do you want to use Host header value?

yes

When set to empty string would it result in blocking all domains?

yes

— Reply to this email directly or view it on GitHub https://github.com/pagespeed/mod_pagespeed/issues/1088#issuecomment-108352020 .

jeffkaufman commented 9 years ago

I was thinking of not doing wildcards here since I couldn't think of a case where that would be better than just listing out domains, but now I'm thinking about something like *.example.com that sounds pretty useful.

jmarantz commented 9 years ago

I was thinking that the default value of these options could be "*", so that a value of "" means "completely disable viewing of stats".

On Wed, Jun 3, 2015 at 9:14 AM, Jeff Kaufman notifications@github.com wrote:

I was thinking of not doing wildcards here since I couldn't think of a case where that would be better than just listing out domains, but now I'm thinking about something like *.example.com that sounds pretty useful.

— Reply to this email directly or view it on GitHub https://github.com/pagespeed/mod_pagespeed/issues/1088#issuecomment-108401759 .

jeffkaufman commented 9 years ago

@jmarantz I was thinking of implementing that by having unset mean full allow and "" mean full deny.

jmarantz commented 9 years ago

The semantic of unset meaning something that you can't achieve with a value is not a pattern we use except (I believe) in the options-implementation itself.

And in particular I think you might want to allow killing access in the root, but overriding in a vhost (though not in a directory scope or htaccess file). To override you have to set a value.

On Wed, Jun 3, 2015 at 9:24 AM, Jeff Kaufman notifications@github.com wrote:

@jmarantz https://github.com/jmarantz I was thinking of implementing that by having unset mean full allow and "" mean full deny.

— Reply to this email directly or view it on GitHub https://github.com/pagespeed/mod_pagespeed/issues/1088#issuecomment-108408224 .

jeffkaufman commented 9 years ago

I'm currently planning to make merging work with these, so you could do:

ModPagespeedMessagesDomains allowed.example.com,more.example.com
ModPagespeedMessagesDomains allow-allow-allow.example.com
<VirtualHost>
    ServerName allowed.example.com
    ServerAlias more.example.com
    ServerAlias even-more-messages-allowed.example.com

    ModPagespeedMessagesDomains even-more-messages-allowed.example.com
</VirtualHost>

If we go this way, the idea is that you're building up a list of allowed domains across multiple calls and scopes. A full deny would look like:

ModPagespeedMessagesDomains ""

These two configs would be equivalent in practice, but because of how people organize their configs this might be handy:

ModPagespeedMessagesDomains ""
<VirtualHost>
    ServerName a.example.com
    ModPagespeedMessagesDomains a.example.com
</VirtualHost>

and

ModPagespeedMessagesDomains a.example.com
<VirtualHost>
    ServerName a.example.com
</VirtualHost>
jeffkaufman commented 9 years ago

The idea is that we normally don't restrict in this way, but if you list some domains to allow then all others are blocked.

jeffkaufman commented 9 years ago

But I'd be up for adding things like:

# Meaningless, but allows you to affirm the default case and avoid the
# semantic pattern Josh dislikes.
ModPagespeedMessagesDomains DEFAULT_ALLOW

# This is what I was going to have [ModPagespeedMessagesDomains ""] mean
ModPagespeedMessagesDomains DENY

# If this is set anywhere all other ModPagespeedMessagesDomains
# commands are ignored and requests are dropped.
ModPagespeedMessagesDomains DENY_NO_OVERRIDE
jmarantz commented 9 years ago

For a vhost to recover the default state after a global setting of "", it would have to set it to "*".

I'm not saying a vhost would want to do that, I just think it's cleaner (more symmetrical?) to allow an explicit setting to recover the default state.

On Wed, Jun 3, 2015 at 9:41 AM, Jeff Kaufman notifications@github.com wrote:

The idea is that we normally don't restrict in this way, but if you list some domains to allow then all others are blocked.

— Reply to this email directly or view it on GitHub https://github.com/pagespeed/mod_pagespeed/issues/1088#issuecomment-108418973 .

jeffkaufman commented 9 years ago

@jmarantz I see, yes. So DEFAULT_ALLOW above isn't actually meaningless.

Being able to add DENY_NO_OVERRIDE makes me lean towards DEFAULT_ALLOW/DENY/DENY_NO_OVERRIDE instead of ""/"*", but if we do want to include wildcard support then * is easy. I'll think more.

jeffkaufman commented 9 years ago

Currently planning to offer CLEAR_INHERITED as a way for VHosts to get the default back, and not support wildcards.

ghost commented 9 years ago

Hi Jeff,
Thanks for these features. Are those available to stable or beta version now ?

jeffkaufman commented 9 years ago

I'm sorry, these haven't been released yet.

Pawel-Panek commented 9 years ago

Hi,

Sorry for bugging on this but we are getting more people interested in mod_pagespeed and we can't really use it for Apache server. I see there are some updates to Nginx module already so it would be perfect if we can get those pushed to Apache httpd. Thanks!

jeffkaufman commented 9 years ago

We recently finished the 1.9.32.6 update for both Apache and Nginx, but it doesn't include this feature because on point releases we only fix bugs. 1.10.x.x will include this feature, but it's not ready yet. Sorry!

jeffkaufman commented 9 years ago

Talking to other team members they've convinced me this should count as a bugfix and not a new feature, so I'll aim to get this into our next bugfix release on 1.9.

Pawel-Panek commented 9 years ago

Wow! This is getting exciting. Looking forward to run the tests.

ghost commented 8 years ago

Hi Jeff,

Thanks for bringing this up to 1.9. Is it available for Apache now?

jeffkaufman commented 8 years ago

This will be in the 1.10 release, which should be out soon.