Re: [FFmpeg-devel] [PATCH] avformat/httpauth.c [support both RFC 2617 and RFC 7617]
Hello, thank you for taking the time to review my code. I've made a new commit incorporating the code style improvements you suggested. This includes adding spaces before pointers, removing unnecessary comments, performing case-insensitive comparisons, applying consistent brace styles, and adding spaces when concatenating strings. Regarding the use of strcmp, I gave it some thought, but decided to stick with av_stristr for now to avoid the complexity of handling all four cases: md5, md5-sess, MD5, and MD5-sess. However, I acknowledge the benefits of strict string comparison with strcmp and will keep exploring this approach for future enhancements. Thank you very much for your thorough review and valuable feedback. -Original Message- From: ffmpeg-devel On Behalf Of Stefano Sabatini Sent: Saturday, April 13, 2024 6:38 PM To: FFmpeg development discussions and patches Subject: Re: [FFmpeg-devel] [PATCH] avformat/httpauth.c [support both RFC 2617 and RFC 7617] On date Thursday 2024-04-11 07:48:14 +,| Eugene wrote: > - Updated the make_digest_auth() function to support both RFC 2617 and RFC > 7617 digest authentication. > - Supports sha256 , sha512-256 along with the existing md5. MD5 and sha256 > were tested, but sha512-256 was not tested due to lack of testable server. > - AVHashContext instead of AVMD5 structure. > - update_md5_strings() -> update_hash_strings(). > - There are some lynt issues in the old code of make_digest_auth, but this is > a feature update patch, so I didn't fix it. > - modified the implementation of RFC7616 based on community feedback. > > Signed-off-by: Eugene-bitsensing > --- > libavformat/httpauth.c | 105 > ++--- > 1 file changed, 57 insertions(+), 48 deletions(-) > > diff --git a/libavformat/httpauth.c b/libavformat/httpauth.c index > 9780928357..ba2ebea3a4 100644 > --- a/libavformat/httpauth.c > +++ b/libavformat/httpauth.c > @@ -24,7 +24,7 @@ > #include "libavutil/avstring.h" > #include "internal.h" > #include "libavutil/random_seed.h" > -#include "libavutil/md5.h" > +#include "libavutil/hash.h" > #include "urldecode.h" > > static void handle_basic_params(HTTPAuthState *state, const char > *key, @@ -117,35 +117,35 @@ void ff_http_auth_handle_header(HTTPAuthState > *state, const char *key, > } > } > > - > -static void update_md5_strings(struct AVMD5 *md5ctx, ...) > +/* Generate hash string, updated to use AVHashContext to support > +other algorithms */ static void update_hash_strings(struct > +AVHashContext* hash_ctx, ...) > { > va_list vl; > > -va_start(vl, md5ctx); > +va_start(vl, hash_ctx); > while (1) { > const char* str = va_arg(vl, const char*); > if (!str) > break; > -av_md5_update(md5ctx, str, strlen(str)); > +av_hash_update(hash_ctx, (const uint8_t*)str, strlen(str)); nit++: (const uint8_t *)str note the space before the "*" > } > va_end(vl); > } > > -/* Generate a digest reply, according to RFC 2617. */ > +/* Generate a digest reply, according to RFC 2617. Update to support > +RFC 7617*/ nit: according to RFC 2617/7617 > static char *make_digest_auth(HTTPAuthState *state, const char *username, >const char *password, const char *uri, >const char *method) { > DigestParams *digest = &state->digest_params; > -int len; > +size_t len;// change int -> size_t drop this comment > uint32_t cnonce_buf[2]; > char cnonce[17]; > char nc[9]; > int i; > -char A1hash[33], A2hash[33], response[33]; > -struct AVMD5 *md5ctx; > -uint8_t hash[16]; > +char a1_hash[65], a2_hash[65], response[65]; // increase hash > + size for SHA-2 same here, drop the comment or this will be confusing without reference to the diff > +struct AVHashContext* hash_ctx = NULL; // use AVHashContext for > + other algorithm support nit: here and below, use: TYPE *VAR rather than: TYPE* VAR style for consistency > +size_t len_hash = 33; // Modifiable hash length, MD5:32, SHA-2:64 > char *authstr; > > digest->nc++; > @@ -156,52 +156,61 @@ static char *make_digest_auth(HTTPAuthState *state, > const char *username, > cnonce_buf[i] = av_get_random_seed(); > ff_data_to_hex(cnonce, (const uint8_t*) cnonce_buf, > sizeof(cnonce_buf), 1); > > -md5ctx = av_md5_alloc(); > -if (!md5ctx) > -return NULL; > - > -av_md5_init(md5ctx); > -update_md5_strings(md5ctx, username, ":", state->
Re: [FFmpeg-devel] [PATCH] avformat/httpauth.c [support both RFC 2617 and RFC 7617]
On date Thursday 2024-04-11 07:48:14 +, �� | Eugene wrote: > - Updated the make_digest_auth() function to support both RFC 2617 and RFC > 7617 digest authentication. > - Supports sha256 , sha512-256 along with the existing md5. MD5 and sha256 > were tested, but sha512-256 was not tested due to lack of testable server. > - AVHashContext instead of AVMD5 structure. > - update_md5_strings() -> update_hash_strings(). > - There are some lynt issues in the old code of make_digest_auth, but this is > a feature update patch, so I didn't fix it. > - modified the implementation of RFC7616 based on community feedback. > > Signed-off-by: Eugene-bitsensing > --- > libavformat/httpauth.c | 105 ++--- > 1 file changed, 57 insertions(+), 48 deletions(-) > > diff --git a/libavformat/httpauth.c b/libavformat/httpauth.c > index 9780928357..ba2ebea3a4 100644 > --- a/libavformat/httpauth.c > +++ b/libavformat/httpauth.c > @@ -24,7 +24,7 @@ > #include "libavutil/avstring.h" > #include "internal.h" > #include "libavutil/random_seed.h" > -#include "libavutil/md5.h" > +#include "libavutil/hash.h" > #include "urldecode.h" > > static void handle_basic_params(HTTPAuthState *state, const char *key, > @@ -117,35 +117,35 @@ void ff_http_auth_handle_header(HTTPAuthState *state, > const char *key, > } > } > > - > -static void update_md5_strings(struct AVMD5 *md5ctx, ...) > +/* Generate hash string, updated to use AVHashContext to support other > algorithms */ > +static void update_hash_strings(struct AVHashContext* hash_ctx, ...) > { > va_list vl; > > -va_start(vl, md5ctx); > +va_start(vl, hash_ctx); > while (1) { > const char* str = va_arg(vl, const char*); > if (!str) > break; > -av_md5_update(md5ctx, str, strlen(str)); > +av_hash_update(hash_ctx, (const uint8_t*)str, strlen(str)); nit++: (const uint8_t *)str note the space before the "*" > } > va_end(vl); > } > > -/* Generate a digest reply, according to RFC 2617. */ > +/* Generate a digest reply, according to RFC 2617. Update to support RFC > 7617*/ nit: according to RFC 2617/7617 > static char *make_digest_auth(HTTPAuthState *state, const char *username, >const char *password, const char *uri, >const char *method) > { > DigestParams *digest = &state->digest_params; > -int len; > +size_t len;// change int -> size_t drop this comment > uint32_t cnonce_buf[2]; > char cnonce[17]; > char nc[9]; > int i; > -char A1hash[33], A2hash[33], response[33]; > -struct AVMD5 *md5ctx; > -uint8_t hash[16]; > +char a1_hash[65], a2_hash[65], response[65]; // increase hash size for > SHA-2 same here, drop the comment or this will be confusing without reference to the diff > +struct AVHashContext* hash_ctx = NULL; // use AVHashContext for other > algorithm support nit: here and below, use: TYPE *VAR rather than: TYPE* VAR style for consistency > +size_t len_hash = 33; // Modifiable hash length, MD5:32, SHA-2:64 > char *authstr; > > digest->nc++; > @@ -156,52 +156,61 @@ static char *make_digest_auth(HTTPAuthState *state, > const char *username, > cnonce_buf[i] = av_get_random_seed(); > ff_data_to_hex(cnonce, (const uint8_t*) cnonce_buf, sizeof(cnonce_buf), > 1); > > -md5ctx = av_md5_alloc(); > -if (!md5ctx) > -return NULL; > - > -av_md5_init(md5ctx); > -update_md5_strings(md5ctx, username, ":", state->realm, ":", password, > NULL); > -av_md5_final(md5ctx, hash); > -ff_data_to_hex(A1hash, hash, 16, 1); > - > -if (!strcmp(digest->algorithm, "") || !strcmp(digest->algorithm, "MD5")) > { > -} else if (!strcmp(digest->algorithm, "MD5-sess")) { > -av_md5_init(md5ctx); > -update_md5_strings(md5ctx, A1hash, ":", digest->nonce, ":", cnonce, > NULL); > -av_md5_final(md5ctx, hash); > -ff_data_to_hex(A1hash, hash, 16, 1); > -} else { > -/* Unsupported algorithm */ > -av_free(md5ctx); > +/* Generate hash context by algorithm. */ > +const char* algorithm = digest->algorithm; > +const char* hashing_algorithm; > +if (!*algorithm) { > +algorithm = "MD5"; // if empty, use MD5 as Default > +hashing_algorithm = "MD5"; > +} > +else if (av_stristr(algorithm, "md5") || av_stristr(algorithm, "MD5")) { this should be case-insensitive so you can skip one check also I wonder if this should be a strcmp, for example can it happen to have "foo-md5-bar" and should the algorithm be tolerant in such cases? style nit: here and below, put { and else on the same line } else ... { > +hashing_algorithm = "MD5"; > +} > +else if (av_stristr(algorithm, "sha256") || av_stristr(algorithm, > "sha-256")) { > +hashing_algorithm = "SHA256"; > +len_hash = 65; /
[FFmpeg-devel] [PATCH] avformat/httpauth.c [support both RFC 2617 and RFC 7617]
- Updated the make_digest_auth() function to support both RFC 2617 and RFC 7617 digest authentication. - Supports sha256 , sha512-256 along with the existing md5. MD5 and sha256 were tested, but sha512-256 was not tested due to lack of testable server. - AVHashContext instead of AVMD5 structure. - update_md5_strings() -> update_hash_strings(). - There are some lynt issues in the old code of make_digest_auth, but this is a feature update patch, so I didn't fix it. - modified the implementation of RFC7616 based on community feedback. Signed-off-by: Eugene-bitsensing --- libavformat/httpauth.c | 105 ++--- 1 file changed, 57 insertions(+), 48 deletions(-) diff --git a/libavformat/httpauth.c b/libavformat/httpauth.c index 9780928357..ba2ebea3a4 100644 --- a/libavformat/httpauth.c +++ b/libavformat/httpauth.c @@ -24,7 +24,7 @@ #include "libavutil/avstring.h" #include "internal.h" #include "libavutil/random_seed.h" -#include "libavutil/md5.h" +#include "libavutil/hash.h" #include "urldecode.h" static void handle_basic_params(HTTPAuthState *state, const char *key, @@ -117,35 +117,35 @@ void ff_http_auth_handle_header(HTTPAuthState *state, const char *key, } } - -static void update_md5_strings(struct AVMD5 *md5ctx, ...) +/* Generate hash string, updated to use AVHashContext to support other algorithms */ +static void update_hash_strings(struct AVHashContext* hash_ctx, ...) { va_list vl; -va_start(vl, md5ctx); +va_start(vl, hash_ctx); while (1) { const char* str = va_arg(vl, const char*); if (!str) break; -av_md5_update(md5ctx, str, strlen(str)); +av_hash_update(hash_ctx, (const uint8_t*)str, strlen(str)); } va_end(vl); } -/* Generate a digest reply, according to RFC 2617. */ +/* Generate a digest reply, according to RFC 2617. Update to support RFC 7617*/ static char *make_digest_auth(HTTPAuthState *state, const char *username, const char *password, const char *uri, const char *method) { DigestParams *digest = &state->digest_params; -int len; +size_t len;// change int -> size_t uint32_t cnonce_buf[2]; char cnonce[17]; char nc[9]; int i; -char A1hash[33], A2hash[33], response[33]; -struct AVMD5 *md5ctx; -uint8_t hash[16]; +char a1_hash[65], a2_hash[65], response[65]; // increase hash size for SHA-2 +struct AVHashContext* hash_ctx = NULL; // use AVHashContext for other algorithm support +size_t len_hash = 33; // Modifiable hash length, MD5:32, SHA-2:64 char *authstr; digest->nc++; @@ -156,52 +156,61 @@ static char *make_digest_auth(HTTPAuthState *state, const char *username, cnonce_buf[i] = av_get_random_seed(); ff_data_to_hex(cnonce, (const uint8_t*) cnonce_buf, sizeof(cnonce_buf), 1); -md5ctx = av_md5_alloc(); -if (!md5ctx) -return NULL; - -av_md5_init(md5ctx); -update_md5_strings(md5ctx, username, ":", state->realm, ":", password, NULL); -av_md5_final(md5ctx, hash); -ff_data_to_hex(A1hash, hash, 16, 1); - -if (!strcmp(digest->algorithm, "") || !strcmp(digest->algorithm, "MD5")) { -} else if (!strcmp(digest->algorithm, "MD5-sess")) { -av_md5_init(md5ctx); -update_md5_strings(md5ctx, A1hash, ":", digest->nonce, ":", cnonce, NULL); -av_md5_final(md5ctx, hash); -ff_data_to_hex(A1hash, hash, 16, 1); -} else { -/* Unsupported algorithm */ -av_free(md5ctx); +/* Generate hash context by algorithm. */ +const char* algorithm = digest->algorithm; +const char* hashing_algorithm; +if (!*algorithm) { +algorithm = "MD5"; // if empty, use MD5 as Default +hashing_algorithm = "MD5"; +} +else if (av_stristr(algorithm, "md5") || av_stristr(algorithm, "MD5")) { +hashing_algorithm = "MD5"; +} +else if (av_stristr(algorithm, "sha256") || av_stristr(algorithm, "sha-256")) { +hashing_algorithm = "SHA256"; +len_hash = 65; // SHA-2: 64 characters. +} +else if (av_stristr(algorithm, "sha512-256") || av_stristr(algorithm, "sha-512-256")) { +hashing_algorithm = "SHA512_256"; +len_hash = 65; // SHA-2: 64 characters. +} +else { // Unsupported algorithm return NULL; } -av_md5_init(md5ctx); -update_md5_strings(md5ctx, method, ":", uri, NULL); -av_md5_final(md5ctx, hash); -ff_data_to_hex(A2hash, hash, 16, 1); +int ret = av_hash_alloc(&hash_ctx, hashing_algorithm); +if (ret < 0) +return NULL; -av_md5_init(md5ctx); -update_md5_strings(md5ctx, A1hash, ":", digest->nonce, NULL); -if (!strcmp(digest->qop, "auth") || !strcmp(digest->qop, "auth-int")) { -update_md5_strings(md5ctx, ":", nc, ":", cnonce, ":", digest->qop, NULL); +/* a1 hash calculation */ +av_hash_init(hash_ctx); +updat