New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Calculate digest of body only if auth-int specified. #254

Open
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@colinnewell
Contributor

colinnewell commented Apr 20, 2017

This applies the roughly the patch from issue #130 and adds tests to
check the changes work correctly.

This isn't quite complete yet. I still want to do the following,

  • Test against a live web server manually to verify the original issue and that servers definitely work with this behaviour.
  • Consider adding a mechanism to allow for the legacy mode of operation in case there are also legitimate servers in the wild that do work in the way LWP was operating.

colinnewell added some commits Apr 19, 2017

Calculate digest of body only if auth-int specified.
This applies the roughly the patch from issue #130 and adds tests to
check the changes work correctly.
@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Apr 20, 2017

Coverage Status

Coverage decreased (-0.5%) to 67.989% when pulling f93ed2e on cv-library:authentication_digest into 782e400 on libwww-perl:master.

coveralls commented Apr 20, 2017

Coverage Status

Coverage decreased (-0.5%) to 67.989% when pulling f93ed2e on cv-library:authentication_digest into 782e400 on libwww-perl:master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Apr 20, 2017

Coverage Status

Coverage decreased (-0.5%) to 67.989% when pulling f93ed2e on cv-library:authentication_digest into 782e400 on libwww-perl:master.

coveralls commented Apr 20, 2017

Coverage Status

Coverage decreased (-0.5%) to 67.989% when pulling f93ed2e on cv-library:authentication_digest into 782e400 on libwww-perl:master.

Fixed problem where digest wasn't getting calculated at all.
That was a bit embarrasing.  The fix was actually a break.  This should
hopefully now do what it was supposed to (and check it for some value of
check).
@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Apr 20, 2017

Coverage Status

Coverage increased (+0.02%) to 68.484% when pulling e9e794e on cv-library:authentication_digest into 782e400 on libwww-perl:master.

coveralls commented Apr 20, 2017

Coverage Status

Coverage increased (+0.02%) to 68.484% when pulling e9e794e on cv-library:authentication_digest into 782e400 on libwww-perl:master.

@colinnewell

This comment has been minimized.

Show comment
Hide comment
@colinnewell

colinnewell Apr 21, 2017

Contributor

I'm actually struggling to find a web server that supports auth-int. I'm assuming they are out there because people complain about issues with authenticating with them, but the big 3 don't appear to support it. Apache, nginx and IIS don't appear to support that option.

Contributor

colinnewell commented Apr 21, 2017

I'm actually struggling to find a web server that supports auth-int. I'm assuming they are out there because people complain about issues with authenticating with them, but the big 3 don't appear to support it. Apache, nginx and IIS don't appear to support that option.

@genio

This comment has been minimized.

Show comment
Hide comment
@genio

genio Apr 21, 2017

Member

Auth-int with apache requires http://httpd.apache.org/docs/2.2/mod/mod_auth_digest.xml I'm not aware of a public and available test server for digest auth though.

Member

genio commented Apr 21, 2017

Auth-int with apache requires http://httpd.apache.org/docs/2.2/mod/mod_auth_digest.xml I'm not aware of a public and available test server for digest auth though.

@colinnewell

This comment has been minimized.

Show comment
Hide comment
@colinnewell

colinnewell Apr 21, 2017

Contributor

Note the footnote on that though. I got an apache server going and was talking to it before I came across that in the fine detail.

auth-int is not implemented yet.

Contributor

colinnewell commented Apr 21, 2017

Note the footnote on that though. I got an apache server going and was talking to it before I came across that in the fine detail.

auth-int is not implemented yet.

@genio

This comment has been minimized.

Show comment
Hide comment
@genio

genio Apr 21, 2017

Member

Ah. You're right. I didn't even notice that.

Member

genio commented Apr 21, 2017

Ah. You're right. I didn't even notice that.

@genio

This comment has been minimized.

Show comment
Hide comment
Member

genio commented Apr 21, 2017

@colinnewell

This comment has been minimized.

Show comment
Hide comment
@colinnewell

colinnewell Apr 21, 2017

Contributor

Again, doesn't look like that has support. I've only looked at the commit related to the bug fix, but in that it blocks it off with a 'not supported' message.

Just use https if you want to protect the connection; qop=auth-int won't be supported.

Contributor

colinnewell commented Apr 21, 2017

Again, doesn't look like that has support. I've only looked at the commit related to the bug fix, but in that it blocks it off with a 'not supported' message.

Just use https if you want to protect the connection; qop=auth-int won't be supported.

@colinnewell

This comment has been minimized.

Show comment
Hide comment
@colinnewell
Contributor

colinnewell commented Apr 21, 2017

@genio

This comment has been minimized.

Show comment
Hide comment
@genio

genio Apr 21, 2017

Member

I'm starting to wonder if we should drop support for it since it seems to be a non-real-world feature.

Member

genio commented Apr 21, 2017

I'm starting to wonder if we should drop support for it since it seems to be a non-real-world feature.

@colinnewell

This comment has been minimized.

Show comment
Hide comment
@colinnewell

colinnewell Apr 21, 2017

Contributor

I'm half with you on that, but I'll give it a bit more of a go before abandoning this as a bad job. I could see complaints with other client libraries reporting the same problem so this isn't a completely isolated report, it's just not a very common option.

Contributor

colinnewell commented Apr 21, 2017

I'm half with you on that, but I'll give it a bit more of a go before abandoning this as a bad job. I could see complaints with other client libraries reporting the same problem so this isn't a completely isolated report, it's just not a very common option.

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