| Message ID | SL2P216MB14817B28DE9285C26CDAF86DB8052@SL2P216MB1481.KORP216.PROD.OUTLOOK.COM |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] avformat/httpauth.c [support both RFC 2617 and RFC 7617] | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/configure_loongarch64 | warning | Failed to apply patch |
| andriy/configure_x86 | warning | Failed to apply patch |
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.
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.
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); + 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); + update_hash_strings(hash_ctx, NULL); + av_hash_final_hex(hash_ctx, response, len_hash); + av_hash_freep(&hash_ctx); + /* Authorization header generation */ len = strlen(username) + strlen(state->realm) + strlen(digest->nonce) + strlen(uri) + strlen(response) + strlen(digest->algorithm) + strlen(digest->opaque) + strlen(digest->qop) + strlen(cnonce) +
- 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(-)