Re: [FFmpeg-devel] [PATCH] avformat/httpauth.c [support both RFC 2617 and RFC 7617]

["정지우 | Eugene" <eugene@bitsensing.com>]

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 <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Stefano Sabatini
Sent: Saturday, April 13, 2024 6:38 PM
To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
Subject: Re: [FFmpeg-devel] [PATCH] avformat/httpauth.c [support both RFC 2617 and RFC 7617]

On date Thursday 2024-04-11 07:48:14 +0000,        | 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 <eugene@bitsensing.com>
> ---
>  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; // 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);
> +    update_hash_strings(hash_ctx, username, ":", state->realm, ":", password, NULL);
> +    if (av_stristr(algorithm, "-sess")) {
> +        update_hash_strings(hash_ctx, ":", digest->nonce, ":", 
> + cnonce, NULL);
>      }
> -    update_md5_strings(md5ctx, ":", A2hash, NULL);
> -    av_md5_final(md5ctx, hash);
> -    ff_data_to_hex(response, hash, 16, 1);
> -
> -    av_free(md5ctx);
> -
> -    if (!strcmp(digest->qop, "") || !strcmp(digest->qop, "auth")) {
> -    } else if (!strcmp(digest->qop, "auth-int")) {
> -        /* qop=auth-int not supported */
> -        return NULL;
> -    } else {
> -        /* Unsupported qop value. */
> +    av_hash_final_hex(hash_ctx, a1_hash, len_hash);
> +
> +    /* a2 hash calculation */
> +    av_hash_init(hash_ctx);
> +    update_hash_strings(hash_ctx, method, ":", uri, NULL);
> +    av_hash_final_hex(hash_ctx, a2_hash, len_hash);
> +    
> +    /* response hash calculation */
> +    av_hash_init(hash_ctx);
> +    update_hash_strings(hash_ctx, a1_hash, ":", digest->nonce, NULL);
> +    if (!strcmp(digest->qop, "auth")) {
> +        update_hash_strings(hash_ctx, ":", nc, ":", cnonce, ":", digest->qop, NULL);
> +    }
> +    else if (!strcmp(digest->qop, "auth-int")) { // unsupported
> +        av_hash_freep(&hash_ctx);
>          return NULL;
>      }

> +    update_hash_strings(hash_ctx,":", a2_hash, NULL);

nit+++: hash_ctx,_":"

[...]

Should be good otherwise.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".