acme-client: improve account creation error message

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

acme-client: improve account creation error message

Florian Obser-2
not helpful:
$ doas acme-client $(hostname)
acme-client: https://api.test4.buypass.no/acme-v02/new-acct: bad HTTP: 400

vomitting unformated json is not better:
$ doas acme-client -v $(hostname)
acme-client: transfer buffer: [{"type":"urn:ietf:params:acme:error:malformed","detail":"Email is a required contact","code":400,"message":"MALFORMED_BAD_REQUEST","details":"HTTP 400 Bad Request"}] (164 bytes)

let's do this:
$ doas obj/acme-client -v $(hostname)
acme-client: Email is a required contact

OK?

diff --git extern.h extern.h
index 529d3350205..364425b0500 100644
--- extern.h
+++ extern.h
@@ -259,6 +259,7 @@ int json_parse_order(struct jsmnn *, struct order *);
 int json_parse_upd_order(struct jsmnn *, struct order *);
 void json_free_capaths(struct capaths *);
 int json_parse_capaths(struct jsmnn *, struct capaths *);
+char *json_getstr(struct jsmnn *, const char *);
 
 char *json_fmt_newcert(const char *);
 char *json_fmt_chkacc(void);
diff --git json.c json.c
index 61d2631359f..a6762eeb258 100644
--- json.c
+++ json.c
@@ -297,7 +297,7 @@ json_getobj(struct jsmnn *n, const char *name)
  * that it's the correct type.
  * Returns NULL on failure.
  */
