Resolve string handling for "https://" or "http://" in update_rhsm_conf #17222

robbmanes · 2018-03-28T14:53:44Z

Previously, if a proxy URI was entered into the registration page leading with
"https://" or "http://", it would be incorrectly entered into the rhsm.conf.
This patch prevents this by ensuring it is a valid URI, and correctly parsing
the string regardless if it has only a host in the URI or if it leads with the
protocol.

robbmanes · 2018-03-28T14:59:09Z

From BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1561448

The issue arises if, in the editing window where you type the URL of the proxy (Configuration=> Settings=> Region=> Redhat Updates=> Edit), you specify the proxy prefixed with "https://" or "http://". This will incorrectly assume everything after the colon is a port number.

This is likely a text parsing issue, reading the colon as the port indicator and subsequently breaking up the line like so:

proxy_hostname = https
proxy_port = //rmanes-server.someserver.com

Workaround is to either:

Specify the proxy without prepending "https://" or "http://"
Edit the /etc/rhsm/rhsm.conf and change the proxy_hostname and proxy_port fields to the correct proxy location.

The above patch didn't seem to need any new specs, and I call into question my own if/else statement, but couldn't make a smaller unless work for me.

robbmanes · 2018-03-28T15:23:42Z

I stupidly didn't test with raw IP's - correcting and squashing shortly.

Build fails when we try something like:

irb(main):001:0> Addressable::URI.parse('192.0.2.0:3128')
Addressable::URI::InvalidURIError: Invalid scheme format: 192.0.2.0

So need to revamp the test before we handle a URI - open to suggestions as well.

Previously, if a proxy URI was entered into the registration page leading with "https://" or "http://", it would be incorrectly entered into the rhsm.conf. This patch prevents this by ensuring it is a valid URI, and correctly parsing the string regardless if it has only a host in the URI or if it leads with the protocol. https://bugzilla.redhat.com/show_bug.cgi?id=1561448

https://bugzilla.redhat.com/show_bug.cgi?id=1561448

- Allow for http(s) or other schemes - Allow for hostname, IPv4 or IPv6 addresses https://bugzilla.redhat.com/show_bug.cgi?id=1561448

bdunne · 2018-04-02T18:58:32Z

@carbonin Can I get another set of 👀 on this since I'm also a committer here?

bdunne

👍

miq-bot · 2018-04-02T19:00:48Z

Checked commits https://github.com/robbmanes/manageiq/compare/ab7f65233e51e43c983c88f726efcd3d7a39f40d~...963a3f3a6c2a5d2b5f1059b06c75756323f22a13 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. ⭐

carbonin · 2018-04-02T19:21:11Z

app/models/registration_system.rb

@@ -83,8 +83,9 @@ def self.update_rhsm_conf(options = {})

    return unless option_values[:proxy_address]

-    write_rhsm_config(:proxy_hostname => option_values[:proxy_address].split(':')[0],
-                      :proxy_port     => option_values[:proxy_address].split(':')[1],
+    proxy_uri = URI.parse(option_values[:proxy_address].include?("://") ? option_values[:proxy_address] : "http://#{option_values[:proxy_address]}")


I feel like this just punts the issue down the road. Is there no field validation for this? Couldn't we just say "This thing needs to be a valid URI" and give a flash message otherwise?

For example if someone were to forget a / and enter "http:/example.com:123" you would end up in this situation right?

irb(main):007:0> uri = URI.parse("http://http:/example.com:123") => #<URI::HTTP http://http/example.com:123> irb(main):008:0> uri.host => "http" irb(main):009:0> uri.port => 80

I agree - I tried using Addressable::URI objects initially before I did string checking but am not sure there is error checking by the caller of this method, and testing with that manner threw other ugly exceptions further down the stack that similar didn't appear in the UI or any easy-to-interpret log, so I instead just altered the string validation.

I can look at it deeper after this week, I am onsite with a customer now unfortunately, or we can close this PR in favor of a method that does better error handling.

¯\(ツ)/¯ I think I'm okay with this as is for now. @carbonin and I were just discussing that we may want to open a RFE to the UI to add validation to this field. At least this adds the ability to accept URLs

carbonin

Agree with @bdunne. This is nothing but an improvement so LGTM.

simaishi · 2018-04-12T02:52:29Z

@robbmanes @carbonin Can this be gaprindashvili/yes ?

Resolve string handling for "https://" or "http://" in update_rhsm_conf (cherry picked from commit b44f6b1) https://bugzilla.redhat.com/show_bug.cgi?id=1566562

simaishi · 2018-04-12T14:11:43Z

Gaprindashvili backport details:

$ git log -1
commit 6501817774fc8c83bda0047cc90ac39991090d85
Author: Nick Carboni <ncarboni@redhat.com>
Date:   Tue Apr 3 10:25:46 2018 -0400

    Merge pull request #17222 from robbmanes/properly_validate_proxy_urls
    
    Resolve string handling for "https://" or "http://" in update_rhsm_conf
    (cherry picked from commit b44f6b17977f8f8e317360ecde03ea92e35b32ba)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1566562

simaishi · 2018-04-27T12:44:49Z

@robbmanes @bdunne Can this be fine/yes? This is a blocker for the next Fine build.

Resolve string handling for "https://" or "http://" in update_rhsm_conf (cherry picked from commit b44f6b1) https://bugzilla.redhat.com/show_bug.cgi?id=1572621

simaishi · 2018-04-27T13:49:14Z

Fine backport details:

$ git log -1
commit f25f8108571e619800d56e8c4bffdf8cc144778d
Author: Nick Carboni <ncarboni@redhat.com>
Date:   Tue Apr 3 10:25:46 2018 -0400

    Merge pull request #17222 from robbmanes/properly_validate_proxy_urls
    
    Resolve string handling for "https://" or "http://" in update_rhsm_conf
    (cherry picked from commit b44f6b17977f8f8e317360ecde03ea92e35b32ba)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1572621

…oxy_urls Resolve string handling for "https://" or "http://" in update_rhsm_conf (cherry picked from commit b44f6b1) https://bugzilla.redhat.com/show_bug.cgi?id=1572621

bdunne self-assigned this Mar 28, 2018

bdunne added bug core labels Mar 28, 2018

robbmanes and others added 6 commits April 2, 2018 14:56

Add tests for RegistrationSystem.update_rhsm_conf with http and https

58c65e4

https://bugzilla.redhat.com/show_bug.cgi?id=1561448

Reduce duplicate test logic

65bfb6a

https://bugzilla.redhat.com/show_bug.cgi?id=1561448

Test against multiple addresses (hostname, IPv4, IPv6)

47ddbb4

https://bugzilla.redhat.com/show_bug.cgi?id=1561448

myport is not a valid port, 0 is reserved so use that instead

dafb5f7

https://bugzilla.redhat.com/show_bug.cgi?id=1561448

Fix issues related to proxy_address

963a3f3

- Allow for http(s) or other schemes - Allow for hostname, IPv4 or IPv6 addresses https://bugzilla.redhat.com/show_bug.cgi?id=1561448

bdunne requested a review from carbonin April 2, 2018 18:59

bdunne approved these changes Apr 2, 2018

View reviewed changes

carbonin reviewed Apr 2, 2018

View reviewed changes

bdunne added the blocker label Apr 3, 2018

carbonin approved these changes Apr 3, 2018

View reviewed changes

carbonin self-assigned this Apr 3, 2018

carbonin merged commit b44f6b1 into ManageIQ:master Apr 3, 2018

carbonin added this to the Sprint 83 Ending Apr 9, 2018 milestone Apr 3, 2018

bdunne added the gaprindashvili/yes label Apr 12, 2018

simaishi added gaprindashvili/backported and removed gaprindashvili/yes labels Apr 12, 2018

bdunne added the fine/yes label Apr 27, 2018

simaishi added fine/backported and removed fine/yes labels Apr 27, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resolve string handling for "https://" or "http://" in update_rhsm_conf #17222

Resolve string handling for "https://" or "http://" in update_rhsm_conf #17222

robbmanes commented Mar 28, 2018

robbmanes commented Mar 28, 2018 •

edited

Loading

robbmanes commented Mar 28, 2018 •

edited

Loading

bdunne commented Apr 2, 2018

bdunne left a comment

miq-bot commented Apr 2, 2018

carbonin Apr 2, 2018 •

edited

Loading

robbmanes Apr 3, 2018

bdunne Apr 3, 2018

carbonin left a comment

simaishi commented Apr 12, 2018

simaishi commented Apr 12, 2018

simaishi commented Apr 27, 2018

simaishi commented Apr 27, 2018

Resolve string handling for "https://" or "http://" in update_rhsm_conf #17222

Resolve string handling for "https://" or "http://" in update_rhsm_conf #17222

Conversation

robbmanes commented Mar 28, 2018

robbmanes commented Mar 28, 2018 • edited Loading

robbmanes commented Mar 28, 2018 • edited Loading

bdunne commented Apr 2, 2018

bdunne left a comment

Choose a reason for hiding this comment

miq-bot commented Apr 2, 2018

carbonin Apr 2, 2018 • edited Loading

Choose a reason for hiding this comment

robbmanes Apr 3, 2018

Choose a reason for hiding this comment

bdunne Apr 3, 2018

Choose a reason for hiding this comment

carbonin left a comment

Choose a reason for hiding this comment

simaishi commented Apr 12, 2018

simaishi commented Apr 12, 2018

simaishi commented Apr 27, 2018

simaishi commented Apr 27, 2018

robbmanes commented Mar 28, 2018 •

edited

Loading

robbmanes commented Mar 28, 2018 •

edited

Loading

carbonin Apr 2, 2018 •

edited

Loading