-
Notifications
You must be signed in to change notification settings - Fork 13.8k
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
Drop python 3.6 string formatting syntax in rdp_web_login #14953
Drop python 3.6 string formatting syntax in rdp_web_login #14953
Conversation
For future reference, do we have a minimum version of Python that we should be supporting in our external modules moving forward? I know it's 3.x something but there are certainly starting to be some appealing features coming out in the later versions that would make it helpful to know what's the oldest we need to maintain compatibility with. |
I think this is up for grabs, as it stands current automation uses 3.5 and supports older platforms which may never update beyond 3.5 is official package repositories, if a consensus on a new version is reached tooling can be updated. On that note I have a branch currently in development to add support for all current external modules into the docker environment provided by the Framework repository. Once dev on that branch is complete we could use that environment as the litmus test for desired version support although compatibility with various other distributions may not be well supported by using that environment as the bar. |
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.
The changes overall look fine to me, except for one string replace. I haven't tested it locally, but will try to do so in the coming days on my setup
@@ -88,7 +88,7 @@ def get_ad_domain(rhost, rport, user_agent): | |||
'Host': rhost} | |||
session = requests.Session() | |||
for url in domain_urls: | |||
target_url = f"https://{rhost}:{rport}/{url}" | |||
target_url = 'https://' + rhost +':' + rport+ '/' +targeturi |
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 string formatting should use + url instead of + targeturi, since we're using the local url variable to build the target_url
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.
@bwatters-r7 I pulled your PR down and had to make a few additional changes in order for the string formatting to work. The autocomplete works for me now, but the requested changes will be needed to keep the module functional
url = f'https://{rhost}:{rport}/{targeturi}' | ||
body = f'DomainUserName={domain}%5C{username}&UserPass={password}' | ||
url = 'https://' + rhost + ':' + rport + '/' + targeturi | ||
body = 'DomainUserName=' + 'domain' + '%5C' + username + '&UserPass=' + password |
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 should be domain instead of 'domain' so that it uses the variable not the string 'domain'
except requests.exceptions.Timeout: | ||
module.log(f'Login {domain}\\{username}:{password} is invalid! No response received in {timeout} milliseconds', | ||
module.log('Login ' + domain + '\\' + username + ':' + password + ' is invalid! No response received in ' + timeout + ' milliseconds', |
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 timeout needs a str() wrapper to get the concatenation to work. So replace with str(timeout)
I fixed this by using python string.format, which should be compatible with all python versions and avoids having to convert the arguments into a str. |
Release notesFixed python string formatting compatibility in |
The module cache builder is using a slightly older python syntax, so it will error quietly and fail to add the
rdp_web_login
module to the module cache. This PR removes the cool new string interpolation present in the module and reverts to old, stone-age string concatenation.See the original module PR for testing strategies: #14544, but also:
use auxiliary/scanner/http/rdp_
@jmartin-r7, @k0pak4