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

filehashes: fix for unique filehash filenames #343

Closed
wants to merge 3 commits into from

Conversation

jasonish
Copy link
Member

Commit 8725e56 gave each downloaded a file a unique name so dataset files from different sources wouldn't clobber each other, but this was applied to all files breaking file hash lists as that code wasn't updated for the new filename scheme.

Update the file hashing code to find the files based on the filename prefix of the rule referencing the file.

Bug: #6854

Test ruleset: https://github.com/jasonish/suricata-test-rules/archive/refs/heads/main.zip

Perhaps in combination with https://rules.pawpatrules.fr/suricata/paw-patrules.tar.gz which has a dataset.

Commit 8725e56 gave each downloaded a
file a unique name so dataset files from different sources wouldn't
clobber each other, but this was applied to all files breaking file
hash lists as that code wasn't updated for the new filename scheme.

Update the file hashing code to find the files based on the filename
prefix of the rule referencing the file.

Bug: #6854
@jasonish jasonish requested a review from inashivb March 12, 2024 19:08
@victorjulien
Copy link
Member

Should we have SV test support and tests for SU?

Copy link
Member

@inashivb inashivb left a comment

Choose a reason for hiding this comment

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

This does seem to work. Was it always broken with --local?

@inashivb
Copy link
Member

Should we have SV test support and tests for SU?

Suricata-update has its own test suite in tests/ , could you please tell what do you aim for the s-v test to look like?
The test suite already covers:

  • commands
  • configuration options
  • modifier confs
  • flowbit dependency handling
  • signature mapping
  • classification config merging
    and more.

I think one thing we could do is ensure that if all free sources are enabled, then suricata-update does not error out.

@jasonish
Copy link
Member Author

This does seem to work. Was it always broken with --local?

Somethings broken with --local?

@jasonish
Copy link
Member Author

I think one thing we could do is ensure that if all free sources are enabled, then suricata-update does not error out.

The integration tests avoid hitting the network now, so not sure how that will work unless they go and hit all the rule sources. Which I'm not that keen to do in automated tests.

@victorjulien
Copy link
Member

Should we have SV test support and tests for SU?

Suricata-update has its own test suite in tests/ , could you please tell what do you aim for the s-v test to look like? The test suite already covers:

* commands

* configuration options

* modifier confs

* flowbit dependency handling

* signature mapping

* classification config merging
  and more.

I think one thing we could do is ensure that if all free sources are enabled, then suricata-update does not error out.

Can this issue be properly tested in the framework? If so that should be sufficient.

@jasonish
Copy link
Member Author

This does seem to work. Was it always broken with --local?

Somethings broken with --local?

I see. If you take my test ruleset and extract it to disk and point to it with --local it won't find the hash list files. I believe this has always been broken.

However, datasets are picked up with --local, and were in 1.3.0 as well. So we're good there. Probably won't fix this release, since it was broken already.

@inashivb
Copy link
Member

Somethings broken with --local?

yeah. i get the same error as w/o this patch. Haven't tried to check the why though.
See:

$./bin/suricata-update --local ~/Downloads/rules/ #this dir contains all the rules you've mentioned for testing
13/3/2024 -- 21:24:49 - <Error> -- filemd5 file testmyids.md5 was not found
13/3/2024 -- 21:24:49 - <Error> -- filesha1 file testmyids.sha1 was not found
13/3/2024 -- 21:24:49 - <Error> -- filesha256 file testmyids.sha256 was not found

@jasonish
Copy link
Member Author

Can this issue be properly tested in the framework? If so that should be sufficient.

Yes, the integration test was updated to use my test ruleset and verify the extraction of the file hash lists to the proper location.

Copy link
Member

@inashivb inashivb left a comment

Choose a reason for hiding this comment

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

Comments were addressed. LGTM 🚀

@inashivb
Copy link
Member

I think one thing we could do is ensure that if all free sources are enabled, then suricata-update does not error out.

The integration tests avoid hitting the network now, so not sure how that will work unless they go and hit all the rule sources. Which I'm not that keen to do in automated tests.

Could we keep a copy that we update at certain intervals? ref: https://github.com/OISF/suricata-verify/tree/master/tests/test-ruleparse-etopen-01

@jasonish
Copy link
Member Author

I think one thing we could do is ensure that if all free sources are enabled, then suricata-update does not error out.

The integration tests avoid hitting the network now, so not sure how that will work unless they go and hit all the rule sources. Which I'm not that keen to do in automated tests.

Could we keep a copy that we update at certain intervals? ref: https://github.com/OISF/suricata-verify/tree/master/tests/test-ruleparse-etopen-01

We have a copy of emerging threats. I'm not sure how keeping a copy of all would be helpful.

Now something else that could be interesting is an external tool that validates all public rulesets.. Makesure the links are all there, and which versions of Suricata they parse for. Interesting project I think, but outside of Suricata-Update.

@jasonish
Copy link
Member Author

Merged.

@jasonish jasonish closed this Mar 13, 2024
@jasonish jasonish deleted the filehashes-fix/v1 branch March 19, 2024 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants