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

Add OSSEC rule to indicate to admins what ansible-apt-key alert means #2224

Merged
merged 6 commits into from
Sep 5, 2017

Conversation

redshiftzero
Copy link
Contributor

@redshiftzero redshiftzero commented Aug 30, 2017

Status

Ready for review

Description of Changes

This PR:

Testing

  • Config tests fail on 1da1664 in the staging environment (error should involve rule_id)
  • After 7d8af79 all config tests pass

Deployment

Rules should be deployed via the SecureDrop OSSEC deb packages without issue.

Checklist

If you made changes to the system configuration:

If you made changes to documentation:

  • Doc linting passed locally

@redshiftzero redshiftzero requested a review from conorsch August 30, 2017 02:08
@conorsch
Copy link
Contributor

conorsch commented Sep 5, 2017

Reviewing now.

G5SR00+00VHzBVE6fDg027e#012N2sAnjNLOYbRSBxBnELUDKC7Vjaz/sAM
iEwEExECAAwFAkqg7nQFgwll/3cACgkQ#0123nqvbpTAnH+GJA
level: "13"
rule_id: "400001"
Copy link
Contributor

Choose a reason for hiding this comment

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

Beautiful job on the new test vars! The additions even pass YAML linting standards. 🥇

part of each SecureDrop release. This upgrade will occur automatically.

.. note:: This also means that any changes made locally by administrators to
their OSSEC rules will not persist after a SecureDrop update.
Copy link
Contributor

Choose a reason for hiding this comment

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

In the note::, the leading "This" refers to statements made in the preceding paragraph text, which somewhat defeats the purpose of using a Note. How about:

The use of automatic upgrades for release deployment means that any changes made locally by administrators to their OSSEC rules will not persist after a SecureDrop update.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair! Let me know when you're done reviewing / when it's cool for me to force push without being annoying and I'll fix this.

@conorsch
Copy link
Contributor

conorsch commented Sep 5, 2017

Confirmed that the testinfra tests fail when run against staging machines provisioned on latest develop, and pass when run against staging machines provisioned with new deb packages based on this feature branch.

The docs additions are solid. We'll certainly have to flesh out the docs more in the coming releases—some of the info such as useful logfiles should be ported from the OSSEC guide to the writing alerts guide—but these additions are great to have in as soon as possible.

One question: won't only the first Ansible-related alert get a custom message, and all the other events will continue to fire? It appears as if only the apt_key module events will get a custom message, and all the rest will operate as before. If that's the case, then perhaps we should leave #2156 open, since it's still very much a problem.

For example, during testing just now, running the playbooks against staging creates dozens of alerts—if we detect an Ansible playbook run, I'd prefer to silence the subsequent alerts, as well.

@redshiftzero
Copy link
Contributor Author

What you say re: this PR only addressing the first Ansible-related alert is true. I agree in that case that #2156 should stay open until we silence the other alerts (if they are not providing useful information). Edited PR description accordingly.

@conorsch
Copy link
Contributor

conorsch commented Sep 5, 2017

Excellent, thanks for the clarification, @redshiftzero.

And add failing test for Ansible OSSEC alert described in ticket
#2156
Noting that admins may not actually understand what Ansible
playbooks are now that we abstract that away from the user now.
@redshiftzero
Copy link
Contributor Author

FYI I made that docs change @conorsch

Copy link
Contributor

@conorsch conorsch left a comment

Choose a reason for hiding this comment

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

Solid progress toward the goal of reducing IDS noise. Excellent additions to testing and developer documentation. 😎 :shipit:

@redshiftzero redshiftzero merged commit 9cf9b29 into develop Sep 5, 2017
@msheiny msheiny deleted the ossec-ansible-alert branch April 10, 2018 15:14
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.

2 participants