Httpauth digest response #944

cHuberCoffee · 2023-09-06T06:28:27Z

No description provided.

alfredh · 2023-09-09T07:44:57Z

src/httpauth/digest.c

@@ -16,21 +16,37 @@
 #include <re_httpauth.h>


+enum {
+	CNONCE_NC_LENGTH = 9, /* 8 characters + '\0' */


perhaps SIZE is better ?

if you know the length, why do you need a trailing null terminator ?

Renaming to CNONCE_NC_SIZE is a good idea.
The enum is meant for the string size when the uint32_t is hex encoded. I added the NULL terminator, so i don't need to add 1 whenever i handle the array/string.

alfredh · 2023-09-09T07:47:17Z

src/httpauth/digest.c

+	/* HASH A2 */
+	if (str_isset(resp->qop) && str_str(resp->qop, "auth-int")) {
+		if (!entitybody || str_casecmp(entitybody, "") == 0) {
+			resp->hash_function((uint8_t *)"", str_len(""), hash1);


perhaps check if hash_function is set, otherwise it might crash.

perhaps also use the "handler" paradigm with a trailing h.

i will change to the "handler" paradigm.
The check does not hurt so i will add it. Since digest_response is an static function, it gets called only after an successful set of the hash_function. That is the reason i didn't include the check.

alfredh · 2023-09-09T07:47:52Z

src/httpauth/digest.c

+
+	/* HASH A2 */
+	if (str_isset(resp->qop) && str_str(resp->qop, "auth-int")) {
+		if (!entitybody || str_casecmp(entitybody, "") == 0) {


str_len("") is always 0.

alfredh · 2023-09-09T07:50:32Z

src/httpauth/digest.c

+int httpauth_digest_response_set_cnonce(struct httpauth_digest_enc_resp *resp,
+	const struct httpauth_digest_chall *chall, const struct pl *method,
+	const char *user,	const char *passwd, const char *entitybody,
+	const uint32_t cnonce, const uint32_t nc_)


remove trailing underscore from "nc_"

const is not needed for params passed by value

I didn't want to write out the nonce_count and nc is already an static internal variable. Therefore the underscore.
I will write it out and remove the const.

alfredh · 2023-09-09T07:52:12Z

src/httpauth/digest.c

+		pl_strstr(&chall->algorithm, "MD5")) {
+		resp->hash_function = &md5;
+		resp->hash_length = MD5_SIZE;
+		err = str_dup(&resp->algorithm, "MD5");


We should consider how to deal with weak crypto such as MD5 or SHA1.

SHA1 is not in the specification anyway, i can remove it as supported algorithm. MD5 on the other hand is the default fallback if no algorithm is specified. Also the MD5 is needed for backwards compatibility.

In the RFC there are 3 algorithms, MD5, SHA256 and SHA512-256. The SHA512-256 is a special case of SHA512 which is not implemented in re for now. I checked the microsoft and apple documentation and non of these two have an interface for SHA512-256. OpenSSL has the capability to implement it, but it is not a single function call like the others for now.

for now it should be enough with MD5 and SHA256.

I am wondering if we should add a global macro ENABLE_WEAK_CIPHERS that we can put around
the MD5 and SHA1 code... ?

Disabling the MD5 via an macro is quite risky. It might break a lot of digest authentication implementations.
For SHA1 on the other hand it would be a good idea since the SHA2 family should be used instead.

I will remove the SHA1 algorithm for now.

alfredh · 2023-09-17T10:15:52Z

src/httpauth/digest.c

+		goto out;
+	}
+
+	n = re_snprintf(resp->nc, CNONCE_NC_SIZE, "%08x", nonce_counter);


Can cnonce and nonce_counter be stored as is on the struct ?

And the printing to hex can be done when needed ?

alfredh · 2023-09-17T10:16:21Z

src/httpauth/digest.c

+
+	n = re_snprintf(resp->nc, CNONCE_NC_SIZE, "%08x", nonce_counter);
+	if (n == -1 || n != CNONCE_NC_SIZE -1) {
+		err = ERANGE;


this error code can be returned directly

alfredh · 2023-09-17T10:17:46Z

include/re_httpauth.h

+	char *username_star;
+	char *uri;
+	char *cnonce;
+	char *nc;


Unrelated, this can also be fixed size: char nc[8+1]

alfredh · 2023-09-17T10:18:30Z

test/httpauth.c

+
+	int err;
+
+	for (unsigned int i = 0; i < RE_ARRAY_SIZE(testv); i++) {


better to use size_t for i here ...

alfredh · 2023-09-17T10:19:58Z

Could you also please add a link to the relevant RFC ?

cHuberCoffee · 2023-09-19T06:29:58Z

These are the two RFCs, the last, this, and the upcoming PRs are based on.
HTTP Digest Access Authentication: https://www.rfc-editor.org/rfc/rfc7616
The Session Initiation Protocol (SIP) Digest Access Authentication Scheme: https://www.rfc-editor.org/rfc/rfc8760

alfredh · 2023-09-29T11:24:54Z

src/httpauth/digest.c

@@ -16,21 +16,37 @@
 #include <re_httpauth.h>


+enum {
+	CNONCE_NC_SIZE = 9, /* 8 characters + '\0' */
+};


is this used ?

you are right, its not used anymore. it was to store the cnonce and nc as strings
it is removed now

alfredh · 2023-09-29T11:26:47Z

These are the two RFCs, the last, this, and the upcoming PRs are based on. HTTP Digest Access Authentication: https://www.rfc-editor.org/rfc/rfc7616 The Session Initiation Protocol (SIP) Digest Access Authentication Scheme: https://www.rfc-editor.org/rfc/rfc8760

could you also add RFC7617 to the README ?

I had one more comment, apart from that it looks ready to merge.

alfredh · 2023-10-11T07:44:00Z

The next release (3.6.0) is scheduled for Wed 18. October.
Should we merge this before or after the release ?

cHuberCoffee · 2023-10-14T09:22:50Z

Sry for the late response. I can make the changes only after the 18th. So i would suggest to wait for the release and my last fixes. Then its ready to go for 3.7

alfredh · 2023-10-14T10:09:05Z

Okay, it is scheduled for after 3.6.0 release

If you dont have time to work on fixing review comments, please consider to close the PR
and reopen it when you have time...

+ definition of response struct using char* to save the parameters + added decoding for new fields in http digest challenge struct + response calculation supports qop: none, auth, auth-int + supported hash algorithm: MD5, SHA1, SHA265 and -sess variants

…tion

cHuberCoffee · 2023-10-19T11:32:17Z

I'm not sure what happens in the FreeBSD test. But i looks like the test is hanging in an bootloop. I checked it and it gets the same output over and over again. Its running for over 3h right now. There is no other output than the repeating boot messages.

sreimers · 2023-10-19T12:36:16Z

I've just disabled the FreeBSD workflow for now (until its more stable):

vmactions/freebsd-vm#74

sreimers added this to the v3.6.0 milestone Sep 6, 2023

alfredh reviewed Sep 9, 2023

View reviewed changes

alfredh reviewed Sep 17, 2023

View reviewed changes

alfredh reviewed Sep 29, 2023

View reviewed changes

alfredh removed this from the v3.6.0 milestone Sep 29, 2023

alfredh added this to the v3.7.0 milestone Oct 11, 2023

cHuberCoffee added 8 commits October 19, 2023 09:41

httpauth: test cases for http digest response calculation and printing

074b0b6

httpauth: fix mentioned review points

45b8350

httpauth: remove SHA1 support

d72cb04

httpauth: save cnonce and nc as uint32_t in struct.

11ee127

httpauth: change loop counter to size_t

bfa1812

httpauth: remove unused enum

09cfe5c

README: update RFC list, add RFC 7616 - HTTP Digest Access Authentica…

c0bcd40

…tion

cHuberCoffee force-pushed the httpauth_digest_response branch from e6a2361 to c0bcd40 Compare October 19, 2023 07:44

alfredh merged commit e922af5 into baresip:main Oct 20, 2023
34 of 35 checks passed

cHuberCoffee deleted the httpauth_digest_response branch December 11, 2023 12:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Httpauth digest response #944

Httpauth digest response #944

cHuberCoffee commented Sep 6, 2023

alfredh Sep 9, 2023

cHuberCoffee Sep 12, 2023

alfredh Sep 9, 2023

cHuberCoffee Sep 12, 2023

alfredh Sep 9, 2023

alfredh Sep 9, 2023

cHuberCoffee Sep 12, 2023

alfredh Sep 9, 2023

cHuberCoffee Sep 12, 2023

alfredh Sep 12, 2023

cHuberCoffee Sep 13, 2023

alfredh Sep 17, 2023

alfredh Sep 17, 2023

alfredh Sep 17, 2023

alfredh Sep 17, 2023

alfredh commented Sep 17, 2023

cHuberCoffee commented Sep 19, 2023

alfredh Sep 29, 2023

cHuberCoffee Oct 19, 2023

alfredh commented Sep 29, 2023

alfredh commented Oct 11, 2023

cHuberCoffee commented Oct 14, 2023

alfredh commented Oct 14, 2023

cHuberCoffee commented Oct 19, 2023

sreimers commented Oct 19, 2023


		int err;

		for (unsigned int i = 0; i < RE_ARRAY_SIZE(testv); i++) {

Httpauth digest response #944

Httpauth digest response #944

Conversation

cHuberCoffee commented Sep 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alfredh commented Sep 17, 2023

cHuberCoffee commented Sep 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alfredh commented Sep 29, 2023

alfredh commented Oct 11, 2023

cHuberCoffee commented Oct 14, 2023

alfredh commented Oct 14, 2023

cHuberCoffee commented Oct 19, 2023

sreimers commented Oct 19, 2023