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

Singular ClamAV instance #2660

Closed
wants to merge 35 commits into from
Closed

Singular ClamAV instance #2660

wants to merge 35 commits into from

Conversation

George-Hudson
Copy link

@George-Hudson George-Hudson commented Aug 14, 2023

…CAN enpoint to tanf-prod clam av scanner only

Summary of Changes

Provide a brief summary of changes
Pull request closes #2429 _

How to Test

Deploy the branch to one of the dev/staging instances and then follow the steps below:

  1. Open the app and sign in.
  2. Proceed with registration (if necessary)
  3. Once you have access to submit file, do so with one of the correct test files.

Deliverables

More details on how deliverables herein are assessed included here.

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

…CAN enpoint to tanf-prod clam av scanner only
@codecov
Copy link

codecov bot commented Aug 14, 2023

Codecov Report

Merging #2660 (9e2c437) into develop (8ec31f9) will decrease coverage by 0.25%.
Report is 41 commits behind head on develop.
The diff coverage is 92.48%.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2660      +/-   ##
===========================================
- Coverage    92.99%   92.75%   -0.25%     
===========================================
  Files          219      235      +16     
  Lines         4482     5201     +719     
  Branches       385      452      +67     
===========================================
+ Hits          4168     4824     +656     
- Misses         242      290      +48     
- Partials        72       87      +15     
Flag Coverage Δ
dev-backend 92.57% <92.48%> (-0.27%) ⬇️
dev-frontend 93.59% <ø> (ø)

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

Files Coverage Δ
tdrs-backend/tdpservice/core/logger.py 100.00% <100.00%> (ø)
...rs-backend/tdpservice/data_files/test/factories.py 100.00% <100.00%> (ø)
tdrs-backend/tdpservice/data_files/views.py 90.65% <100.00%> (+0.08%) ⬆️
tdrs-backend/tdpservice/email/email.py 76.78% <100.00%> (-1.55%) ⬇️
...vice/email/helpers/account_deactivation_warning.py 100.00% <100.00%> (ø)
...backend/tdpservice/email/helpers/account_status.py 84.84% <100.00%> (ø)
tdrs-backend/tdpservice/email/helpers/data_file.py 100.00% <100.00%> (ø)
tdrs-backend/tdpservice/parsers/admin.py 100.00% <100.00%> (ø)
...rs/migrations/0002_alter_parsererror_error_type.py 100.00% <ø> (ø)
...vice/parsers/migrations/0006_auto_20230810_1500.py 100.00% <100.00%> (ø)
... and 42 more

Continue to review full report in Codecov by Sentry.

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

Copy link
Collaborator

@andrew-jameson andrew-jameson left a comment

Choose a reason for hiding this comment

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

I believe we need to ensure network-policy are added that enables non-prod instances to talk to the prod clamav. Should only be a couple lines in deploy-backend.sh.

@George-Hudson
Copy link
Author

George-Hudson commented Aug 22, 2023

@andrew-jameson
Not sure this is the way. We are currently not using the add-network-policy to establish these container to container connections, instead are designating an http endpoint to hit instead. Shouldn't we instead allow traffic coming from our different spaces into the prod ClamAV server?

@ADPennington
Copy link
Collaborator

Question: does this require us to share prod space keys with dev? @George-Hudson

@George-Hudson
Copy link
Author

George-Hudson commented Aug 23, 2023

Question: does this require us to share prod space keys with dev? @George-Hudson

@ADPennington @andrew-jameson that is unclear. The documentation does not mention it and it doesn't make sense to me to do it this way. I would think it's more of a dns routing issue and less of an internal networking issue. We currently don't establish a network policy in order to connect to in-space clamAV servers, so I am not sure if we need it to reach a different-cf-space clamAV server.

@raftmsohani
Copy link

raftmsohani commented Aug 28, 2023

The devspace apps need to be able to connect to prod clamav, therefore we need to add network policy for all instances and open up their connection protocol and port using the command:

cf add-network-policy SOURCE_APP --destination-app DESTINATION_APP [-s DESTINATION_SPACE_NAME [-o DESTINATION_ORG_NAME]] [--protocol (tcp | udp) --port RANGE]

e.g.: cf add-network-policy tdp-backend-sandbox clamav-rest -s tanf-prod --protocol tcp --port 9000 
Screenshot 2023-08-27 at 11 25 37 AM

@@ -0,0 +1,49 @@
# CLAMAV

Choose a reason for hiding this comment

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

Need to consolidate this with the README file in /Technical-Documentation

@@ -107,6 +106,17 @@ update_backend()
# Add network policy to allow frontend to access backend
cf add-network-policy "$CGAPPNAME_FRONTEND" "$CGAPPNAME_BACKEND" --protocol tcp --port 8080

if ["$CF_SPACE" = "tanf-prod" ]; then
# Add network policy to allow backend to access tanf-prod services
cf add-network-policy "$CGAPPNAME_BACKEND" clamav-rest --protocol tcp --port 9000

Choose a reason for hiding this comment

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

Question: Do we need to set the AV_SCAN_URL in prod?

Choose a reason for hiding this comment

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

we definitely need the env var in prod

@@ -40,7 +40,6 @@ set_cf_envs()
"AMS_CLIENT_ID"
"AMS_CLIENT_SECRET"
"AMS_CONFIGURATION_ENDPOINT"
"AV_SCAN_URL"

Choose a reason for hiding this comment

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

The env var cannot come from outside envs since it is dynamic

}
server {
listen 9000;
location /scan {

Choose a reason for hiding this comment

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

This opens a route to clamav prod, we might want to add a note somewhere for revising this later on. Although all traffic is from cloud.gov space, but they are from a space out of prod

@George-Hudson George-Hudson added raft review This issue is ready for raft review and removed Deploy with CircleCI-sandbox labels Sep 25, 2023
@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 Sep 28, 2023
@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 Sep 28, 2023
@raftmsohani
Copy link

Testing this on cloud.gov, it looks like cloud.gov doesn't allow opening any port in NGINX other than default 8080. Still investigating to see if there is any other way to do this

@George-Hudson
Copy link
Author

GitHub isn't showing the changes to scripts/deploy-backend.sh for this PR

https://github.com/raft-tech/TANF-app/blob/9e2c43748bac90c7043693905c0b3d2f5f68009e/scripts/deploy-backend.sh#L110C5-L122C2

going to close this and remake

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

Successfully merging this pull request may close these issues.

Singular ClamAV Scanner
4 participants