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

[Martin Storsjö]

On Mon, 22 Mar 2010, Ronald S. Bultje wrote:

> 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.
> 
> > +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).

Ok, sounds good. With the current design, it would be easier to make more 
of the digest parameters dynamically allocated, but as long as they're all 
static buffers, we could skip them.

> >  typedef enum HTTPAuthType {
> >      HTTP_AUTH_NONE = 0,
> >      HTTP_AUTH_BASIC,
> > +    HTTP_AUTH_DIGEST,
> >  } HTTPAuthType;
> 
> Mention their RFC is probably a good idea.

Ok

> > @@ -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.

Hmm, I didn't know that a HTTP response could include more than one 
challenge - I need to think about how to handle this properly.

> > +    /* 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.

Hmm, do you mean that I should skip the spaces after the commas, to shrink 
the amount of data to send?

> I think algo should also be quoted (not sure, it's a while since I last 
> did this)

Doesn't seem so according to the RFC...

> > +        if (*inptr == '\\') {
> > +            if (!inptr[1]) {
> > +                inptr++;
> > +                break;
> > +            }
> > +            *outptr++ = inptr[1];
> > +            inptr += 2;
> > +        } else {
> 
> Hm... \n or \r?

No, this handles escaped quotes (or any other escaped char) within quoted 
strings, doesn't have anything to do with newline handling.

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

Thanks :-)

// Martin