Allow to query for passwords in CowrieSession API - Closes #607 #641
Conversation
|
@regulartim @mlodic , kindly review this PR. It contains all the fixes discussed in #611, implemented in a cleaner format. Apologies for the delay. |
|
Please fix the formatting @IshaanXCoder |
|
Also, the PR title is not very informative. Please stick to our contribution guidelines and our PR template. |
IshaanXCoder
changed the title
IshaanXCoder
changed the title
Made the required changes |
No you did not. Since I asked you to fix the formatting you did not push a single commit! |
|
Oh really sorry! I misunderstood it with formatting the PR description, pushed the changes, PTAL @regulartim. |
|
CI still fails. Please read into the contribution guidelines again, learn how to avoid such problems and make sure your code is correctly formatted before pushing. |
|
Hey @regulartim PTAL. I fixed it, took time, apologies for the same.
|
regulartim
left a comment
regulartim
left a comment
There was a problem hiding this comment.
No problem, take your time! :) However, in my opinion, this is not ready to be merged yet. We have to discuss some things.
| if len(observable) == 64 and not is_sha256hash(observable): | ||
| return HttpResponseBadRequest("Invalid hash format: must be 64 hexadecimal characters") |
There was a problem hiding this comment.
Ok, so I is not possible to query passwords of length 64, right?
| else: | ||
| return HttpResponseBadRequest("Query must be a valid IP address or SHA-256 hash") | ||
| password_pattern = f" | {observable}" | ||
| sessions = CowrieSession.objects.filter(duration__gt=0, credentials__icontains=password_pattern).prefetch_related("source", "commands") |
There was a problem hiding this comment.
Why did you choose to use icontains instead of contains here? Passwords are usually case sensitive, right?
There was a problem hiding this comment.
By the way: I am very surprised that credentials__icontains=password_pattern works at all! The django docs suggest that at least __contains expects a list.
https://docs.djangoproject.com/en/6.0/ref/contrib/postgres/fields/#contains
Could you point me to some resource where the behavior of ArrayField.__icontains is described the way you use it here?
There was a problem hiding this comment.
okay I checked the django and PostgreSQL documentation and this behavior is not documented there.
ArrayField.__icontains with a string operand appears to work due to implicit casting of the array to text by PostgreSQL ad not because django officially supports it.
so yeah I agree, i'll change it to the documented approach. will make the changes
There was a problem hiding this comment.
But I you wrote tests for that and they pass, right? Where did you get the idea to use filter like that?
| class Meta: | ||
| indexes = [ | ||
| models.Index(fields=["source"]), | ||
| GinIndex(fields=["credentials"], name="greedybear_credentials_gin_idx"), |
There was a problem hiding this comment.
Are you sure that this index helps in our specific use case? I looked it up and that's really advanced database stuff! At least for me it is hard to understand what this type of index actually does.
|
@regulartim shall i implement the above conversation? |
Sorry for my late reply. Was busy with Christmas stuff. :) Could you first reply to the rest of my comments please? |
2 participants
Description
This PR includes the fixes discussed in #611 . This adds the ability to query the CowrieSession API by password, in addition to the existing IP address and SHA-256 hash query methods. Changes are mentioned as follows -
api/views/cowrie_session.pygreedybear/models.pyGinIndexonCowrieSession.credentialsinMeta.indexesto speed up credential lookups.I also added .DS_Store to .gitignore since it can be irritating for developers.
Related issues
SOLVES #607
Type of change
Checklist
develop.cowrie_session_view)Black,Flake,Isort) gave 0 errors. If you have correctly installed pre-commit, it does these checks and adjustments on your behalf.CONTRIBUTE.md). (Updated view docstring)