Added apache__redirect_to_https variable #9

muelli · 2017-02-07T13:56:26Z

Currently, each vhost can define "redirect_to_https". If True, which is
the current default, then the Apache configuration will include a
Redirect line.

This is good, except that VirtualHost rules seem to have precedence over
other rules. That is annoying when you want to define a global rule to
serve, say, ACME requests, because Apache would respect the VirtualHost
rules and redirect the plain HTTP request instead of serving the token.
To help debops users to not having to touch each and every defined
vhost, the new variable defines the default value for redirect_to_https,
which used to be "True". The default is still "True", but now you can
override that easily.

muelli · 2017-02-07T14:07:03Z

FWIW: A group snippet like the following could be employed to enable global redirection to HTTPS:

    httpsredirect:
        enabled: True
        type: raw
        raw: |
            # From http://serverfault.com/a/739128/193114
            <ifmodule mod_rewrite.c>
                RewriteEngine On
                RewriteOptions InheritDown
                RewriteCond %{HTTPS} off
                RewriteCond %{REQUEST_URI} !^/\.well\-known/acme\-challenge/
                RewriteRule (.*) https://%{HTTP_HOST}%{REQUEST_URI} [R=301,L]
            </ifmodule>

ypid · 2017-02-07T18:40:22Z

Currently, each vhost can define "redirect_to_https". If True, which is
the current default, then the Apache configuration will include a
Redirect line.

Redirect is not necessarily being used for this anymore. While working on support for Apache status (#8) I had basically the same problem because the Apache status is one of a very limited set of valid cases where a redirect to HTTPS should not be the default (localhost). So I made the role smart enough to use the rewrite module to redirect all requests except for Apache status.

This is what I would prefer for this valid use case as well, dedicated support for ACME (ref: #1). I don’t see much valid use cases except those two and would prefer to avoid a global default like apache__redirect_to_https.

Does that work for you?

muelli · 2017-02-08T22:12:55Z

hm. It doesn't seem to work. I also don't see why it would. As I've written in the commit message, at least with Apache 2.4, the VirtualHost rules have higher priority. Thus the vhosts' Redirect statements have higher priority than the global rewrite I added. It's Apache being a bit silly there, especially given the existence of the InheritDown option which forces rules to be injected in vhosts' space. But now we have to live with it somehow. One option for me would be to add the same configuration snippet to each and every vhost. But that feels more silly than to just disable redirection by default. Especially, because HSTS seems to be the more appropriate tool to perform the job of upgrading clients.

Note that this is also not necessarily about not redirecting the default host, only. I guess that I don't want any vhost to be redirected to a https version for ACME URLs.

The test is relatively simple. I execute wget -O- http://localhost/.well-known/acme-challenge/G6P3G9djqx8Lx0QkijKvT6Rfz7CB0DoQHZqu_OtOwpY and see whether redirects. I don't want it to redirect by itself for the ACME URLs.

ypid · 2017-02-08T22:20:34Z

Configuring ACME in the vhosts using an include snippet should do, similar as it is done on nginx.

About using HSTS for this that can be discussed if the coverage on client support is high enough but I would still like a HTTP redirect which all clients understand.

Have you checked my workaround and template code which I used to implement Apache status? The same can be done for ACME

muelli · 2017-02-08T23:16:00Z

Which clients do you think still need to understand HSTS?
It seems that HSTS works with all browsers except Opera mini: http://caniuse.com/#search=hsts. I even had to delete my ~/.wget-hsts to test properly.

The server-info is a bit of a different usecase, I think. In that case you only want to serve server-status from localhost. In the ACME case, you want to serve the ACME files unconditionally. Unfortunately, Apache does not allow overwriting a vhost's decision to Rewrite. As far as I am aware at least. So whenever the vhost does a Rewrite of ACME URLs, we're in trouble. That said, I tried to comprehend how the default for apache__status_vhost sets https_enabled: False and how the macro then checks for a vhost's redirect_https in order to not Rewrite the status location. But I still don't think I've fully grasped how it works let alone how to make it work for the ACME case.

If you are alluding to implementing another conditional rewrite like the one for the server-status but for ACME for every defined vhost, then yes, that should be possible. But that seems to require making the role too complex for me to patch. Having a global rewrite for ACME URLs seems to be a bit cleaner and more comprehensible than to inject something into every vhost. For both admins to understand and the debops role to implement.

Another option might be to use something like RewriteMatch !^.well-known/acme... instead of the current Rewrite in every rewriting vhost. But whatever works best with the role is great.

ypid · 2017-02-09T08:08:24Z

HSTS

The 301 redirect is an important part of deploying an HTTPS website. As part of the HTTP protocol, it is supported by more browsers than HSTS. It serves as the primary means for upgrading a plaintext connection to HTTPS, updating search indexes, and avoiding link rot.

Also, I don’t think we should make assumptions that all clients support HSTS. Ref: List of podcatchers. I think there is no reason for removing the HTTP redirect default.

@drybjed What do you think about HSTS and HTTP redirecting?

Apache does not allow overwriting a vhost's decision to Rewrite. As far as I am aware at least.

I also don’t know a way and assume it is not supported, mod_alias takes precedence. That is why get_vhost_content_directives uses the rewrite module in such cases. You could check in this macro if either Apache status or ACME require mod_rewrite and then use this. Maybe introduce a array of booleans where True means mod_rewrite is needed, then use that array to generate the directives. Complexity of this should be manageable. In the end, debops.nginx already does exactly that. Ref: /etc/nginx/snippets/acme-challenge.conf.j2

drybjed · 2017-02-09T08:30:29Z

I don't believe that HTTP -> HTTPS redirect will ever go away, in this field that rarely happens. Even in 100+ years servers will have it. Only reasonable way to make it less needed would be if HTTP clients were changed to try and connect to https port first and then fallback to http, but that's unlikely and breaks conventions. An alternative would be something like STARTTLS for websites but that still needs to be made stronger and requires client support. So the redirect should be kept.

Maybe an alternative would be to use nginx beside apache2 to redirect HTTP connections to HTTPS. With current debops.nginx it should be farily easy, just disable all HTTPS support and listening on 443/tcp port. Although the whole setup will be a bit more complicated in terms of maintainability. Still, there's an idea.

ypid · 2017-02-09T08:57:46Z

Only reasonable way to make it less needed would be if HTTP clients were changed to try and connect to https port first and then fallback to http, but that's unlikely and breaks conventions.

Nice and consequent idea 👍 Don’t say that it is unlikely. I think with the trend in the last few years, this could happen in the foreseeable future.

Maybe an alternative would be to use nginx beside apache2 to redirect HTTP connections to HTTPS.

Is an idea at least, but I assume not one that we would like/recommend. For clarification, nginx would listen on 80/tcp and Apache would listen on 443/tcp (not supported/tested by debops.apache currently).

muelli · 2017-02-09T13:27:36Z

I think there is no reason for removing the HTTP redirect default.

note that I have not proposed that. My proposal was to make it easy to disable it if it gets in the way. As far as I understand, #8 does not provide an easy way to disable it. It would be easy to add, though.

That is why get_vhost_content_directives uses the rewrite module in such cases. You could check in this macro if either Apache status or ACME require mod_rewrite and then use this. Maybe introduce a array of booleans where True means mod_rewrite is needed, then use that array to generate the directives. Complexity of this should be manageable.

If I understand correctly then you think of introducing more template code to check for ACME directives in order to make it work.
Do you want me to try to code something up? Or do you think it should be possible to configure ACME compatibility with the currently proposed changes?

I predict that there will be cases where the redirect by default will get in the way and in which it will be beneficial to have an option to disable redirect by default. The cost of creating and maintaining such a switch should be low. So my feeling is that the benefits of such a mechanism outweigh the costs.

ypid · 2017-02-09T14:45:05Z

As far as I understand, #8 does not provide an easy way to disable it. It would be easy to add, though.

The cost of creating and maintaining such a switch should be low. So my feeling is that the benefits of such a mechanism outweigh the costs.

Sure, but your use case is not a good example because this use case is supposed to have native support by the role. Any other good examples?

Do you want me to try to code something up?

That would be great! I have not had time to look into this yet. But note that this is a bit of work to do it properly: Checking debops.nginx, implementing it for this role, writing documentation and updating debops.pki.

muelli · 2017-02-09T15:16:33Z

Sure, but your use case is not a good example because this use case is supposed to have native support by the role. Any other good examples?

As I've said: I believe that there will be an instance in the future where one wishes for the automatic redirection to be disabled. If only because the configuration is unexpectedly incompatible with something else.

So I see value in being able to disable the role's redirection facilities with a simple switch even if it's not about this very ACME use case. Especially because I think that the value comes at a surprisingly low cost.

A more concrete issue I'm seeing right now is that one might simply don't want to upgrade using HTTP redirects.

Do you want me to try to code something up?

That would be great!

As I've indicated: It's probably too complex for me. I might give it a shot, but don't hold your breath. It's likely that I will go for disabling the role's http redirect in its current form because I think it's too expensive in terms of complexity for what it achieves.

ypid

All right, I guess this global options might have it’s use cases and as you said it comes at a low cost. I just don’t want to add variables from which use case I am not convinced and I probably started of on the wrong foot because your use case is supposed to be handled properly/differently.

Thanks for your work!

Please mention the new variable in the changelog.

ypid · 2017-02-10T07:54:19Z

defaults/main.yml

+#
+# Each vhost can define a boolean ``redirect_to_https``.
+# If not set, it defaults to the value defined in ``apache__redirect_to_https``.
+# Defaults to ``True``.


Defaults to True.

Seems redundant as the variable is a simple assignment it is clear what the default is 😉

Ah, yeah. I wasn't sure how to handle that. I think I wanted to express that the ultimate decision is made based on the other variable but also make the documentation useful, e.g. to make it show the actual value that's relevant to the user. But I don't like the redundancy.
What do you think should be done here? Make it Defaults to apache__redirect_to_https and remove the previous sentence?

This is the defaults/main.yml and basically the last resort default, there is no more fallback default for this. You can simply remove the "Defaults to True." sentence I would say.

I've made a change. I hope it's better now.

ypid · 2017-02-10T08:00:33Z

templates/etc/apache2/sites-available/default.conf.j2

@@ -12,7 +12,8 @@

 {% if item.redirect_http|d() %}
    {{ apache__tpl_macros.get_redirect( item.redirect_http_code|d(307), "/", item.redirect_http) | indent(4) }}
-{% elif item.redirect_to_https|d(True) | bool %}
+{% elif item.redirect_to_https|default(apache__redirect_to_https) | bool %}


d for default is quite common in DebOps since we use the default filter all day long. Can you change that back?

I can. I think it's not good though. I, for one, needed to find out what d stands for. And as the only benefit of that shorthand, I see six saved characters. The cost of having to type and read extra six characters will have amortised as soon as only a single person will not have to look up what d means.
But of course, my voice is not worth much because I'm not the one who has to maintain that ;-)

I know what you mean and I generally prefer explicit over implicit and I know that it can be difficult to lookup a macro name of one character but I think that one lookup is worth it as it stands against 4645 uses in DebOps currently.

~/.local/share/debops/debops-playbooks/roles $ ag '\|\s*d\(' --stats-only 4645 matches 400 files contained matches 2338 files searched 8921162 bytes searched

Your voice is very appreciated and valued! Of course we all have our opinions how to do things and I hope we can find a solution we can all agree on.

ypid · 2017-02-10T08:00:45Z

templates/etc/apache2/sites-available/default.conf.j2

@@ -12,7 +12,8 @@

 {% if item.redirect_http|d() %}
    {{ apache__tpl_macros.get_redirect( item.redirect_http_code|d(307), "/", item.redirect_http) | indent(4) }}
-{% elif item.redirect_to_https|d(True) | bool %}
+{% elif item.redirect_to_https|default(apache__redirect_to_https) | bool %}
+    # Item's redirect: {{ item.redirect_to_https | default() }}   default: {{ apache__redirect_to_https }}


Please remove the debugging code.

I like seeing the values on which debops makes decisions when something fails, because I find it quite annoying to find out why something happens and why not.
But I can understand that it's not everyone's cup of tea.
I will remove it.

I also like that but normally it should not be needed. But I think you are on to something and I am open to it but it should be optional and turned of by default. Then this would be a very handy feature I agree. Want to add apache__template_debugging boolean to make it happen?

yeah, maybe in a separate pull request. I've removed that statement for now.

ypid · 2017-02-10T10:44:02Z

CHANGES.rst

@@ -17,6 +17,9 @@ debops.apache v0.1.0 - unreleased
 Added
 ~~~~~

+- Added a apache__redirect_to_https to control the role's default behaviour for
+  redirecting to https. [muelli_]


You are not yet defined in 80post.rst, feel free to add yourself to the file and run make includes/global.rst to get docs working.

Added a apache__redirect_to_https to control →

Add :envvar:`apache__redirect_to_https to control`

or

Add ``apache__redirect_to_https to control``

and debops-optimize will take care of it.

hrm. creating that entry seems to require a non trivial addition to debops-keyring, too. Is the procedure defined anywhere? Anyway, I've tried to create the necessary pull requests.

But there is no change the file here requires, right?

Right, no change is needed in https://github.com/debops/ansible-apache.

ypid · 2017-02-10T15:49:06Z

A more concrete issue I'm seeing right now is that one might simply don't want to upgrade using HTTP redirects.

How else would you do it then while being in compliance with the RFC?

An HSTS Host MUST NOT include the STS header field in HTTP responses conveyed over non-secure transport.

muelli · 2017-02-10T17:18:21Z

You seem to be assuming that one wants to use HSTS. That's not necessarily the case with someone who just doesn't want the requests to be redirected.

Currently, each vhost can define "redirect_to_https". If True, which is the current default, then the Apache configuration will include a Redirect line. This is good, except that VirtualHost rules seem to have precedence over other rules. That is annoying when you want to define a global rule to serve, say, ACME requests, because Apache would respect the VirtualHost rules and redirect the plain HTTP request instead of serving the token. To help debops users to not having to touch each and every defined vhost, the new variable defines the default value for redirect_to_https, which used to be "True". The default is still "True", but now you can override that easily.

ypid · 2017-02-14T09:59:23Z

defaults/main.yml

@@ -465,7 +465,14 @@ apache__combined_snippets: '{{ apache__dependent_snippets
 apache__https_enabled: '{{ ansible_local|d() and ansible_local.pki|d() and
                           (ansible_local.pki.enabled|d() | bool) and
                           apache__https_listen|length > 0 }}'
+                                                                   # ]]]
+


There is a linting tool available called yaml4rst which implements the current format. Can you give it a try?

hm. doesn't look too good:

>/tmp/ansiblepip/bin/yaml4rst -e ansible_full_role_name=debops.apache defaults/main.yml Traceback (most recent call last): File "/tmp/ansiblepip/bin/yaml4rst", line 11, in <module> load_entry_point('yaml4rst==0.1.3', 'console_scripts', 'yaml4rst')() File "/tmp/ansiblepip/local/lib/python2.7/site-packages/yaml4rst/cli.py", line 145, in main reformatter.reformat() File "/tmp/ansiblepip/local/lib/python2.7/site-packages/yaml4rst/reformatter.py", line 150, in reformat yaml.load(self.get_content()) File "/tmp/ansiblepip/local/lib/python2.7/site-packages/yaml4rst/reformatter.py", line 116, in get_content return '\n'.join(self._lines) UnicodeDecodeError: 'ascii' codec can't decode byte 0xe2 in position 24: ordinal not in range(128)

Seems to be that yaml4rst is run by Python2 which is unsupported. Please run it with Python3.

Do you need further help? Have you had time to check with Python3?

ypid

Can you have a look at one minor detail? I would consider to merge this independently of debops/debops-keyring#17

ypid · 2017-02-24T23:16:25Z

Merged, thanks!

ypid added the question label Feb 7, 2017

ypid requested changes Feb 10, 2017

View reviewed changes

muelli force-pushed the redirect branch from 8dbc8df to 49c9cc1 Compare February 10, 2017 09:58

ypid reviewed Feb 10, 2017

View reviewed changes

This was referenced Feb 10, 2017

added Tobias Mueller debops/debops-keyring#17

Closed

Added Tobias Mueller debops/docs#258

Merged

muelli force-pushed the redirect branch 2 times, most recently from 5f36807 to 5b92d8f Compare February 13, 2017 21:54

muelli added 3 commits February 14, 2017 02:54

fixed simple typo in apache template macros

146a3ec

added a changelog entry

1982f74

muelli force-pushed the redirect branch from 5b92d8f to 1982f74 Compare February 14, 2017 02:22

ypid reviewed Feb 14, 2017

View reviewed changes

ypid requested changes Feb 14, 2017

View reviewed changes

ypid added enhancement and removed question labels Feb 20, 2017

fixup whitespace issues reported by linter

136af82

ypid merged commit 136af82 into debops:master Feb 24, 2017

ypid added a commit that referenced this pull request Feb 24, 2017

Fix reST syntax in #9 using debops-optimize