-static char *
+char *
 json_getstr(struct jsmnn *n, const char *name)
 {
  size_t i;
diff --git netproc.c netproc.c
index 7b8152196d1..05e36897c38 100644
--- netproc.c
+++ netproc.c
@@ -371,15 +371,27 @@ sreq(struct conn *c, const char *addr, int kid, const char *req, char **loc)
 static int
 donewacc(struct conn *c, const struct capaths *p)
 {
+ struct jsmnn *j = NULL;
  int rc = 0;
- char *req;
+ char *req, *detail, *error = NULL;
  long lc;
 
  if ((req = json_fmt_newacc()) == NULL)
  warnx("json_fmt_newacc");
  else if ((lc = sreq(c, p->newaccount, 0, req, &c->kid)) < 0)
  warnx("%s: bad comm", p->newaccount);
- else if (lc != 200 && lc != 201)
+ else if (lc == 400) {
+ if ((j = json_parse(c->buf.buf, c->buf.sz)) == NULL)
+ warnx("%s: bad JSON object", p->newaccount);
+ else {
+ detail = json_getstr(j, "detail");
+ if (detail != NULL && stravis(&error, detail, VIS_SAFE)
+    != -1) {
+ warnx("%s", error);
+ free(error);
+ }
+ }
+ } else if (lc != 200 && lc != 201)
  warnx("%s: bad HTTP: %ld", p->newaccount, lc);
  else if (c->buf.buf == NULL || c->buf.sz == 0)
  warnx("%s: empty response", p->newaccount);


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

Reply | Threaded
Open this post in threaded view
|

Re: acme-client: improve account creation error message

Bob Beck-2

But what if I like json and I am already set up to be a hipster and
feed all the untrusted inputs through jq..

(ok beck@)

On Mon, Sep 14, 2020 at 03:37:25PM +0200, Florian Obser wrote:

> not helpful:
> $ doas acme-client $(hostname)
> acme-client: https://api.test4.buypass.no/acme-v02/new-acct: bad HTTP: 400
>
> vomitting unformated json is not better:
> $ doas acme-client -v $(hostname)
> acme-client: transfer buffer: [{"type":"urn:ietf:params:acme:error:malformed","detail":"Email is a required contact","code":400,"message":"MALFORMED_BAD_REQUEST","details":"HTTP 400 Bad Request"}] (164 bytes)
>
> let's do this:
> $ doas obj/acme-client -v $(hostname)
> acme-client: Email is a required contact
>
> OK?
>
> diff --git extern.h extern.h
> index 529d3350205..364425b0500 100644
> --- extern.h
> +++ extern.h
> @@ -259,6 +259,7 @@ int json_parse_order(struct jsmnn *, struct order *);
>  int json_parse_upd_order(struct jsmnn *, struct order *);
>  void json_free_capaths(struct capaths *);
>  int json_parse_capaths(struct jsmnn *, struct capaths *);
> +char *json_getstr(struct jsmnn *, const char *);
>  
>  char *json_fmt_newcert(const char *);
>  char *json_fmt_chkacc(void);
> diff --git json.c json.c
> index 61d2631359f..a6762eeb258 100644
> --- json.c
> +++ json.c
> @@ -297,7 +297,7 @@ json_getobj(struct jsmnn *n, const char *name)
>   * that it's the correct type.
>   * Returns NULL on failure.
>   */
> -static char *
> +char *
>  json_getstr(struct jsmnn *n, const char *name)
>  {
>   size_t i;
> diff --git netproc.c netproc.c
> index 7b8152196d1..05e36897c38 100644
> --- netproc.c
> +++ netproc.c
> @@ -371,15 +371,27 @@ sreq(struct conn *c, const char *addr, int kid, const char *req, char **loc)
>  static int
>  donewacc(struct conn *c, const struct capaths *p)
>  {
> + struct jsmnn *j = NULL;
>   int rc = 0;
> - char *req;
> + char *req, *detail, *error = NULL;
>   long lc;
>  
>   if ((req = json_fmt_newacc()) == NULL)
>   warnx("json_fmt_newacc");
>   else if ((lc = sreq(c, p->newaccount, 0, req, &c->kid)) < 0)
>   warnx("%s: bad comm", p->newaccount);
> - else if (lc != 200 && lc != 201)
> + else if (lc == 400) {
> + if ((j = json_parse(c->buf.buf, c->buf.sz)) == NULL)
> + warnx("%s: bad JSON object", p->newaccount);
> + else {
> + detail = json_getstr(j, "detail");
> + if (detail != NULL && stravis(&error, detail, VIS_SAFE)
> +    != -1) {
> + warnx("%s", error);
> + free(error);
> + }
> + }
> + } else if (lc != 200 && lc != 201)
>   warnx("%s: bad HTTP: %ld", p->newaccount, lc);
>   else if (c->buf.buf == NULL || c->buf.sz == 0)
>   warnx("%s: empty response", p->newaccount);
>
>
> --
> I'm not entirely sure you are real.
>

Reply | Threaded
Open this post in threaded view
|

Re: acme-client: improve account creation error message

Sven F.
On Mon, Sep 14, 2020 at 9:45 AM Bob Beck <[hidden email]> wrote:

>
> But what if I like json and I am already set up to be a hipster and
> feed all the untrusted inputs through jq..
>
> (ok beck@)
>
> On Mon, Sep 14, 2020 at 03:37:25PM +0200, Florian Obser wrote:
> > not helpful:
> > $ doas acme-client $(hostname)
> > acme-client: https://api.test4.buypass.no/acme-v02/new-acct: bad HTTP:
> 400
> >
> > vomitting unformated json is not better:
> > $ doas acme-client -v $(hostname)
> > acme-client: transfer buffer:
> [{"type":"urn:ietf:params:acme:error:malformed","detail":"Email is a
> required
> contact","code":400,"message":"MALFORMED_BAD_REQUEST","details":"HTTP 400
> Bad Request"}] (164 bytes)
> >
> > let's do this:
> > $ doas obj/acme-client -v $(hostname)
> > acme-client: Email is a required contact
> >
> > OK?
> >
> > diff --git extern.h extern.h
> > index 529d3350205..364425b0500 100644
> > --- extern.h
> > +++ extern.h
> > @@ -259,6 +259,7 @@ int                json_parse_order(struct jsmnn *,
> struct order *);
> >  int           json_parse_upd_order(struct jsmnn *, struct order *);
> >  void          json_free_capaths(struct capaths *);
> >  int           json_parse_capaths(struct jsmnn *, struct capaths *);
> > +char         *json_getstr(struct jsmnn *, const char *);
> >
> >  char         *json_fmt_newcert(const char *);
> >  char         *json_fmt_chkacc(void);
> > diff --git json.c json.c
> > index 61d2631359f..a6762eeb258 100644
> > --- json.c
> > +++ json.c
> > @@ -297,7 +297,7 @@ json_getobj(struct jsmnn *n, const char *name)
> >   * that it's the correct type.
> >   * Returns NULL on failure.
> >   */
> > -static char *
> > +char *
> >  json_getstr(struct jsmnn *n, const char *name)
> >  {
> >       size_t           i;
> > diff --git netproc.c netproc.c
> > index 7b8152196d1..05e36897c38 100644
> > --- netproc.c
> > +++ netproc.c
> > @@ -371,15 +371,27 @@ sreq(struct conn *c, const char *addr, int kid,
> const char *req, char **loc)
> >  static int
> >  donewacc(struct conn *c, const struct capaths *p)
> >  {
> > +     struct jsmnn    *j = NULL;
> >       int              rc = 0;
> > -     char            *req;
> > +     char            *req, *detail, *error = NULL;
> >       long             lc;
> >
> >       if ((req = json_fmt_newacc()) == NULL)
> >               warnx("json_fmt_newacc");
> >       else if ((lc = sreq(c, p->newaccount, 0, req, &c->kid)) < 0)
> >               warnx("%s: bad comm", p->newaccount);
> > -     else if (lc != 200 && lc != 201)
> > +     else if (lc == 400) {
> > +             if ((j = json_parse(c->buf.buf, c->buf.sz)) == NULL)
> > +                     warnx("%s: bad JSON object", p->newaccount);
> > +             else {
> > +                     detail = json_getstr(j, "detail");
> > +                     if (detail != NULL && stravis(&error, detail,
> VIS_SAFE)
> > +                         != -1) {
> > +                             warnx("%s", error);
> > +                             free(error);
> > +                     }
> > +             }
> > +     } else if (lc != 200 && lc != 201)
> >               warnx("%s: bad HTTP: %ld", p->newaccount, lc);
> >       else if (c->buf.buf == NULL || c->buf.sz == 0)
> >               warnx("%s: empty response", p->newaccount);
> >
> >
> > --
> > I'm not entirely sure you are real.
> >
>
>
please dont drop the all buffer , or keep it with -vv ?
example : warnx("%s: bad JSON object:%s", p->newaccount, c->buf.buf);

i don't want to ktrace it to see why the new certbot version is not working

--
--
---------------------------------------------------------------------------------------------------------------------
Knowing is not enough; we must apply. Willing is not enough; we must do
Reply | Threaded
Open this post in threaded view
|

Re: acme-client: improve account creation error message

Rafael Possamai-2
>please dont drop the all buffer , or keep it with -vv ?
>example : warnx("%s: bad JSON object:%s", p->newaccount, c->buf.buf);
>
>i don't want to ktrace it to see why the new certbot version is not working

Yeah, I think it's good to have the choice to inspect the vomit, maybe you stumble upon some useful nuggets for troubleshooting an obscure issue.

I believe the JSON output is valid, just not pretty. Maybe we could add syntax highlighting? (just kidding)

Reply | Threaded
Open this post in threaded view
|

Re: acme-client: improve account creation error message

Florian Obser-2
On Mon, Sep 14, 2020 at 04:26:20PM -0500, Rafael Possamai wrote:
> >please dont drop the all buffer , or keep it with -vv ?
> >example : warnx("%s: bad JSON object:%s", p->newaccount, c->buf.buf);
> >
> >i don't want to ktrace it to see why the new certbot version is not working
>
> Yeah, I think it's good to have the choice to inspect the vomit, maybe you stumble upon some useful nuggets for troubleshooting an obscure issue.
>
> I believe the JSON output is valid, just not pretty. Maybe we could add syntax highlighting? (just kidding)
>

I don't understand what you people are going on about, I didn't touch
the -v output. You can still roll around in json.

It's not about if it's valid json or not, it's untrusted input that we
spit on your console when used with -v.

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

Reply | Threaded
Open this post in threaded view
|

Re: acme-client: improve account creation error message

Demi M. Obenour
On 2020-09-15 04:25, Florian Obser wrote:

> On Mon, Sep 14, 2020 at 04:26:20PM -0500, Rafael Possamai wrote:
>>> please dont drop the all buffer , or keep it with -vv ?
>>> example : warnx("%s: bad JSON object:%s", p->newaccount, c->buf.buf);
>>>
>>> i don't want to ktrace it to see why the new certbot version is not working
>>
>> Yeah, I think it's good to have the choice to inspect the vomit, maybe you stumble upon some useful nuggets for troubleshooting an obscure issue.
>>
>> I believe the JSON output is valid, just not pretty. Maybe we could add syntax highlighting? (just kidding)
>>
>
> I don't understand what you people are going on about, I didn't touch
> the -v output. You can still roll around in json.
>
> It's not about if it's valid json or not, it's untrusted input that we
> spit on your console when used with -v.

Simple fix: ensure that all control characters are represented as
Unicode escape sequences.  I believe that the JSON spec requires
this anyway.