-
Notifications
You must be signed in to change notification settings - Fork 359
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
payload/probe separation #870
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably a bit more refinement to do here but this looks like a good first pass.
The biggest (philosophical) question is whether we should include some kind of information in the payload that informs the detector.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initial items, still working thru the logic here to connect all the dots.
One primary concern is here is the file enumeration needs to account for collisions and set precedence for files in the data_dir
to fully supersede the same file if also found in the package_dir
.
This should also come with at least tests that show the intended use of search()
. The current tests look to only validate type restrictions on output.
tests/test_payloads.py
Outdated
PAYLOAD_NAMES = list( | ||
garak.payloads.Manager().search() | ||
) # default includes local custom payloads to help test them |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code will be executed on import, this may result in brittle or missed results.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair point. I haven't found a fixture-based solution to this yet. There's also something a bit circular about using the Manager class to get valid inputs for the Manager class, but that's a secondary point to your comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sunk enough time into this, didn't find a resolution using a fixture - would like to have a fixture detemine the value at session
scope, and then use this fixture to iterate, but did not succeed.
Co-authored-by: Jeffrey Martin <[email protected]> Signed-off-by: Leon Derczynski <[email protected]>
… add module-level loader for simple stuff; check XDG_DATA priority merges correctly
…ector config value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect the lack of utilization for Loadmaster
is masking the need for informative data that is missing from this design. Currently there seems to be no way to select
only payloads that would meet the needs of the selected example probe when other probes in the selected module are converted to use PayloadGroup
.
Yeah. There are three degrees of flexibility here and one example
implementation is insufficient to illustrate that.
1. Invocation - Loadmaster is the first class citizen for payload use, but
the _plugins.load_plugin pattern is so useful, it's intentionally
paralleled here
2. Selection. One can select by typology or name; this probe selects by
typology but would probably be better off selecting by name. A refactor of
this module to use typology might be down the line.
3. Typology. The typology is flexible and going to be living; this is v0
and I expect many restructurings. But it's unclear how much should be real
into the current structure rn (the answer is "not much")
…On Tue, Sep 10, 2024, 15:48 Jeffrey Martin ***@***.***> wrote:
***@***.**** requested changes on this pull request.
I suspect the lack of utilization for Loadmaster is masking the need for
informative data that is missing from this design. Currently there seems to
be no way to select only payloads that would meet the needs of the
selected example probe when other probes in the selected module are
converted to use PayloadGroup.
------------------------------
In garak/probes/grandma.py
<#870 (comment)>:
> + win10_payload = garak.payloads._load_payload("keyedprod_win10")
+ product_names = win10_payload.payloads
I would not expect access to _load_payload() in a probe.
from garak.payloads import Loadmaster
...
class Win10(Probe)
_payload_types = ( "Security circumvention instructions/Product activation codes" )
...
def __init__(self, config_root=_config):
product_names = []
p_group_names = Loadmaster.search(types=_payload_types)
for group in p_groups:
product_names.append(Loadmaster.load(group).payloads)
...
Based on the above, the search tooling does not provide a method of
selecting payload for this probe if support for other probes in this module
were to be introduced. I suspect there is some more structure needed in the
payload files or it is I am missing something on how this is expected to
evolve.
—
Reply to this email directly, view it on GitHub
<#870 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAA5YTW5BKVURBZATFYJPQDZV32D7AVCNFSM6AAAAABNQPCPI6VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDEOJSGU2DSNJRHA>
.
You are receiving this because you were assigned.Message ID:
***@***.***>
|
@jmartin-tech changes complete, I want to land this - any further blockers? |
Initial concerns are addressed, I am not completely sold on how this works yet. Will get it merged for now and iterate further. |
Create structure for separating payloads from probes, to allow easier transfer.
Payload definition:
Docs:
garak/resources/typology_payloads.tsv
. This covers content and not behaviour.name
, a string they're summoned by, keyed withgarak_payload_name
, a magic field used to identify the JSON as a payloadtypes
, a list of strings corresponding to entries and implicit parent entries in the typology. can be emptypayloads
, a list of stringsdetector_name
, an optional string referring to an appropriate detector for this payload groupdetector_config
, an optional dict configuring the above detectorgarak.payloads.PayloadGroup
Manager
, which recursively enumerates payloads in both package and XDG dataresources/payloads/
Loadmaster().search()
searches the loaded payload metadata cacheLoadmaster().load(name)
loads a payload specified by the string parameter and returns an instance of aPayloadGroup
Possible extensions beyond scope of this PR:
Notes:
probes.grandma.Win10
look? We have almost too many options heredetector_name
recommendation indicates an interesting responsibility conflicts between probes and payloads. Maybe the payload is overspecified.Loadmaster.load()
can come out and become module-level, as with_plugins.load_plugin()