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

Adds details about OSSEC rule addition #199

Merged
merged 5 commits into from
Nov 1, 2021
Merged

Conversation

kushaldas
Copy link
Contributor

@kushaldas kushaldas commented Apr 12, 2021

Status

Ready for review

Description of Changes

Adds steps for adding new OSSEC rules. This is focused on the developers.

Testing

This is based on the wiki page at https://github.com/freedomofpress/securedrop/wiki/How-to-add-new-ossec-rules%3F

Release

Checklist (Optional)

  • Doc linting (make docs-lint) passed locally
  • Doc link linting (make docs-linkcheck) passed
  • You have previewed (make docs) docs at http://localhost:8000

After merge

Copy link
Contributor

@gonzalo-bulnes gonzalo-bulnes left a comment

Choose a reason for hiding this comment

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

I left two suggestion to fix an encoding issue.

I'm assuming that the encoding mismatch happened when copy-pasting the command output and that fixing it in the docs would avoid confusion.

Also worth reviewing: I think both were some flavor of double quotes?

docs/development/updating_ossec.rst Show resolved Hide resolved
docs/development/updating_ossec.rst Show resolved Hide resolved
Copy link
Contributor

@emkll emkll left a comment

Choose a reason for hiding this comment

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

@kushaldas thanks for preparing these docs, I think there's a bit of duplication with existing sections (perhaps be can update the existing sections and/or link to them, to avoid duplication). See inline for comments


There are two main files involved in this.

- `install_files/securedrop-ossec-server/var/ossec/rules/local_rules.xml` the rules file
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the natural order (as you presented it below) is decoder file and then rules file. Perhaps inverting the order makes sense here, along with a sentence to briefly describe what they do.

Copy link
Contributor

Choose a reason for hiding this comment

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

reading this for the first time, i also would have found it useful to see a short description for each item listed, regardless of order


You can then add a test for the `molecule/testinfra/mon/test_ossec_ruleset.py`
file. Here the test loops over different log lines mentioned in
`log_events_without_ossec_alerts` variable in
Copy link
Contributor

Choose a reason for hiding this comment

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

there's also log_events_with_ossec_alerts to tests that the alerts are correctly generated


In the above example, we are creating a new `decoder` based on the
`program_name` value. We can find this `program_name` value using the
`/var/ossec/bin/ossec-logtest` command, you can paste the login as input to
Copy link
Contributor

Choose a reason for hiding this comment

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

See the section above (L26, there may be some duplicate information here. Perhaps folding it in or maintaining a reference to it makes sense here.

Copy link
Contributor

@sssoleileraaa sssoleileraaa Jul 14, 2021

Choose a reason for hiding this comment

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

+1 to including a reference to the section on how to use ossec-logtest, but it would also be useful to include the command used to obtain the output you copy-pasted below.

docs/development/updating_ossec.rst Show resolved Hide resolved
docs/development/updating_ossec.rst Outdated Show resolved Hide resolved
@eloquence eloquence requested a review from sssoleileraaa June 3, 2021 17:37
@sssoleileraaa sssoleileraaa removed their assignment Jul 7, 2021
@sssoleileraaa
Copy link
Contributor

hey I'm going to pick up this review to help get it over the finish line!

Copy link
Contributor

@sssoleileraaa sssoleileraaa left a comment

Choose a reason for hiding this comment

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

Looks good! Just some minor changes requested before we merge. Thanks for the docs improvements kushal :)


In the above example, we are creating a new `decoder` based on the
`program_name` value. We can find this `program_name` value using the
`/var/ossec/bin/ossec-logtest` command, you can paste the login as input to
Copy link
Contributor

@sssoleileraaa sssoleileraaa Jul 14, 2021

Choose a reason for hiding this comment

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

+1 to including a reference to the section on how to use ossec-logtest, but it would also be useful to include the command used to obtain the output you copy-pasted below.

</group>


Verify the configuration change
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we reword to:

Verify new OSSEC rule

Verify the configuration change
--------------------------------

On the monitor server you can use the following command as `root` to verify the changes.
Copy link
Contributor

Choose a reason for hiding this comment

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

verify the changes -> verify the new rule

for more clarity?

docs/development/updating_ossec.rst Show resolved Hide resolved

There are two main files involved in this.

- `install_files/securedrop-ossec-server/var/ossec/rules/local_rules.xml` the rules file
Copy link
Contributor

Choose a reason for hiding this comment

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

reading this for the first time, i also would have found it useful to see a short description for each item listed, regardless of order

docs/development/updating_ossec.rst Outdated Show resolved Hide resolved
@cfm cfm force-pushed the add_ossec_rules branch from a8684f5 to 19b1d9e Compare October 28, 2021 18:21
@cfm
Copy link
Member

cfm commented Oct 28, 2021

I believe I've addressed all the substantive review feedback here. @creviera, would you be willing to re-review and resolve the individual review threads as you see fit?

If you think this needs a deeper overhaul (à la freedomofpress/securedrop-dev-docs#153), I think I'd benefit from having a brief conversation about the goals and structure of this page before tinkering further with this pull request in its current form.

@cfm cfm requested a review from sssoleileraaa October 28, 2021 18:26
Copy link
Contributor

@sssoleileraaa sssoleileraaa left a comment

Choose a reason for hiding this comment

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

looks like everyone's review comments were addressed :-)

@sssoleileraaa sssoleileraaa merged commit d631d74 into main Nov 1, 2021
@sssoleileraaa sssoleileraaa deleted the add_ossec_rules branch November 1, 2021 17:09
@cfm cfm mentioned this pull request Jan 10, 2022
3 tasks
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.

5 participants