Skip to content
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

httpauth: http digest challenge request using RFC 7616 #919

Merged
merged 7 commits into from
Aug 30, 2023

Conversation

cHuberCoffee
Copy link
Contributor

No description provided.

@cHuberCoffee
Copy link
Contributor Author

It is the first of my PR regarding HTTP digest auth. The upcoming one will handle the response creation and the verification steps.

src/httpauth/digest.c Outdated Show resolved Hide resolved
@sreimers
Copy link
Member

sreimers commented Aug 28, 2023

Unsure why there is a segfault for mingw now (the stack trace looks broken after this):

13: test_httpauth_digest_request (\home\runner\work\re\re\baresip-win32\re\test\httpauth.c:446)

Nothing obvious to me, at first look.

@cHuberCoffee
Copy link
Contributor Author

Currently i'm setting up the mingw on my local machine to run the tests.

*
* @return 0 if success, otherwise errorcode
*/
int httpauth_digest_chall_request(struct httpauth_digest_chall_req **preq,
Copy link
Collaborator

@cspiel1 cspiel1 Aug 28, 2023

Choose a reason for hiding this comment

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

I suggest to call this function:
httpauth_digest_chall_request_full()
and the short form
httpauth_digest_chall_request().

Don't forget to update the doxygen!

@cHuberCoffee cHuberCoffee marked this pull request as draft August 30, 2023 07:32
@cHuberCoffee cHuberCoffee force-pushed the httpauth_digest_requests branch 2 times, most recently from 4aadc08 to a9856fc Compare August 30, 2023 09:50
{
struct mbuf *mb = NULL;
char *nonce = NULL;
uint8_t *hash = NULL;
Copy link
Member

Choose a reason for hiding this comment

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

Maybe simply use uint8_t hash[SHA256_DIGEST_LENGTH]; I see no need for heap usage here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The hashlen variable was the problematic piece. Using the hashlen variable in mbuf_printf(mb, "%w", hash, hashlen) produced a wrong behavior inside the printing function re_vhprintf. Changing the hashlen variable to size_t (required by %w) it works fine. Also the cast of time_t is now a fixed size integer type uint64_t with its corresponding PRIxx macro.

I can't tell why this phenomena is only observed if it was cross-compiled in the workflow. I cross-compiled it on my local machine and executed the binary on an windows machine without this behavior. In case i downloaded the binary from the workflow, i got the same result as the workflow itself.

Copy link
Member

@sreimers sreimers Aug 30, 2023

Choose a reason for hiding this comment

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

Nice, but there is no need for hash heap usage, you can use a stack hash array variable for better performance, since its only 32 bytes (no need for mem_zalloc). So you can use simply sizeof(hash) instead of hashlen.

@cHuberCoffee cHuberCoffee marked this pull request as ready for review August 30, 2023 12:55
@sreimers sreimers changed the title Httpauth digest requests httpauth: http digest challenge request using RFC 7616 Aug 30, 2023
@sreimers sreimers merged commit 72b532f into baresip:main Aug 30, 2023
29 checks passed
@cHuberCoffee cHuberCoffee deleted the httpauth_digest_requests branch August 30, 2023 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants