Skip to content
This repository has been archived by the owner on Dec 9, 2021. It is now read-only.

22: Enable SSL redirect and other security settings #203

Merged

Conversation

stevejalim
Copy link
Contributor

@stevejalim stevejalim commented Sep 23, 2019

This changeset deals with the need for an application-level redirect from HTTP to HTTPS without causing a redirect loop.

It also enables all-but-one of the recomended Django security settings as defaults (HSTS being the exception.)

It also deals with a regression that occurs with wagtail-bakery when that SSL redirect is enabled.

When deployed, it should resolve issue #22.

Key changes:

  • Enable recommended-by-Django security settings as defaults

    The base settings now follow all-but-one of the recommendations from manage.py check --deploy.
    The only one that's not been done in this changeset is SECURE_HSTS_SECONDS, because of the risk of "serious, irreversible problems". That needs to be planned in to enable it properly.

    Note that we're explicitly setting what HTTP header to look for to detect the SSL-forwarded header, which should stop the redirect loop in production.

    Development settings turn off SSL redirect, because the local build isn't set to use HTTPS. (It could be tweaked to use HTTPS, too - but that's out of scope for this piece)

  • Address issue with wagtail-bakery where SECURE_SSL_REDIRECT=True bakes out empty HTML

    This commit subclasses wagtail-bakery's AllPublishedPagesView in a way that detects
    application-level SSL redirection in order to avoid an issue where rendered pages end up
    being 0 bytes.

    See Generated pages are blank when SECURE_SSL_REDIRECT is enabled wagtail-nest/wagtail-bakery#24 for confirmation of the
    issue and the discussion on Create secure request when site port is 443 or SECURE_SSL_REDIRECT is set to True wagtail-nest/wagtail-bakery#25
    that points to a custom view being the (current) workaround. Ideally we'll be
    able to replace this when that issue is resolved.

    The code in this commit is basically taken from that closed PR, which adds the
    secure_request variable. Hat-tip to @loicteixeira - thanks!

    No unit test added, but manually tested locally to confirm this does indeed fix the
    static build while SECURE_SSL_REDIRECT is True

How to test

Because the local docker setup (currently?) has no HTTPS support, this will need to be tested in staging. However, it is possible to test that the static site can be baked out when SECURE_SSL_REDIRECT is True by using the same steps as from PR #195

  • check out local build
  • update the hostname of the Site in Wagtail admin to be something different from the default localhost and save
  • confirm the CMS site still responds (bounce docker then reload some CMS views, view live pages, etc) -- the Site change won't have broken that.
  • run the static build (just the build is enough - I don't think we need to push to S3 to test this) with an env setting for EXPORTED_SITE_URL that matches the domain you configured for the site - eg:
docker-compose exec -e EXPORTED_SITE_URL="https://example.net" app python manage.py build --settings=developerportal.settings.production -v3
  • shell into the app Docker container
  • cd into build
  • inspect the contents of index.html -- where previously you'd have seen localhost you will see the hostname you've configured for the main Site.

eg:

Screenshot 2019-09-20 at 15 33 13

Steve Jalim added 2 commits September 23, 2019 14:21
The base settings now follow all-but-one of the recommendations from manage.py `check --deploy`.
The only one that's not been done in this changeset is SECURE_HSTS_SECONDS, because of the risk of "serious, irreversible problems". That needs to be planned in to enable it properly.

Note that we're explicitly setting what HTTP header to look for to detect the SSL-forwarded header, which should stop the redirect loop in production.

Development settings turn off SSL redirect, because the local build isn't set to use HTTPS. (It could be tweaked to use HTTPS, too - but that's out of scope for this piece)
…bakes out empty HTML

This commit subclasses wagtail-bakery's `AllPublishedPagesView` in a way that detects
application-level SSL redirection in order to avoid an issue where rendered pages end up
being 0 bytes.

See wagtail-nest/wagtail-bakery#24 for confirmation of the
issue and the discussion on wagtail-nest/wagtail-bakery#25
that points to a custom view being the (current) workaround. Ideally we'll be
able to replace this when that issue is resolved.

The code in this commit is basically taken from that closed PR, which adds the
`secure_request` variable. Hat-tip to @loicteixeira - thanks!

No unit test added, but manually tested locally to confirm this does indeed fix the
static build while `SECURE_SSL_REDIRECT` is `True`
settings, "SECURE_SSL_REDIRECT", False
)
self.request = RequestFactory(SERVER_NAME=site.hostname).get(
self.get_url(obj), secure=secure_request
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

secure=secure_request and the creation of thesecure_request variable itself were the two things added in wagtail-nest/wagtail-bakery#25 -- the rest of the method is unchanged

@stevejalim
Copy link
Contributor Author

@limed Once this is approved and you're happy yourself, please feel free to deploy this at any point you want - we don't have to both be online at the same time. Worst-case, it'll be another revert, but am thinking this will sort the need for the redirect (and also show up any new issues that come from tighter default settings)

@stevejalim stevejalim changed the title 22 enable ssl redirect and other security settings 22: Enable SSL redirect and other security settings Sep 23, 2019
Copy link
Contributor

@limed limed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r+ on the secure header portions

@stevejalim stevejalim merged commit d308d32 into master Sep 25, 2019
@limed
Copy link
Contributor

limed commented Sep 26, 2019

See #232 unfortunately it was breaking health checks and readiness checks so i undid it again.

@stevejalim stevejalim deleted the 22--enable-ssl-redirect-and-other-security-settings branch September 26, 2019 09:58
@stevejalim stevejalim restored the 22--enable-ssl-redirect-and-other-security-settings branch October 8, 2019 11:14
@stevejalim stevejalim deleted the 22--enable-ssl-redirect-and-other-security-settings branch October 8, 2019 11:19
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants