HTTP proxy: correctly handle multiple auth headers.

This is a piece I forgot in the initial implementation of HTTP Digest:
an HTTP server can send _more than one_ authentication request header
(WWW-Authenticate for normal servers, Proxy-Authenticate for proxies),
and if it does, they're supposed to be treated as alternatives to each
other, so that the client chooses one to reply to.

I suppose that technically we were 'complying' with that spec already,
in that HttpProxyNegotiator would have read each new header and
overwritten all the fields set by the previous one, so we'd always
have gone with the last header presented by the server. But that seems
inelegant: better to choose the one we actually like best.

So now we do that. All the details of an auth header are moved out of
the main HttpProxyNegotiator struct into a sub-struct we can have
multiple copies of. Each new header is parsed into a fresh struct of
that kind, and then we can compare it with the previous one and decide
which we prefer.

The preference order, naturally, is 'more secure is better': Digest
beats Basic, and between two Digest headers, SHA-256 beats MD5. (And
anything beats a header we can't make sense of at all.)

Another side effect of this change is that a 407 response which
contains _no_ Proxy-Authenticate headers will trigger an error message
saying so, instead of just going with whatever happened to be left in
the relevant variables from the previous attempt.

proxy/cproxy.h

@@ -73,6 +73,9 @@ extern const bool http_digest_available;
  *  - algorithm to use for computing it (as a const ssh_hashalg *)
  *  - length to truncate the output to
  *  - whether we accept it in http.c or not.
+ *
+ * Finally, the ordering of the accepted hashes is our preference
+ * order among them if the server offers a choice.
  */
 #define HTTP_DIGEST_HASHES(X)                                           \
     X(HTTP_DIGEST_MD5, "MD5", &ssh_md5, 128, true)                      \

proxy/http.c

@@ -41,7 +41,22 @@ static bool read_line(bufchain *input, strbuf *output, bool is_header)
     return true;
 }
 
-typedef enum HttpAuthType { AUTH_NONE, AUTH_BASIC, AUTH_DIGEST } HttpAuthType;
+/* Types of HTTP authentication, in preference order. */
+typedef enum HttpAuthType {
+    AUTH_ERROR, /* if an HttpAuthDetails was never satisfactorily filled in */
+    AUTH_NONE,  /* if no auth header is seen, assume no auth required */
+    AUTH_BASIC, /* username + password sent in clear (only keyless base64) */
+    AUTH_DIGEST, /* cryptographic hash, most preferred if available */
+} HttpAuthType;
+
+typedef struct HttpAuthDetails {
+    HttpAuthType auth_type;
+    bool digest_nonce_was_stale;
+    HttpDigestHash digest_hash;
+    strbuf *realm, *nonce, *opaque, *error;
+    bool got_opaque;
+    bool hash_username;
+} HttpAuthDetails;
 
 typedef struct HttpProxyNegotiator {
     int crLine;
@@ -51,20 +66,51 @@ typedef struct HttpProxyNegotiator {
     strbuf *username, *password;
     int http_status;
     bool connection_close;
-    HttpAuthType next_auth_type;
+    HttpAuthDetails *next_auth;
     bool try_auth_from_conf;
-    strbuf *realm, *nonce, *opaque, *uri;
-    bool got_opaque;
+    strbuf *uri;
     uint32_t nonce_count;
-    bool digest_nonce_was_stale;
-    HttpDigestHash digest_hash;
-    bool hash_username;
     prompts_t *prompts;
     int username_prompt_index, password_prompt_index;
     size_t content_length;
     ProxyNegotiator pn;
 } HttpProxyNegotiator;
 
+static inline HttpAuthDetails *auth_error(HttpAuthDetails *d,
+                                          const char *fmt, ...)
+{
+    d->auth_type = AUTH_ERROR;
+    put_fmt(d->error, "Unable to parse auth header from HTTP proxy");
+    if (fmt) {
+        va_list ap;
+        va_start(ap, fmt);
+        put_datalit(d->error, ": ");
+        put_fmtv(d->error, fmt, ap);
+        va_end(ap);
+    }
+    return d;
+}
+
+static HttpAuthDetails *http_auth_details_new(void)
+{
+    HttpAuthDetails *d = snew(HttpAuthDetails);
+    memset(d, 0, sizeof(*d));
+    d->realm = strbuf_new();
+    d->nonce = strbuf_new();
+    d->opaque = strbuf_new();
+    d->error = strbuf_new();
+    return d;
+}
+
+static void http_auth_details_free(HttpAuthDetails *d)
+{
+    strbuf_free(d->realm);
+    strbuf_free(d->nonce);
+    strbuf_free(d->opaque);
+    strbuf_free(d->error);
+    sfree(d);
+}
+
 static ProxyNegotiator *proxy_http_new(const ProxyNegotiatorVT *vt)
 {
     HttpProxyNegotiator *s = snew(HttpProxyNegotiator);
@@ -75,9 +121,6 @@ static ProxyNegotiator *proxy_http_new(const ProxyNegotiatorVT *vt)
     s->token = strbuf_new();
     s->username = strbuf_new();
     s->password = strbuf_new_nm();
-    s->realm = strbuf_new();
-    s->nonce = strbuf_new();
-    s->opaque = strbuf_new();
     s->uri = strbuf_new();
     s->nonce_count = 0;
     /*
@@ -85,7 +128,8 @@ static ProxyNegotiator *proxy_http_new(const ProxyNegotiatorVT *vt)
      * proxy rejects that, it will tell us what kind of auth it would
      * prefer.
      */
-    s->next_auth_type = AUTH_NONE;
+    s->next_auth = http_auth_details_new();
+    s->next_auth->auth_type = AUTH_NONE;
     return &s->pn;
 }
 
@@ -97,10 +141,8 @@ static void proxy_http_free(ProxyNegotiator *pn)
     strbuf_free(s->token);
     strbuf_free(s->username);
     strbuf_free(s->password);
-    strbuf_free(s->realm);
-    strbuf_free(s->nonce);
-    strbuf_free(s->opaque);
     strbuf_free(s->uri);
+    http_auth_details_free(s->next_auth);
     if (s->prompts)
         free_prompts(s->prompts);
     sfree(s);
@@ -222,6 +264,118 @@ static bool get_quoted_string(HttpProxyNegotiator *s)
     return true;
 }
 
+static HttpAuthDetails *parse_http_auth_header(HttpProxyNegotiator *s)
+{
+    HttpAuthDetails *d = http_auth_details_new();
+
+    /* Default hash for HTTP Digest is MD5, if none specified explicitly */
+    d->digest_hash = HTTP_DIGEST_MD5;
+
+    if (!get_token(s))
+        return auth_error(d, "parse error");
+
+    if (!stricmp(s->token->s, "Basic")) {
+        /* For Basic authentication, we don't need anything else. The
+         * realm string is not required for the protocol. */
+        d->auth_type = AUTH_BASIC;
+        return d;
+    }
+
+    if (!stricmp(s->token->s, "Digest")) {
+        /* Parse all the additional parts of the Digest header. */
+        if (!http_digest_available)
+            return auth_error(d, "Digest authentication not supported");
+
+        /* Parse the rest of the Digest header */
+        while (true) {
+            if (!get_token(s))
+                return auth_error(d, "parse error in Digest header");
+
+            if (!stricmp(s->token->s, "realm")) {
+                if (!get_separator(s, '=') ||
+                    !get_quoted_string(s))
+                    return auth_error(d, "parse error in Digest realm field");
+                put_datapl(d->realm, ptrlen_from_strbuf(s->token));
+            } else if (!stricmp(s->token->s, "nonce")) {
+                if (!get_separator(s, '=') ||
+                    !get_quoted_string(s))
+                    return auth_error(d, "parse error in Digest nonce field");
+                put_datapl(d->nonce, ptrlen_from_strbuf(s->token));
+            } else if (!stricmp(s->token->s, "opaque")) {
+                if (!get_separator(s, '=') ||
+                    !get_quoted_string(s))
+                    return auth_error(d, "parse error in Digest opaque field");
+                put_datapl(d->opaque,
+                           ptrlen_from_strbuf(s->token));
+                d->got_opaque = true;
+            } else if (!stricmp(s->token->s, "stale")) {
+                if (!get_separator(s, '=') ||
+                    !get_token(s))
+                    return auth_error(d, "parse error in Digest stale field");
+                d->digest_nonce_was_stale = !stricmp(
+                    s->token->s, "true");
+            } else if (!stricmp(s->token->s, "userhash")) {
+                if (!get_separator(s, '=') ||
+                    !get_token(s))
+                    return auth_error(d, "parse error in Digest userhash "
+                                      "field");
+                d->hash_username = !stricmp(s->token->s, "true");
+            } else if (!stricmp(s->token->s, "algorithm")) {
+                if (!get_separator(s, '=') ||
+                    !get_token(s))
+                    return auth_error(d, "parse error in Digest algorithm "
+                                      "field");
+                bool found = false;
+                size_t i;
+
+                for (i = 0; i < N_HTTP_DIGEST_HASHES; i++) {
+                    if (!stricmp(s->token->s, httphashnames[i])) {
+                        found = true;
+                        break;
+                    }
+                }
+
+                if (!found) {
+                    /* We don't even recognise the name */
+                    return auth_error(d, "Digest hash algorithm '%s' not "
+                                      "recognised", s->token->s);
+                }
+
+                if (!httphashaccepted[i]) {
+                    /* We do recognise the name but we
+                     * don't like it (see comment in cproxy.h) */
+                    return auth_error(d, "Digest hash algorithm '%s' not "
+                                      "supported", s->token->s);
+                }
+
+                d->digest_hash = i;
+            } else if (!stricmp(s->token->s, "qop")) {
+                if (!get_separator(s, '=') ||
+                    !get_quoted_string(s))
+                    return auth_error(d, "parse error in Digest qop field");
+                if (stricmp(s->token->s, "auth"))
+                    return auth_error(d, "quality-of-protection type '%s' not "
+                                      "supported", s->token->s);
+            } else {
+                /* Ignore any other auth-param */
+                if (!get_separator(s, '=') ||
+                    (!get_quoted_string(s) && !get_token(s)))
+                    return auth_error(d, "parse error in Digest header");
+            }
+
+            if (get_end_of_header(s))
+                break;
+            if (!get_separator(s, ','))
+                return auth_error(d, "parse error in Digest header");
+        }
+        d->auth_type = AUTH_DIGEST;
+        return d;
+    }
+
+    return auth_error(d, "authentication type '%s' not supported",
+                      s->token->s);
+}
+
 static void proxy_http_process_queue(ProxyNegotiator *pn)
 {
     HttpProxyNegotiator *s = container_of(pn, HttpProxyNegotiator, pn);
@@ -257,7 +411,7 @@ static void proxy_http_process_queue(ProxyNegotiator *pn)
         /*
          * Add an auth header, if we're planning to this time round.
          */
-        if (s->next_auth_type == AUTH_BASIC) {
+        if (s->next_auth->auth_type == AUTH_BASIC) {
             put_datalit(pn->output, "Proxy-Authorization: Basic ");
 
             strbuf *base64_input = strbuf_new_nm();
@@ -274,21 +428,28 @@ static void proxy_http_process_queue(ProxyNegotiator *pn)
             strbuf_free(base64_input);
             smemclr(base64_output, sizeof(base64_output));
             put_datalit(pn->output, "\r\n");
-        } else if (s->next_auth_type == AUTH_DIGEST) {
+        } else if (s->next_auth->auth_type == AUTH_DIGEST) {
             put_datalit(pn->output, "Proxy-Authorization: Digest ");
+
+            /* If we have a fresh nonce, reset the
+             * nonce count. Otherwise, keep incrementing it. */
+            if (!ptrlen_eq_ptrlen(ptrlen_from_strbuf(s->token),
+                                  ptrlen_from_strbuf(s->next_auth->nonce)))
+                s->nonce_count = 0;
+
             http_digest_response(BinarySink_UPCAST(pn->output),
                                  ptrlen_from_strbuf(s->username),
                                  ptrlen_from_strbuf(s->password),
-                                 ptrlen_from_strbuf(s->realm),
+                                 ptrlen_from_strbuf(s->next_auth->realm),
                                  PTRLEN_LITERAL("CONNECT"),
                                  ptrlen_from_strbuf(s->uri),
                                  PTRLEN_LITERAL("auth"),
-                                 ptrlen_from_strbuf(s->nonce),
-                                 (s->got_opaque ?
-                                  ptrlen_from_strbuf(s->opaque) :
+                                 ptrlen_from_strbuf(s->next_auth->nonce),
+                                 (s->next_auth->got_opaque ?
+                                  ptrlen_from_strbuf(s->next_auth->opaque) :
                                   make_ptrlen(NULL, 0)),
-                                 ++s->nonce_count, s->digest_hash,
-                                 s->hash_username);
+                                 ++s->nonce_count, s->next_auth->digest_hash,
+                                 s->next_auth->hash_username);
             put_datalit(pn->output, "\r\n");
         }
 
@@ -324,6 +485,20 @@ static void proxy_http_process_queue(ProxyNegotiator *pn)
             }
         }
 
