Bug 1566615

According to the URI spec[1] the userinfo component must be pct-encoded.

Since pct-encoding is not idempotent, if we attempt to encode it ourselves we would be causing an error when IT IS encoded properly.

Also if it is not properly encoded, we could not even parse the URL to figure out what the userinfo component is! (password can contain '@' '//' '/' etc)


[1] https://www.ietf.org/rfc/rfc3986.txt (section 3.2.1)

Are we talking about a global proxy server for ALL of the container providers configured in EVM-> Configuration -> Advanced -> "http_proxy" -> "openshift"

Or a proxy server for an INDIVIDUAL provider configured in
"Add New Container Provider" page -> bottom of the screen(only visible after selecting "Openshift") "Settings" section -> "Proxy" tab -> Proxy Settings -> "HTTP Proxy"?

I've been working since morning on getting a test environment so I can test all scenarios since I might have misunderstood, but a clarification would help.

Sorry if I closed prematurely, I'll get to the bottom this and address accordingly, thanks.

The root cause of this problem is that the `URI.parse` method does not decode the percent encoded characters that appear in the `userinfo` part of the URL. For example, the following code:

  require 'uri'
  u = URI.parse('http://myuser:%24%3fxxxx@192.168.100.10:3128')
  puts u.password

Produces the following output:

  %24%3fxxxx
But according to RFC 3986 it should decode the precent encoded characters, and produce the following output:

  $?xxxx

The `rest-client` gem, for example, extracts directly the proxy password from the URI calling this method. As a result it fails to authenticate, because it sends the encoded characters. We use that gem for the HTTP communications with the Kubernetes and OpenShift API servers.

The only simple solution that I can find to this problem is to patch the URI class, with a file `uri_patch.rb` inside the `config/initializers` directory:

---8<---
require 'uri'

class URI::HTTP
  alias_method :_password, :password

  def password
    value = _password
    value = URI.decode(value) if value
    value
  end
end
--->8---

Note however that this has to be combined with manually encoding of the URL by the user. For example, if the user name is `myuser` and the password is `$?xxxx` the user will have to put exactly this in the UI:

  http://myuser:%24%3fxxxx@192.168.100.10:3128

That patch is what I am proposing in this pull request:

  Patch URI to decode password
  https://github.com/ManageIQ/manageiq/pull/17318

However I expect objections to merge this patch, as it affects all users of the `URI.parse` method, not just the Kubernetes and OpenShift providers.

The right solution would probably be, as suggested by David Luong, to add to the UI two new fields for the user name and the password. But that would require changes to the UI, to the backend, to the `kubeclient` gem, and possible to the `rest-client` gem (which we don't maintain).

My suggestion is to see if dropping that patch file in the `config/initializers` directory solves the problem for the user. In parallel, but with less urgency, we can consider what it takes to do the right solution.

A less intrusive way to solve this is to add a new `proxy` URI schema that does the percent decoding automatically. I have updated the pull request accordingly:

  Add proxy URI schema
  https://github.com/ManageIQ/manageiq/pull/17318

To use it drop a `proxy_uri.rb` file with the following content into the `config/initializers` directory:

---8<---
require 'cgi'
require 'uri'

module URI
  class PROXY < HTTP
    def password
      CGI.unescape(super())
    end
  end

  @@schemes['PROXY'] = PROXY
end
--->8---

Then, when adding or editing the provider, use a proxy URI like this:

  proxy://myuser:%24%3fxxxx.122.40:3128

Note the use of `proxy` instead of `http` and the percent encoding of the characters.

A simple command to generate the percent encoded password:

  $ ruby -e "require 'cgi'; puts CGI.escape(ARGV[0])" '$?xxxx'
  %24%3Fxxxx

Created attachment 1470341 [details]
Monkey patch rest-client to support percent encoded proxy user/password.

Based on changes not yet merged in rest-client:
https://github.com/rest-client/rest-client/pull/665

I have the proposed attached monkey patch to rest-client.  Note, it needs to load parts of rest-client in order to monkey patch it.  This could make normal processes that never load it consume more memory and ultimately changes how rest-client is loaded.

If we really want to ship this fix, I can open a PR to manageiq with this change.

Here is how I verified it worked in rails console:

Before:

irb(main):004:0> r = RestClient::Request.new(method: :get, url: 'http://127.0.0.1/api', proxy: 'http://%24myuser:%24%3Fxxxx@127.0.0.1')
=> <RestClient::Request @method="get", @url="http://127.0.0.1/api">=> <RestClient::Request @method="get", @url="https://127.0.0.1/api">
irb(main):002:0> r.net_http_object('host', 80).proxy_user
=> "%24myuser"
irb(main):003:0> r.net_http_object('host', 80).proxy_pass
=> "%24%3Fxxxx"

After:
irb(main):004:0> r = RestClient::Request.new(method: :get, url: 'http://127.0.0.1/api', proxy: 'http://%24myuser:%24%3Fxxxx@127.0.0.1')
=> <RestClient::Request @method="get", @url="http://127.0.0.1/api">=> <RestClient::Request @method="get", @url="https://127.0.0.1/api">
irb(main):002:0> r.net_http_object('host', 80).proxy_user
=> "$myuser"
irb(main):003:0> r.net_http_object('host', 80).proxy_pass
=> "$?xxxx"

I changed the target release to 5.10 so am not looking to do anything in 5.9.x anymore, have concerns like you on monkey patching. Maybe we have this same conversation in a couple months if nothing has happened on that PR in rest-client project.

Looks like we are now at that point where we need to carry the patch ourselves. Joe R lets move ahead as you describe in comment #42.

https://github.com/ManageIQ/manageiq/pull/18105

Opened PR to manageiq to monkey patch the rest client library since it's still not merged and released upstream, see comment #46.

New commit detected on ManageIQ/manageiq/master:

https://github.com/ManageIQ/manageiq/commit/d2b0e320ac475e6a16a8cd52c30de49b3665695f
commit d2b0e320ac475e6a16a8cd52c30de49b3665695f
Author:     Joe Rafaniello <jrafanie>
AuthorDate: Wed Oct 17 13:56:48 2018 -0400
Commit:     Joe Rafaniello <jrafanie>
CommitDate: Wed Oct 17 13:56:48 2018 -0400

    RestClient: Support percent encoded proxy user/pass

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

    Monkey patch RestClient for patch releases of rest-client 2.0.0.
    The underlying method, net_http_object, has not been modified since 2015
    so it should be relatively safe to monkey patch this until the upstream
    PR gets merged/released.

    Upstream PR: https://github.com/rest-client/rest-client/pull/665

    To verify this works, run bin/rails console:

    Before:

    ```
    irb(main):004:0> r = RestClient::Request.new(method: :get, url:
    'http://127.0.0.1/api', proxy: 'http://%24myuser:%24%3Fxxxx@127.0.0.1')
    => <RestClient::Request @method="get", @url="http://127.0.0.1/api">=>
    <RestClient::Request @method="get", @url="https://127.0.0.1/api">
    irb(main):002:0> r.net_http_object('host', 80).proxy_user
    => "%24myuser"
    irb(main):003:0> r.net_http_object('host', 80).proxy_pass
    => "%24%3Fxxxx"
    ```

    After:

    ```
    irb(main):004:0> r = RestClient::Request.new(method: :get, url:
    'http://127.0.0.1/api', proxy: 'http://%24myuser:%24%3Fxxxx@127.0.0.1')
    => <RestClient::Request @method="get", @url="http://127.0.0.1/api">=>
    <RestClient::Request @method="get", @url="https://127.0.0.1/api">
    irb(main):002:0> r.net_http_object('host', 80).proxy_user
    => "$myuser"
    irb(main):003:0> r.net_http_object('host', 80).proxy_pass
    => "$?xxxx"
    ```

 lib/patches/rest_client_patch.rb | 29 +
 lib/vmdb_helper.rb | 1 +
 2 files changed, 30 insertions(+)

New commit detected on ManageIQ/manageiq/hammer:

https://github.com/ManageIQ/manageiq/commit/81bec89af724da633bfa316f40e04cca8f55e2de
commit 81bec89af724da633bfa316f40e04cca8f55e2de
Author:     Brandon Dunne <brandondunne>
AuthorDate: Fri Oct 19 10:19:26 2018 -0400
Commit:     Brandon Dunne <brandondunne>
CommitDate: Fri Oct 19 10:19:26 2018 -0400

    Merge pull request #18105 from jrafanie/rest_client_monkey_patch4percent_encoded_proxy_user_password

    RestClient: Support percent encoded proxy user/pass
    (cherry picked from commit 5a784c39bfc87d4d281259061b7578ce140d0283)

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

 lib/patches/rest_client_patch.rb | 29 +
 lib/vmdb_helper.rb | 1 +
 2 files changed, 30 insertions(+)

Verified in 5.10.0.24.20181113213923_03b81fd

1) Setup proxy server with htpasswd authentication:

first request is the correct username and password, cfme:$?rhel, and the second is a bad username:

juwatts@localhost /tmp $ export http_proxy=http://cfme:%24%3Frhel@10.8.198.140:3128
juwatts@localhost /tmp $ wget www.google.com
--2018-11-14 16:09:22--  http://www.google.com/
Connecting to 10.8.198.140:3128... connected.
Proxy request sent, awaiting response... 200 OK
Length: unspecified [text/html]
Saving to: ‘index.html.10’

index.html.10                                                           [ <=>                                                                                                                                                                ]  11.98K  --.-KB/s    in 0.01s  

2018-11-14 16:09:22 (1.19 MB/s) - ‘index.html.10’ saved [12271]

juwatts@localhost /tmp $ export http_proxy=http://cfm2:%24%3Frhel@10.8.198.140:3128
juwatts@localhost /tmp $ wget www.google.com
--2018-11-14 16:09:30--  http://www.google.com/
Connecting to 10.8.198.140:3128... connected.
Proxy request sent, awaiting response... 407 Proxy Authentication Required
2018-11-14 16:09:30 ERROR 407: Proxy Authentication Required.

juwatts@localhost /tmp $ 

2) Added an openshift provider and in the proxy setting, added the following URI:

http://cfme:%24%3Frhel@10.8.198.140:3128

Successfully saw access in squid proxy

1542296585.051     18 10.8.198.119 TCP_TUNNEL/200 11070 CONNECT epacific-ocp-master-v3.cmqe.lab.eng.rdu2.redhat.com:8443 cfme HIER_DIRECT/10.8.218.81 -
1542296585.068      9 10.8.198.119 TCP_TUNNEL/200 2855 CONNECT epacific-ocp-master-v3.cmqe.lab.eng.rdu2.redhat.com:8443 cfme HIER_DIRECT/10.8.218.81 -

Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://access.redhat.com/errata/RHSA-2019:0212