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

Feat/1544 add nginx configuration increase security performance #2173

Conversation

raftmsohani
Copy link

@raftmsohani raftmsohani commented Oct 3, 2022

Summary of Changes

The Nginx CSP and server configuration need to be revised and correctly documented for both local and Cloud.gov deployment to increase security.

Pull request closes #1544 , #1543 and #2031

How to Test

source commands.sh
tdrs-start

It is important to use commands.sh to start the application since local deployment needs external network to be created first to connect the backend and frontend docker-compose configs.

Deliverables

More details on how deliverables herein are assessed included here.

  1. Nginx configuration for local and cloud.gov
  2. Added documentation and comments for Nginx configs
  3. Gunicorn configuration and documentation
  4. Separated local and cloud.gov configurations for both Nginx and Gunicorn

Deliverable 1: Accepted Features

Checklist of ACs:

  • [insert ACs here]
  • lfrohlich and/or adpennington confirmed that ACs are met.

Deliverable 2: Tested Code

  • Are all areas of code introduced in this PR meaningfully tested?
    • If this PR introduces backend code changes, are they meaningfully tested?
    • If this PR introduces frontend code changes, are they meaningfully tested?
  • Are code coverage minimums met?
    • Frontend coverage: [insert coverage %] (see CodeCov Report comment in PR)
    • Backend coverage: [insert coverage %] (see CodeCov Report comment in PR)

Deliverable 3: Properly Styled Code

  • Are backend code style checks passing on CircleCI?
  • Are frontend code style checks passing on CircleCI?
  • Are code maintainability principles being followed?

Deliverable 4: Accessible

  • Does this PR complete the epic?
  • Are links included to any other gov-approved PRs associated with epic?
  • Does PR include documentation for Raft's a11y review?
  • Did automated and manual testing with iamjolly and ttran-hub using Accessibility Insights reveal any errors introduced in this PR?

Deliverable 5: Deployed

  • Was the code successfully deployed via automated CircleCI process to development on Cloud.gov?

Deliverable 6: Documented

  • Does this PR provide background for why coding decisions were made?
  • If this PR introduces backend code, is that code easy to understand and sufficiently documented, both inline and overall?
  • If this PR introduces frontend code, is that code easy to understand and sufficiently documented, both inline and overall?
  • If this PR introduces dependencies, are their licenses documented?
  • Can reviewer explain and take ownership of these elements presented in this code review?

Deliverable 7: Secure

  • Does the OWASP Scan pass on CircleCI?
  • Do manual code review and manual testing detect any new security issues?
  • If new issues detected, is investigation and/or remediation plan documented?

Deliverable 8: User Research

Research product(s) clearly articulate(s):

  • the purpose of the research
  • methods used to conduct the research
  • who participated in the research
  • what was tested and how
  • impact of research on TDP
  • (if applicable) final design mockups produced for TDP development

@raftmsohani raftmsohani self-assigned this Oct 28, 2022
@codecov
Copy link

codecov bot commented Nov 27, 2022

Codecov Report

Merging #2173 (abd8226) into develop (8b9dc4e) will not change coverage.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #2173   +/-   ##
========================================
  Coverage    94.13%   94.13%           
========================================
  Files           96       96           
  Lines         2846     2846           
  Branches       263      263           
========================================
  Hits          2679     2679           
  Misses         120      120           
  Partials        47       47           
Flag Coverage Δ
dev-backend 94.14% <100.00%> (ø)
dev-frontend 94.11% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
tdrs-backend/tdpservice/settings/common.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8b9dc4e...abd8226. Read the comment docs.

@raftmsohani raftmsohani added Deploy with CircleCI-raft Deploy to https://tdp-frontend-raft.app.cloud.gov through CircleCI and removed Deploy with CircleCI-raft Deploy to https://tdp-frontend-raft.app.cloud.gov through CircleCI labels Dec 6, 2022
@raftmsohani raftmsohani removed the Deploy with CircleCI-raft Deploy to https://tdp-frontend-raft.app.cloud.gov through CircleCI label Dec 9, 2022

In general for TDRS, the *Nginx* handles incoming HTTP requests to both frontend and backend. The frontend requests are handled directly while the backend requests are forwarded to the backend server.

![Cloud.gov Architecture](./src/arch1.jpg)
Copy link
Collaborator

Choose a reason for hiding this comment

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

We might be able to keep a single diagram and just update this one:

https://github.com/raft-tech/TANF-app/blob/develop/docs/Security-Compliance/boundary-diagram.md

@andrew-jameson andrew-jameson added Deploy with CircleCI-raft Deploy to https://tdp-frontend-raft.app.cloud.gov through CircleCI and removed Deploy with CircleCI-raft Deploy to https://tdp-frontend-raft.app.cloud.gov through CircleCI labels Jan 4, 2023
@andrew-jameson
Copy link
Collaborator

@raftmsohani Can you verify that the ip whitelist allows the territories (Puerto Rico, Guam, Virgin Islands)? Credit to @ADPennington

@@ -93,7 +93,7 @@ services:
bash -c "./wait_for_services.sh &&
./gunicorn_start.sh && celery -A tdpservice.settings worker -l info"
ports:
- "8080:8080"
#- "8080:8080"
Copy link
Author

Choose a reason for hiding this comment

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

Port assignment should be removed -> delete #- "8080:8080"

Comment on lines +24 to +47
location = /profile {
index index.html index.htm;
try_files $uri $uri/ /index.html;
}

location = /home {
index index.html index.htm;
try_files $uri $uri/ /index.html;
}

location ^~ /data-files {
index index.html index.htm;
try_files $uri $uri/ /index.html;
}

location = /login {
index index.html index.htm;
try_files $uri $uri/ /index.html;
}

location = / {
index index.html index.htm;
try_files $uri $uri/ /index.html;
}
Copy link
Collaborator

@andrew-jameson andrew-jameson Jan 9, 2023

Choose a reason for hiding this comment

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

Can these locations be collapsed into one block? They appear identical.

EDIT: Looks like this was the same as from prior locations.conf. It's not a real change but didn't notice initially since we don't have before/after diff on several files.

Copy link
Author

Choose a reason for hiding this comment

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

it can be done for sure. An easy fix

The before after missing is because of file relocation -> moved some of the files to another directory

Copy link
Collaborator

Choose a reason for hiding this comment

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

The before after missing is because of file relocation -> moved some of the files to another directory

Did you just manually move it vs using git mv? Minor nit

Copy link
Author

Choose a reason for hiding this comment

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

Virgin Islands

I bet the test will fail because I will have to add the following lists:

https://lite.ip2location.com/virgin-islands-(u.s.)-ip-address-ranges https://lite.ip2location.com/guam-ip-address-ranges?lang=en_US

Virgin Islands

I bet the test will fail because I will have to add the following lists:

https://lite.ip2location.com/virgin-islands-(u.s.)-ip-address-ranges https://lite.ip2location.com/guam-ip-address-ranges?lang=en_US

Added both location to whitelist

@@ -0,0 +1,81 @@

Copy link
Collaborator

Choose a reason for hiding this comment

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

Before merging, I would want to deploy-to/test-in prod and would want to coordinate this with @ADPennington and setup maintenance window w/ UX

# prevent MIME sniffing
add_header X-Content-Type-Options: nosniff;

# CSP header options. All options are set either to none or self except
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like the second half of this sentence got missed

Copy link
Author

Choose a reason for hiding this comment

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

Good catch

Comment on lines +1 to +10
#location ~ \.(css)$ {
# add_header Content-Type text/css;
#}

#location = /nginx_status {
# stub_status on;
# access_log off;
# deny all;
#}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
#location ~ \.(css)$ {
# add_header Content-Type text/css;
#}
#location = /nginx_status {
# stub_status on;
# access_log off;
# deny all;
#}

Copy link
Author

Choose a reason for hiding this comment

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

resolved!

Copy link
Author

Choose a reason for hiding this comment

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

kept the /nginx_status for local.

set $CSP "${CSP}media-src 'none';";
set $CSP "${CSP}prefetch-src 'none';";
set $CSP "${CSP}form-action 'none';";
set $CSP "${CSP}script-src-elem 'self' http://localhost:* http://www.w3.org;";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
set $CSP "${CSP}script-src-elem 'self' http://localhost:* http://www.w3.org;";
set $CSP "${CSP}script-src-elem 'self' http://localhost:*;";

