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

multimanifest extension support #1397

Merged
merged 12 commits into from
Dec 19, 2023
Merged

Conversation

C-Sto
Copy link
Contributor

@C-Sto C-Sto commented Aug 31, 2023

Card

IDK what a Card is

Details

Allowed for extension manifests to be in an array, as well as a single json object. This should allow for a single manifest per repo in the cases where multiple BOF's (or multiple DLL's I guess) are present, and the structure of the repo might make having manifests per-folder a bit tricky.

This seems to work mostly pretty seamlessly with current extension functionality, but I haven't thoroughly tested everything in anger.

One of the parts I'm not completely sure about is how this will impact armory - as it seems armory relies on the manifest being coupled to a single command (which is not really going to work with multimanifest).

The other part is how tar.gz files are handled (related to above I assume), I've worked around it here but I can see the way this function works being misleading to anyone who wants to fiddle with extensions in the future.

@C-Sto C-Sto requested a review from a team as a code owner August 31, 2023 03:06
@rkervella
Copy link
Member

One of the parts I'm not completely sure about is how this will impact armory - as it seems armory relies on the manifest being coupled to a single command (which is not really going to work with multimanifest).

Not a big deal, we can adapt the armory, it will just take some time to rework all the existing manifests.

@rkervella
Copy link
Member

I had the idea of doing something a bit different for 1.6: one extension artifact supporting multiple entrypoints, and thus multiple commands in the client. This means we could have a single DLL with 3 exports that would become 3 commands, all in one manifest. It's doable with your scheme, but I need to see if we can optimize that or not.

@C-Sto
Copy link
Contributor Author

C-Sto commented Aug 31, 2023

With multiple manifests in the same file you'd be able to just have multiple manifests with different entrypoints to support the DLL's like you described.

The only way that this approach is less efficient is there will be duplicated copies of the DLL in the .sliver-client folder, but unless you have hundreds of functions to call in a giant DLL, I don't see that being too much of an issue.

I'd also like to add the ability to reload/unload multiple commands - so the multimanifest structure might need to change a bit from just a straight up array, to something like:

{ "namespace": "cats", "manifests": [{manifests go here}, {etc} ]}

This would also probably help armory, but happy to defer to how you mob decide you want to do it. The failover manifest loading behaviour should work with that structure as well - but failover will only get us so far, so probably want to decide on a suitably flexible structure sooner rather than later

@C-Sto
Copy link
Contributor Author

C-Sto commented Sep 30, 2023

Updates as discussed in Slack. tl;dr:

  • manifest schema changed; single manifest per repo now supports multiple command extensions
  • works with armory based on initial tests (pls test more)
  • legacy schema manifests are upgraded internally on the fly, likely to be able to drop this feature down the line
  • single extensions with multiple entrypoints now supported, single file will be copied to the install dir rather than duplicating it

Also, had to add extensions back to the CLI - I suspect this will slightly break #1353. In saying that, I think the extensions CLI probably needs a little love as well (install vs load, etc)

@C-Sto C-Sto mentioned this pull request Oct 7, 2023
@rkervella rkervella self-requested a review December 18, 2023 22:35
Copy link
Member

@rkervella rkervella left a comment

Choose a reason for hiding this comment

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

Minor change, otherwise looks good. I'll have a look at the other PR soon.

client/command/armory/install.go Outdated Show resolved Hide resolved
@rkervella rkervella self-assigned this Dec 19, 2023
@C-Sto C-Sto requested a review from rkervella December 19, 2023 05:13
@rkervella rkervella merged commit 478dabf into BishopFox:master Dec 19, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants