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

Move security-related HTTP response headers from Rust to nginx.conf #2100

Merged
merged 3 commits into from
Feb 13, 2020
Merged

Move security-related HTTP response headers from Rust to nginx.conf #2100

merged 3 commits into from
Feb 13, 2020

Conversation

kzys
Copy link
Contributor

@kzys kzys commented Jan 7, 2020

We'd like to have these headers on the FastBoot server as well.

@rust-highfive
Copy link

r? @carols10cents

(rust_highfive has picked a reviewer for you, use r? to override)

@kzys
Copy link
Contributor Author

kzys commented Jan 7, 2020

Regarding Content-Security-Policy, we could either 1) implement a similar logic on nginx.conf.erb or 2) introduce a new environment variable just for this purpose.

@kzys kzys mentioned this pull request Jan 7, 2020
Closed
19 tasks
@kzys
Copy link
Contributor Author

kzys commented Jan 27, 2020

I'm going to implement the URL construction logic by reading S3_CDN, S3_BUCKET and S3_REGION from nginx.conf.erb. Please let me know we do want to avoid the duplication of the logic!

@kzys
Move static HTTP response headers to nginx.conf
728f1d2 
We'd like to have these headers on the FastBoot server as well.
@kzys kzys force-pushed the nginx-headers branch 2 times, most recently from 6e401b5 to 083bf7f Compare January 29, 2020 15:57
jtgeibel
jtgeibel requested changes Feb 6, 2020
View reviewed changes
Copy link
Member

@jtgeibel jtgeibel left a comment

Choose a reason for hiding this comment

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

I think the entire security headers middleware can be removed now that this PR moves the CSP logic into nginx. With that change (and the other comment below) I'll upload to staging to test.

config/nginx.conf.erb Outdated Show resolved Hide resolved
@kzys
Add Content-Security-Policy on nginx.conf.erb
ece67ec 
It allows us to share the header between Rust and FastBoot server.
@kzys
Completely remove SecurityHeaders middleware
93ea166 
Now the functionality is owned by Nginx.
@kzys kzys changed the title Move static HTTP response headers to nginx.conf Move security-related HTTP response headers from Rust to nginx.conf Feb 6, 2020
@jtgeibel
Copy link
Member

LGTM!

@bors r+

@bors
Copy link
Contributor

bors commented Feb 13, 2020

📌 Commit 93ea166 has been approved by jtgeibel

bors added a commit that referenced this pull request Feb 13, 2020
@bors
Auto merge of #2100 - kzys:nginx-headers, r=jtgeibel
c82d5db 
Move security-related HTTP response headers from Rust to nginx.conf

We'd like to have these headers on the FastBoot server as well.
@bors
Copy link
Contributor

bors commented Feb 13, 2020

⌛ Testing commit 93ea166 with merge c82d5db...

@bors
Copy link
Contributor

bors commented Feb 13, 2020

☀️ Test successful - checks-travis
Approved by: jtgeibel
Pushing c82d5db to master...

@bors bors merged commit 93ea166 into rust-lang:master Feb 13, 2020
@bors bors mentioned this pull request Feb 13, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jtgeibel jtgeibel jtgeibel requested changes

Assignees

@carols10cents carols10cents

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@kzys @rust-highfive @jtgeibel @bors @carols10cents