This project is archived and is in readonly mode.

#1230 ✓ resolved
Gregg Kellogg

Implement HTTP Digest authentication

Reported by Gregg Kellogg | October 17th, 2008 @ 04:13 AM

This patch implements HTTP Digest authentication for the http_authentication plugin.

Differences in the API from Basic authenticaton:

== Passwords * Basic authentication yields the username and password to the controller. * Digest authentication needs to know the expected password to authenticate. * WWW-Authenticate required == WWW-Authenticate header required Performing a request first requires that any request be made of the server, which will result in an WWW-Authenticate header being generated. Within this header are realm, nonce, and opaque values that are necessary for formatting subsequent requests.

== Nonce creation The nonce is based on time-stamp H(time-stamp ":" session.session_id). An associated nonce_valid? method verifies that the time-stamp is recent. Nc is not checked, as this would require saving state.

== History Originally filed as Ticket #7291 by dccmanges updated by xaviershay.

Comments and changes to this ticket

  • Xavier Shay

    Xavier Shay November 11th, 2008 @ 12:24 AM

    Wow, I totally forgot I wrote this

  • Gregg Kellogg

    Gregg Kellogg November 11th, 2008 @ 12:54 AM

    Yeah, it's been pretty useful. I have some more updates to the patch, but I'm waiting for my project to complete before I put them there.

    Thanks for getting this started, hopefully it will pass muster for inclusion into the trunk at some point.

    Gregg

  • Joshua Peek

    Joshua Peek December 20th, 2008 @ 05:35 PM

    • Milestone cleared.
    • Assigned user set to “Joshua Peek”
    • State changed from “new” to “open”
  • Joshua Peek

    Joshua Peek December 20th, 2008 @ 08:15 PM

    Cool, I want to apply this soon but it needs to be rebased against edge.

  • Gregg Kellogg

    Gregg Kellogg December 20th, 2008 @ 09:14 PM

    I'll work on an update to the patch now. I have a couple of other fixes I wanted to get in anyway.

  • Gregg Kellogg
  • Repository

    Repository December 28th, 2008 @ 09:14 PM

    • State changed from “open” to “resolved”

    (from [45dee3842d68359a189fe7c0729359bd5a905ea4]) HTTP Digest authentication [#1230 state:resolved] http://github.com/rails/rails/co...

  • Repository

    Repository January 13th, 2009 @ 04:16 PM

    (from [c99ef814b0ce5d6b6a677ee6116edac03c8a35b3]) Revert "HTTP Digest authentication [#1230 state:resolved]"

    This reverts commit 45dee3842d68359a189fe7c0729359bd5a905ea4.

    Reasons :

    1. The code is not working in it's current state
    2. Should not be using exceptions for flow control http://github.com/rails/rails/co...
  • Pratik

    Pratik January 13th, 2009 @ 05:41 PM

    • Tests shouldn't use mocking. Just use normal integration tests.
    • ActionController::HttpAuthentication::Digest::ControllerMethods should get included somewhere.
    • Object#send! is long gone.
    • Shouldn't use exceptions for flow control
  • Pratik

    Pratik January 13th, 2009 @ 05:42 PM

    • State changed from “resolved” to “open”
    • Assigned user changed from “Joshua Peek” to “Pratik”

    Btw, thanks for the nice work. I didn't mean to be discouraging by reverting the patch. We should definitely try to have it in for 2.3.

    Thanks!

  • Gregg Kellogg

    Gregg Kellogg January 13th, 2009 @ 07:09 PM

    Here is a new patch that addresses the issues raised:

    • Replace controller.send! with controller.send, mirroring the same case in Basic authentication
    • Refactor exception control flow in authenticate_or_request_with_http_digest.
    • Minimal changes to tests, simply to mirror the basic authentication test, which also uses a Mock controller.
    • Include HttpAuthentication::Digest::ControllerMethods in base.rb
  • Pratik

    Pratik January 13th, 2009 @ 07:14 PM

    I'm strongly in favor of removing mocking from these tests. It should be removed from basic authentication tests too, but that's not in the scope of this ticket.

    Also, "raise Error.new" should be gone.

    Thanks!

  • Gregg Kellogg

    Gregg Kellogg January 13th, 2009 @ 07:37 PM

    Exceptions are used within the body of Digest Authentication, and presented to the application level, as the nature of the encoded digest is dense, and it's useful for the library to provide the application with more detail as to the nature of the failure. The alternative would be to do a simple return of true/false and provide a separate analysis interface to help determine why an authentication attempt has failed. This seems like a reasonable use of exceptions, and is certainly used elsewhere within Rails. Is there some style-guide that relates to this that I'm unaware of?

    If you'd like to eliminate the unit tests, which require the use of a Mock controller, I can integrate the elements tested with the Integration tests in integration_test.rb.

  • Gregg Kellogg

    Gregg Kellogg January 14th, 2009 @ 05:44 AM

    This patch refactors HTTP Digest to remove exception usage. It adds http_digest_authentication_response_detail to be able to determine the cause of an authentication failure without changing the basic pattern used by Basic authentication.

    Unit tests still use Mock controller.

  • Gregg Kellogg

    Gregg Kellogg January 15th, 2009 @ 08:17 PM

    Fixed bug in http_digest_authentication_response_detail.

  • Gregg Kellogg

    Gregg Kellogg January 27th, 2009 @ 02:44 AM

    Here’s an updated patch. I ended up making some slight modifications to the digest code. Tests are now pretty much along the lines of Basic authentication.

  • Pratik

    Pratik January 29th, 2009 @ 01:37 AM

    Hey Gregg,

    
        def authenticate
          authenticate_or_request_with_http_digest("Super Secret") do |username, password|
            username == "hello" && 'world'
          end
        end
    

    The "hello && world" part is misleading. If the block is only concerned about the username, why is it passed the password too ?

  • Gregg Kellogg

    Gregg Kellogg January 29th, 2009 @ 02:16 AM

    Yes, this is a vestage from the basic use case. authenticate_or_request_with_http_digest and authenticate_with_http_digest only pass the username parameter. You can remove the password parameter from these tests.

    
    def authenticate
      authenticate_or_request_with_http_digest("Super Secret") do |username|
        username == "hello" && 'world'
      end
    end
    
  • Pratik

    Pratik January 29th, 2009 @ 02:42 AM

    Thanks. I'll make that modification.

  • Repository

    Repository January 29th, 2009 @ 04:02 PM

    • State changed from “open” to “resolved”

    (from [306cc2b920203cfa51cee82d2fc452484efc72f8]) Implement HTTP Digest authentication. [#1230 state:resolved] [Gregg Kellogg, Pratik Naik]

    Signed-off-by: Pratik Naik pratiknaik@gmail.com http://github.com/rails/rails/co...

  • Don Parish

    Don Parish February 2nd, 2009 @ 09:24 PM

    Found a problem while trying to use http digest authentication at http://ryandaigle.com/articles/2.... The digest authentication failed using both IE and FireFox. The current implementation is using the actual URI from the request instead of the URI passed in the authentication header, which is stored in the credentials hash. It seems as if the client is responsible for the URI passed in. It could be an absolute URI, as suggested in http://tools.ietf.org/html/rfc2617, but every example I've seen, only the relative path is used.

    With the attached change, the tests pass, and the digest authentication work with my test controller using IE and FireFox on Windows.

  • Pratik

    Pratik February 2nd, 2009 @ 11:14 PM

    Hey Donald,

    A test would be very helpful to ensure the same issue doesn't happen again.

  • Don Parish

    Don Parish February 4th, 2009 @ 09:49 PM

    • Tag changed from actionpack, authentication, controller, digest, http_authentication to actionpack, authentication, bug, controller, digest, http_authentication, patch

    Pratik,

    Patch with test attached that will fail until fix is applied. Then, all tests pass. I had to modify the encode_credentials method so I could override the absolute URI it normally uses with a a relative path.

  • Pratik

    Pratik February 4th, 2009 @ 09:53 PM

    • State changed from “resolved” to “open”
  • Pratik

    Pratik February 16th, 2009 @ 07:23 PM

    • State changed from “open” to “resolved”
  • Don Parish

    Don Parish February 17th, 2009 @ 05:51 PM

    Pratik,

    Thanks for committing my patch last night, but I'm afraid I made a mistake in that patch that is fixed by this one. RFC 2617, Section 3.2.2.5, says that the authenticating server must ensure the document designated by the uri parameter is the same as the document served. To do this, the uri in the credentials should be compared to the uri in the request line.
    
    

    This patch does that. The prior implementation before my first patch, passed in the full URL instead of the URI, that's why the tests passed, but it failed with most browsers which send a uri without the full URL.

    I've added two tests. One that makes sure the uris in the request and credentials match, whether using a url or relative uri. The other test checks to see that the authorization will fail if the header uri is changed.

    Sorry about my mistake (and on my first commit :( ). Just wanted to make sure you had a chance to fix it before Rails 2.3 is released.

    Don

    P.S. It is easy to modify this implementation to handle passwords stored as a hash: MD5(username:realm:password). Is it too late to make a patch for this?

    Being able to store a hashed version on the server instead of a plaintext password is one reason Digest is so complicated.

  • Pratik

    Pratik February 17th, 2009 @ 05:55 PM

    Don,

    Can you please open a new ticket and assign to me ?

    Thanks.

  • Don Parish

    Don Parish February 18th, 2009 @ 01:00 PM

    Pratik, Added ticket with inital patch. Later attached an updated patch to support use of storing a hash of user credentials instead of a plain text password, like htdigest in Apache. Let me know if that needs to be its own ticket. Thanks.

  • Jacob Burkhart

    Jacob Burkhart July 24th, 2009 @ 06:22 PM

    Any activity going on in fixing this? Just found out today that http auth is broken. What's the active ticket for fixing it? Need patches or +1's ?

    a little test that exposes the problem

    
        username_to_encode = "username_username_username"
        password_to_encode = "password_password_password"
        
        encoded_to = ActionController::HttpAuthentication::Basic.encode_credentials(username_to_encode, password_to_encode)    
        req_thing = OpenStruct.new(:env => {})
        req_thing.env['HTTP_AUTHORIZATION'] = encoded_to
        username_decoded, password_decoded = ActionController::HttpAuthentication::Basic.user_name_and_password(req_thing)
        
        message = "Expected #{username_to_encode}, #{password_to_encode} but got #{username_decoded} , #{password_decoded}"
        assert_equal(username_to_encode, username_decoded, message)
        assert_equal(password_to_encode, password_decoded, message)
    

    I see the fix was taken out in http://github.com/rails/rails/commit/c99ef814b0ce5d6b6a677ee6116eda... which references this ticket

    
           def decode_credentials(request)
    -        # Properly decode credentials spanning a new-line
    -        auth = authorization(request)
    -        auth.slice!('Basic ')
    -        ActiveSupport::Base64.decode64(auth || '')
    +        ActiveSupport::Base64.decode64(authorization(request).split.last || '')
           end
    

    How do we get this fix back in?

  • Jacob Burkhart

    Jacob Burkhart July 24th, 2009 @ 06:44 PM

    So it was fixed in a later commit... but it doesn't appear when I look at it in github... maybe there's just a bug in github?

    http://github.com/rails/rails/blob/master/actionpack/lib/action_con... shows the old code...

    Yet

    $ git branch -r --contains 306cc2b920203cfa51cee82d2fc452484efc72f8
      origin/2-3-stable
      origin/3-0-unstable
      origin/HEAD
      origin/master
    

    clearly shows the commit fixing it is in master... maybe there was a problem merging? How do I check which tags the commit made it into?

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

Tickets have moved to Github

The new ticket tracker is available at https://github.com/rails/rails/issues

Shared Ticket Bins

Referenced by

Pages