New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

allow prohibiting handler access #1088

Closed
jeffkaufman opened this Issue Jun 2, 2015 · 22 comments

Comments

Projects
None yet
4 participants
@jeffkaufman
Copy link
Contributor

jeffkaufman commented Jun 2, 2015

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.)

@mirajcloudaccess

This comment has been minimized.

Copy link

mirajcloudaccess commented Jun 3, 2015

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

@Pawel-Panek

This comment has been minimized.

Copy link

Pawel-Panek commented Jun 3, 2015

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

This comment has been minimized.

Copy link
Contributor

jeffkaufman commented Jun 3, 2015

@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

This comment has been minimized.

Copy link
Contributor

jmarantz commented Jun 3, 2015

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
#1088 (comment)
.

@jeffkaufman

This comment has been minimized.

Copy link
Contributor

jeffkaufman commented Jun 3, 2015

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

This comment has been minimized.

Copy link
Contributor

jmarantz commented Jun 3, 2015

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
#1088 (comment)
.

@jeffkaufman

This comment has been minimized.

Copy link
Contributor

jeffkaufman commented Jun 3, 2015

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

@jmarantz

This comment has been minimized.

Copy link
Contributor

jmarantz commented Jun 3, 2015

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
#1088 (comment)
.

@jeffkaufman

This comment has been minimized.

Copy link
Contributor

jeffkaufman commented Jun 3, 2015

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

This comment has been minimized.

Copy link
Contributor

jeffkaufman commented Jun 3, 2015

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

This comment has been minimized.

Copy link
Contributor

jeffkaufman commented Jun 3, 2015

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

This comment has been minimized.

Copy link
Contributor

jmarantz commented Jun 3, 2015

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
#1088 (comment)
.

@jeffkaufman

This comment has been minimized.

Copy link
Contributor

jeffkaufman commented Jun 3, 2015

@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

This comment has been minimized.

Copy link
Contributor

jeffkaufman commented Jun 3, 2015

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

jeffkaufman added a commit to apache/incubator-pagespeed-ngx that referenced this issue Jun 3, 2015

jeffkaufman added a commit to apache/incubator-pagespeed-ngx that referenced this issue Jun 4, 2015

jeffkaufman added a commit to apache/incubator-pagespeed-ngx that referenced this issue Jun 4, 2015

jeffkaufman added a commit to apache/incubator-pagespeed-ngx that referenced this issue Jun 5, 2015

@mirajcloudaccess

This comment has been minimized.

Copy link

mirajcloudaccess commented Jun 29, 2015

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

@jeffkaufman

This comment has been minimized.

Copy link
Contributor

jeffkaufman commented Jun 29, 2015

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

@Pawel-Panek

This comment has been minimized.

Copy link

Pawel-Panek commented Aug 6, 2015

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

This comment has been minimized.

Copy link
Contributor

jeffkaufman commented Aug 7, 2015

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

This comment has been minimized.

Copy link
Contributor

jeffkaufman commented Aug 7, 2015

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

This comment has been minimized.

Copy link

Pawel-Panek commented Aug 7, 2015

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

jeffkaufman added a commit to apache/incubator-pagespeed-ngx that referenced this issue Sep 18, 2015

@jeffkaufman jeffkaufman self-assigned this Oct 23, 2015

jeffkaufman added a commit to apache/incubator-pagespeed-ngx that referenced this issue Nov 16, 2015

jeffkaufman added a commit to apache/incubator-pagespeed-ngx that referenced this issue Nov 16, 2015

@mirajcloudaccess

This comment has been minimized.

Copy link

mirajcloudaccess commented Nov 27, 2015

Hi Jeff,

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

@jeffkaufman

This comment has been minimized.

Copy link
Contributor

jeffkaufman commented Nov 28, 2015

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment