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

config: simplify default set response headers #4196

Merged
merged 1 commit into from May 30, 2023

Conversation

calebdoxsey
Copy link
Contributor

Summary

Currently we selectively remove the Strict-Transport-Security header for routes that use a generated certificate. This is done to help prevent the occurrence of an error page that requires users to manually type thisisunsafe to bypass.

We do this by searching the entire list of certificates for any that match the route. If there are none, we assume the certificate used will be a generated one and remove the Strict-Transport-Security header.

This PR updates the code to instead look for any certificates at all. If at least one certificate is supplied, we will assume the user would prefer to have this header added. If none are supplied, we will assume Pomerium is running in an entirely generated mode where the inclusion of this header is more annoying than helpful.

Related issues

Checklist

  • reference any related issues
  • updated docs
  • updated unit tests
  • updated UPGRADING.md
  • add appropriate tag (improvement / bug / etc)
  • ready for review

@calebdoxsey calebdoxsey requested a review from a team as a code owner May 25, 2023 15:02
@calebdoxsey calebdoxsey requested a review from wasaga May 25, 2023 15:02
@calebdoxsey calebdoxsey changed the title config: simply default set response headers config: simplify default set response headers May 25, 2023
@coveralls
Copy link

coveralls commented May 25, 2023

Coverage Status

Coverage: 63.525% (-0.04%) from 63.561% when pulling 3deb6b9 on cdoxsey/simplify-hsts into d315e68 on main.

Copy link
Contributor

@kenjenkins kenjenkins left a comment

Choose a reason for hiding this comment

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

What does "generated certificate" mean in this context? Is this the fallback self-signed certificate, or something else?

@calebdoxsey
Copy link
Contributor Author

We have a catch-all certificate:

	return cryptutil.GenerateCertificate(sharedKey, "*")

Copy link
Contributor

@desimone desimone left a comment

Choose a reason for hiding this comment

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

Nice improvement to readability as well.

@calebdoxsey calebdoxsey merged commit a741cce into main May 30, 2023
13 checks passed
@calebdoxsey calebdoxsey deleted the cdoxsey/simplify-hsts branch May 30, 2023 23:44
calebdoxsey added a commit that referenced this pull request May 31, 2023
config: simplify default set response headers (#4196)

Co-authored-by: Caleb Doxsey <cdoxsey@pomerium.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants