Post Receive URLs and HTTPS

niosHD commented 12 years ago

First of all I want to thank you for your work on this really great plugin!

I am currently using version 0.4.6x of the redmine_git_hosting plugin and played around with the new Post Receive URLs. Unfortunately the get/post seems to fail when https is used. As far as I can tell your new version (v0.5.0x) should have the same issue. I already implemented a fast fix which can be found here [1]. As I am not that experienced in ruby it would be great if you could have a quick look at it. Maybe you can even integrate it into your plugin ...

regards niosHD

[1] nioshd@c825fe6a0226425b89e48fa397f2ffe290d4e799

kubitron commented 12 years ago

Can you tell me what the failure is? It looks like you added a lot of code...

niosHD commented 12 years ago

Unfortunately not. There is neither an error in the rails production.log nor in the response to the hook which made it a real hassle to locate the bug. Judging from the received response to the hook it seems like it simply crashed.

To test out the involved commands in irb I used the following script:

require 'net/http'
uri = URI('https://twitter.com/') # try different webpages (google, twitter, github, ...)
response = Net::HTTP.get_response(uri)

Using HTTP all requests (google, twitter, github) worked and returned a valid response (= status codes 200, 301 or 302). However, using HTTPS all of them had issues.

For https://twitter.com/ and https://www.google.com/ the script crashed and I got the following:

EOFError: end of file reached
        from /usr/lib64/ruby/1.9.1/net/protocol.rb:141:in `read_nonblock'
        from /usr/lib64/ruby/1.9.1/net/protocol.rb:141:in `rbuf_fill'
        from /usr/lib64/ruby/1.9.1/net/protocol.rb:122:in `readuntil'
        from /usr/lib64/ruby/1.9.1/net/protocol.rb:132:in `readline'
        from /usr/lib64/ruby/1.9.1/net/http.rb:2562:in `read_status_line'
        from /usr/lib64/ruby/1.9.1/net/http.rb:2551:in `read_new'
        from /usr/lib64/ruby/1.9.1/net/http.rb:1319:in `block in transport_request'
        from /usr/lib64/ruby/1.9.1/net/http.rb:1316:in `catch'
        from /usr/lib64/ruby/1.9.1/net/http.rb:1316:in `transport_request'
        from /usr/lib64/ruby/1.9.1/net/http.rb:1293:in `request'
        from /usr/lib64/ruby/1.9.1/net/http.rb:1195:in `request_get'
        from /usr/lib64/ruby/1.9.1/net/http.rb:455:in `block in get_response'
        from /usr/lib64/ruby/1.9.1/net/http.rb:745:in `start'
        from /usr/lib64/ruby/1.9.1/net/http.rb:454:in `get_response'
        from (irb):3
        from /usr/bin/irb:12:in `<main>'

https://github.com/ behaves more gracefully and returns status code 400 (=Bad Request).

On my own test server I got:

Net::HTTPBadResponse: wrong status line: "<!DOCTYPE HTML PUBLIC \"-//IETF//DTD HTML 2.0//EN\">"
        from /usr/lib64/ruby/1.9.1/net/http.rb:2564:in `read_status_line'
        from /usr/lib64/ruby/1.9.1/net/http.rb:2551:in `read_new'
        from /usr/lib64/ruby/1.9.1/net/http.rb:1319:in `block in transport_request'
        from /usr/lib64/ruby/1.9.1/net/http.rb:1316:in `catch'
        from /usr/lib64/ruby/1.9.1/net/http.rb:1316:in `transport_request'
        from /usr/lib64/ruby/1.9.1/net/http.rb:1293:in `request'
        from /usr/lib64/ruby/1.9.1/net/http.rb:1195:in `request_get'
        from /usr/lib64/ruby/1.9.1/net/http.rb:455:in `block in get_response'
        from /usr/lib64/ruby/1.9.1/net/http.rb:745:in `start'
        from /usr/lib64/ruby/1.9.1/net/http.rb:454:in `get_response'
        from (irb):15
        from /usr/bin/irb:12:in `<main>'

I assume that the different behavior depends mainly on the web server configuration.

I searched for a solution and came to the following script for doing the get request:

require 'net/http'
url = URI.parse('https://twitter.com/') # try different webpages (google, twitter, github, ...)
http = Net::HTTP.new(url.host, url.port)
http.use_ssl = (url.scheme == 'https')
request = Net::HTTP::Get.new(url.path)
response = http.start {|http| http.request(request) }

This version works (= status codes 200, 301 or 302) for HTTP and HTTPS on all earlier mentioned hosts and can already be used as a first patch for the empty get case. (only three lines of additional code)

What I have not mentioned before is that my version additionally deals with the redirects (301 and 302). Therefore the above code has been moved in a method (=get_uri) which recursively follows the redirects up to a certain limit. To keep it more readable the same bug fix for post has been done in a dedicated method (=post_uri).

If you do not think that the Post Receive URLs should follow redirects than at least the HTTPS fix should be applied.

Remark: I just found a small bug in the new implementation as well. While using "http://www.google.com/" is working "http://www.google.com" crashes as the url.path is empty which seems to be a problem for Net::HTTP::Get.new. With the following version this problem is solved.

request = Net::HTTP::Get.new(url.path != "" ? url.path : '/')

(I will fix this on my repository as well and post the hash when it is done.)

niosHD commented 12 years ago

This is slowly getting embarrassing... In addition to the previously announced bug I just found out that the query part of the url was not forwarded correctly. So I quickly fixed this as well. The two commits are [1] and [2] which brings me to my current proposals for the bugfix.

[1] niosHD@78aae5b5f5b2a880abd69d08ad4dcf7e54023488 [2] niosHD@e76b7f8db7522ca34b92fc73901a25eda68ab0f1

Simple fix without redirects:

url = URI.parse(prurl.url)
http = Net::HTTP.new(url.host, url.port)
http.use_ssl = (url.scheme == 'https')
path = url.path != "" ? url.path : '/'
payloads.each {|payload|
  if prurl.mode == :github
    request = Net::HTTP::Post.new("#{path}?#{url.query}")
    request.set_form_data({"payload" => payload.to_json})
  else
    request = Net::HTTP::Get.new("#{path}?#{url.query}")
  end
  res = http.start {|http| http.request(request) }
  output.write res.is_a?(Net::HTTPSuccess) ? "[success] " : "[failure] "
  output.flush
}

More complex fix with redirects:

    def get_uri(uri_str, limit = 10)
      # You should choose better exception.
      raise ArgumentError, 'HTTP redirect too deep' if limit == 0

      url = URI.parse(uri_str)
      http = Net::HTTP.new(url.host, url.port)
      http.use_ssl = (url.scheme == 'https')
      path = url.path != "" ? url.path : '/'
      request = Net::HTTP::Get.new("#{path}?#{url.query}")
      response = http.start {|http| http.request(request) }

      case response
      when Net::HTTPSuccess     then response
      when Net::HTTPRedirection then get_uri(response['location'], limit - 1)
      else
        response
      end
    end

    def post_uri(uri_str, content, limit = 10)
      # You should choose better exception.
      raise ArgumentError, 'HTTP redirect too deep' if limit == 0

      url = URI.parse(uri_str)
      http = Net::HTTP.new(url.host, url.port)
      http.use_ssl = (url.scheme == 'https')
      path = url.path != "" ? url.path : '/'
      request = Net::HTTP::Post.new("#{path}?#{url.query}")
      request.set_form_data(content)
      response = http.start {|http| http.request(request) }

      case response
      when Net::HTTPSuccess     then response
      when Net::HTTPRedirection then post_uri(response['location'],content, limit - 1)
      else
        response
      end
    end

# ....

             payloads.each {|payload|
                    if prurl.mode == :github
                        res = post_uri(prurl.url, {"payload" => payload.to_json})
                    else
                        res = get_uri(prurl.url)
                    end
                    output.write res.is_a?(Net::HTTPSuccess) ? "[success] " : "[failure] "
                    output.flush
                }

It would be possible to combine the function as well. However, I am not sure if this would improve the readability of the code.

kubitron commented 12 years ago

OK. I apologize for the problems that you are having. The post-receive url functionality has probably not been fully tested with the http interface. I would like to get you up to speed as soon as possible.