+        if (s->http_status == 407) {
+            /*
+             * If this is going to be an auth request, we expect to
+             * see at least one Proxy-Authorization header offering us
+             * auth options. Start by preloading s->next_auth with a
+             * fallback error message, which will be used if nothing
+             * better is available.
+             */
+            http_auth_details_free(s->next_auth);
+            s->next_auth = http_auth_details_new();
+            auth_error(s->next_auth, "no Proxy-Authorization header seen in "
+                       "HTTP 407 Proxy Authentication Required response");
+        }
+
         /*
          * Read the HTTP response header section.
          */
@@ -363,136 +538,47 @@ static void proxy_http_process_queue(ProxyNegotiator *pn)
                 else if (!stricmp(s->token->s, "keep-alive"))
                     s->connection_close = false;
             } else if (hdr == HDR_PROXY_AUTHENTICATE) {
-                if (!get_token(s))
-                    continue;
+                HttpAuthDetails *auth = parse_http_auth_header(s);
 
-                if (!stricmp(s->token->s, "Basic")) {
-                    s->next_auth_type = AUTH_BASIC;
-                } else if (!stricmp(s->token->s, "Digest")) {
-                    if (!http_digest_available) {
-                        pn->error = dupprintf(
-                            "HTTP proxy requested Digest authentication "
-                            "which we do not support");
-                        crStopV;
-                    }
+                /*
+                 * See if we prefer this set of auth details to the
+                 * previous one we had (either from a previous auth
+                 * header, or the fallback when no auth header is
+                 * provided at all).
+                 */
+                bool change;
 
-                    /* Parse the rest of the Digest header */
-                    s->digest_nonce_was_stale = false;
-                    s->digest_hash = HTTP_DIGEST_MD5;
-                    strbuf_clear(s->realm);
-                    strbuf_clear(s->nonce);
-                    strbuf_clear(s->opaque);
-                    s->got_opaque = false;
-                    s->hash_username = false;
-
-                    while (true) {
-                        if (!get_token(s))
-                            goto bad_digest;
-
-                        if (!stricmp(s->token->s, "realm")) {
-                            if (!get_separator(s, '=') ||
-                                !get_quoted_string(s))
-                                goto bad_digest;
-                            put_datapl(s->realm, ptrlen_from_strbuf(s->token));
-                        } else if (!stricmp(s->token->s, "nonce")) {
-                            if (!get_separator(s, '=') ||
-                                !get_quoted_string(s))
-                                goto bad_digest;
-
-                            /* If we have a fresh nonce, reset the
-                             * nonce count. Otherwise, keep incrementing it. */
-                            if (!ptrlen_eq_ptrlen(
-                                    ptrlen_from_strbuf(s->token),
-                                    ptrlen_from_strbuf(s->nonce)))
-                                s->nonce_count = 0;
-
-                            put_datapl(s->nonce, ptrlen_from_strbuf(s->token));
-                        } else if (!stricmp(s->token->s, "opaque")) {
-                            if (!get_separator(s, '=') ||
-                                !get_quoted_string(s))
-                                goto bad_digest;
-                            put_datapl(s->opaque,
-                                       ptrlen_from_strbuf(s->token));
-                            s->got_opaque = true;
-                        } else if (!stricmp(s->token->s, "stale")) {
-                            if (!get_separator(s, '=') ||
-                                !get_token(s))
-                                goto bad_digest;
-                            s->digest_nonce_was_stale = !stricmp(
-                                s->token->s, "true");
-                        } else if (!stricmp(s->token->s, "userhash")) {
-                            if (!get_separator(s, '=') ||
-                                !get_token(s))
-                                goto bad_digest;
-                            s->hash_username = !stricmp(s->token->s, "true");
-                        } else if (!stricmp(s->token->s, "algorithm")) {
-                            if (!get_separator(s, '=') ||
-                                !get_token(s))
-                                goto bad_digest;
-                            bool found = false;
-                            size_t i;
-
-                            for (i = 0; i < N_HTTP_DIGEST_HASHES; i++) {
-                                if (!stricmp(s->token->s, httphashnames[i])) {
-                                    found = true;
-                                    break;
-                                }
-                            }
-
-                            if (!found) {
-                                /* We don't even recognise the name */
-                                pn->error = dupprintf(
-                                    "HTTP proxy requested Digest hash "
-                                    "algorithm '%s' which we do not recognise",
-                                    s->token->s);
-                                crStopV;
-                            }
-
-                            if (!httphashaccepted[i]) {
-                                /* We do recognise the name but we
-                                 * don't like it (see comment in cproxy.h) */
-                                pn->error = dupprintf(
-                                    "HTTP proxy requested Digest hash "
-                                    "algorithm '%s' which we do not support",
-                                    s->token->s);
-                                crStopV;
-                            }
-
-                            s->digest_hash = i;
-                        } else if (!stricmp(s->token->s, "qop")) {
-                            if (!get_separator(s, '=') ||
-                                !get_quoted_string(s))
-                                goto bad_digest;
-                            if (stricmp(s->token->s, "auth")) {
-                                pn->error = dupprintf(
-                                    "HTTP proxy requested Digest quality-of-"
-                                    "protection type '%s' which we do not "
-                                    "support", s->token->s);
-                                crStopV;
-                            }
+                if (auth->auth_type != s->next_auth->auth_type) {
+                    /* Use the preference order implied by the enum */
+                    change = auth->auth_type > s->next_auth->auth_type;
+                } else if (auth->auth_type == AUTH_DIGEST &&
+                           auth->digest_hash != s->next_auth->digest_hash) {
+                    /* Choose based on the hash functions */
+                    change = auth->digest_hash > s->next_auth->digest_hash;
                 } else {
-                            /* Ignore any other auth-param */
-                            if (!get_separator(s, '=') ||
-                                (!get_quoted_string(s) && !get_token(s)))
-                                goto bad_digest;
+                    /*
+                     * If in doubt, go with the later one of the
+                     * headers.
+                     *
+                     * The main reason for this is so that an error in
+                     * interpreting an auth header will supersede the
+                     * default error we preload saying 'no header
+                     * found', because that would be a particularly
+                     * bad error to report if there _was_ one.
+                     *
+                     * But we're in a tie-breaking situation by now,
+                     * so there's no other reason to choose - we might
+                     * as well apply the same policy everywhere else
+                     * too.
+                     */
+                    change = true;
                 }
 
-                        if (get_end_of_header(s))
-                            break;
-                        if (!get_separator(s, ','))
-                            goto bad_digest;
-                    }
-                    s->next_auth_type = AUTH_DIGEST;
-                    continue;
-                  bad_digest:
-                    pn->error = dupprintf("HTTP proxy sent Digest auth "
-                                          "request we could not parse");
-                    crStopV;
+                if (change) {
+                    http_auth_details_free(s->next_auth);
+                    s->next_auth = auth;
                 } else {
-                    pn->error = dupprintf("HTTP proxy asked for unsupported "
-                                          "authentication type '%s'",
-                                          s->token->s);
-                    crStopV;
+                    http_auth_details_free(auth);
                 }
             }
         } while (s->header->len > 0);
@@ -513,6 +599,16 @@ static void proxy_http_process_queue(ProxyNegotiator *pn)
                 crStopV;
             }
 
+            /* If the best we can do is report some kind of error from
+             * a Proxy-Auth header (or an error saying there wasn't
+             * one at all), and no successful parsing of an auth
+             * header superseded that, then just throw that error and
+             * die. */
+            if (s->next_auth->auth_type == AUTH_ERROR) {
+                pn->error = dupstr(s->next_auth->error->s);
+                crStopV;
+            }
+
             /* If we have auth details from the Conf and haven't tried
              * them yet, that's our first step. */
             if (s->try_auth_from_conf) {
@@ -524,7 +620,7 @@ static void proxy_http_process_queue(ProxyNegotiator *pn)
              * header, that means we _don't_ need to request a new
              * password yet; just try again with the existing details
              * and the fresh nonce it sent us. */
-            if (s->digest_nonce_was_stale)
+            if (s->next_auth->digest_nonce_was_stale)
                 continue;
 
             /* Either we never had a password in the first place, or