Re: acme-client(1) and Buypass Go SSL

2020-04-25 Thread Bartosz Kuzma

I fixed warn() and changed certproc.c to be less confusing.

On 21/04/2020 21:17, Sebastian Benoit wrote:

Bartosz Kuzma(bartosz.ku...@release11.com) on 2020.04.21 20:59:54 +0200:

Hello,

thanks for looking at this!

On 21/04/2020 17:43, Florian Obser wrote:

Hi,

thanks for working on this and finding another acme implementor!

On Mon, Apr 20, 2020 at 06:51:17PM +0200, Bartosz Kuzma wrote:

Hello,

I've tried to get a certificate from Buypass Go SSL provider using
acme-client(1) but it ends with the following error:

acme-client: https://api.buypass.com/acme-v02/new-acct: bad HTTP: 400
acme-client: transfer buffer:
[{"type":"urn:ietf:params:acme:error:malformed","d
etail":"Email is a required
contact","code":400,"message":"MALFORMED_BAD_REQUEST
","details":"HTTP 400 Bad Request"}] (164 bytes)

Buypass Go SSL requires that optional field contact in Account Objects
will contain email address.

I've added new option "contact" to acme-client.conf to fullfil this
requirement. It is also required to change MARKER in certproc.c to fix
line
endings handling in pem files.


they are leaving out the last newline?


It seems that chain can be provided with "\n" or "\r\n" line separator.
I removed it from MARKER to handle both cases.





--
Kind regards, Bartosz Kuzma.



Index: usr.sbin/acme-client/acme-client.conf.5
===
RCS file: /cvs/src/usr.sbin/acme-client/acme-client.conf.5,v
retrieving revision 1.22
diff -u -p -r1.22 acme-client.conf.5
--- usr.sbin/acme-client/acme-client.conf.5 10 Feb 2020 13:18:21 -
1.22
+++ usr.sbin/acme-client/acme-client.conf.5 20 Apr 2020 16:24:46 -
@@ -98,6 +98,10 @@ It defaults to
  Specify the
  .Ar url
  under which the ACME API is reachable.
+.It Ic contact Ar contact
+Optional
+.Ar contact
+URLs that the authority can use to contact the client for issues related
to this account.


I think we should call it emails. That's what people actually have.
According to the RFC it's a list of contacts, but I'm fine with having
only one.


For me one contact is also enough.




  .El
  .Sh DOMAINS
  The certificates to be obtained through ACME.
Index: usr.sbin/acme-client/certproc.c
===
RCS file: /cvs/src/usr.sbin/acme-client/certproc.c,v
retrieving revision 1.12
diff -u -p -r1.12 certproc.c
--- usr.sbin/acme-client/certproc.c 7 Jun 2019 08:07:52 -   1.12
+++ usr.sbin/acme-client/certproc.c 20 Apr 2020 16:24:46 -
@@ -28,7 +28,7 @@
  
  #include "extern.h"
  
-#define MARKER "-END CERTIFICATE-\n"

+#define MARKER "-END CERTIFICATE-"
  
  int

  certproc(int netsock, int filesock)
@@ -94,6 +94,12 @@ certproc(int netsock, int filesock)
}
  
  	chaincp += strlen(MARKER);

+
+   if ((chaincp = strchr(chaincp, '-')) == NULL) {
+   warn("strchr");
+   goto out;
+   }
+


I don't quite understand what this is doing.


Because I removed new line character from MARKER strchr() just move
pointer to -BEGIN CERTIFICATE to skip any new lines character
from chain. It can be replaced by strstr("-BEGIN CERTIFICATE-")
if this improve readability.


The warn() should be a warnx() then, because strchr() does not set errno.
When it returns NULL it just means that it did not find the string. So

warnx("invalid certificate chain");

would be a good error message here i think.

Index: etc/examples/acme-client.conf
===
RCS file: /cvs/src/etc/examples/acme-client.conf,v
retrieving revision 1.2
diff -u -p -r1.2 acme-client.conf
--- etc/examples/acme-client.conf   7 Jun 2019 08:08:30 -   1.2
+++ etc/examples/acme-client.conf   25 Apr 2020 09:52:11 -
@@ -11,6 +11,18 @@ authority letsencrypt-staging {
account key "/etc/acme/letsencrypt-staging-privkey.pem"
 }
 
+authority buypass {
+api url "https://api.buypass.com/acme/directory;
+account key "/etc/acme/buypass-privkey.pem"
+contact "mailto:j...@example.net;
+}
+
+authority buypass-test {
+api url "https://api.test4.buypass.no/acme/directory;
+account key "/etc/acme/buypass-test-privkey.pem"
+contact "mailto:j...@example.net;
+}
+
 domain example.com {
alternative names { secure.example.com }
domain key "/etc/ssl/private/example.com.key"
Index: usr.sbin/acme-client/acme-client.conf.5
===
RCS file: /cvs/src/usr.sbin/acme-client/acme-client.conf.5,v
retrieving revision 1.22
diff -u -p -r1.22 acme-client.conf.5
--- usr.sbin/acme-client/acme-client.conf.5 10 Feb 2020 13:18:21 -  
1.22
+++ usr.s

Re: acme-client(1) and Buypass Go SSL

2020-04-21 Thread Bartosz Kuzma




On 21/04/2020 21:17, Sebastian Benoit wrote:

Bartosz Kuzma(bartosz.ku...@release11.com) on 2020.04.21 20:59:54 +0200:

Hello,

thanks for looking at this!

On 21/04/2020 17:43, Florian Obser wrote:

Hi,

thanks for working on this and finding another acme implementor!

On Mon, Apr 20, 2020 at 06:51:17PM +0200, Bartosz Kuzma wrote:

Hello,

I've tried to get a certificate from Buypass Go SSL provider using
acme-client(1) but it ends with the following error:

acme-client: https://api.buypass.com/acme-v02/new-acct: bad HTTP: 400
acme-client: transfer buffer:
[{"type":"urn:ietf:params:acme:error:malformed","d
etail":"Email is a required
contact","code":400,"message":"MALFORMED_BAD_REQUEST
","details":"HTTP 400 Bad Request"}] (164 bytes)

Buypass Go SSL requires that optional field contact in Account Objects
will contain email address.

I've added new option "contact" to acme-client.conf to fullfil this
requirement. It is also required to change MARKER in certproc.c to fix
line
endings handling in pem files.


they are leaving out the last newline?


It seems that chain can be provided with "\n" or "\r\n" line separator.
I removed it from MARKER to handle both cases.





--
Kind regards, Bartosz Kuzma.



Index: usr.sbin/acme-client/acme-client.conf.5
===
RCS file: /cvs/src/usr.sbin/acme-client/acme-client.conf.5,v
retrieving revision 1.22
diff -u -p -r1.22 acme-client.conf.5
--- usr.sbin/acme-client/acme-client.conf.5 10 Feb 2020 13:18:21 -
1.22
+++ usr.sbin/acme-client/acme-client.conf.5 20 Apr 2020 16:24:46 -
@@ -98,6 +98,10 @@ It defaults to
  Specify the
  .Ar url
  under which the ACME API is reachable.
+.It Ic contact Ar contact
+Optional
+.Ar contact
+URLs that the authority can use to contact the client for issues related
to this account.


I think we should call it emails. That's what people actually have.
According to the RFC it's a list of contacts, but I'm fine with having
only one.


For me one contact is also enough.




  .El
  .Sh DOMAINS
  The certificates to be obtained through ACME.
Index: usr.sbin/acme-client/certproc.c
===
RCS file: /cvs/src/usr.sbin/acme-client/certproc.c,v
retrieving revision 1.12
diff -u -p -r1.12 certproc.c
--- usr.sbin/acme-client/certproc.c 7 Jun 2019 08:07:52 -   1.12
+++ usr.sbin/acme-client/certproc.c 20 Apr 2020 16:24:46 -
@@ -28,7 +28,7 @@
  
  #include "extern.h"
  
-#define MARKER "-END CERTIFICATE-\n"

+#define MARKER "-END CERTIFICATE-"
  
  int

  certproc(int netsock, int filesock)
@@ -94,6 +94,12 @@ certproc(int netsock, int filesock)
}
  
  	chaincp += strlen(MARKER);

+
+   if ((chaincp = strchr(chaincp, '-')) == NULL) {
+   warn("strchr");
+   goto out;
+   }
+


I don't quite understand what this is doing.


Because I removed new line character from MARKER strchr() just move
pointer to -BEGIN CERTIFICATE to skip any new lines character
from chain. It can be replaced by strstr("-BEGIN CERTIFICATE-")
if this improve readability.


The warn() should be a warnx() then, because strchr() does not set errno.
When it returns NULL it just means that it did not find the string. So

warnx("invalid certificate chain");

would be a good error message here i think.



Thanks for pointing it. I was not aware of that.

Kind bartosz Kuzma.



Re: acme-client(1) and Buypass Go SSL

2020-04-21 Thread Bartosz Kuzma

Hello,

thanks for looking at this!

On 21/04/2020 17:43, Florian Obser wrote:

Hi,

thanks for working on this and finding another acme implementor!

On Mon, Apr 20, 2020 at 06:51:17PM +0200, Bartosz Kuzma wrote:

Hello,

I've tried to get a certificate from Buypass Go SSL provider using
acme-client(1) but it ends with the following error:

acme-client: https://api.buypass.com/acme-v02/new-acct: bad HTTP: 400
acme-client: transfer buffer:
[{"type":"urn:ietf:params:acme:error:malformed","d
etail":"Email is a required
contact","code":400,"message":"MALFORMED_BAD_REQUEST
","details":"HTTP 400 Bad Request"}] (164 bytes)

Buypass Go SSL requires that optional field contact in Account Objects
will contain email address.

I've added new option "contact" to acme-client.conf to fullfil this
requirement. It is also required to change MARKER in certproc.c to fix line
endings handling in pem files.


they are leaving out the last newline?


It seems that chain can be provided with "\n" or "\r\n" line separator. 
I removed it from MARKER to handle both cases.






--
Kind regards, Bartosz Kuzma.



Index: usr.sbin/acme-client/acme-client.conf.5
===
RCS file: /cvs/src/usr.sbin/acme-client/acme-client.conf.5,v
retrieving revision 1.22
diff -u -p -r1.22 acme-client.conf.5
--- usr.sbin/acme-client/acme-client.conf.5 10 Feb 2020 13:18:21 -  
1.22
+++ usr.sbin/acme-client/acme-client.conf.5 20 Apr 2020 16:24:46 -
@@ -98,6 +98,10 @@ It defaults to
  Specify the
  .Ar url
  under which the ACME API is reachable.
+.It Ic contact Ar contact
+Optional
+.Ar contact
+URLs that the authority can use to contact the client for issues related to 
this account.


I think we should call it emails. That's what people actually have.
According to the RFC it's a list of contacts, but I'm fine with having only one.


For me one contact is also enough.




  .El
  .Sh DOMAINS
  The certificates to be obtained through ACME.
Index: usr.sbin/acme-client/certproc.c
===
RCS file: /cvs/src/usr.sbin/acme-client/certproc.c,v
retrieving revision 1.12
diff -u -p -r1.12 certproc.c
--- usr.sbin/acme-client/certproc.c 7 Jun 2019 08:07:52 -   1.12
+++ usr.sbin/acme-client/certproc.c 20 Apr 2020 16:24:46 -
@@ -28,7 +28,7 @@
  
  #include "extern.h"
  
-#define MARKER "-END CERTIFICATE-\n"

+#define MARKER "-END CERTIFICATE-"
  
  int

  certproc(int netsock, int filesock)
@@ -94,6 +94,12 @@ certproc(int netsock, int filesock)
}
  
  	chaincp += strlen(MARKER);

+
+   if ((chaincp = strchr(chaincp, '-')) == NULL) {
+   warn("strchr");
+   goto out;
+   }
+


I don't quite understand what this is doing.

Because I removed new line character from MARKER strchr() just move 
pointer to -BEGIN CERTIFICATE to skip any new lines character 
from chain. It can be replaced by strstr("-BEGIN CERTIFICATE-") 
if this improve readability.



if ((chain = strdup(chaincp)) == NULL) {
warn("strdup");
goto out;
Index: usr.sbin/acme-client/json.c
===
RCS file: /cvs/src/usr.sbin/acme-client/json.c,v
retrieving revision 1.16
diff -u -p -r1.16 json.c
--- usr.sbin/acme-client/json.c 22 Jan 2020 22:25:22 -  1.16
+++ usr.sbin/acme-client/json.c 20 Apr 2020 16:24:46 -
@@ -616,19 +616,30 @@ json_fmt_chkacc(void)
   * Format the "newAccount" resource request.
   */
  char *
-json_fmt_newacc(void)
+json_fmt_newacc(const char *contact)
  {
int  c;
-   char*p;
+   char*p, *cnt;
+
+   c = asprintf(, "\"contact\": [ \"%s\" ], ", contact);
+   if (c == -1) {
+   warn("asprintf");
+   goto err;
+   }
  


bzzzrt, you can't do this. contact might be NULL, you already need to
check it here, not just further down.


You are right! I've improved json_fmt_newacc function and it should be 
good now.



In this particular case I think it's fine do something like this, makes the 
error handling easier, you don't need a goto etc.

if (contact == NULL) {
p = strdup("{"
"\"termsOfServiceAgreed\": true"
"}");
} else {
c = asprintf(, "{"
"\"contact\": [ \"mailto:%s\; ], "
"\"termsOfServiceAgreed\": true"
"}", contact);
if (c == -1)
p = NULL;
}

yes, we have two places n

acme-client(1) and Buypass Go SSL

2020-04-20 Thread Bartosz Kuzma

Hello,

I've tried to get a certificate from Buypass Go SSL provider using
acme-client(1) but it ends with the following error:

acme-client: https://api.buypass.com/acme-v02/new-acct: bad HTTP: 400
acme-client: transfer buffer: 
[{"type":"urn:ietf:params:acme:error:malformed","d
etail":"Email is a required 
contact","code":400,"message":"MALFORMED_BAD_REQUEST

","details":"HTTP 400 Bad Request"}] (164 bytes)

Buypass Go SSL requires that optional field contact in Account Objects
will contain email address.

I've added new option "contact" to acme-client.conf to fullfil this 
requirement. It is also required to change MARKER in certproc.c to fix 
line endings handling in pem files.


--
Kind regards, Bartosz Kuzma.
Index: usr.sbin/acme-client/acme-client.conf.5
===
RCS file: /cvs/src/usr.sbin/acme-client/acme-client.conf.5,v
retrieving revision 1.22
diff -u -p -r1.22 acme-client.conf.5
--- usr.sbin/acme-client/acme-client.conf.5 10 Feb 2020 13:18:21 -  
1.22
+++ usr.sbin/acme-client/acme-client.conf.5 20 Apr 2020 16:24:46 -
@@ -98,6 +98,10 @@ It defaults to
 Specify the
 .Ar url
 under which the ACME API is reachable.
+.It Ic contact Ar contact
+Optional
+.Ar contact
+URLs that the authority can use to contact the client for issues related to 
this account.
 .El
 .Sh DOMAINS
 The certificates to be obtained through ACME.
Index: usr.sbin/acme-client/certproc.c
===
RCS file: /cvs/src/usr.sbin/acme-client/certproc.c,v
retrieving revision 1.12
diff -u -p -r1.12 certproc.c
--- usr.sbin/acme-client/certproc.c 7 Jun 2019 08:07:52 -   1.12
+++ usr.sbin/acme-client/certproc.c 20 Apr 2020 16:24:46 -
@@ -28,7 +28,7 @@
 
 #include "extern.h"
 
-#define MARKER "-END CERTIFICATE-\n"
+#define MARKER "-END CERTIFICATE-"
 
 int
 certproc(int netsock, int filesock)
@@ -94,6 +94,12 @@ certproc(int netsock, int filesock)
}
 
chaincp += strlen(MARKER);
+
+   if ((chaincp = strchr(chaincp, '-')) == NULL) {
+   warn("strchr");
+   goto out;
+   }
+
if ((chain = strdup(chaincp)) == NULL) {
warn("strdup");
goto out;
Index: usr.sbin/acme-client/extern.h
===
RCS file: /cvs/src/usr.sbin/acme-client/extern.h,v
retrieving revision 1.17
diff -u -p -r1.17 extern.h
--- usr.sbin/acme-client/extern.h   7 Feb 2020 14:34:15 -   1.17
+++ usr.sbin/acme-client/extern.h   20 Apr 2020 16:24:46 -
@@ -261,13 +261,13 @@ intjson_parse_capaths(struct jsmnn *,
 
 char   *json_fmt_newcert(const char *);
 char   *json_fmt_chkacc(void);
-char   *json_fmt_newacc(void);
+char   *json_fmt_newacc(const char *);
 char   *json_fmt_neworder(const char *const *, size_t);
 char   *json_fmt_protected_rsa(const char *,
const char *, const char *, const char *);
 char   *json_fmt_protected_ec(const char *, const char *, const char *,
const char *);
-char   *json_fmt_protected_kid(const char*, const char *, const char *,
+char   *json_fmt_protected_kid(const char *, const char *, const char 
*,
const char *);
 char   *json_fmt_revokecert(const char *);
 char   *json_fmt_thumb_rsa(const char *, const char *);
Index: usr.sbin/acme-client/json.c
===
RCS file: /cvs/src/usr.sbin/acme-client/json.c,v
retrieving revision 1.16
diff -u -p -r1.16 json.c
--- usr.sbin/acme-client/json.c 22 Jan 2020 22:25:22 -  1.16
+++ usr.sbin/acme-client/json.c 20 Apr 2020 16:24:46 -
@@ -616,19 +616,30 @@ json_fmt_chkacc(void)
  * Format the "newAccount" resource request.
  */
 char *
-json_fmt_newacc(void)
+json_fmt_newacc(const char *contact)
 {
int  c;
-   char*p;
+   char*p, *cnt;
+
+   c = asprintf(, "\"contact\": [ \"%s\" ], ", contact);
+   if (c == -1) {
+   warn("asprintf");
+   goto err;
+   }
 
c = asprintf(, "{"
+   "%s"
"\"termsOfServiceAgreed\": true"
-   "}");
+   "}", contact == NULL ? "" : cnt);
+   free(cnt);
if (c == -1) {
warn("asprintf");
p = NULL;
}
return p;
+
+err:
+   return NULL;
 }
 
 /*
Index: usr.sbin/acme-client/netproc.c
===
RCS file: /cvs/src/usr.sbin/acm