[FFmpeg-devel] [PATCH] Add support for digest auth in the http and rtsp protocols

[Ronald S. Bultje]

Hi,

On Mon, Mar 22, 2010 at 2:40 PM, Martin Storsj? <martin at martin.st> wrote:
> As in subject - this series adds http digest auth support to http and
> rtsp.

Great idea! Let's start with general setup as in #1/#2 (i.e. add
digest to HTTP and separate it into its own code).

> +void ff_http_auth_init(HTTPAuthState *state);
> +void ff_http_auth_handle_header(HTTPAuthState *state, const char *key,
> +                                const char *value);
> +char *ff_http_auth_create_response(HTTPAuthState *state, const char *auth,
> +                                   const char *path, const char *method);
> +void ff_http_auth_close(HTTPAuthState *state);

This is a little much, maybe. If you inline DigestParams, close/init
are not needed (if you doxy that caller should memset(0) this before
using the other two functions).

>  typedef enum HTTPAuthType {
>      HTTP_AUTH_NONE = 0,
>      HTTP_AUTH_BASIC,
> +    HTTP_AUTH_DIGEST,
>  } HTTPAuthType;

Mention their RFC is probably a good idea.

> @@ -34,8 +249,26 @@ void ff_http_auth_handle_header(HTTPAuthState *state, const char *key,
>          const char *p;
>          if (av_stristart(value, "Basic", &p)) {
>              state->auth_type = HTTP_AUTH_BASIC;
> +        } else if (av_stristart(value, "Digest", &p)) {
> +            char *copy;
> +            state->auth_type = HTTP_AUTH_DIGEST;

So what happens (and what does it mean) if the server says Basic,
Digest? I think it's a non-preferential list so we would ideally still
choose the second (Digest), since it's safer. Also, what if Digest2 is
in the list? Better to split this comma-separated string into its
elements, see if Digest is in there, if so use digest, else see if
Basic is in there, if so use basic, etc.

> +    /* TODO: Quote the quoted strings properly. */
> +    av_strlcatf(authstr, len, "username=\"%s\"",   username);
> +    av_strlcatf(authstr, len, ", realm=\"%s\"",    digest->realm);
> +    av_strlcatf(authstr, len, ", nonce=\"%s\"",    digest->nonce);
> +    av_strlcatf(authstr, len, ", uri=\"%s\"",      uri);
> +    av_strlcatf(authstr, len, ", response=\"%s\"", response);
> +    if (digest->algorithm[0])
> +        av_strlcatf(authstr, len, ", algorithm=%s",  digest->algorithm);
> +    if (digest->opaque[0])
> +        av_strlcatf(authstr, len, ", opaque=\"%s\"", digest->opaque);
> +    if (digest->qop[0]) {
> +        av_strlcatf(authstr, len, ", qop=\"%s\"",    digest->qop);
> +        av_strlcatf(authstr, len, ", cnonce=\"%s\"", cnonce);
> +        av_strlcatf(authstr, len, ", nc=%s",         nc);

Spaces cost wiredata, bad idea. I think algo should also be quoted
(not sure, it's a while since I last did this)

> +        if (*inptr == '\\') {
> +            if (!inptr[1]) {
> +                inptr++;
> +                break;
> +            }
> +            *outptr++ = inptr[1];
> +            inptr += 2;
> +        } else {

Hm... \n or \r?

In general, the actual MD5 generation looks pretty good, the code is
almost identical to what I wrote a long time ago. ;-).

Didn't review RTSP patches.

Ronald