Are we sure we want the w3 part in there? Confused on why. Are we pulling JS/CSS?

Copy link
Author

Choose a reason for hiding this comment

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

I believe the icons needed w3. In any case, this should be safe.

Copy link
Author

Choose a reason for hiding this comment

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

removed as suggested!

'"http_user_agent": $http_user_agent, '
'"cookies=$http_cookie;" "server=$server_name" "http_host=$http_host"';

limit_req_zone $binary_remote_addr zone=limitreqsbyaddr:20m rate=10r/s;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think I commented on this elsewhere already but I think we might need to stress test or up the rate-limit here.

Copy link
Author

Choose a reason for hiding this comment

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

We discussed this briefly and I think we should have to increase the limit. I have done some testing and got a rejection only once. But one caveat is: when frontend auth check function gets rejected, if user is already logged in, the backend will log them out.

Copy link
Author

Choose a reason for hiding this comment

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

Increased the rate! Resolved

Comment on lines +1 to +10
#location ~ \.(css)$ {
# add_header Content-Type text/css;
#}

#location = /nginx_status {
# stub_status on;
# access_log off;
# deny all;
#}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
#location ~ \.(css)$ {
# add_header Content-Type text/css;
#}
#location = /nginx_status {
# stub_status on;
# access_log off;
# deny all;
#}

Copy link
Author

Choose a reason for hiding this comment

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

Resolved!

@raftmsohani
Copy link
Author

@raftmsohani Can you verify that the ip whitelist allows the territories (Puerto Rico, Guam, Virgin Islands)? Credit to @ADPennington

I will see what can be done here! will update this post once done!

@@ -117,6 +117,7 @@ update_backend()
cf map-route tdp-backend-prod api-tanfdata.acf.hhs.gov
Copy link
Author

Choose a reason for hiding this comment

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

Add map-route to prod

Copy link
Author

Choose a reason for hiding this comment

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

RESOLVED!

# deny all;
#}

location ^~ /v1/ {
Copy link
Author

Choose a reason for hiding this comment

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

Have to add /admin location

@andrew-jameson
Copy link
Collaborator

@raftmsohani Can you verify that the ip whitelist allows the territories (Puerto Rico, Guam, Virgin Islands)? Credit to @ADPennington

I will see what can be done here! will update this post once done!

UX might be able to coordinate tests with Virgin Islands or Guam to test against develop/staging environment.

cc @ADPennington

@raftmsohani
Copy link
Author

raftmsohani commented Jan 10, 2023

Virgin Islands

I bet the test will fail because I will have to add the following lists:

https://lite.ip2location.com/virgin-islands-(u.s.)-ip-address-ranges
https://lite.ip2location.com/guam-ip-address-ranges?lang=en_US

@@ -1,10 +1,12 @@
types {
text/html html htm shtml;
text/css css;
text/css chunk.css;
Copy link
Author

Choose a reason for hiding this comment

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

This is really not a requirement, but I thought it is better to add to save us from future problem.

@raftmsohani raftmsohani changed the title [WIP] Feat/1544 add nginx configuration increase security performance Feat/1544 add nginx configuration increase security performance Jan 11, 2023
raftmsohani and others added 4 commits January 25, 2023 08:21
Co-authored-by: Andrew <84722778+andrew-jameson@users.noreply.github.com>
Co-authored-by: Andrew <84722778+andrew-jameson@users.noreply.github.com>
Co-authored-by: Andrew <84722778+andrew-jameson@users.noreply.github.com>
Co-authored-by: Andrew <84722778+andrew-jameson@users.noreply.github.com>
@andrew-jameson andrew-jameson changed the base branch from develop to 1544-Nginx-with-domain April 10, 2023 19:41
@andrew-jameson
Copy link
Collaborator

Out of date per @raftmsohani. Please see #2449

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend Deploy with CircleCI-raft Deploy to https://tdp-frontend-raft.app.cloud.gov through CircleCI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[SPIKE] Add Nginx configuration - increase security and performance
2 participants