Skip to content
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

Add support for including a Strict-Transport-Security header (for use only when a proxy or load balancer is handling TLS/SSL) (was PR # 85 for drdrew42 repo) #6

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

taniwallach
Copy link
Member

Same as drdrew42#85


Add support for including a Strict-Transport-Security header.

This header is meant to force browsers to only contact site via TLS/SSL ("https"). Using this header is a commonly recommended security practice, but is dangerous should the site have any need to work over plain (port 80) HTTP.

The value for the header is provided in render.conf as a string value called HSTS_HEADER, and when that value is not provided (or is false for Perl purposes) no Strict-Transport-Security header will be set.

No default value is being provided in render.conf.dist so the header will not be enabled by accident.

The header should only be used on a server which is available via a proxy or load balancer which has a valid SSL certificate and handles the TLS/SSL level (and which will continue to do so for the long-term).

Note: It could be that some proxy setups could add the header at the proxy, but the AWS Elastic Application Load Balancer does not have the option to add this header, but can provide SSL termination, so the header needs to be added on the "back end" (in our case the Standalone renderer).

This header is meant to force browsers to only contact site via TLS/SSL
("https"). Using this header is a commonly recommended security
practice, but is dangerous should the site have any need to work over
plain (port 80) HTTP.

The value for the header is provided in render.conf as a string value
called HSTS_HEADER, and when that value is not provided (or is "false"
for Perl purposes) no Strict-Transport-Security header will be set.

No default value is being provided in render.conf.dist so the header will
not be enabled by accident.

The header should only be used on a server which is available via a proxy
or load balancer which has a valid SSL certificate and handles the TLS/SSL
level (and which will continue to do so for the long-term).
@taniwallach taniwallach requested a review from drgrice1 July 22, 2022 14:28
@taniwallach taniwallach requested a review from drdrew42 as a code owner July 22, 2022 14:28
@taniwallach taniwallach changed the base branch from main to develop July 22, 2022 14:38
);
});
}

Copy link
Member

Choose a reason for hiding this comment

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

Rather than adding a separate before_dispatch hook to the CORS settings, just use the same hook. We don't want a bunch of separate hooks like this. Although, I don't think either of these belong in the renderer code. This is something that should be set by the proxy.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not using a proxy. The renderer seems to work reasonably well without one, and in my use case is behind an AWS application load balancer which does SSL offloading.

Copy link
Member

Choose a reason for hiding this comment

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

You should be running hypnotoad behind some sort of proxy. In any case, see my later comment.

@drgrice1
Copy link
Member

I think that what would be best here, rather than adding a hook and config setting for each possible header that someone might want to add, would be to add a single config array of headers and a single hook that sets any that are in that array. Previously you added the CORS header, and now you are adding this particular security header. Someone else might want another header, and decide to add another setting in the conf file and another hook. That just doesn't make sense, and we can't predict what custom headers someone might want. So if we make a general config array that someone can add any header they want to, and implement a single hook to add any headers therein it will be more versatile, and won't require changing the config file structure and code any further.

@taniwallach taniwallach added the do not merge Needs revision or should be replaced label Aug 1, 2022
@taniwallach taniwallach removed the request for review from drdrew42 August 1, 2022 07:15
@taniwallach
Copy link
Member Author

I think that what would be best here, rather than adding a hook and config setting for each possible header that someone might want to add, would be to add a single config array of headers and a single hook that sets any that are in that array. Previously you added the CORS header, and now you are adding this particular security header. Someone else might want another header, and decide to add another setting in the conf file and another hook. That just doesn't make sense, and we can't predict what custom headers someone might want. So if we make a general config array that someone can add any header they want to, and implement a single hook to add any headers therein it will be more versatile, and won't require changing the config file structure and code any further.

I think that is a very good idea. I will try to implement such an approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do not merge Needs revision or should be replaced
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants