Skip to content

Fix bug #55639: Digest autentication dont work #14328

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

Closed
wants to merge 3 commits into from

Conversation

nielsdos
Copy link
Member

RFC 2617 and 7616 describe that for the "Authorization" header we should not put the qop nor nc value inside quotes. This differs from the WWW-Authenticate header, which may have been the source of the confusion in the implementation. While the version with quotes seems to work fine in some cases, clearly not all servers accept the non-standard form. To fix the issue, simply removing the quotes of those two header fields of the client request to be in line with the RFC suffices.

I refer further to example 3.5 in RFC 2617 and example 3.9.1 in RFC 7616.

RFC 2617: https://datatracker.ietf.org/doc/html/rfc2617
RFC 7616: https://datatracker.ietf.org/doc/html/rfc7616

RFC 2617 and 7616 describe that for the "Authorization" header we should
not put the qop nor nc value inside quotes. This differs from the
WWW-Authenticate header, which may have been the source of the confusion
in the implementation. While the version with quotes seems to work fine
in some cases, clearly not all servers accept the non-standard form.
To fix the issue, simply removing the quotes of those two header fields
of the client request to be in line with the RFC suffices.

I refer further to example 3.5 in RFC 2617 and example 3.9.1 in
RFC 7616.

RFC 2617: https://datatracker.ietf.org/doc/html/rfc2617
RFC 7616: https://datatracker.ietf.org/doc/html/rfc7616
@devnexen
Copy link
Member

I think what you ve done makes sense but may I suggest you wait before merging ? cc @kocsismate

@nielsdos
Copy link
Member Author

nielsdos commented Jul 5, 2024

Ping! Anyone who is able to review this? ;) Thanks

Copy link
Member

@SakiTakamachi SakiTakamachi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have carefully read the RFC and I think you are definitely correct.

@staabm
Copy link
Contributor

staabm commented Jul 17, 2024

is this something which can be unit tested?

@nielsdos
Copy link
Member Author

is this something which can be unit tested?

A bit difficult but I added a test, let's see if CI likes it.

@nielsdos nielsdos closed this in 911dc5b Jul 17, 2024
@andypost
Copy link
Contributor

andypost commented Aug 13, 2024

Building 8.2.23RC1 on Alpinelinux the test failed, pipeline

TEST 10997/16553 [ext/soap/tests/bugs/bug55639.phpt]
========DIFF========
     Unauthorized
002+ string(423) "POST / HTTP/1.1
002- string(424) "POST / HTTP/1.1
     Host: %s
     Connection: Keep-Alive
     User-Agent: %s
--
========DONE========

@andypost
Copy link
Contributor

andypost commented Aug 13, 2024

Fixed in 8.3+ 1b52ecd

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

Successfully merging this pull request may close these issues.

5 participants