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 bqm #104

Merged
merged 3 commits into from
Apr 3, 2023
Merged

add bqm #104

merged 3 commits into from
Apr 3, 2023

Conversation

noraj
Copy link
Contributor

@noraj noraj commented Feb 25, 2023

@ShutdownRepo
Copy link
Member

Like #102 and #103, switching to dev branch. Please take notice for future pull request 😉

@ShutdownRepo ShutdownRepo changed the base branch from main to dev February 25, 2023 23:52
@noraj
Copy link
Contributor Author

noraj commented Feb 25, 2023

Copy link
Member

@ShutdownRepo ShutdownRepo left a comment

Choose a reason for hiding this comment

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

One change required and PR will be ready for testing. One comment though, Exegol already has a set of custom queries. It would be nice to include its dataset in those queried by bqm.
Also, this tool will need some further manual testing. I'm wondering if this aggregator takes the categories into consideration when deduplicating and merging custom queries.

sources/install.sh Show resolved Hide resolved
@ShutdownRepo ShutdownRepo added waiting for additional changes Further changes are requested new tool(s) This adds one or multiple tools to Exegol labels Feb 25, 2023
@noraj
Copy link
Contributor Author

noraj commented Feb 26, 2023

One comment though, Exegol already has a set of custom queries. It would be nice to include its dataset in those queried by bqm.

Yeah that was the point of attention I brought

maybe it could be used to update https://github.com/ThePorgs/Exegol-images/blob/main/sources/bloodhound/customqueries.json

If the sub-set provided by bqm is larger than the actual one if could replace it unless there are queries that are not in bqm.

I'm wondering if this aggregator takes the categories into consideration when deduplicating and merging custom queries.

Yeah I use it and categories are preserved.

@ShutdownRepo
Copy link
Member

Ok nice 👌
Beware though, exegol custom queries include some homemade queries that probably won't be in the datasets bqm is fetching.
Also, any reason why https://github.com/noraj/bqm would be used instead of origin https://github.com/Acceis/bqm ? I also notice your fork is a few commits behind origin, hence my interrogation

@ShutdownRepo ShutdownRepo self-assigned this Feb 26, 2023
@noraj
Copy link
Contributor Author

noraj commented Feb 26, 2023

Also, any reason why https://github.com/noraj/bqm would be used instead of origin https://github.com/Acceis/bqm ? I also notice your fork is a few commits behind origin, hence my interrogation

An error from my side. The gem comes from acceis repo, I just pasted the wrong URL in the issue.

@ShutdownRepo
Copy link
Member

Also, any reason why https://github.com/noraj/bqm would be used instead of origin https://github.com/Acceis/bqm ? I also notice your fork is a few commits behind origin, hence my interrogation

An error from my side. The gem comes from acceis repo, I just pasted the wrong URL in the issue.

No biggie.
Once the pipeline tests pass successfully, we (you if possible) will need to make some further testing to make sure the tool integrates swiftly with Exegol, not removing queries 😉

@ShutdownRepo ShutdownRepo removed the waiting for additional changes Further changes are requested label Feb 26, 2023
@noraj
Copy link
Contributor Author

noraj commented Feb 26, 2023

Once the pipeline tests pass successfully, we (you if possible) will need to make some further testing to make sure the tool integrates swiftly with Exegol, not removing queries wink

The tool doesn't overwrite anyfile, the user has to provide an output path eg. bqm --output-path ~/.config/bloodhound/customqueries.json so it's the user choice to overwrite the existing queries or to save the output file anywhere. I can guarantee 0% conflcit. No default location is written to if no option is provided either.

@ShutdownRepo
Copy link
Member

Once the pipeline tests pass successfully, we (you if possible) will need to make some further testing to make sure the tool integrates swiftly with Exegol, not removing queries wink

The tool doesn't overwrite anyfile, the user has to provide an output path eg. bqm --output-path ~/.config/bloodhound/customqueries.json so it's the user choice to overwrite the existing queries or to save the output file anywhere. I can guarantee 0% conflcit. No default location is written to if no option is provided either.

Yes, but if I'm correct, BloodHound only loads custom queries from ~/.config/bloodhound/customqueries.json. And that's what's in the history command. Meaning the user will overwrite the existing custom queries in most cases. We need to make sure the already existing queries are taken into account, probably by adding Exegol's custom queries to bqm's datasets, or by having an Exegol fork of bqm or whatever different solution.

@noraj
Copy link
Contributor Author

noraj commented Feb 26, 2023

Yes, but if I'm correct, BloodHound only loads custom queries from ~/.config/bloodhound/customqueries.json. And that's what's in the history command. Meaning the user will overwrite the existing custom queries in most cases. We need to make sure the already existing queries are taken into account, probably by adding Exegol's custom queries to bqm's datasets, or by having an Exegol fork of bqm or whatever different solution.

Right, I can add https://github.com/ThePorgs/Exegol-images/blob/main/sources/bloodhound/customqueries.json to the data set of bqm. Acceis/bqm#14

@ShutdownRepo ShutdownRepo mentioned this pull request Feb 26, 2023
@ShutdownRepo
Copy link
Member

Also, or in addition to, I think it would be nice to have bqm check if the file indicated in the path already exist, and use it’s content as part of the dataset. This would allow, whether it’s Exegol or somethings else, to always sync with what’s already there locally

@noraj
Copy link
Contributor Author

noraj commented Feb 26, 2023

Also, or in addition to, I think it would be nice to have bqm check if the file indicated in the path already exist, and use it’s content as part of the dataset. This would allow, whether it’s Exegol or somethings else, to always sync with what’s already there locally

Nice idea Acceis/bqm#15

@noraj
Copy link
Contributor Author

noraj commented Feb 27, 2023

@ShutdownRepo https://github.com/Acceis/bqm/releases/tag/v1.3.0 should satisfy the requirements for this PR.

@ShutdownRepo
Copy link
Member

@ShutdownRepo https://github.com/Acceis/bqm/releases/tag/v1.3.0 should satisfy the requirements for this PR.

That was fast!! Can you resolve the conflicts? We'll then proceed to manual checks before merging

@ShutdownRepo ShutdownRepo added the manual tests required Something needs to be tested manually label Feb 27, 2023
@ShutdownRepo
Copy link
Member

Also, what happens if twp exact same queries exist in different categories? Both are kept I suppose?

@noraj
Copy link
Contributor Author

noraj commented Feb 27, 2023

Also, what happens if twp exact same queries exist in different categories? Both are kept I suppose?

A query is considered duplicate if it has the same name and same queryList. It was primarily to avoid copy/paste because some data set are some custom queries + merged ones from other datasets. But duplicates can exist if the same query has different names.

@noraj
Copy link
Contributor Author

noraj commented Mar 24, 2023

@ShutdownRepo Have you been able to test it?

@ShutdownRepo
Copy link
Member

Didn't find the time to do the tests manually. Sorry for the delay, it's been a few crazy weeks. Let's merge and iterate later on if needed. Thank you again for the addition and edits on bqm by taking our discussion into account.

@ShutdownRepo ShutdownRepo merged commit 4ff17f1 into ThePorgs:dev Apr 3, 2023
@noraj noraj deleted the noraj/bqm branch April 3, 2023 21:24
@ShutdownRepo ShutdownRepo mentioned this pull request Apr 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
manual tests required Something needs to be tested manually new tool(s) This adds one or multiple tools to Exegol
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants