~sircmpwn/gmni-devel

This thread contains a patchset. You're looking at the original emails, but you may wish to use the patch review UI. Review patch
9 2

[RFC PATCH] WIP: client side certificates

Details
Message ID
<20210304222713.31280-1-sir@cmpwn.com>
DKIM signature
pass
Download raw message
Patch: +435 -9
---
This is a WIP patch which implements client-side certificates, at first
only for gmni, but later for gmnlm as well. Posting to the mailing list
for general feedback, but also in case there's any BearSSL users around
who might be able to help work out the bugs - it doesn't actually appear
to work. BR_ERR_INVALID_ALGORITHM.

Also, BearSSL lacks support for creating new certificates, so automatic
certificate creation will have to be postponed. I might actually end up
ditching BearSSL over this, mere hours after I decided to switch to it,
which I'm rather annoyed about.

Cc ~mcf, I know you're familiar with BearSSL - thoughts?

 configure            |   1 +
 doc/gmni.scd         |   7 +-
 include/gmni/certs.h |  10 ++
 include/gmni/gmni.h  |   4 +
 src/certs.c          | 369 +++++++++++++++++++++++++++++++++++++++++++
 src/client.c         |   5 +
 src/gmni.c           |  48 +++++-
 7 files changed, 435 insertions(+), 9 deletions(-)
 create mode 100644 include/gmni/certs.h
 create mode 100644 src/certs.c

diff --git a/configure b/configure
index e82a0e2..8d3b67a 100755
--- a/configure
+++ b/configure
@@ -4,6 +4,7 @@ eval ". $srcdir/config.sh"

gmni() {
	genrules gmni \
		src/certs.c \
		src/client.c \
		src/escape.c \
		src/gmni.c \
diff --git a/doc/gmni.scd b/doc/gmni.scd
index 1f20672..c3bf8ce 100644
--- a/doc/gmni.scd
+++ b/doc/gmni.scd
@@ -38,11 +38,8 @@ performed with the user's input supplied to the server.
	second request is performed with the contents of _path_ as the user
	input.

*-E* _path_[:_password_]
	Sets the path to the client certificate to use (and optionally a
	password). If the filename contains ":" but the certificate does not
	accept a password, append ":" to the path and it will be intepreted as
	an empty password.
*-E* _path_:_key_
	Sets the path to the client certificate and priavte key file to use.

*-l*
	For *text/\** responses, *gmni* normally adds a line feed if stdout is a
diff --git a/include/gmni/certs.h b/include/gmni/certs.h
new file mode 100644
index 0000000..58a8955
--- /dev/null
+++ b/include/gmni/certs.h
@@ -0,0 +1,10 @@
#ifndef GEMINI_CERTS_H
#define GEMINI_CERTS_H
#include <bearssl_ssl.h>
#include <bearssl_x509.h>
#include <stdio.h>

const br_ssl_client_certificate_class **gmni_ccert_load(
		FILE *certin, FILE *skin);

#endif
diff --git a/include/gmni/gmni.h b/include/gmni/gmni.h
index 16bef51..4bef997 100644
--- a/include/gmni/gmni.h
+++ b/include/gmni/gmni.h
@@ -69,6 +69,10 @@ struct gemini_options {
	// If non-NULL, these hints are provided to getaddrinfo. Useful, for
	// example, to force IPv4/IPv6.
	struct addrinfo *hints;

	// If non-NULL, this will be used as the client certificate for the
	// request.
	const br_ssl_client_certificate_class **client_cert;
};

struct gemini_tofu;
diff --git a/src/certs.c b/src/certs.c
new file mode 100644
index 0000000..56023ae
--- /dev/null
+++ b/src/certs.c
@@ -0,0 +1,369 @@
#include <assert.h>
#include <bearssl_hash.h>
#include <bearssl_pem.h>
#include <bearssl_ssl.h>
#include <bearssl_x509.h>
#include <errno.h>
#include <gmni/certs.h>
#include <stdio.h>
#include <stdlib.h>

struct gmni_ccert_ctx {
	const br_ssl_client_certificate_class *vtable;
	br_x509_certificate *chain;
	size_t nchain;
	int key_type;
	union {
		const br_rsa_private_key *rsa;
		const br_ec_private_key *ec;
	};
};

// These are intentionally left blank: we always know in advance which cert
// we're going to use.
static void
cc_start_name_list(const br_ssl_client_certificate_class **pctx)
{
	(void)pctx;
}

static void
cc_start_name(const br_ssl_client_certificate_class **pctx, size_t len)
{
	(void)pctx; (void)len;
}

static void
cc_append_name(const br_ssl_client_certificate_class **pctx,
	const unsigned char *data, size_t len)
{
	(void)pctx; (void)data; (void)len;
}

static void
cc_end_name(const br_ssl_client_certificate_class **pctx)
{
	(void)pctx;
}

static void
cc_end_name_list(const br_ssl_client_certificate_class **pctx)
{
	(void)pctx;
}

static int
choose_hash(unsigned hh)
{
	static const int f[] = {
		br_sha256_ID, br_sha224_ID, br_sha384_ID, br_sha512_ID,
		br_sha1_ID, br_md5sha1_ID, -1
	};

	size_t u;
	for (u = 0; f[u] >= 0; u ++) {
		if (((hh >> f[u]) & 1) != 0) {
			return f[u];
		}
	}
	return -1;
}

void
cc_choose(const br_ssl_client_certificate_class **pctx,
	const br_ssl_client_context *cc, uint32_t auth_types,
	br_ssl_client_certificate *choices)
{
	struct gmni_ccert_ctx *ctx = (struct gmni_ccert_ctx *)(void *)pctx;
	switch (ctx->key_type) {
	case BR_KEYTYPE_RSA:
		if ((choices->hash_id = choose_hash(auth_types)) >= 0) {
			choices->auth_type = BR_AUTH_RSA;
			choices->chain = ctx->chain;
			choices->chain_len = ctx->nchain;
			return;
		}
		abort();
	case BR_KEYTYPE_EC:
		if ((choices->hash_id = choose_hash(auth_types >> 8)) >= 0) {
			choices->auth_type = BR_AUTH_ECDSA;
			choices->chain = ctx->chain;
			choices->chain_len = ctx->nchain;
			return;
		}
		abort();
	}
	(void)pctx; (void)cc;
	abort();
}

uint32_t 
cc_do_keyx(const br_ssl_client_certificate_class **pctx,
	unsigned char *data, size_t *len)
{
	struct gmni_ccert_ctx *ctx = (struct gmni_ccert_ctx *)(void *)pctx;
	const br_ec_impl *iec = br_ec_get_default();
	uint32_t r = iec->mul(data, *len,
		ctx->ec->x, ctx->ec->xlen, ctx->ec->curve);
	size_t xlen;
	size_t xoff = iec->xoff(ctx->ec->curve, &xlen);
	memmove(data, data + xoff, xlen);
	*len = xlen;
	return r;
}

static const unsigned char HASH_OID_SHA1[] = {
	0x05, 0x2B, 0x0E, 0x03, 0x02, 0x1A
};

static const unsigned char HASH_OID_SHA224[] = {
	0x09, 0x60, 0x86, 0x48, 0x01, 0x65, 0x03, 0x04, 0x02, 0x04
};

static const unsigned char HASH_OID_SHA256[] = {
	0x09, 0x60, 0x86, 0x48, 0x01, 0x65, 0x03, 0x04, 0x02, 0x01
};

static const unsigned char HASH_OID_SHA384[] = {
	0x09, 0x60, 0x86, 0x48, 0x01, 0x65, 0x03, 0x04, 0x02, 0x02
};

static const unsigned char HASH_OID_SHA512[] = {
	0x09, 0x60, 0x86, 0x48, 0x01, 0x65, 0x03, 0x04, 0x02, 0x03
};

static const unsigned char *HASH_OID[] = {
	HASH_OID_SHA1,
	HASH_OID_SHA224,
	HASH_OID_SHA256,
	HASH_OID_SHA384,
	HASH_OID_SHA512
};

static const unsigned char *
get_hash_oid(int id)
{
	if (id >= 2 && id <= 6) {
		return HASH_OID[id - 2];
	} else {
		return NULL;
	}
}

typedef struct {
	const char *name;
	const br_hash_class *hclass;
	const char *comment;
} hash_function;

const hash_function hash_functions[] = {
	{ "md5",     &br_md5_vtable,     "MD5" },
	{ "sha1",    &br_sha1_vtable,    "SHA-1" },
	{ "sha224",  &br_sha224_vtable,  "SHA-224" },
	{ "sha256",  &br_sha256_vtable,  "SHA-256" },
	{ "sha384",  &br_sha384_vtable,  "SHA-384" },
	{ "sha512",  &br_sha512_vtable,  "SHA-512" },
	{ NULL, 0, NULL }
};

static const br_hash_class *
get_hash_impl(int hash_id)
{
	size_t u;

	if (hash_id == 0) {
		return &br_md5sha1_vtable;
	}
	for (u = 0; hash_functions[u].name; u ++) {
		const br_hash_class *hc;
		int id;

		hc = hash_functions[u].hclass;
		id = (hc->desc >> BR_HASHDESC_ID_OFF) & BR_HASHDESC_ID_MASK;
		if (id == hash_id) {
			return hc;
		}
	}
	return NULL;
}

size_t
cc_do_sign(const br_ssl_client_certificate_class **pctx,
	int hash_id, size_t hv_len, unsigned char *data, size_t len)
{
	struct gmni_ccert_ctx *ctx = (struct gmni_ccert_ctx *)(void *)pctx;
	unsigned char hv[64];

	const br_hash_class *hc;
	const unsigned char *hash_oid;
	uint32_t x;
	size_t sig_len;
	assert(hv_len < sizeof(hv));
	memcpy(hv, data, hv_len);
	switch (ctx->key_type) {
	case BR_KEYTYPE_RSA:
		hash_oid = get_hash_oid(hash_id);
		if (hash_oid == NULL && hash_id != 0) {
			return 0;
		}
		sig_len = (ctx->rsa->n_bitlen + 7) >> 3;
		if (len < sig_len) {
			return 0;
		}
		x = br_rsa_pkcs1_sign_get_default()(
			hash_oid, hv, hv_len, ctx->rsa, data);
		if (!x) {
			return 0;
		}
		return sig_len;
	case BR_KEYTYPE_EC:
		hc = get_hash_impl(hash_id);
		if (hc == NULL) {
			return 0;
		}
		if (len < 139) {
			return 0;
		}
		sig_len = br_ecdsa_sign_asn1_get_default()(
			br_ec_get_default(), hc, hv, ctx->ec, data);
		if (sig_len == 0) {
			return 0;
		}
		return sig_len;
	}
	abort();
	(void)pctx;
}

static const br_ssl_client_certificate_class ccert_class = {
	sizeof(struct gmni_ccert_ctx),
	cc_start_name_list,
	cc_start_name,
	cc_append_name,
	cc_end_name,
	cc_end_name_list,
	cc_choose,
	cc_do_keyx,
	cc_do_sign,
};

static void
crt_append(void *ctx, const void *src, size_t len)
{
	br_x509_certificate *crt = (br_x509_certificate *)ctx;
	crt->data = realloc(crt->data, crt->data_len + len);
	assert(crt->data);
	memcpy(&crt->data[crt->data_len], src, len);
	crt->data_len += len;
}

static void
key_append(void *ctx, const void *src, size_t len)
{
	br_skey_decoder_context *skctx = (br_skey_decoder_context *)ctx;
	br_skey_decoder_push(skctx, src, len);
}

const br_ssl_client_certificate_class **
gmni_ccert_load(FILE *certin, FILE *skin)
{
	// TODO: Better error propagation to caller
	static unsigned char buf[BUFSIZ];
	struct gmni_ccert_ctx *ctx = calloc(1, sizeof(struct gmni_ccert_ctx));

	br_pem_decoder_context pemdec;
	br_pem_decoder_init(&pemdec);

	ctx->chain = NULL;
	ctx->nchain = 0;

	static const char *certname = "CERTIFICATE";
	while (!feof(certin)) {
		int n = fread(&buf, 1, sizeof(buf), certin);
		if (n < 0) {
			goto error;
		}
		int q = 0;
		while (q < n) {
			q += br_pem_decoder_push(&pemdec, &buf[q], n - q);
			switch (br_pem_decoder_event(&pemdec)) {
			case BR_PEM_BEGIN_OBJ:
				if (strcmp(br_pem_decoder_name(&pemdec), certname) != 0) {
					break;
				}
				ctx->chain = realloc(ctx->chain,
					sizeof(br_x509_certificate) * (ctx->nchain + 1));
				memset(&ctx->chain[ctx->nchain], 0, sizeof(*ctx->chain));
				br_pem_decoder_setdest(&pemdec, &crt_append,
						&ctx->chain[ctx->nchain]);
				++ctx->nchain;
				break;
			case BR_PEM_END_OBJ:
				break;
			case BR_PEM_ERROR:
				fprintf(stderr, "Error decoding PEM certificate\n");
				errno = EINVAL;
				goto error;
			}
		}
	}
	assert(ctx->nchain != 0);

	br_skey_decoder_context skdec = {0};
	br_skey_decoder_init(&skdec);
	br_pem_decoder_init(&pemdec);

	// TODO: Better validation of PEM file
	while (!feof(skin)) {
		int n = fread(&buf, 1, sizeof(buf), skin);
		if (n < 0) {
			goto error;
		}
		int q = 0;
		while (q < n) {
			q += br_pem_decoder_push(&pemdec, &buf[q], n - q);
			switch (br_pem_decoder_event(&pemdec)) {
			case BR_PEM_BEGIN_OBJ:
				br_pem_decoder_setdest(&pemdec, &key_append, &skdec);
				break;
			case BR_PEM_END_OBJ:
				// no-op
				break;
			case BR_PEM_ERROR:
				fprintf(stderr, "Error decoding PEM private key\n");
				errno = EINVAL;
				return NULL;
			}
		}
	}

	int err = br_skey_decoder_last_error(&skdec);
	if (err != 0) {
		fprintf(stderr, "Error loading private key: %d\n", err);
		errno = EINVAL;
		goto error;
	}
	ctx->key_type = br_skey_decoder_key_type(&skdec);
	switch (ctx->key_type) {
	case BR_KEYTYPE_RSA:
		ctx->rsa = br_skey_decoder_get_rsa(&skdec);
		break;
	case BR_KEYTYPE_EC:
		ctx->ec = br_skey_decoder_get_ec(&skdec);
		break;
	default:
		assert(0);
	}

	fclose(certin);
	fclose(skin);
	ctx->vtable = &ccert_class;
	return &ctx->vtable;

error:
	fclose(certin);
	fclose(skin);
	free(ctx->chain);
	free(ctx);
	return NULL;
}
diff --git a/src/client.c b/src/client.c
index e402cc9..215c351 100644
--- a/src/client.c
+++ b/src/client.c
@@ -169,7 +169,12 @@ gemini_request(const char *url, struct gemini_options *options,

	// TODO: session reuse
	resp->sc = &tofu->sc;
	if (options->client_cert) {
		br_ssl_client_set_client_certificate(
			resp->sc, options->client_cert);
	}
	br_ssl_client_reset(resp->sc, host, 0);

	br_sslio_init(&resp->body, &resp->sc->eng,
		sock_read, &resp->fd, sock_write, &resp->fd);

diff --git a/src/gmni.c b/src/gmni.c
index a8321d0..1b2182d 100644
--- a/src/gmni.c
+++ b/src/gmni.c
@@ -11,6 +11,7 @@
#include <sys/types.h>
#include <termios.h>
#include <unistd.h>
#include <gmni/certs.h>
#include <gmni/gmni.h>
#include <gmni/tofu.h>
#include <gmni/url.h>
@@ -109,6 +110,45 @@ tofu_callback(enum tofu_error error, const char *fingerprint,
	return action;
}

static const br_ssl_client_certificate_class **
load_client_cert(char *argv_0, char *path)
{
	char *certpath = strtok(path, ":");
	if (!certpath) {
		usage(argv_0);
		exit(1);
	}

	FILE *certf = fopen(certpath, "r");
	if (!certf) {
		fprintf(stderr, "Failed to open certificate: %s\n",
				strerror(errno));
		exit(1);
	}

	char *keypath = strtok(NULL, ":");
	if (!keypath) {
		usage(argv_0);
		exit(1);
	}

	FILE *keyf = fopen(keypath, "r");
	if (!keyf) {
		fprintf(stderr, "Failed to open certificate: %s\n",
				strerror(errno));
		exit(1);
	}

	const br_ssl_client_certificate_class **ccert =
		gmni_ccert_load(certf, keyf);
	if (!ccert) {
		fprintf(stderr, "Failed to load client certificate: %s\n",
				strerror(errno));
		exit(1);
	}
	return ccert;
}

int
main(int argc, char *argv[])
{
@@ -165,7 +205,7 @@ main(int argc, char *argv[])
			}
			break;
		case 'E':
			assert(0); // TODO: Client certificates
			opts.client_cert = load_client_cert(argv[0], optarg);
			break;
		case 'h':
			usage(argv[0]);
@@ -226,7 +266,7 @@ main(int argc, char *argv[])
	bool exit = false;
	struct Curl_URL *url = curl_url();

	if(curl_url_set(url, CURLUPART_URL, argv[optind], 0) != CURLUE_OK) {
	if (curl_url_set(url, CURLUPART_URL, argv[optind], 0) != CURLUE_OK) {
		// TODO: Better error
		fprintf(stderr, "Error: invalid URL\n");
		return 1;
@@ -238,8 +278,8 @@ main(int argc, char *argv[])
		curl_url_get(url, CURLUPART_URL, &buf, 0);

		struct gemini_response resp;
		enum gemini_result r = gemini_request(
				buf, &opts, &cfg.tofu, &resp);
		enum gemini_result r = gemini_request(buf,
			&opts, &cfg.tofu, &resp);

		free(buf);

-- 
2.30.1
Details
Message ID
<2I6XAGAWXZGAB.2VLLBP29N0KN8@mforney.org>
In-Reply-To
<20210304222713.31280-1-sir@cmpwn.com> (view parent)
DKIM signature
missing
Download raw message
Drew DeVault <sir@cmpwn.com> wrote:
> This is a WIP patch which implements client-side certificates, at first
> only for gmni, but later for gmnlm as well. Posting to the mailing list
> for general feedback, but also in case there's any BearSSL users around
> who might be able to help work out the bugs - it doesn't actually appear
> to work. BR_ERR_INVALID_ALGORITHM.
> 
> Also, BearSSL lacks support for creating new certificates, so automatic
> certificate creation will have to be postponed. I might actually end up
> ditching BearSSL over this, mere hours after I decided to switch to it,
> which I'm rather annoyed about.

I think it wouldn't be too difficult to write a small utility/library
to wrap a public key in a self-signed certificate. This would be
quite useful even outside the context of gemini. It should just be
a matter of constructing the appropriate ASN.1 structure (though
I'm not quite sure what that looks like).

I agree that this is an annoying shortcoming with BearSSL.

> Cc ~mcf, I know you're familiar with BearSSL - thoughts?
> 
>  configure            |   1 +
>  doc/gmni.scd         |   7 +-
>  include/gmni/certs.h |  10 ++
>  include/gmni/gmni.h  |   4 +
>  src/certs.c          | 369 +++++++++++++++++++++++++++++++++++++++++++
>  src/client.c         |   5 +
>  src/gmni.c           |  48 +++++-
>  7 files changed, 435 insertions(+), 9 deletions(-)
>  create mode 100644 include/gmni/certs.h
>  create mode 100644 src/certs.c
> 
> diff --git a/configure b/configure
> index e82a0e2..8d3b67a 100755
> --- a/configure
> +++ b/configure
> @@ -4,6 +4,7 @@ eval ". $srcdir/config.sh"
>  
>  gmni() {
>  	genrules gmni \
> +		src/certs.c \
>  		src/client.c \
>  		src/escape.c \
>  		src/gmni.c \
> diff --git a/doc/gmni.scd b/doc/gmni.scd
> index 1f20672..c3bf8ce 100644
> --- a/doc/gmni.scd
> +++ b/doc/gmni.scd
> @@ -38,11 +38,8 @@ performed with the user's input supplied to the server.
>  	second request is performed with the contents of _path_ as the user
>  	input.
>  
> -*-E* _path_[:_password_]
> -	Sets the path to the client certificate to use (and optionally a
> -	password). If the filename contains ":" but the certificate does not
> -	accept a password, append ":" to the path and it will be intepreted as
> -	an empty password.
> +*-E* _path_:_key_
> +	Sets the path to the client certificate and priavte key file to use.

s/priavte/private/

>  
>  *-l*
>  	For *text/\** responses, *gmni* normally adds a line feed if stdout is a
> diff --git a/include/gmni/certs.h b/include/gmni/certs.h
> new file mode 100644
> index 0000000..58a8955
> --- /dev/null
> +++ b/include/gmni/certs.h
> @@ -0,0 +1,10 @@
> +#ifndef GEMINI_CERTS_H
> +#define GEMINI_CERTS_H
> +#include <bearssl_ssl.h>
> +#include <bearssl_x509.h>
> +#include <stdio.h>
> +
> +const br_ssl_client_certificate_class **gmni_ccert_load(
> +		FILE *certin, FILE *skin);
> +
> +#endif
> diff --git a/include/gmni/gmni.h b/include/gmni/gmni.h
> index 16bef51..4bef997 100644
> --- a/include/gmni/gmni.h
> +++ b/include/gmni/gmni.h
> @@ -69,6 +69,10 @@ struct gemini_options {
>  	// If non-NULL, these hints are provided to getaddrinfo. Useful, for
>  	// example, to force IPv4/IPv6.
>  	struct addrinfo *hints;
> +
> +	// If non-NULL, this will be used as the client certificate for the
> +	// request.
> +	const br_ssl_client_certificate_class **client_cert;
>  };
>  
>  struct gemini_tofu;
> diff --git a/src/certs.c b/src/certs.c
> new file mode 100644
> index 0000000..56023ae
> --- /dev/null
> +++ b/src/certs.c
> @@ -0,0 +1,369 @@
> +#include <assert.h>
> +#include <bearssl_hash.h>
> +#include <bearssl_pem.h>
> +#include <bearssl_ssl.h>
> +#include <bearssl_x509.h>

This is probably fine, but a comment in bearssl.h says that
applications should just include <bearssl.h> instead of these
individual headers.

> +#include <errno.h>
> +#include <gmni/certs.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +
> +struct gmni_ccert_ctx {
> +	const br_ssl_client_certificate_class *vtable;
> +	br_x509_certificate *chain;
> +	size_t nchain;
> +	int key_type;
> +	union {
> +		const br_rsa_private_key *rsa;
> +		const br_ec_private_key *ec;
> +	};
> +};
> +
> +// These are intentionally left blank: we always know in advance which cert
> +// we're going to use.
> +static void
> +cc_start_name_list(const br_ssl_client_certificate_class **pctx)
> +{
> +	(void)pctx;
> +}
> +
> +static void
> +cc_start_name(const br_ssl_client_certificate_class **pctx, size_t len)
> +{
> +	(void)pctx; (void)len;
> +}
> +
> +static void
> +cc_append_name(const br_ssl_client_certificate_class **pctx,
> +	const unsigned char *data, size_t len)
> +{
> +	(void)pctx; (void)data; (void)len;
> +}
> +
> +static void
> +cc_end_name(const br_ssl_client_certificate_class **pctx)
> +{
> +	(void)pctx;
> +}
> +
> +static void
> +cc_end_name_list(const br_ssl_client_certificate_class **pctx)
> +{
> +	(void)pctx;
> +}
> +
> +static int
> +choose_hash(unsigned hh)
> +{
> +	static const int f[] = {
> +		br_sha256_ID, br_sha224_ID, br_sha384_ID, br_sha512_ID,
> +		br_sha1_ID, br_md5sha1_ID, -1
> +	};
> +
> +	size_t u;
> +	for (u = 0; f[u] >= 0; u ++) {
> +		if (((hh >> f[u]) & 1) != 0) {
> +			return f[u];
> +		}
> +	}
> +	return -1;
> +}
> +
> +void
> +cc_choose(const br_ssl_client_certificate_class **pctx,
> +	const br_ssl_client_context *cc, uint32_t auth_types,
> +	br_ssl_client_certificate *choices)
> +{
> +	struct gmni_ccert_ctx *ctx = (struct gmni_ccert_ctx *)(void *)pctx;
> +	switch (ctx->key_type) {
> +	case BR_KEYTYPE_RSA:
> +		if ((choices->hash_id = choose_hash(auth_types)) >= 0) {
> +			choices->auth_type = BR_AUTH_RSA;
> +			choices->chain = ctx->chain;
> +			choices->chain_len = ctx->nchain;
> +			return;
> +		}
> +		abort();
> +	case BR_KEYTYPE_EC:
> +		if ((choices->hash_id = choose_hash(auth_types >> 8)) >= 0) {
> +			choices->auth_type = BR_AUTH_ECDSA;
> +			choices->chain = ctx->chain;
> +			choices->chain_len = ctx->nchain;
> +			return;
> +		}
> +		abort();
> +	}
> +	(void)pctx; (void)cc;
> +	abort();
> +}
> +
> +uint32_t 
> +cc_do_keyx(const br_ssl_client_certificate_class **pctx,
> +	unsigned char *data, size_t *len)
> +{
> +	struct gmni_ccert_ctx *ctx = (struct gmni_ccert_ctx *)(void *)pctx;
> +	const br_ec_impl *iec = br_ec_get_default();
> +	uint32_t r = iec->mul(data, *len,
> +		ctx->ec->x, ctx->ec->xlen, ctx->ec->curve);
> +	size_t xlen;
> +	size_t xoff = iec->xoff(ctx->ec->curve, &xlen);
> +	memmove(data, data + xoff, xlen);
> +	*len = xlen;
> +	return r;
> +}
> +
> +static const unsigned char HASH_OID_SHA1[] = {
> +	0x05, 0x2B, 0x0E, 0x03, 0x02, 0x1A
> +};
> +
> +static const unsigned char HASH_OID_SHA224[] = {
> +	0x09, 0x60, 0x86, 0x48, 0x01, 0x65, 0x03, 0x04, 0x02, 0x04
> +};
> +
> +static const unsigned char HASH_OID_SHA256[] = {
> +	0x09, 0x60, 0x86, 0x48, 0x01, 0x65, 0x03, 0x04, 0x02, 0x01
> +};
> +
> +static const unsigned char HASH_OID_SHA384[] = {
> +	0x09, 0x60, 0x86, 0x48, 0x01, 0x65, 0x03, 0x04, 0x02, 0x02
> +};
> +
> +static const unsigned char HASH_OID_SHA512[] = {
> +	0x09, 0x60, 0x86, 0x48, 0x01, 0x65, 0x03, 0x04, 0x02, 0x03
> +};
> +
> +static const unsigned char *HASH_OID[] = {
> +	HASH_OID_SHA1,
> +	HASH_OID_SHA224,
> +	HASH_OID_SHA256,
> +	HASH_OID_SHA384,
> +	HASH_OID_SHA512
> +};
> +
> +static const unsigned char *
> +get_hash_oid(int id)
> +{
> +	if (id >= 2 && id <= 6) {
> +		return HASH_OID[id - 2];
> +	} else {
> +		return NULL;
> +	}
> +}
> +
> +typedef struct {
> +	const char *name;
> +	const br_hash_class *hclass;
> +	const char *comment;
> +} hash_function;
> +
> +const hash_function hash_functions[] = {
> +	{ "md5",     &br_md5_vtable,     "MD5" },
> +	{ "sha1",    &br_sha1_vtable,    "SHA-1" },
> +	{ "sha224",  &br_sha224_vtable,  "SHA-224" },
> +	{ "sha256",  &br_sha256_vtable,  "SHA-256" },
> +	{ "sha384",  &br_sha384_vtable,  "SHA-384" },
> +	{ "sha512",  &br_sha512_vtable,  "SHA-512" },
> +	{ NULL, 0, NULL }
> +};
> +
> +static const br_hash_class *
> +get_hash_impl(int hash_id)
> +{
> +	size_t u;
> +
> +	if (hash_id == 0) {
> +		return &br_md5sha1_vtable;
> +	}
> +	for (u = 0; hash_functions[u].name; u ++) {
> +		const br_hash_class *hc;
> +		int id;
> +
> +		hc = hash_functions[u].hclass;
> +		id = (hc->desc >> BR_HASHDESC_ID_OFF) & BR_HASHDESC_ID_MASK;
> +		if (id == hash_id) {
> +			return hc;
> +		}
> +	}
> +	return NULL;
> +}
> +
> +size_t
> +cc_do_sign(const br_ssl_client_certificate_class **pctx,
> +	int hash_id, size_t hv_len, unsigned char *data, size_t len)
> +{
> +	struct gmni_ccert_ctx *ctx = (struct gmni_ccert_ctx *)(void *)pctx;
> +	unsigned char hv[64];
> +
> +	const br_hash_class *hc;
> +	const unsigned char *hash_oid;
> +	uint32_t x;
> +	size_t sig_len;
> +	assert(hv_len < sizeof(hv));
> +	memcpy(hv, data, hv_len);
> +	switch (ctx->key_type) {
> +	case BR_KEYTYPE_RSA:
> +		hash_oid = get_hash_oid(hash_id);
> +		if (hash_oid == NULL && hash_id != 0) {
> +			return 0;
> +		}
> +		sig_len = (ctx->rsa->n_bitlen + 7) >> 3;
> +		if (len < sig_len) {
> +			return 0;
> +		}
> +		x = br_rsa_pkcs1_sign_get_default()(
> +			hash_oid, hv, hv_len, ctx->rsa, data);
> +		if (!x) {
> +			return 0;
> +		}
> +		return sig_len;
> +	case BR_KEYTYPE_EC:
> +		hc = get_hash_impl(hash_id);
> +		if (hc == NULL) {
> +			return 0;
> +		}
> +		if (len < 139) {
> +			return 0;
> +		}
> +		sig_len = br_ecdsa_sign_asn1_get_default()(
> +			br_ec_get_default(), hc, hv, ctx->ec, data);
> +		if (sig_len == 0) {
> +			return 0;
> +		}
> +		return sig_len;
> +	}
> +	abort();
> +	(void)pctx;
> +}
> +
> +static const br_ssl_client_certificate_class ccert_class = {
> +	sizeof(struct gmni_ccert_ctx),
> +	cc_start_name_list,
> +	cc_start_name,
> +	cc_append_name,
> +	cc_end_name,
> +	cc_end_name_list,
> +	cc_choose,
> +	cc_do_keyx,
> +	cc_do_sign,
> +};

Can you use the single EC/RSA client cert classes provided by bearssl
itself (br_ssl_client_set_single_ec/br_ssl_client_set_single_rsa)?
Since you aren't doing anything fancy here like choosing a certificate
to present to the server based on the trust anchor names in the
CertificateRequest, I think those should behave the same as what
you have here, without all the boilerplate.

> +static void
> +crt_append(void *ctx, const void *src, size_t len)
> +{
> +	br_x509_certificate *crt = (br_x509_certificate *)ctx;
> +	crt->data = realloc(crt->data, crt->data_len + len);
> +	assert(crt->data);
> +	memcpy(&crt->data[crt->data_len], src, len);
> +	crt->data_len += len;
> +}
> +
> +static void
> +key_append(void *ctx, const void *src, size_t len)
> +{
> +	br_skey_decoder_context *skctx = (br_skey_decoder_context *)ctx;
> +	br_skey_decoder_push(skctx, src, len);
> +}
> +
> +const br_ssl_client_certificate_class **
> +gmni_ccert_load(FILE *certin, FILE *skin)
> +{
> +	// TODO: Better error propagation to caller
> +	static unsigned char buf[BUFSIZ];
> +	struct gmni_ccert_ctx *ctx = calloc(1, sizeof(struct gmni_ccert_ctx));
> +
> +	br_pem_decoder_context pemdec;
> +	br_pem_decoder_init(&pemdec);
> +
> +	ctx->chain = NULL;
> +	ctx->nchain = 0;
> +
> +	static const char *certname = "CERTIFICATE";
> +	while (!feof(certin)) {
> +		int n = fread(&buf, 1, sizeof(buf), certin);
> +		if (n < 0) {
> +			goto error;
> +		}

fread returns a size_t, so this error check isn't quite right. I
think changing n to size_t and replacing n < 0 with ferror(certin)
should work.

> +		int q = 0;
> +		while (q < n) {
> +			q += br_pem_decoder_push(&pemdec, &buf[q], n - q);
> +			switch (br_pem_decoder_event(&pemdec)) {
> +			case BR_PEM_BEGIN_OBJ:
> +				if (strcmp(br_pem_decoder_name(&pemdec), certname) != 0) {
> +					break;
> +				}
> +				ctx->chain = realloc(ctx->chain,
> +					sizeof(br_x509_certificate) * (ctx->nchain + 1));
> +				memset(&ctx->chain[ctx->nchain], 0, sizeof(*ctx->chain));
> +				br_pem_decoder_setdest(&pemdec, &crt_append,
> +						&ctx->chain[ctx->nchain]);
> +				++ctx->nchain;
> +				break;
> +			case BR_PEM_END_OBJ:
> +				break;
> +			case BR_PEM_ERROR:
> +				fprintf(stderr, "Error decoding PEM certificate\n");
> +				errno = EINVAL;
> +				goto error;
> +			}
> +		}
> +	}
> +	assert(ctx->nchain != 0);

This should probably be an error rather than assert. I hit it during
testing by accidentally passing key:cert instead of cert:key.

> +
> +	br_skey_decoder_context skdec = {0};
> +	br_skey_decoder_init(&skdec);
> +	br_pem_decoder_init(&pemdec);
> +
> +	// TODO: Better validation of PEM file
> +	while (!feof(skin)) {
> +		int n = fread(&buf, 1, sizeof(buf), skin);
> +		if (n < 0) {
> +			goto error;
> +		}
> +		int q = 0;
> +		while (q < n) {
> +			q += br_pem_decoder_push(&pemdec, &buf[q], n - q);
> +			switch (br_pem_decoder_event(&pemdec)) {
> +			case BR_PEM_BEGIN_OBJ:
> +				br_pem_decoder_setdest(&pemdec, &key_append, &skdec);
> +				break;
> +			case BR_PEM_END_OBJ:
> +				// no-op
> +				break;
> +			case BR_PEM_ERROR:
> +				fprintf(stderr, "Error decoding PEM private key\n");
> +				errno = EINVAL;
> +				return NULL;
> +			}
> +		}
> +	}
> +
> +	int err = br_skey_decoder_last_error(&skdec);
> +	if (err != 0) {
> +		fprintf(stderr, "Error loading private key: %d\n", err);
> +		errno = EINVAL;
> +		goto error;
> +	}
> +	ctx->key_type = br_skey_decoder_key_type(&skdec);
> +	switch (ctx->key_type) {
> +	case BR_KEYTYPE_RSA:
> +		ctx->rsa = br_skey_decoder_get_rsa(&skdec);
> +		break;
> +	case BR_KEYTYPE_EC:
> +		ctx->ec = br_skey_decoder_get_ec(&skdec);
> +		break;

This is likely the source of your problems. BearSSL doesn't do any
memory allocation itself, so these functions return pointers to
members of the br_skey_decoder_context struct which doesn't live
past the end of this function.

You'll need to allocate memory yourself then copy the key data.
Here's an example of how that might look:

https://git.sr.ht/~mcf/dnssec-rr/tree/master/item/key.c#L7-60

I used a single structure for both key types, with a flexible array
member to store the key data. Something like:

struct key {
        int type;
        union {
                br_ec_private_key ec;
                br_rsa_private_key rsa;
        };
        unsigned char data[];
};

> +	default:
> +		assert(0);
> +	}
> +
> +	fclose(certin);
> +	fclose(skin);
> +	ctx->vtable = &ccert_class;
> +	return &ctx->vtable;
> +
> +error:
> +	fclose(certin);
> +	fclose(skin);
> +	free(ctx->chain);
> +	free(ctx);
> +	return NULL;
> +}
> diff --git a/src/client.c b/src/client.c
> index e402cc9..215c351 100644
> --- a/src/client.c
> +++ b/src/client.c
> @@ -169,7 +169,12 @@ gemini_request(const char *url, struct gemini_options *options,
>  
>  	// TODO: session reuse
>  	resp->sc = &tofu->sc;
> +	if (options->client_cert) {
> +		br_ssl_client_set_client_certificate(
> +			resp->sc, options->client_cert);
> +	}
>  	br_ssl_client_reset(resp->sc, host, 0);
> +
>  	br_sslio_init(&resp->body, &resp->sc->eng,
>  		sock_read, &resp->fd, sock_write, &resp->fd);
>  
> diff --git a/src/gmni.c b/src/gmni.c
> index a8321d0..1b2182d 100644
> --- a/src/gmni.c
> +++ b/src/gmni.c
> @@ -11,6 +11,7 @@
>  #include <sys/types.h>
>  #include <termios.h>
>  #include <unistd.h>
> +#include <gmni/certs.h>
>  #include <gmni/gmni.h>
>  #include <gmni/tofu.h>
>  #include <gmni/url.h>
> @@ -109,6 +110,45 @@ tofu_callback(enum tofu_error error, const char *fingerprint,
>  	return action;
>  }
>  
> +static const br_ssl_client_certificate_class **
> +load_client_cert(char *argv_0, char *path)
> +{
> +	char *certpath = strtok(path, ":");
> +	if (!certpath) {
> +		usage(argv_0);
> +		exit(1);
> +	}
> +
> +	FILE *certf = fopen(certpath, "r");
> +	if (!certf) {
> +		fprintf(stderr, "Failed to open certificate: %s\n",
> +				strerror(errno));
> +		exit(1);
> +	}
> +
> +	char *keypath = strtok(NULL, ":");
> +	if (!keypath) {
> +		usage(argv_0);
> +		exit(1);
> +	}
> +
> +	FILE *keyf = fopen(keypath, "r");
> +	if (!keyf) {
> +		fprintf(stderr, "Failed to open certificate: %s\n",
> +				strerror(errno));
> +		exit(1);
> +	}
> +
> +	const br_ssl_client_certificate_class **ccert =
> +		gmni_ccert_load(certf, keyf);
> +	if (!ccert) {
> +		fprintf(stderr, "Failed to load client certificate: %s\n",
> +				strerror(errno));
> +		exit(1);
> +	}
> +	return ccert;
> +}
> +
>  int
>  main(int argc, char *argv[])
>  {
> @@ -165,7 +205,7 @@ main(int argc, char *argv[])
>  			}
>  			break;
>  		case 'E':
> -			assert(0); // TODO: Client certificates
> +			opts.client_cert = load_client_cert(argv[0], optarg);
>  			break;
>  		case 'h':
>  			usage(argv[0]);
> @@ -226,7 +266,7 @@ main(int argc, char *argv[])
>  	bool exit = false;
>  	struct Curl_URL *url = curl_url();
>  
> -	if(curl_url_set(url, CURLUPART_URL, argv[optind], 0) != CURLUE_OK) {
> +	if (curl_url_set(url, CURLUPART_URL, argv[optind], 0) != CURLUE_OK) {
>  		// TODO: Better error
>  		fprintf(stderr, "Error: invalid URL\n");
>  		return 1;
> @@ -238,8 +278,8 @@ main(int argc, char *argv[])
>  		curl_url_get(url, CURLUPART_URL, &buf, 0);
>  
>  		struct gemini_response resp;
> -		enum gemini_result r = gemini_request(
> -				buf, &opts, &cfg.tofu, &resp);
> +		enum gemini_result r = gemini_request(buf,
> +			&opts, &cfg.tofu, &resp);
>  
>  		free(buf);
>  
Details
Message ID
<C9PGED8R9PSF.15ZJWHSKTY218@taiga>
In-Reply-To
<2I6XAGAWXZGAB.2VLLBP29N0KN8@mforney.org> (view parent)
DKIM signature
missing
Download raw message
Thanks for the comments! That totally helped me smooth out the issues
with the implementation. It's much more concise now, and, importantly,
it actually works.

On Thu Mar 4, 2021 at 8:49 PM EST, Michael Forney wrote:
> I think it wouldn't be too difficult to write a small utility/library
> to wrap a public key in a self-signed certificate. This would be
> quite useful even outside the context of gemini. It should just be
> a matter of constructing the appropriate ASN.1 structure (though
> I'm not quite sure what that looks like).
>
> I agree that this is an annoying shortcoming with BearSSL.

Yeah, I had a similar line of thought, and started looking around for
easily embeddable and liberally licensed C ASN.1 encoders. A cursory
search did not turn up any promising leads, so I deferred further
research for now - also have no idea what fields actually go into an
X.509 cert. If you know of anything useful here, would be glad to hear
about it.
Details
Message ID
<CAGw6cBtwmvGSvxmB-+wTqHj4ArmSgeRsbYqs_=Zc_JCa90oDNQ@mail.gmail.com>
In-Reply-To
<2I6XAGAWXZGAB.2VLLBP29N0KN8@mforney.org> (view parent)
DKIM signature
missing
Download raw message
On 2021-03-04, Michael Forney <mforney@mforney.org> wrote:
> Drew DeVault <sir@cmpwn.com> wrote:
>> Also, BearSSL lacks support for creating new certificates, so automatic
>> certificate creation will have to be postponed. I might actually end up
>> ditching BearSSL over this, mere hours after I decided to switch to it,
>> which I'm rather annoyed about.
>
> I think it wouldn't be too difficult to write a small utility/library
> to wrap a public key in a self-signed certificate. This would be
> quite useful even outside the context of gemini.

I ended up writing such a utility/library: https://git.sr.ht/~mcf/x509cert

> It should just be
> a matter of constructing the appropriate ASN.1 structure (though
> I'm not quite sure what that looks like).

Turns out this is pretty annoying. The Certificate structure is very
complex and deeply nested, and due to the DER encoding rules having
variable-length length, you can't be sure about the offsets for
anything until you compute the signature of the inner data (the
encoded length of the signature depends on the actual signature).
Details
Message ID
<CB9TQ2IKCUI0.27TLQLHGVSVPE@sayaka>
In-Reply-To
<CAGw6cBtwmvGSvxmB-+wTqHj4ArmSgeRsbYqs_=Zc_JCa90oDNQ@mail.gmail.com> (view parent)
DKIM signature
pass
Download raw message
On Sun May 9, 2021 at 4:28 PM HST, Michael Forney wrote:
> I ended up writing such a utility/library:
> https://git.sr.ht/~mcf/x509cert

Nice! I'll take a look at this when I get back from vacation.

> Turns out this is pretty annoying. The Certificate structure is very
> complex and deeply nested, and due to the DER encoding rules having
> variable-length length, you can't be sure about the offsets for
> anything until you compute the signature of the inner data (the
> encoded length of the signature depends on the actual signature).

Yeah, I'm not surprised. ASN.1 and x.509 are legendarily complicated.
Details
Message ID
<CBEZ8R4UKNT3.3BVX3G684FL7V@taiga>
In-Reply-To
<CAGw6cBtwmvGSvxmB-+wTqHj4ArmSgeRsbYqs_=Zc_JCa90oDNQ@mail.gmail.com> (view parent)
DKIM signature
pass
Download raw message
Do you think you could add a pkg-config file to this? The BearSSL
maintainer isn't interested in this, but doesn't provide any rational
reasons, so hopefully you're easier to convince.
Details
Message ID
<CAGw6cBuqPQTHLfTTUkPNXSL38k8=hPzwO+AoxeqTg1GaCRSyDg@mail.gmail.com>
In-Reply-To
<CBEZ8R4UKNT3.3BVX3G684FL7V@taiga> (view parent)
DKIM signature
missing
Download raw message
On 2021-05-16, Drew DeVault <sir@cmpwn.com> wrote:
> Do you think you could add a pkg-config file to this? The BearSSL
> maintainer isn't interested in this, but doesn't provide any rational
> reasons, so hopefully you're easier to convince.

Sure, just added one.
Details
Message ID
<CBFGEW6BXNZZ.U94O3EDP9TZI@taiga>
In-Reply-To
<CAGw6cBuqPQTHLfTTUkPNXSL38k8=hPzwO+AoxeqTg1GaCRSyDg@mail.gmail.com> (view parent)
DKIM signature
pass
Download raw message
Thanks. Any chance for a 0.3 as well?
Details
Message ID
<CAGw6cBsDnQDxW1m4G-_B3WSd6P+kGr5L+bHDYhLYaM9eDD6LSw@mail.gmail.com>
In-Reply-To
<CBFGEW6BXNZZ.U94O3EDP9TZI@taiga> (view parent)
DKIM signature
missing
Download raw message
On 2021-05-17, Drew DeVault <sir@cmpwn.com> wrote:
> Thanks. Any chance for a 0.3 as well?

Before I do so, can you confirm that the API looks sufficient for your
use case? I believe you are just creating self-signed certs with some
particular DN, so I think it should be fine, but I just want to
double-check.
Details
Message ID
<CBICJ3MBPKSV.3AYOWUC97LXRV@taiga>
In-Reply-To
<CAGw6cBsDnQDxW1m4G-_B3WSd6P+kGr5L+bHDYhLYaM9eDD6LSw@mail.gmail.com> (view parent)
DKIM signature
pass
Download raw message
On Thu May 20, 2021 at 3:51 PM EDT, Michael Forney wrote:
> On 2021-05-17, Drew DeVault <sir@cmpwn.com> wrote:
> > Thanks. Any chance for a 0.3 as well?
>
> Before I do so, can you confirm that the API looks sufficient for your
> use case? I believe you are just creating self-signed certs with some
> particular DN, so I think it should be fine, but I just want to
> double-check.

I only did a look-see rather than actually testing it, but it seems
alright to me. I'm not sure when I'll have the time to update gmnisrv,
since it also involves rewriting the whole thing from OpenSSL to
BearSSL, rather than just the x509 bits, so I can't offer a more
battle-proven answer than that yet.
Reply to thread Export thread (mbox)