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

Use correct case for SSL option #12168

Merged
merged 1 commit into from
Aug 20, 2019
Merged

Conversation

egypt
Copy link
Contributor

@egypt egypt commented Aug 6, 2019

Fix redirection to an HTTPS url from an HTTP url.

Verification

List the steps needed to make sure this thing works

make an aux module with a run method like

def run_host(ip)
       response = send_request_cgi!(                                                                                                                                                                            
         opts={'uri' => '/admin', 'method' => 'GET' }, 5, 5                                                                                                                                                     
       )                                                                                                                                                                                                        
       p opts                                                                                                                                                                                                   
end
  • Find or make a webserver that 302's to an https URL.
  • Verify the outputted opts['SSL'] is true
  • Verify redirect is followed as expected

@@ -448,16 +448,18 @@ def reconfig_redirect_opts!(res, opts)

opts['ssl'] = ssl
Copy link
Member

Choose a reason for hiding this comment

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

There's another one up here right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this line is basically a no-op because it will never differ from the previous request.

@busterb
Copy link
Member

busterb commented Aug 7, 2019

The oddity also is that send_request_cgi says:

# Passesopts through directly to {Rex::Proto::Http::Client#request_cgi}.

and Rex::Proto::Http::Client also uses opts['ssl'] lowercase, or at least sets that internally, but maybe that value not actually used, and instead it's self.ssl? Seems like those would also be upper-case converted. Agree this fixes a bug, but maybe a few others scurry out of the light when looking at this closer!

@sempervictus
Copy link
Contributor

sempervictus commented Aug 9, 2019

This is handled, usually, by Socket:: Parameters, which IIRC will take both upper and lowercase for this. Need to double check that, as my recollection is from params refactoring for sslclient params a while back.

@egypt
Copy link
Contributor Author

egypt commented Aug 13, 2019

Travis error looks like an unrelated network problem on Travis's end.

@busterb busterb self-assigned this Aug 20, 2019
@bcook-r7 bcook-r7 merged commit 3b7abfc into rapid7:master Aug 20, 2019
@busterb
Copy link
Member

busterb commented Aug 20, 2019

Added bd90241 for consistency within the function, even if is a no-op.

@busterb
Copy link
Member

busterb commented Aug 20, 2019

Release Notes

This fixes redirection to an HTTPS url from an HTTP url with the HTTP client library.

@tdoan-r7 tdoan-r7 added the rn-fix release notes fix label Sep 5, 2019
@egypt egypt deleted the bug/redirect-to-ssl branch September 10, 2019 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug library rn-fix release notes fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants