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

made nessus subfolder and added script for fixing container license #320

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

wz-gsa
Copy link
Contributor

@wz-gsa wz-gsa commented Jun 12, 2024

Changes proposed in this pull request:

  • added nessus folder
  • added fix_nessus_license.sh
  • added readme_fix_nessus_license.md

security considerations

Improves security by ensuring plugins remain updated

@wz-gsa wz-gsa requested a review from a team June 12, 2024 20:39
@wz-gsa wz-gsa self-assigned this Jun 12, 2024
@@ -157,7 +157,7 @@ def add_commit_push_security_md(repo_path, branch_name):

# Create a pull request
def create_pull_request(repo_path, branch_name, default_branch):
"""Create a pull request for the branch, attempt to add reviewers, and assign 'wz-gsa'."""
""f"Create a pull request for the branch, attempt to add reviewers, and assign '{ASSIGNEE}'."""
Copy link
Contributor

Choose a reason for hiding this comment

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

where is ASSIGNEE being set?

@@ -157,7 +157,7 @@ def add_commit_push_security_md(repo_path, branch_name):

# Create a pull request
def create_pull_request(repo_path, branch_name, default_branch):
"""Create a pull request for the branch, attempt to add reviewers, and assign 'wz-gsa'."""
""f"Create a pull request for the branch, attempt to add reviewers, and assign '{ASSIGNEE}'."""
Copy link
Contributor

Choose a reason for hiding this comment

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

With the f in the middle of the quotes, you no longer have a triple-quoted string, you have three strings being silently concatenated.

This should probably be either:
f"""Create a pull request for the branch, attempt to add reviewers, and assign '{ASSIGNEE}'."""

or:
f"Create a pull request for the branch, attempt to add reviewers, and assign '{ASSIGNEE}'." (since the triple-quotes aren't actually necessary here)

Copy link
Contributor

Choose a reason for hiding this comment

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

what's this for? doesn't seem to be mentioned in the PR description or READMEs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's addition was unintentional on my part. updating my commit and reverting this to draft

Copy link
Contributor

Choose a reason for hiding this comment

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

No idea if this is relevant, but this is the script on the Nessus BOSH release the determines if the license is valid and alerts us:

https:/cloud-gov/cg-nessus-manager-boshrelease/blob/main/jobs/nessus-manager/templates/bin/health.sh

Wondering if any of these improvements can go to that script so we don't get alerts in the first place?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was based off of that, and may serve as a replacement, but that bosh script was not fixing the underlying issue and required manual intervention yesterday.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I understand. It looks like this script does the same thing as that BOSH script: detecting if the license is outdated/plugins are outdated and setting Prometheus metrics. So if the changes in this script make things work "correctly" so that we don't get false positives on the alerts, why wouldn't we just replace the BOSH script with this one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We probably can. I wanted change control on it and to make this available and use this a few times before moving it there to ensure I tested it thoroughly before shipping it in the bosh release.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, my main concern is that I don't want two versions of a script that do the same thing floating around. And as long as we're using the BOSH release with a version of this script already, it seems like the right place for it to go.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants