Bugfix: acme-client 301 redirect issue

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

Bugfix: acme-client 301 redirect issue

he4d
Bugfix for the 301 redirect issue reported by some users.

This patch is by user picoh from github and works fine for me (tested
with -current)
see
https://github.com/kristapsdz/acme-client-portable/issues/50#issuecomment-372119303

--- netproc.c.orig      Wed Feb  1 16:20:14 2017
+++ netproc.c   Sun Mar 11 15:15:01 2018
@@ -553,8 +553,28 @@
  {
         int      rc = 0;
         long     lc;
+       char     *http_prefix= "http:";
+       char     *https_prefix= "https:";
+       char     addrbuf[256];

         dodbg("%s: full chain", addr);
+
+       /* If the scheme is 'http' (as opposed to 'https') then rewrite
+        * the scheme to 'https'. The alternative is to follow the 301
+        * that results from trying to fetch the http URL.
+        */
+       if (strncmp(http_prefix, addr, strlen(http_prefix)) == 0)
+       {
+               lc= snprintf(addrbuf, sizeof(addrbuf), "%s%s",
+                       https_prefix, addr+strlen(http_prefix));
+               if (lc < 0 || lc >= sizeof(addrbuf))
+               {
+                       warnx("%s: string too long", addr);
+                       return (rc);
+               }
+               dodbg("using %s instead of %s", addrbuf, addr);
+               addr= addrbuf;
+       }

         if ((lc = nreq(c, addr)) < 0)
                 warnx("%s: bad comm", addr);

Anyway.. my opinion would be to just do that (also works fine with
-current)

--- usr.sbin/acme-client/netproc.c      6 Feb 2018 05:08:27 -0000      
1.15
+++ usr.sbin/acme-client/netproc.c      11 Mar 2018 14:24:36 -0000
@@ -549,7 +549,7 @@ dofullchain(struct conn *c, const char *

         if ((lc = nreq(c, addr)) < 0)
                 warnx("%s: bad comm", addr);
-       else if (lc != 200 && lc != 201)
+       else if (lc != 200 && lc != 201 && lc != 301)
                 warnx("%s: bad HTTP: %ld", addr, lc);
         else
                 rc = 1;

Reply | Threaded
Open this post in threaded view
|

Re: Bugfix: acme-client 301 redirect issue

Florian Obser-2

I think we should just follow the 301.

OK?

diff --git netproc.c netproc.c
index 26033a3fc3c..14da5a8c1a9 100644
--- netproc.c
+++ netproc.c
@@ -180,15 +180,18 @@ nreq(struct conn *c, const char *addr)
 {
  struct httpget *g;
  struct source src[MAX_SERVERS_DNS];
+ struct httphead *st;
  char *host, *path;
  short port;
  size_t srcsz;
  ssize_t ssz;
  long code;
+ int redirects = 0;
 
  if ((host = url2host(addr, &port, &path)) == NULL)
  return -1;
 
+again:
  if ((ssz = urlresolve(c->dfd, host, src)) < 0) {
  free(host);
  free(path);
@@ -202,6 +205,28 @@ nreq(struct conn *c, const char *addr)
  if (g == NULL)
  return -1;
 
+ if (g->code == 301) {
+ redirects++;
+ if (redirects > 3) {
+ warnx("too many redirects");
+ http_get_free(g);
+ return -1;
+ }
+
+ if ((st = http_head_get("Location", g->head, g->headsz)) ==
+    NULL) {
+ warnx("redirect without location header");
+ return -1;
+ }
+
+ dodbg("Location: %s", st->val);
+ host = url2host(st->val, &port, &path);
+ http_get_free(g);
+ if (host == NULL)
+ return -1;
+ goto again;
+ }
+
  code = g->code;
 
  /* Copy the body part into our buffer. */

--
I'm not entirely sure you are real.

Reply | Threaded
Open this post in threaded view
|

Re: Bugfix: acme-client 301 redirect issue

Stuart Henderson
On 2018/03/11 17:52, Florian Obser wrote:
>
> I think we should just follow the 301.

I didn't hear back from @letsencrypt_ops about why they were
issue 301s, but I do agree it makes sense to follow them.

> OK?
>
> diff --git netproc.c netproc.c
> index 26033a3fc3c..14da5a8c1a9 100644
> --- netproc.c
> +++ netproc.c
> @@ -180,15 +180,18 @@ nreq(struct conn *c, const char *addr)
>  {
>   struct httpget *g;
>   struct source src[MAX_SERVERS_DNS];
> + struct httphead *st;
>   char *host, *path;
>   short port;
>   size_t srcsz;
>   ssize_t ssz;
>   long code;
> + int redirects = 0;
>  
>   if ((host = url2host(addr, &port, &path)) == NULL)
>   return -1;
>  
> +again:
>   if ((ssz = urlresolve(c->dfd, host, src)) < 0) {
>   free(host);
>   free(path);
> @@ -202,6 +205,28 @@ nreq(struct conn *c, const char *addr)
>   if (g == NULL)
>   return -1;
>  
> + if (g->code == 301) {

301 is the one we saw, but shouldn't we follow others as well?

https://http.cat/302
https://http.cat/303
https://http.cat/307


> + redirects++;
> + if (redirects > 3) {
> + warnx("too many redirects");
> + http_get_free(g);
> + return -1;
> + }
> +
> + if ((st = http_head_get("Location", g->head, g->headsz)) ==
> +    NULL) {
> + warnx("redirect without location header");
> + return -1;
> + }
> +
> + dodbg("Location: %s", st->val);
> + host = url2host(st->val, &port, &path);
> + http_get_free(g);
> + if (host == NULL)

nitpicking, trailing tabs

> + return -1;
> + goto again;
> + }
> +
>   code = g->code;
>  
>   /* Copy the body part into our buffer. */
>
> --
> I'm not entirely sure you are real.
>

Reply | Threaded
Open this post in threaded view
|

Re: Bugfix: acme-client 301 redirect issue

Florian Obser-2
On Wed, Mar 14, 2018 at 01:08:33AM +0000, Stuart Henderson wrote:
> On 2018/03/11 17:52, Florian Obser wrote:
> >
> > I think we should just follow the 301.
>
> I didn't hear back from @letsencrypt_ops about why they were
> issue 301s, but I do agree it makes sense to follow them.

update diff with more redirect status codes and EOL tab fixed

diff --git netproc.c netproc.c
index 26033a3fc3c..ea901a1bda5 100644
--- netproc.c
+++ netproc.c
@@ -180,15 +180,18 @@ nreq(struct conn *c, const char *addr)
 {
  struct httpget *g;
  struct source src[MAX_SERVERS_DNS];
+ struct httphead *st;
  char *host, *path;
  short port;
  size_t srcsz;
  ssize_t ssz;
  long code;
+ int redirects = 0;
 
  if ((host = url2host(addr, &port, &path)) == NULL)
  return -1;
 
+again:
  if ((ssz = urlresolve(c->dfd, host, src)) < 0) {
  free(host);
  free(path);
@@ -202,7 +205,36 @@ nreq(struct conn *c, const char *addr)
  if (g == NULL)
  return -1;
 
- code = g->code;
+ switch (g->code) {
+ case 301:
+ case 302:
+ case 303:
+ case 307:
+ case 308:
+ redirects++;
+ if (redirects > 3) {
+ warnx("too many redirects");
+ http_get_free(g);
+ return -1;
+ }
+
+ if ((st = http_head_get("Location", g->head, g->headsz)) ==
+    NULL) {
+ warnx("redirect without location header");
+ return -1;
+ }
+
+ dodbg("Location: %s", st->val);
+ host = url2host(st->val, &port, &path);
+ http_get_free(g);
+ if (host == NULL)
+ return -1;
+ goto again;
+ break;
+ default:
+ code = g->code;
+ break;
+ }
 
  /* Copy the body part into our buffer. */
 


--
I'm not entirely sure you are real.

Reply | Threaded
Open this post in threaded view
|

Re: Bugfix: acme-client 301 redirect issue

Sebastian Benoit-3
ok

Florian Obser([hidden email]) on 2018.03.14 08:15:35 +0100:

> On Wed, Mar 14, 2018 at 01:08:33AM +0000, Stuart Henderson wrote:
> > On 2018/03/11 17:52, Florian Obser wrote:
> > >
> > > I think we should just follow the 301.
> >
> > I didn't hear back from @letsencrypt_ops about why they were
> > issue 301s, but I do agree it makes sense to follow them.
>
> update diff with more redirect status codes and EOL tab fixed
>
> diff --git netproc.c netproc.c
> index 26033a3fc3c..ea901a1bda5 100644
> --- netproc.c
> +++ netproc.c
> @@ -180,15 +180,18 @@ nreq(struct conn *c, const char *addr)
>  {
>   struct httpget *g;
>   struct source src[MAX_SERVERS_DNS];
> + struct httphead *st;
>   char *host, *path;
>   short port;
>   size_t srcsz;
>   ssize_t ssz;
>   long code;
> + int redirects = 0;
>  
>   if ((host = url2host(addr, &port, &path)) == NULL)
>   return -1;
>  
> +again:
>   if ((ssz = urlresolve(c->dfd, host, src)) < 0) {
>   free(host);
>   free(path);
> @@ -202,7 +205,36 @@ nreq(struct conn *c, const char *addr)
>   if (g == NULL)
>   return -1;
>  
> - code = g->code;
> + switch (g->code) {
> + case 301:
> + case 302:
> + case 303:
> + case 307:
> + case 308:
> + redirects++;
> + if (redirects > 3) {
> + warnx("too many redirects");
> + http_get_free(g);
> + return -1;
> + }
> +
> + if ((st = http_head_get("Location", g->head, g->headsz)) ==
> +    NULL) {
> + warnx("redirect without location header");
> + return -1;
> + }
> +
> + dodbg("Location: %s", st->val);
> + host = url2host(st->val, &port, &path);
> + http_get_free(g);
> + if (host == NULL)
> + return -1;
> + goto again;
> + break;
> + default:
> + code = g->code;
> + break;
> + }
>  
>   /* Copy the body part into our buffer. */
>  
>
>
> --
> I'm not entirely sure you are real.
>