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

VIRTS-896 adding microsoft winio library files to gocat extensions #279

Closed
wants to merge 1 commit into from

Conversation

uruwhy
Copy link
Contributor

@uruwhy uruwhy commented Mar 9, 2020

Adding auxiliary files for SMB pipes

@uruwhy uruwhy requested review from scottctaylor12 and a user March 9, 2020 17:50
@ghost
Copy link

ghost commented Mar 10, 2020

i'm not sure how much i like pulling these libraries into the code base. i get that this helps offline installation and keeps with our golang extension design -- but the cost seems greater, which is us maintaining and storing code that we didn't write.

we should aim to do a 'go get' within the extensions design so they can be retrieved from the Microsoft GitHub library on each server start vs dropped in the code base.

@uruwhy
Copy link
Contributor Author

uruwhy commented Mar 10, 2020

Understood, the only issue I see is that I had to make one edit in one of the microsoft winio go file methods to accept remote clients

@ghost
Copy link

ghost commented Mar 13, 2020

@khyberspache what's ur take on this?

@khyberspache
Copy link
Contributor

khyberspache commented Mar 16, 2020

Put some thought to this... here's what I see as our options:

  1. Wait for Microsoft to fix this:

The fix for this is in microsoft/go-winio#74 and discussion is in issue microsoft/go-winio#144 with more details found here moby/moby#38627.

But that couldn't hasn't been touched for fixed in 1+ year.

  1. Fork go-winio under MITRE's GitHub.

We would push the changes, and directly specify the MITRE version of go-winio when doing the sandcat extensions installation install. There are some peculiarities of doing this in Go however and the options basically boil down to Vender package managers or mucking around in the local git repository and grabbing the install (https://medium.com/@sasom/how-to-fork-go-packages-f863e370bbb6).

  1. Stick with "go get" on go-winio repo

This would be a bit of a nightmare depending how we implemented it. If we want to go the route you're saying @privateducky, we have two real options: 1) we would need to back trace all of the function calls to the the makeServerPipeHandle function so that we can DI/mock the calls from Sandcat to establish remote named pipe connections or 2) sed replace the string in the $GOPATH copy of pipe.go file after running a blocking go get.

Both are janky options.

  1. Include the files in our local repo.

Not ideal for your aforementioned reasons.


In terms of ranking best to worst:

  1. Ideal, but we are at the mercy of Microsoft and it might never get patched.
  2. Feasible and we could probably come up with an elegant pythonic solution to patch the import path within the smb.py dependency loader file.
  3. Straight forward.
  4. Could be simple or an total nightmare depending how we approach it.

@ghost
Copy link

ghost commented Mar 16, 2020

@khyberspache thanks for your in-depth review on this.

@uruwhy let's go with #2 in @khyberspache's review above-- but let's move it to our internal source repository when we do this, so we can work through any potential hiccups for awhile privately and not have anyone else rely on it until we're comfortable w/our solution.

@uruwhy
Copy link
Contributor Author

uruwhy commented Mar 16, 2020

Sounds good, will get started on that

@khyberspache
Copy link
Contributor

Make sense to close this given our new direction

@elegantmoose elegantmoose deleted the VIRTS-896-winio branch February 24, 2023 21:55
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