However, I suspect that the real problem here is that I have only had sporadic debugging with Ruby > 1.8.7 (I run on a RedHat system that doesn't yet support Ruby 1.9.x.).

Could you do two things for me? (1) is it at all possible to try with Ruby 1.8.7? If not, I understand, (2) Could you work with Version 0.5.0x? I want to make sure that any fixes go to the latest release.

Also, could you post the information about your environment? To do that, you need to set the RAILS_ENV variable and execute the "about" script. For instance, on bash:

prompt% cd path_of_redmine_root
prompt% export RAILS_ENV=production
prompt% ./script/about

thanks.

niosHD commented 12 years ago

No need to apologize! As I already mentioned I am truly grateful that you work on this plugin and for your support. In my opinion it is the best version of the redmine_git_hosting plugin with a lot of really cool features. Furthermore I am more than glad if I can help to make it even better.

About my server environment:

Ruby version              1.8.7 (x86_64-linux)
RubyGems version          1.8.15
Rack version              1.1
Rails version             2.3.14
Active Record version     2.3.14
Active Resource version   2.3.14
Action Mailer version     2.3.14
Active Support version    2.3.14
Edge Rails revision       unknown
Application root          /var/redmine-1.3.2
Environment               production
Database adapter          mysql
Database schema version   20120803043256

About your Redmine plugins
Redmine Gitrevision Download plugin   0.0.8
Redmine Code Review plugin            0.4.8
Embedded                              0.0.1
Redmine Close Button plugin           0.0.8
Google Analytics plugin               0.2.0
Redmine Git Hosting Plugin            0.4.6x

(1): This is definitely not (only) a ruby > 1.8.7 issue. The errors I posted above are from my laptop with ruby 1.9.x when I execute the described script. On our real redmine server I actually use ruby 1.8.7. My tests there come to the same conclusion. I suppose that Net::HTTP.get_response is simply incompatible with HTTPS. A short Internet research leads to the same assumption as it is never used in combination with HTTPS.

(2): I plan to upgrade to the new version when I have time in the course of this week. I only made this bug report for the old version as I did not had the chance to do the upgrade earlier. Furthermore I do not think that the described problem has changed in the new version.

After doing the upgrade I will test the Post Receive URLs again and reapply (and merge) the patches if necessary. Would you prefer the simple fix or the more complex one with redirect support?

Anyway, I will keep you posted.

niosHD commented 12 years ago

I found now the time to do the update to the recent 0.5.0x version of the git hosting plugin. Like I expected the behavior of the post receive urls stayed the same. As you can see at the pull request I decided to apply the minimal fix. I tested both request types and for me they are working now.

What are your thoughts about this fix?

I think it is unrelated to this issue but since I updated the plugin I get the following error as soon as I open the settings tab of a project:

Processing ProjectsController#settings (for 193.170.226.203 at 2012-10-10 08:29:26) [GET]
  Parameters: {"id"=>"demo", "action"=>"settings", "controller"=>"projects"}
Fetching changes from gitolite-admin repository to /tmp/redmine_git_hosting/gitolite/gitolite-admin
Our hook is already installed
***> undefined method `identifier' for #<ActiveRecord::Associations::HasOneAssociation:0x972eb48>
***> /var/redmine-1.3.2/vendor/rails/activerecord/lib/active_record/associations/association_proxy.rb:217:in `method_missing'
***> /var/redmine-1.3.2/vendor/plugins/redmine_git_hosting/lib/git_hosting.rb:1105:in `closest_path'
***> /var/redmine-1.3.2/vendor/plugins/redmine_git_hosting/lib/git_hosting.rb:854:in `update_repositories'
***> /var/redmine-1.3.2/vendor/plugins/redmine_git_hosting/lib/git_hosting.rb:844:in `each'
***> /var/redmine-1.3.2/vendor/plugins/redmine_git_hosting/lib/git_hosting.rb:844:in `update_repositories'
***> git_hosting: update_repositories() failed
Rendering template within layouts/base
Rendering projects/settings
Completed in 1470ms (View: 104, DB: 4) | 200 OK

Any ideas on that one?

kubitron commented 12 years ago

Hello. Go ahead and pull from master again. I think that you may have missed a recent update (see bug #74). If there are other problems, please put them in as actual bugs. You are an important tester for 0.5.0x at this point, since you are still on the 1.3.x release.

(I actually changed the 0.5.0x tag, because of this problem).

As for your change with respect to https, it looks perfectly ok. I'll take care of getting it into master (probably not for a couple of days, since I have a couple of fires to put out in something else). Thanks for tracking that problem down.

kubitron commented 12 years ago

Oops. @niosHD, that is actually another incompatibility with < 1.4 Redmine. I'll fix it.

kubitron commented 12 years ago

Ok. @niosHD, try pulling from master again and see if that problem with identifiers goes away...

niosHD commented 12 years ago

Great. Thx, for the quick response. The previously reported bug has been fixed by this.

kubitron commented 12 years ago

Hey. So this https fix is going to end up being more complex, since it appears that http.start can throw SSL errors (it is doing that for me at the moment). I will reimplement it with somewhat more intelligent error handling.

Please keep using your code for now. Using of ssl seems to come with a variety of complex problems.

niosHD commented 12 years ago

Ok. Thx, for the update. I have not encountered this problem until now. Just an idea but are you testing with a self signed certificate? (I used only valid/signed certificates so far.)

Please keep me posted on this one.

kubitron commented 12 years ago

Ok. Try pulling from head of master. If this works for you, you can close out the bug. Current code works propoerly as far as I can tell using RequestBin. There is more information given in the log about success/failure, for folks trying to debug things in the future.

[Haven't really figured out what was wrong with my configuration, but probably related to having a certificate with Alternate Names in it..., turns out that it is good that my config was bad, because it pointed out that I need to catch errors thrown by OpenSSL.]

niosHD commented 12 years ago

The first tests look promising. Thank you for taking care of this problem!

kubitron commented 12 years ago

You provided most of the code. Thank you.

kubitron / redmine_git_hosting

Post Receive URLs and HTTPS #73