Skip to content
New issue

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

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

Already on GitHub? # to your account

Fix Authorization header generation #498

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ERobsham
Copy link
Contributor

@ERobsham ERobsham commented Mar 21, 2025

Fixes Authorization header generation to include the query parameters (if present).
This brings the implementation inline with MacOS, and fixes digest auth with certain picky services.

Tested these changes against the IC Realtime ORB camera I was having issues with in #497, and our application successfully authenticated with this fix.

Verified

This commit was signed with the committer’s verified signature. The key has expired.
pablojimpas Pablo Jiménez Pascual
Fixes `Authorization` header generation to include the query parameters (if present).
This brings the implementation inline with MacOS, and fixes digest auth with certain picky services.
@ERobsham ERobsham requested a review from rfm as a code owner March 21, 2025 21:28
@ERobsham
Copy link
Contributor Author

It looks like the one failing check is caused by an unrelated bug in libdispatch

Copy link
Contributor

@rfm rfm left a comment

Choose a reason for hiding this comment

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

The original implementation here was based on RFC2617, which allowed (in section 3.2.1) the URI field to be either absoluteURI or abs_path (and abs_path was chosen, probably because it's simpler), where abs_path does not include the query string.

However, I agree with your diagnosis that the latest/current RFC7616 (section 3.4) has changed this, and abs_path is no longer allowed because that choice of two possibilities is replaced by 'Effective Request URI', and the old abs_path is not valid as an effective request uri.

It seems to me that your patch is a partial but incomplete fix, because my reading of the RFCs is that the new value should be a complete absolute URL, not just path plus query string. As such I think GSHTTPAuthentication should probably be changed to use the scheme, host and port information from the protection space to construct a full URL. Does that work for your application?
I guess there is also a small chance that the format demanded by the new RFC may not work with some old servers, but we can probably ignore that (or add some option to control the format if it does turn out to be a problem).

@rfm
Copy link
Contributor

rfm commented Mar 24, 2025

That being said, I just looked at the curl source code (I think Apple use libcurl).
That doesn't seem to use a full URL (unless higher level calling code is constructing it and passing it in the path argument to the authorisation header generation), however it does have an option to control whether query string is used or not (it truncates the path at the question mark if it is talking to a certain class of servers).

@ERobsham
Copy link
Contributor Author

The RFCs are a bit dense, so I definitely was skimming a bit (and probably biased myself by looking at the raw packet captures from the MacOS implementation), but I was interpreting 'Effective Request URI' as:
origin-form = absolute-path [ "?" query ] where absolute-path = 1*( "/" segment )

So I think we're good without any of the scheme / host / port etc included in that URI.


And I hadn't checked into the libcurl code before, but I see what you mean with that 'if IE style' block.

If thats something we'd want to account for here, do you have any suggestions on how we'd want that exposed in the API.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants