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.

Sign up for GitHub

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

Jump to bottom

Enforcing HTTPS enhancements #9044

Merged
merged 6 commits into from
Oct 23, 2018
Merged

Enforcing HTTPS enhancements #9044

merged 6 commits into from
Oct 23, 2018

Conversation

guardrex
Copy link
Collaborator

@guardrex guardrex commented Oct 16, 2018 •
edited
Loading

Addresses #6538

Internal Review Topic

Organize and enhance the coverage. I've marked this "addresses" for now, but we might find out that other issues cover the remaining parts of #6538 and this PR can close the issue.

Numbering the items from #6538 for reference ...

  1. Setting up HTTPS
    a. Cross-link to existing server specific guidance on enabling HTTPS
    b. Cross-link to the proxy guidance especially around validating forwarded headers Addressed by Forwarded headers updates for Nginx/Apache topics #6644.
  2. Enforcing HTTPS in Azure App Service
  3. Enforcing HTTPS behind other proxies
  4. Surface IIS hosting issues in the IIS topic pertaining to use of the middleware when the site isn't configured for HTTPS in IIS

There are other related issues on this subject, and I don't think we should try to address all of them on this PR.

@guardrex guardrex force-pushed the guardrex/enforce-https branch from ce1aaa6 to 7e89fd3 Compare October 16, 2018 01:11
@guardrex guardrex changed the title Enforcing HTTPS enhancements [WIP] Enforcing HTTPS enhancements Oct 16, 2018
@guardrex guardrex added the WIP label Oct 16, 2018
@guardrex guardrex changed the title [WIP] Enforcing HTTPS enhancements Enforcing HTTPS enhancements Oct 16, 2018
@guardrex guardrex removed the WIP label Oct 16, 2018
@guardrex
Enforcing HTTPS enhancements
a3bb4c4 
Add links

Fix bookmark

Updates
@guardrex guardrex force-pushed the guardrex/enforce-https branch from d982b32 to a3bb4c4 Compare October 16, 2018 19:24
@shirhatti
Copy link
Contributor

I really don't like the structure of this document. It seems to be focused around the RequireHttpsAttribute which only works in MVC whereas the URL Rewriting middleware is just mentioned in passing.

From this doc, it is by no means obvious that adding the RequireHttpAttribute even globally via MvcOptions does nothing to mitigate static files (as an example of another middleware) being served without TLS

I'd rather we focus on the URL Rewriting middleware, and mention using the RequireHttpsAttribute more as a defense in depth mechanism

@guardrex
Copy link
Collaborator Author

focus on the URL Rewriting middleware, and mention using the RequireHttpsAttribute more as a defense in depth mechanism

I'll give that a shot on the next pass.

@guardrex guardrex changed the title Enforcing HTTPS enhancements [WIP] Enforcing HTTPS enhancements Oct 17, 2018
@guardrex guardrex added the WIP label Oct 17, 2018
@Tratcher
Copy link
Member

Tratcher commented Oct 18, 2018 •
edited
Loading

@shirhatti did you read the 1.0 version of the doc? The 2.2 version says "Do not use RequireHttpsAttribute", use UseHttpsRedirection instead.

Tratcher
Tratcher approved these changes Oct 18, 2018
View reviewed changes
@guardrex guardrex changed the title [WIP] Enforcing HTTPS enhancements Enforcing HTTPS enhancements Oct 19, 2018
@guardrex guardrex removed the WIP label Oct 19, 2018
@guardrex guardrex requested a review from scottaddie October 19, 2018 13:36
scottaddie
scottaddie reviewed Oct 19, 2018
View reviewed changes
@guardrex
Copy link
Collaborator Author

@shirhatti Do you need doc structural changes (or was it the 1.x version loading)?

@shirhatti
Copy link
Contributor

Do you need doc structural changes (or was it the 1.x version loading)?

🤦‍♂️ Turns out I was looking at the wrong version of the doc.

@guardrex
Copy link
Collaborator Author

@scottaddie I'll come back to this shortly and check the comments ... I'm still buried in Health Checks code atm.

@guardrex guardrex mentioned this pull request Oct 23, 2018
Closed
@guardrex
Copy link
Collaborator Author

@scottaddie I fixed up those clickjacking sections into ordered lists as you suggested ... good call. 👍

I'm going to merge this after the build completes.

@guardrex
Copy link
Collaborator Author

@scottaddie Unapproved tab names. I'll fix those up first ... then merge it after it passes.

@guardrex guardrex merged commit a5b3914 into master Oct 23, 2018
@delete-merged-branch delete-merged-branch bot deleted the guardrex/enforce-https branch October 23, 2018 18:03
@guardrex guardrex mentioned this pull request Oct 24, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Tratcher Tratcher Tratcher approved these changes

@shirhatti shirhatti shirhatti approved these changes

@scottaddie scottaddie scottaddie approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@guardrex @shirhatti @Tratcher @scottaddie