On Thu, Jun 23, 2016 at 07:52:06PM +0300, Kapetanakis Giannis wrote:
> On 23/06/16 18:14, Kapetanakis Giannis wrote:
> > It adds two switches:
> > -c client_cert_file
> > -k client_key_file
That's fine.
> > Minor modification in CAfile setup as well to match the netcat code.
Please do not change that now. There is a diff for libtls and
syslogd floating around that will make the code much simpler.
> #define DEFUPRI (LOG_USER|LOG_NOTICE)
> #define DEFSPRI (LOG_KERN|LOG_CRIT)
> #define TIMERINTVL 30 /* interval for checking flush, mark */
> +#define DEFAULT_CA_FILE "/etc/ssl/cert.pem"
> #include <sys/ioctl.h>
> #include <sys/stat.h>
> @@ -223,8 +224,16 @@ char *path_ctlsock = NULL; /* Path to co
> struct tls *server_ctx;
> struct tls_config *client_config, *server_config;
> -const char *CAfile = "/etc/ssl/cert.pem"; /* file containing CA certificates
> */
> -int NoVerify = 0; /* do not verify TLS server x509 certificate */
> +int NoVerify = 0; /* verify TLS server x509 certificate */
> +char *CAfile = DEFAULT_CA_FILE; /* file containing CA certificates */
Do not change that stuff in this diff, it is unrelated.
> +char *PubCertfile = NULL; /* file containing public certificate */
> +char *PrivKeyfile = NULL; /* file containing private key */
It is obvious that a cert is public and a key is private. The names
could be ClientCertfile and ClientKeyfile to make clear for which
side they are.
> +uint8_t *cacert;
> +size_t cacertlen;
> +uint8_t *privkey;
> +size_t privkeylen;
> +uint8_t *pubcert;
> +size_t pubcertlen;
These variables should not be global. Declare them in the block
where they are used.
> @@ -553,34 +568,33 @@ main(int argc, char *argv[])
> tls_config_insecure_noverifycert(client_config);
> tls_config_insecure_noverifyname(client_config);
> } else {
> - struct stat sb;
> int fail = 1;
> - fd = -1;
> - p = NULL;
> - if ((fd = open(CAfile, O_RDONLY)) == -1) {
> - logerror("open CAfile");
> - } else if (fstat(fd, &sb) == -1) {
> - logerror("fstat CAfile");
> - } else if (sb.st_size > 50*1024*1024) {
> - logerrorx("CAfile larger than 50MB");
> - } else if ((p = calloc(sb.st_size, 1)) == NULL) {
> - logerror("calloc CAfile");
> - } else if (read(fd, p, sb.st_size) != sb.st_size) {
> - logerror("read CAfile");
> - } else if (tls_config_set_ca_mem(client_config, p,
> - sb.st_size) == -1) {
> - logerrorx("tls_config_set_ca_mem");
> - } else {
> + if ((cacert = tls_load_file(CAfile, &cacertlen, NULL))
> == NULL)
> + logerror("load CAfile");
> + if (tls_config_set_ca_mem(client_config, cacert,
> cacertlen) == -1)
> + logerror("set CAfile");
> + else {
> fail = 0;
> - logdebug("CAfile %s, size %lld\n",
> - CAfile, sb.st_size);
> + logdebug("CAfile %s\n", CAfile);
> }
> /* avoid reading default certs in chroot */
> if (fail)
> tls_config_set_ca_mem(client_config, "", 0);
> - free(p);
> - close(fd);
> + }
As mentioned above, do not touch this code in this diff.
> + if (PubCertfile && PrivKeyfile) {
> + if ((pubcert = tls_load_file(PubCertfile, &pubcertlen,
> NULL)) == NULL)
> + errx(1, "unable to load TLS certificate file
> %s", PubCertfile);
> + if (tls_config_set_cert_mem(client_config, pubcert,
> pubcertlen) == -1)
> + errx(1, "unable to set TLS certificate file
> %s", PubCertfile);
> + else
> + logdebug("TLS certificate %s\n", PubCertfile);
> + if ((privkey = tls_load_file(PrivKeyfile, &privkeylen,
> NULL)) == NULL)
> + errx(1, "unable to load TLS key file %s",
> PrivKeyfile);
> + if (tls_config_set_key_mem(client_config, privkey,
> privkeylen) == -1)
> + errx(1, "unable to set TLS key file %s",
> PrivKeyfile);
> + else
> + logdebug("TLS key %s\n", PrivKeyfile);
Your lines are longer than 80 characters. Make them shorter.
Syslogd errors are not fatal in general. Logging should always run
as far as possible. Do not use errx, the other functions have
logerrorx().
> }
Can we have an else if (PubCertfile || PrivKeyfile) that prints a warning
if -c and -k are not used together?
You e-mail client adds an additional space at the beginning of the
context lines of the diff. So it does not apply.
#define DEFUPRI (LOG_USER|LOG_NOTICE)
#define DEFSPRI (LOG_KERN|LOG_CRIT)
#define TIMERINTVL 30 /* interval for checking flush, mark */
+#define DEFAULT_CA_FILE "/etc/ssl/cert.pem"
bluhm
