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

Incorporate Pennyw0rth/NetExec Impacket changes into Fortra Impacket #1715

Closed
wants to merge 14 commits into from

Conversation

Marshall-Hallenbeck
Copy link
Contributor

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
Copy link
Contributor Author

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

@gabrielg5
Copy link
Collaborator

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
Copy link
Contributor Author

Marshall-Hallenbeck commented Mar 18, 2024

@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
Copy link
Contributor

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
Copy link
Contributor

mpgn commented Mar 19, 2024

Hello @anadrianmanrique

image

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

#947

@mpgn
Copy link
Contributor

mpgn commented Mar 19, 2024

changes in dcomrt,py are related to #1600 ? can you confirm this?

can confirm yes :)

@Marshall-Hallenbeck
Copy link
Contributor Author

@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
Copy link
Contributor Author

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

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.

7 participants