On Tue, Jan 18, 2022 at 04:16:17PM +0100, Claudio Jeker wrote:
> This diff cleans up cert.c a bit.
>
> It removes the X509 handle from cert_parse() and ta_parse(). Callers
> should instead use cert->x509. No need to double the work on us here.
I never understood the point of this handle and I know I chased it down
several times since it confused me. Glad to see it go.
> While there switch auth_insert() to a void function. This function can
> not fail. Again the result is simpler code in parser.c
To save anton some work: the signature change of *_parse() will need
adjustments in regress. With those,
ok
Two small suggestions below.
>
> --
> :wq Claudio
>
> Index: cert.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/cert.c,v
> retrieving revision 1.50
> diff -u -p -r1.50 cert.c
> --- cert.c 18 Jan 2022 13:06:43 -0000 1.50
> +++ cert.c 18 Jan 2022 15:09:32 -0000
> @@ -978,8 +978,7 @@ out:
> * is also dereferenced.
> */
> static struct cert *
> -cert_parse_inner(X509 **xp, const char *fn, const unsigned char *der,
> - size_t len, int ta)
> +cert_parse_inner(const char *fn, const unsigned char *der, size_t len, int
> ta)
> {
> int rc = 0, extsz, c;
> int sia_present = 0;
> @@ -989,8 +988,6 @@ cert_parse_inner(X509 **xp, const char *
> ASN1_OBJECT *obj;
> struct parse p;
>
> - *xp = NULL;
> -
> /* just fail for empty buffers, the warning was printed elsewhere */
> if (der == NULL)
> return NULL;
> @@ -1000,7 +997,7 @@ cert_parse_inner(X509 **xp, const char *
> if ((p.res = calloc(1, sizeof(struct cert))) == NULL)
> err(1, NULL);
>
> - if ((x = *xp = d2i_X509(NULL, &der, len)) == NULL) {
> + if ((x = d2i_X509(NULL, &der, len)) == NULL) {
> cryptowarnx("%s: d2i_X509_bio", p.fn);
> goto out;
> }
> @@ -1139,9 +1136,6 @@ cert_parse_inner(X509 **xp, const char *
> goto out;
> }
>
> - if (X509_up_ref(x) == 0)
> - errx(1, "%s: X509_up_ref failed", __func__);
> -
> p.res->x509 = x;
>
> rc = 1;
> @@ -1149,34 +1143,32 @@ out:
> if (rc == 0) {
> cert_free(p.res);
> X509_free(x);
> - *xp = NULL;
> }
> return (rc == 0) ? NULL : p.res;
> }
>
> struct cert *
> -cert_parse(X509 **xp, const char *fn, const unsigned char *der, size_t len)
> +cert_parse(const char *fn, const unsigned char *der, size_t len)
> {
> - return cert_parse_inner(xp, fn, der, len, 0);
> + return cert_parse_inner(fn, der, len, 0);
> }
>
> struct cert *
> -ta_parse(X509 **xp, const char *fn, const unsigned char *der, size_t len,
> +ta_parse(const char *fn, const unsigned char *der, size_t len,
> const unsigned char *pkey, size_t pkeysz)
> {
> EVP_PKEY *pk = NULL, *opk = NULL;
> struct cert *p;
> int rc = 0;
>
> - if ((p = cert_parse_inner(xp, fn, der, len, 1)) == NULL)
> + if ((p = cert_parse_inner(fn, der, len, 1)) == NULL)
> return NULL;
>
> if (pkey != NULL) {
> - assert(*xp != NULL);
> pk = d2i_PUBKEY(NULL, &pkey, pkeysz);
> assert(pk != NULL);
>
> - if ((opk = X509_get_pubkey(*xp)) == NULL)
> + if ((opk = X509_get_pubkey(p->x509)) == NULL)
You could switch this to X509_get0_pubkey() and get rid of the
EVP_PKEY_free(opk) a few lines down.
> cryptowarnx("%s: RFC 6487 (trust anchor): "
> "missing pubkey", fn);
> else if (EVP_PKEY_cmp(pk, opk) != 1)
> @@ -1192,8 +1184,6 @@ ta_parse(X509 **xp, const char *fn, cons
> if (rc == 0) {
> cert_free(p);
> p = NULL;
> - X509_free(*xp);
> - *xp = NULL;
> }
>
> return p;
> @@ -1305,7 +1295,7 @@ auth_find(struct auth_tree *auths, const
> return RB_FIND(auth_tree, auths, &a);
> }
>
> -int
> +void
> auth_insert(struct auth_tree *auths, struct cert *cert, struct auth *parent)
> {
> struct auth *na;
> @@ -1319,8 +1309,6 @@ auth_insert(struct auth_tree *auths, str
>
> if (RB_INSERT(auth_tree, auths, na) != NULL)
> err(1, "auth tree corrupted");
> -
> - return 1;
> }
>
> static inline int
> Index: extern.h
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/extern.h,v
> retrieving revision 1.105
> diff -u -p -r1.105 extern.h
> --- extern.h 18 Jan 2022 13:06:43 -0000 1.105
> +++ extern.h 18 Jan 2022 15:10:02 -0000
> @@ -279,7 +279,7 @@ RB_HEAD(auth_tree, auth);
> RB_PROTOTYPE(auth_tree, auth, entry, authcmp);
>
> struct auth *auth_find(struct auth_tree *, const char *);
> -int auth_insert(struct auth_tree *, struct cert *, struct auth *);
> +void auth_insert(struct auth_tree *, struct cert *, struct auth *);
>
> /*
> * Resource types specified by the RPKI profiles.
> @@ -407,9 +407,8 @@ struct tal *tal_read(struct ibuf *);
>
> void cert_buffer(struct ibuf *, const struct cert *);
> void cert_free(struct cert *);
> -struct cert *cert_parse(X509 **, const char *, const unsigned char *,
> - size_t);
> -struct cert *ta_parse(X509 **, const char *, const unsigned char *, size_t,
> +struct cert *cert_parse(const char *, const unsigned char *, size_t);
> +struct cert *ta_parse(const char *, const unsigned char *, size_t,
> const unsigned char *, size_t);
> struct cert *cert_read(struct ibuf *);
> void cert_insert_brks(struct brk_tree *, struct cert *);
> Index: parser.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/parser.c,v
> retrieving revision 1.39
> diff -u -p -r1.39 parser.c
> --- parser.c 18 Jan 2022 13:46:07 -0000 1.39
> +++ parser.c 18 Jan 2022 15:12:50 -0000
> @@ -393,25 +393,22 @@ static struct cert *
> proc_parser_cert(char *file, const unsigned char *der, size_t len)
> {
> struct cert *cert;
> - X509 *x509;
> struct auth *a;
> struct crl *crl;
>
> /* Extract certificate data and X509. */
>
> - cert = cert_parse(&x509, file, der, len);
> + cert = cert_parse(file, der, len);
> if (cert == NULL)
> return NULL;
>
> a = valid_ski_aki(file, &auths, cert->ski, cert->aki);
> crl = get_crl(a);
>
> - if (!valid_x509(file, x509, a, crl)) {
> + if (!valid_x509(file, cert->x509, a, crl)) {
> cert_free(cert);
> - X509_free(x509);
> return NULL;
> }
> - X509_free(x509);
>
> cert->talid = a->cert->talid;
>
> @@ -424,12 +421,8 @@ proc_parser_cert(char *file, const unsig
> /*
> * Add validated CA certs to the RPKI auth tree.
> */
> - if (cert->purpose == CERT_PURPOSE_CA) {
> - if (!auth_insert(&auths, cert, a)) {
> - cert_free(cert);
> - return NULL;
> - }
> - }
> + if (cert->purpose == CERT_PURPOSE_CA)
> + auth_insert(&auths, cert, a);
>
> return cert;
> }
> @@ -455,10 +448,11 @@ proc_parser_root_cert(char *file, const
>
> /* Extract certificate data and X509. */
>
> - cert = ta_parse(&x509, file, der, len, pkey, pkeysz);
> + cert = ta_parse(file, der, len, pkey, pkeysz);
> if (cert == NULL)
> return NULL;
>
> + x509 = cert->x509; /* no upref so no need to call X509_free() */
I would drop this comment. I think it will soon be confusing.
> if ((name = X509_get_subject_name(x509)) == NULL) {
> warnx("%s Unable to get certificate subject", file);
> goto badcert;
> @@ -493,22 +487,16 @@ proc_parser_root_cert(char *file, const
> goto badcert;
> }
>
> - X509_free(x509);
> -
> cert->talid = talid;
>
> /*
> * Add valid roots to the RPKI auth tree.
> */
> - if (!auth_insert(&auths, cert, NULL)) {
> - cert_free(cert);
> - return NULL;
> - }
> + auth_insert(&auths, cert, NULL);
>
> return cert;
>
> badcert:
> - X509_free(x509);
> cert_free(cert);
> return NULL;
> }
>
