search

fortra / impacket

Impacket is a collection of Python classes for working with network protocols.
https://www.coresecurity.com
Other
13.38k stars 3.56k forks source link

Incorporate Pennyw0rth/NetExec Impacket changes into Fortra Impacket #1715

Closed Marshall-Hallenbeck closed 6 months ago

Marshall-Hallenbeck commented 6 months ago

NetExec is attempting to get added to Kali Linux as a default package, but we currently have several changes in our Impacket fork that are required to be merged into upstream.

The GKDI stuff has been used for almost a year now with no problems, and @XiaoliChan recently had some other changes merged related to NetExec changes. I just filed #1714 as well to revert some breaking changes that we require.

This is necessary because we do not want to have another impacket Kali package deployed, as that causes confusion, and we're not even sure if Kali would want that. The easiest way is to have our changes merged into upstream.

Please let me know if there are any issues testing these changes.

Marshall-Hallenbeck commented 6 months ago

Apparently #1556 is for the GDKI stuff, so merge that first :D

gabrielg5 commented 6 months ago

Hi @Marshall-Hallenbeck

Replying here to clarify some doubts around the PRs you've been working on these days

With #1714 no doubts. Wrongly merged, will check it today

And related to #1715 and #1556. I'm still checking both changesets, but reading your comments and doing a quick comparison of modified files I see that in #1715 you included changes from #1556 and added a few more, correct?

Our main concern when merging #1556 as is, was related to having different examples for the same purpose using different methods. Will be checking #1556 and when correct will notify you so you can update this one and we can start testing these other changes

Later on a single example to read LAPS password can be worked on.

Marshall-Hallenbeck commented 6 months ago

@gabrielg5 Thanks for the quick response.

Yes, #1715 is essentially a "roll up" of the changes we need merged into Impacket for NetExec, which includes #1556, so that one should be merged first preferably.

I understand the confusion of having 2 ways to do things, but if there's two different ways - why not document both? Maybe I'm not understanding the issue, but I'd be glad to help if I can!

anadrianmanrique commented 6 months ago

readLaps.py and getLAPSv2Password do the same thing, except that the latter adds rpc in addition to ldap support. In order to simplify and speed up the process of integration of getLAPSv2 , and because of your necessities as well, we decided to prioritize and integrate #1556 right now. So regarding this PR,

thanks

mpgn commented 6 months ago

Hello @anadrianmanrique

image

the commit "Add ServerName argument to srvs.hNetrShareEnum" is this PR if you want to merge it

https://github.com/fortra/impacket/pull/947

mpgn commented 6 months ago

changes in dcomrt,py are related to https://github.com/fortra/impacket/issues/1600 ? can you confirm this?

can confirm yes :)

Marshall-Hallenbeck commented 6 months ago

@anadrianmanrique I'm not sure why the GDKI stuff is showing up as a new change for this branch - the commit hashes are the same, so I would think it would be merged fine. I've updated our downstream branch to be up to date with yours.

Something is funky with this PR because the other small changes also have to do with the GDKI stuff that has now been merged. I might close this and open a new one for any remaining changes.

Marshall-Hallenbeck commented 6 months ago

Closing this in favor of the one @mpgn opened since this wasn't properly updating from our downstream for some reason.