Implement HTTP Digest authentication

Wow, I totally forgot I wrote this

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

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

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

New patch uploaded.

(from [c99ef814b0ce5d6b6a677ee6116edac03c8a35b3]) Revert "HTTP Digest authentication [#1230 Implement HTTP Digest authentication 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...

• 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

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

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!

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.

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.

Fixed bug in http_digest_authentication_response_detail.

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.

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 ?

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

Thanks. I'll make that modification.

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.

Hey Donald,

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

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.

Don,

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

Thanks.

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.

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?

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?