6f3107f

ypid added a commit that referenced this pull request Feb 24, 2017

Merge branch '#9'

730f16f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added apache__redirect_to_https variable #9

Added apache__redirect_to_https variable #9

muelli commented Feb 7, 2017

muelli commented Feb 7, 2017

ypid commented Feb 7, 2017

muelli commented Feb 8, 2017

ypid commented Feb 8, 2017

muelli commented Feb 8, 2017

ypid commented Feb 9, 2017 •

edited

Loading

drybjed commented Feb 9, 2017

ypid commented Feb 9, 2017 •

edited

Loading

muelli commented Feb 9, 2017

ypid commented Feb 9, 2017

muelli commented Feb 9, 2017

ypid left a comment

ypid Feb 10, 2017

muelli Feb 10, 2017

ypid Feb 10, 2017

muelli Feb 10, 2017

ypid Feb 10, 2017

muelli Feb 10, 2017

ypid Feb 10, 2017

ypid Feb 10, 2017

muelli Feb 10, 2017

ypid Feb 10, 2017

muelli Feb 10, 2017

ypid Feb 10, 2017

muelli Feb 10, 2017

ypid Feb 10, 2017 •

edited

Loading

ypid commented Feb 10, 2017

muelli commented Feb 10, 2017

ypid Feb 14, 2017

muelli Feb 14, 2017

ypid Feb 14, 2017

ypid Feb 20, 2017

ypid left a comment •

edited

Loading

ypid commented Feb 24, 2017

Added apache__redirect_to_https variable #9

Added apache__redirect_to_https variable #9

Conversation

muelli commented Feb 7, 2017

muelli commented Feb 7, 2017

ypid commented Feb 7, 2017

muelli commented Feb 8, 2017

ypid commented Feb 8, 2017

muelli commented Feb 8, 2017

ypid commented Feb 9, 2017 • edited Loading

drybjed commented Feb 9, 2017

ypid commented Feb 9, 2017 • edited Loading

muelli commented Feb 9, 2017

ypid commented Feb 9, 2017

muelli commented Feb 9, 2017

ypid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ypid Feb 10, 2017 • edited Loading

Choose a reason for hiding this comment

ypid commented Feb 10, 2017

muelli commented Feb 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ypid left a comment • edited Loading

Choose a reason for hiding this comment

ypid commented Feb 24, 2017

ypid commented Feb 9, 2017 •

edited

Loading

ypid commented Feb 9, 2017 •

edited

Loading

ypid Feb 10, 2017 •

edited

Loading

ypid left a comment •

edited

Loading