Pennyw0rth
SMBSpider Enhancement and fixed Modules that depended on Spider
Description
The SMB Spider changes I made that got merged broke a few of the modules that depended on them (See PR https://github.com/Pennyw0rth/NetExec/pull/729). This PR aims to fix those modules as well merge back the changes I've made for SMB Spider.
Type of change
- [x] Bug fix (non-breaking change which fixes an issue)
- [x] Enhancement
Setup guide for the review
Run using poetry on the following system
PRETTY_NAME="Ubuntu 24.04.2 LTS"
NAME="Ubuntu"
VERSION_ID="24.04"
VERSION="24.04.2 LTS (Noble Numbat)"
Bug Fixes
Excluding folders option works
Old Spider
New Spider
The -only-files flag now only show files
Old Spider
New Spider
Remove current and parent directory from search results
Old Spider
New Spider
Enhancements
Useful error messages
Old Spider
New Spider
New --only-folders option
Changes that need further discussion
These are some changes that I made that might need to get a second opinion from the Netexec team
Spider all files and folders by default
Spider all contents in the target share by default instead of having to specify an empty pattern. The reason for this change is that I normally want to search through all files within a share by default and only then use pattern or regex to narrow my searches.
Changing flag from --exclude-dirs to exclude-folders
I thought I'll change the flag name to be consistent with the other flag names such as --spider-folder
Adding a new flag --spider-all instead of --spider "*" to spider all readable shares
Wasn't clear that you had to use the * symbol to spider all the shares, so I made a new flag in case users want to search through all readable shares
Depth starts at 1 instead of 0
The old spider depth option started at 0 meaning if you specified --depth 1 it would crawl through two sub folders. I think it should start at 1 as other linux commands such as find do the same.
Old Spider
New Spider
Improved Recycling Bin Module
Added an option to check if the user wants to download the contents from the recycling bin or not. I think it shouldn't download by default as there was no indication that the module would download files. If it was named differently to recyclebin-download then that would be fine.
Proof of Modules working with new spider
Recycle Bin and GPP_autologin
GPP_Password
Teams_localdb and gpp_privileges
Checklist:
- [x] I have ran Ruff against my changes (via poetry:
poetry run python -m ruff check . --preview, use--fixto automatically fix what it can) - [x] New and existing e2e tests pass locally with my changes
- [x] I have performed a self-review of my own code
The PR #729 not only broke the recyclebin module, but also the following modules:
- gpp_autologin
- gpp_password
- gpp_privileges
- teams_localdb
I am going to revert #729 for now as they all rely on the spider function. @Marshall-Hallenbeck please check for cross references if major functionality of protocols are changed.
EDIT: reverted in #845
Before this is merged we will need to reconsider how spider should be refactored
@NeffIsBack ironic that the thing I merge would have found the errors running e2e tests 😂 Should I test this PR or #729?
@NeffIsBack ironic that the thing I merge would have found the errors running e2e tests 😂 Should I test this PR or #729?
Definitely has its irony😂
That was your opportunity to show the world how INDISPENSABLE your e2e tests are😂
As #729 has been merged there need to be a new PR, but I am not sure if the state it is in is the way to go because too many breaking changes were added. I haven't done a full review of it, there are probably areas that could have been merged but that has to be figured out in a separate, new PR.
If I understood it correctly this PR depends/fixes the changes in #729 so the new PR would have to be merged first.
@NeffIsBack and @Marshall-Hallenbeck sorry for the breaking changes I should have double-checked the other places where the spider function was called.
I just need to tweak the function call connection.spider in the modules that use it. I'll have a look tomorrow and try to make the relevant changes. The function should still return the same paths as the original, I will need to tweak the arguments
@NeffIsBack and @Marshall-Hallenbeck sorry for the breaking changes I should have double-checked the other places where the spider function was called.
No worries about that. In the end it is our job to make sure nothing breaks when merging PRs, because we also know the code base better than contributors.
I just need to tweak the function call
connection.spiderin the modules that use it. I'll have a look tomorrow and try to make the relevant changes. The function should still return the same paths as the original, I will need to tweak the arguments
Sounds good as long as existing arguments are not changed
@NeffIsBack I've created a fix for this issue. The only thing that I couldn't fully test was teams_localdb as I don't currently have the environment setup for it, but it should work as normal as it does not error out now.
I am still not fully convinced. Why changing the existing structure of initializing of creating the object and then spider the share when we can just leave it as is? What benefit do we have by supplying the arguments via the object instantiation than the spider() call?
It's a code preference of mine. I generally prefer setting the attributes of the class within the initialiser rather than setting attributes in the method call, which is what the old SMBSpider class did. I think it's more readable having a one set place (usually in the initialiser) of where attributes are defined. Rather than mixing setting attributes and code logic within a function. In both cases, you still have to create the object and call the method, I've just switched where the arguments are called.
If you're not convinced, then I can change it to how it was normally if you wish.
I've updated the description to show all the changes that I've made to spider including some bug fixes for both modules and SMB spider itself. There are a few changes I've made that I would love to get a second opinion on.