Fix for digest authentication bug - issue #2301 #2323

Closed
wants to merge 5 commits into
from

Conversation

Projects
None yet
5 participants
@arthurpsmith
Contributor

arthurpsmith commented Jul 28, 2011

I updated the test to successfully authenticate, instead of failing, when the credentials URI differs from the request path seen by the server. Without this change, digest authentication cannot go through a proxy that rewrites the request, and this is specifically what the Digest Auth specification, RFC 2617 - http://tools.ietf.org/html/rfc2617 requires.

@arunagw

This comment has been minimized.

Show comment Hide comment
@arunagw

arunagw Dec 22, 2011

Member

Still a issue ??

Member

arunagw commented Dec 22, 2011

Still a issue ??

@arthurpsmith

This comment has been minimized.

Show comment Hide comment
@arthurpsmith

arthurpsmith Dec 22, 2011

Contributor

arunagw - yes, this is still broken in 3.1.3. We're using this patch for several services that rely on digest authentication behind a proxy and it works according to the Digest Auth spec; without the patch any proxied service fails authentication.

Contributor

arthurpsmith commented Dec 22, 2011

arunagw - yes, this is still broken in 3.1.3. We're using this patch for several services that rely on digest authentication behind a proxy and it works according to the Digest Auth spec; without the patch any proxied service fails authentication.

@arunagw

This comment has been minimized.

Show comment Hide comment
@arunagw

arunagw Feb 24, 2012

Member

Can you please check again. I think some file is changed there.

Member

arunagw commented Feb 24, 2012

Can you please check again. I think some file is changed there.

@arthurpsmith

This comment has been minimized.

Show comment Hide comment
@arthurpsmith

arthurpsmith Apr 28, 2012

Contributor

Yes, the code has changed but this problem has not been addressed even in 3.2.3 - the URI being used for the authentication check is not the one provided in the credentials but the one the server knows, which could be quite different. Do I need to create a new pull request for the latest actionpack given the changes in the code since when I posted this many months ago?

Contributor

arthurpsmith commented Apr 28, 2012

Yes, the code has changed but this problem has not been addressed even in 3.2.3 - the URI being used for the authentication check is not the one provided in the credentials but the one the server knows, which could be quite different. Do I need to create a new pull request for the latest actionpack given the changes in the code since when I posted this many months ago?

@rafaelfranca

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca May 7, 2012

Owner

@arthurpsmith you can update this PR pushing to the same branch. Please rebase you commit against the actual master branch and force push to the master branch in your repository.

Owner

rafaelfranca commented May 7, 2012

@arthurpsmith you can update this PR pushing to the same branch. Please rebase you commit against the actual master branch and force push to the master branch in your repository.

@steveklabnik

This comment has been minimized.

Show comment Hide comment
@steveklabnik

steveklabnik May 14, 2012

Member

I'm just gonna mention #2301 here so that GitHub links the two. ;)

Member

steveklabnik commented May 14, 2012

I'm just gonna mention #2301 here so that GitHub links the two. ;)

@schneems

This comment has been minimized.

Show comment Hide comment
@schneems

schneems Aug 2, 2012

Member

@arthurpsmith it looks like you accidentally grabbed two of @rafaelfranca's commits in this PR I believe you can use rebase interactive git rebase -i HEAD~4 to remove those commits. Besides that what is the current status of this PR? Are we waiting on anything else?

Member

schneems commented Aug 2, 2012

@arthurpsmith it looks like you accidentally grabbed two of @rafaelfranca's commits in this PR I believe you can use rebase interactive git rebase -i HEAD~4 to remove those commits. Besides that what is the current status of this PR? Are we waiting on anything else?

@arthurpsmith

This comment has been minimized.

Show comment Hide comment
@arthurpsmith

arthurpsmith Aug 2, 2012

Contributor

schneems - sorry, I didn't request franca's, I have no idea what happened there.

The only thing this PR is waiting on is that since so much time has expired it needs to be updated to apply to the latest rails and I have not been able to do that (I spent an hour on an attempt around that time, but failed to get to the point where I could run the specs, and gave up). Last I looked (3 months ago) the digest authentication problem was still there in the most recent rails.

I think it would be best for somebody else to look at the code and understand the problem - anybody who is running a service requiring digest authentication where there is any URL rewriting going on (load balancing, proxying) will run into this as it rails is using an incorrect implementation of the RFC.

Contributor

arthurpsmith commented Aug 2, 2012

schneems - sorry, I didn't request franca's, I have no idea what happened there.

The only thing this PR is waiting on is that since so much time has expired it needs to be updated to apply to the latest rails and I have not been able to do that (I spent an hour on an attempt around that time, but failed to get to the point where I could run the specs, and gave up). Last I looked (3 months ago) the digest authentication problem was still there in the most recent rails.

I think it would be best for somebody else to look at the code and understand the problem - anybody who is running a service requiring digest authentication where there is any URL rewriting going on (load balancing, proxying) will run into this as it rails is using an incorrect implementation of the RFC.

@schneems

This comment has been minimized.

Show comment Hide comment
@schneems

schneems Aug 2, 2012

Member

@arthurpsmith, pulling in someone else's changes is common, it's happened on a few of my PR, you just have to fix when it does.

The chances of someone else swooping in with the same experience and desire to fix this isn't very likely considering this has been an open issue for over a year. You probably understand this specific problem better than any one else in the planet right now. If you can fix the problem to match the RFC spec, it would help others. If you don't care enough to work on the issue, there is no reason for keeping this PR open.

Member

schneems commented Aug 2, 2012

@arthurpsmith, pulling in someone else's changes is common, it's happened on a few of my PR, you just have to fix when it does.

The chances of someone else swooping in with the same experience and desire to fix this isn't very likely considering this has been an open issue for over a year. You probably understand this specific problem better than any one else in the planet right now. If you can fix the problem to match the RFC spec, it would help others. If you don't care enough to work on the issue, there is no reason for keeping this PR open.

@arthurpsmith

This comment has been minimized.

Show comment Hide comment
@arthurpsmith

arthurpsmith Aug 2, 2012

Contributor

Honestly - the issue is not a deep one, it ought to take somebody else a lot less time to understand than the time I've spent trying to understand how to submit a patch.

I certainly care - however, my git and rails build/spec expertise is close to zero. We're moving some of our apps to rails 3.2 shortly, so I'll have a chance to look at this again at that point, as we'll need to patch it again then.

However, I see no point continuing from this request. I will close it as you suggest. The underlying problem in issue #2301 is still there though.

Contributor

arthurpsmith commented Aug 2, 2012

Honestly - the issue is not a deep one, it ought to take somebody else a lot less time to understand than the time I've spent trying to understand how to submit a patch.

I certainly care - however, my git and rails build/spec expertise is close to zero. We're moving some of our apps to rails 3.2 shortly, so I'll have a chance to look at this again at that point, as we'll need to patch it again then.

However, I see no point continuing from this request. I will close it as you suggest. The underlying problem in issue #2301 is still there though.

@steveklabnik

This comment has been minimized.

Show comment Hide comment
@steveklabnik

steveklabnik Aug 2, 2012

Member

This should not be hard. I'm gonna give it a shot right now.

Member

steveklabnik commented Aug 2, 2012

This should not be hard. I'm gonna give it a shot right now.

@schneems

This comment has been minimized.

Show comment Hide comment
@schneems

schneems Aug 2, 2012

Member

thanks @steveklabnik

Member

schneems commented Aug 2, 2012

thanks @steveklabnik

@steveklabnik

This comment has been minimized.

Show comment Hide comment
@steveklabnik

steveklabnik Aug 2, 2012

Member

Done. @arthurpsmith what do you think?

Member

steveklabnik commented Aug 2, 2012

Done. @arthurpsmith what do you think?

@arthurpsmith

This comment has been minimized.

Show comment Hide comment
@arthurpsmith

arthurpsmith Aug 2, 2012

Contributor

@steveklabnik - thanks!!! That looks almost exactly where I had it with actionpack-3.2 a few months back; the change in the test should do the right thing, the code change looks right, and if the tests pass, it sounds good to me.

Contributor

arthurpsmith commented Aug 2, 2012

@steveklabnik - thanks!!! That looks almost exactly where I had it with actionpack-3.2 a few months back; the change in the test should do the right thing, the code change looks right, and if the tests pass, it sounds good to me.

@steveklabnik

This comment has been minimized.

Show comment Hide comment
@steveklabnik

steveklabnik Aug 2, 2012

Member

Awesome.

Since you only have one commit, what I did was this:

$ git checkout master
$ git reset --hard upstream/master
$ git fetch https://github.com/arthurpsmith/rails master
$ git cherry-pick 1a6f25c
$ git status
$ mvim actionpack/lib/action_controller/metal/http_authentication.rb # fix conflict
$ mvim actionpack/test/controller/http_digest_authentication_test.rb # fix conflict
$ git commit
$ git checkout -b fix_2301
$ git push origin fix_2301

Took five minutes. :)

Member

steveklabnik commented Aug 2, 2012

Awesome.

Since you only have one commit, what I did was this:

$ git checkout master
$ git reset --hard upstream/master
$ git fetch https://github.com/arthurpsmith/rails master
$ git cherry-pick 1a6f25c
$ git status
$ mvim actionpack/lib/action_controller/metal/http_authentication.rb # fix conflict
$ mvim actionpack/test/controller/http_digest_authentication_test.rb # fix conflict
$ git commit
$ git checkout -b fix_2301
$ git push origin fix_2301

Took five minutes. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment