Address several TODOs.

diff --git a/src/address.h b/src/address.h
index 43e8c79e132504d4b6d9d5213eec7e5f56469570..62859b533738e2c7bbdcc02b0228d938aee9b49e 100644 (file)
--- a/src/address.h
+++ b/src/address.h
@@ -35,7 +35,4 @@ int prefix6_decode(IPAddress_t *, struct ipv6_prefix *);
 int range4_decode(IPAddressRange_t *, struct ipv4_range *);
 int range6_decode(IPAddressRange_t *, struct ipv6_range *);
 
-int range4_check_encoding(struct ipv4_range *);
-int range6_check_encoding(struct ipv6_range *);
-
 #endif /* SRC_ADDRESS_H_ */

diff --git a/src/algorithm.c b/src/algorithm.c
index 4df2d1f75e7b58284840bb2d18eb1418b4a6e1c4..84a10d5de07f61c4db5232ad6c74340cebb656a3 100644 (file)
--- a/src/algorithm.c
+++ b/src/algorithm.c
@@ -12,8 +12,8 @@ int
 rpki_public_key_algorithm(void)
 {
        /*
-        * TODO Everyone uses this algorithm, but at a quick glance, it doesn't
-        * seem to match RFC 7935's public key algorithm. Wtf?
+        * TODO Everyone uses this algorithm, but the RFC says that it should
+        * be NID_sha256WithRSAEncryption. Wtf?
         */
        return NID_rsaEncryption;
 }

diff --git a/src/asn1/decode.c b/src/asn1/decode.c
index 2800c52d07bd0530b80aa4af6a40672bfea95f98..4e4f2647e833e485332c2d9435b460fa5fcca914 100644 (file)
--- a/src/asn1/decode.c
+++ b/src/asn1/decode.c
@@ -34,8 +34,9 @@ asn1_decode(const void *buffer, size_t buffer_size,
        if (rval.code != RC_OK) {
                /* Must free partial object according to API contracts. */
                ASN_STRUCT_FREE(*descriptor, *result);
-               /* TODO if rval.code == RC_WMORE (1), more work is needed */
-               return pr_err("Error decoding ASN.1 object: %u", rval.code);
+               /* We expect the data to be complete; RC_WMORE is an error. */
+               return pr_err("Error '%u' decoding ASN.1 object around byte %zu",
+                   rval.code, rval.consumed);
        }
 
        error = validate(descriptor, *result);
@@ -61,6 +62,11 @@ asn1_decode_octet_string(OCTET_STRING_t *string,
        return asn1_decode(string->buf, string->size, descriptor, result);
 }
 
+/*
+ * TODO (next iteration) There's no need to load the entire file into memory.
+ * ber_decode() can take an incomplete buffer, in which case it returns
+ * RC_WMORE.
+ */
 int
 asn1_decode_fc(struct file_contents *fc,
     asn_TYPE_descriptor_t const *descriptor, void **result)

diff --git a/src/asn1/signed_data.c b/src/asn1/signed_data.c
index 4d0b7f0667fa8c3b25834df692d53a40c293dc99..39b47f28c55921c6f1357ade9c612d10e4e4de47 100644 (file)
--- a/src/asn1/signed_data.c
+++ b/src/asn1/signed_data.c
@@ -17,6 +17,27 @@ static const OID oid_mda = OID_MESSAGE_DIGEST_ATTR;
 static const OID oid_sta = OID_SIGNING_TIME_ATTR;
 static const OID oid_bsta = OID_BINARY_SIGNING_TIME_ATTR;
 
+int
+signed_object_args_init(struct signed_object_args *args,
+    struct rpki_uri const *uri, STACK_OF(X509_CRL) *crls)
+{
+       args->res = resources_create();
+       if (args->res == NULL)
+               return pr_enomem();
+
+       args->uri = uri;
+       args->crls = crls;
+       memset(&args->refs, 0, sizeof(args->refs));
+       return 0;
+}
+
+void
+signed_object_args_cleanup(struct signed_object_args *args)
+{
+       resources_destroy(args->res);
+       refs_cleanup(&args->refs);
+}
+
 static int
 is_digest_algorithm(AlgorithmIdentifier_t *id, char const *what)
 {
@@ -95,13 +116,10 @@ handle_sdata_certificate(ANY_t *any, struct signed_object_args *args,
        if (error)
                goto end2;
 
-       if (args->res != NULL) {
-               /* TODO validate resources even if the SO lacks them */
-               resources_set_policy(args->res, policy);
-               error = certificate_get_resources(cert, args->res);
-               if (error)
-                       goto end2;
-       }
+       resources_set_policy(args->res, policy);
+       error = certificate_get_resources(cert, args->res);
+       if (error)
+               goto end2;
 
 end2:
        X509_free(cert);
@@ -392,7 +410,7 @@ validate(struct SignedData *sdata, struct signed_object_args *args)
 
        /* rfc6488#section-3.2 */
        /* rfc6488#section-3.3 */
-       /* TODO */
+       /* TODO (field) */
 
        return 0;
 }
@@ -404,7 +422,8 @@ signed_data_decode(ANY_t *coded, struct signed_object_args *args,
        struct SignedData *sdata;
        int error;
 
-       /* rfc6488#section-3.1.l TODO this is BER, not guaranteed to be DER. */
+       /* rfc6488#section-3.1.l */
+       /* TODO (next iteration) this is BER, not guaranteed to be DER. */
        error = asn1_decode_any(coded, &asn_DEF_SignedData, (void **) &sdata);
        if (error)
                return error;

diff --git a/src/asn1/signed_data.h b/src/asn1/signed_data.h
index 81643cf957fa3c871addb74cc259680e9837f140..bf31fb83744494d31625fcdc1b46b46274776ff7 100644 (file)
--- a/src/asn1/signed_data.h
+++ b/src/asn1/signed_data.h
@@ -10,15 +10,25 @@
 
 /*
  * This only exists to reduce argument lists.
- * TODO document fields.
  */
 struct signed_object_args {
+       /** Location of the signed object. */
+       struct rpki_uri const *uri;
+       /** CRL that might or might not revoke the embedded certificate. */
        STACK_OF(X509_CRL) *crls;
+       /** A copy of the resources carried by the embedded certificate. */
        struct resources *res;
-       struct rpki_uri const *uri;
+       /**
+        * A bunch of URLs found in the embedded certificate's extensions,
+        * recorded for future validation.
+        */
        struct certificate_refs refs;
 };
 
+int signed_object_args_init(struct signed_object_args *,
+    struct rpki_uri const *, STACK_OF(X509_CRL) *);
+void signed_object_args_cleanup(struct signed_object_args *);
+
 int signed_data_decode(ANY_t *, struct signed_object_args *args,
     struct SignedData **);
 void signed_data_free(struct SignedData *);

diff --git a/src/main.c b/src/main.c
index c729b43ec7fc94ed6ce8525e89f5e31f1d72f5b4..65b90ea42a5c3a5f414e8dd2ad7de835a3d548e9 100644 (file)
--- a/src/main.c
+++ b/src/main.c
@@ -197,9 +197,13 @@ main(int argc, char **argv)
        if (!error) {
                if (config_get_shuffle_uris())
                        tal_shuffle_uris(tal);
+
                error = foreach_uri(tal, handle_tal_uri);
-               /* TODO error on no uris were valid. */
-               error = (error >= 0) ? 0 : error;
+               if (error > 0)
+                       error = 0;
+               else if (error == 0)
+                       error = pr_err("None of the URIs of the TAL yielded a successful traversal.");
+
                tal_destroy(tal);
        }
 

diff --git a/src/object/certificate.c b/src/object/certificate.c
index f7b6b6da397049927c0e03b0aa673ee34efeeaed..dd53abe8c4dbe44096e9a6a16980d6ff66df8806 100644 (file)
--- a/src/object/certificate.c
+++ b/src/object/certificate.c
@@ -155,13 +155,12 @@ validate_spki(const unsigned char *cert_spk, int cert_spk_len)
         * trouble. We'll have to decode the TAL's SPKI though.
         */
 
-       /*
-        * TODO we're decoding the TAL's public key, but the stacked file name
-        * is the certificate's. It looks weird when it errors.
-        */
+       fnstack_push(tal_get_file_name(tal));
        tal_get_spki(tal, &_tal_spki, &_tal_spki_len);
        error = asn1_decode(_tal_spki, _tal_spki_len,
            &asn_DEF_SubjectPublicKeyInfo, (void **) &tal_spki);
+       fnstack_pop();
+
        if (error)
                goto fail1;
 

diff --git a/src/object/certificate.h b/src/object/certificate.h
index 5b87e3ea0af816900d57f3f25332ed62e65abafa..22d0a68156850a6e33c2ffab842b5c21feea7ec0 100644 (file)
--- a/src/object/certificate.h
+++ b/src/object/certificate.h
@@ -24,7 +24,11 @@ int certificate_validate_rfc6487(X509 *, bool);
 /**
  * Returns the IP and AS resources declared in the respective extensions.
  *
- * TODO why is this not part of the certificate_validate*s, again?
+ * Note: One reason why this is separate from the validate_extensions functions
+ * is because it needs to be handled after the policy has been extracted from
+ * the certificate policies extension, and handle_extensions() currently does
+ * not care about order. I don't know if you'll find other reasons if you choose
+ * to migrate it.
  */
 int certificate_get_resources(X509 *, struct resources *);
 

diff --git a/src/object/manifest.c b/src/object/manifest.c
index 2f851aea48fc490cd231b28f49896010cb413b18..94cacb86a2fc915dea27fbaddda6198ff4c6d6b0 100644 (file)
--- a/src/object/manifest.c
+++ b/src/object/manifest.c
@@ -179,31 +179,31 @@ handle_manifest(struct rpki_uri const *uri, STACK_OF(X509_CRL) *crls,
        pr_debug_add("Manifest %s {", uri->global);
        fnstack_push(uri->global);
 
-       sobj_args.uri = uri;
-       sobj_args.crls = crls;
-       sobj_args.res = NULL;
-       memset(&sobj_args.refs, 0, sizeof(sobj_args.refs));
+       error = signed_object_args_init(&sobj_args, uri, crls);
+       if (error)
+               goto end1;
        mft.file_path = uri->global;
 
        error = signed_object_decode(&sobj_args, &asn_DEF_Manifest, &arcs,
-           (void **) &mft.obj); /* mft.obj and sobj_args.refs in the heap */
+           (void **) &mft.obj);
        if (error)
-               goto end1;
+               goto end2;
 
        error = validate_manifest(mft.obj);
        if (error)
-               goto end2;
-       error = __handle_manifest(&mft, pp); /* pp in the heap */
+               goto end3;
+       error = __handle_manifest(&mft, pp);
        if (error)
-               goto end2;
+               goto end3;
 
        error = refs_validate_ee(&sobj_args.refs, *pp, uri);
        if (error)
                rpp_destroy(*pp);
 
-end2:
+end3:
        ASN_STRUCT_FREE(asn_DEF_Manifest, mft.obj);
-       refs_cleanup(&sobj_args.refs);
+end2:
+       signed_object_args_cleanup(&sobj_args);
 end1:
        pr_debug_rm("}");
        fnstack_pop();

diff --git a/src/object/roa.c b/src/object/roa.c
index e31a4844c9c22135d805c069b871eeed07eecd70..9948f5a209e237e7698b2e4233a4a689044fdb5e 100644 (file)
--- a/src/object/roa.c
+++ b/src/object/roa.c
@@ -191,7 +191,8 @@ family_error:
        return pr_err("ROA's IP family is not v4 or v6.");
 }
 
-int handle_roa(struct rpki_uri const *uri, struct rpp *pp,
+int
+handle_roa(struct rpki_uri const *uri, struct rpp *pp,
     STACK_OF(X509_CRL) *crls)
 {
        static OID oid = OID_ROA;
@@ -203,14 +204,9 @@ int handle_roa(struct rpki_uri const *uri, struct rpp *pp,
        pr_debug_add("ROA %s {", uri->global);
        fnstack_push(uri->global);
 
-       sobj_args.uri = uri;
-       sobj_args.crls = crls;
-       sobj_args.res = resources_create();
-       if (sobj_args.res == NULL) {
-               error = pr_enomem();
+       error = signed_object_args_init(&sobj_args, uri, crls);
+       if (error)
                goto end1;
-       }
-       memset(&sobj_args.refs, 0, sizeof(sobj_args.refs));
 
        error = signed_object_decode(&sobj_args,
            &asn_DEF_RouteOriginAttestation, &arcs, (void **) &roa);
@@ -225,9 +221,8 @@ int handle_roa(struct rpki_uri const *uri, struct rpp *pp,
 
 end3:
        ASN_STRUCT_FREE(asn_DEF_RouteOriginAttestation, roa);
-       refs_cleanup(&sobj_args.refs);
 end2:
-       resources_destroy(sobj_args.res);
+       signed_object_args_cleanup(&sobj_args);
 end1:
        pr_debug_rm("}");
        fnstack_pop();

diff --git a/src/object/tal.c b/src/object/tal.c
index 429113477d05037f3ceb917dde9c0b1997757d3a..3b83de779f544c721ceb28f03f2b8869b239fea5 100644 (file)
--- a/src/object/tal.c
+++ b/src/object/tal.c
@@ -22,6 +22,7 @@ struct uris {
 };
 
 struct tal {
+       char const *file_name;
        struct uris uris;
        unsigned char *spki; /* Decoded; not base64. */
        size_t spki_len;
@@ -143,8 +144,11 @@ read_spki(struct line_file *lfile, struct tal *tal)
        return error;
 }
 
+/**
+ * @file_name is expected to outlive @result.
+ */
 int
-tal_load(const char *file_name, struct tal **result)
+tal_load(char const *file_name, struct tal **result)
 {
        struct line_file *lfile;
        struct tal *tal;
@@ -160,6 +164,8 @@ tal_load(const char *file_name, struct tal **result)
                goto fail3;
        }
 
+       tal->file_name = file_name;
+
        error = uris_init(&tal->uris);
        if (error)
                goto fail2;
@@ -241,6 +247,12 @@ tal_shuffle_uris(struct tal *tal)
        }
 }
 
+char const *
+tal_get_file_name(struct tal *tal)
+{
+       return tal->file_name;
+}
+
 void
 tal_get_spki(struct tal *tal, unsigned char const **buffer, size_t *len)
 {

diff --git a/src/object/tal.h b/src/object/tal.h
index 87a3e57900ed1a94f57e5f9d9bfe7d68874667a1..e092989faf077f20c2fae4795c953935a9a1a788 100644 (file)
--- a/src/object/tal.h
+++ b/src/object/tal.h
@@ -8,13 +8,14 @@
 
 struct tal;
 
-int tal_load(const char *, struct tal **);
+int tal_load(char const *, struct tal **);
 void tal_destroy(struct tal *);
 
 typedef int (*foreach_uri_cb)(struct tal *, struct rpki_uri const *);
 int foreach_uri(struct tal *, foreach_uri_cb);
 void tal_shuffle_uris(struct tal *);
 
+char const *tal_get_file_name(struct tal *);
 void tal_get_spki(struct tal *, unsigned char const **, size_t *);
 
 #endif /* TAL_OBJECT_H_ */

diff --git a/src/resource.c b/src/resource.c
index b637e41b554dbcb583cdefe765b14af508b49bb0..5366f6ed7b6c3185ad68edb16bb63e570d45fda0 100644 (file)
--- a/src/resource.c
+++ b/src/resource.c
@@ -86,26 +86,24 @@ inherit_aors(struct resources *resources, int family)
 
        parent = get_parent_resources();
        if (parent == NULL)
-               return pr_err("Certificate inherits IP resources, but parent does not define any resources.");
+               return pr_crit("Parent has no resources.");
 
        switch (family) {
        case AF_INET:
                if (resources->ip4s != NULL)
                        return pr_err("Certificate inherits IPv4 resources while also defining others of its own.");
-               if (parent->ip4s == NULL)
-                       return pr_err("Certificate inherits IPv4 resources from parent, but parent lacks IPv4 resources.");
                resources->ip4s = parent->ip4s;
-               res4_get(resources->ip4s);
+               if (resources->ip4s != NULL)
+                       res4_get(resources->ip4s);
                pr_debug("<Inherit IPv4>");
                return 0;
 
        case AF_INET6:
                if (resources->ip6s != NULL)
                        return pr_err("Certificate inherits IPv6 resources while also defining others of its own.");
-               if (parent->ip6s == NULL)
-                       return pr_err("Certificate inherits IPv6 resources from parent, but parent lacks IPv6 resources.");
                resources->ip6s = parent->ip6s;
-               res6_get(resources->ip6s);
+               if (resources->ip6s != NULL)
+                       res6_get(resources->ip6s);
                pr_debug("<Inherit IPv6>");
                return 0;
        }
@@ -388,15 +386,14 @@ inherit_asiors(struct resources *resources)
 
        parent = get_parent_resources();
        if (parent == NULL)
-               return pr_err("Certificate inherits ASN resources, but parent does not define any resources.");
+               return pr_crit("Parent has no resources.");
 
        if (resources->asns != NULL)
                return pr_err("Certificate inherits ASN resources while also defining others of its own.");
-       if (parent->asns == NULL)
-               return pr_err("Certificate inherits ASN resources from parent, but parent lacks ASN resources.");
 
        resources->asns = parent->asns;
-       rasn_get(resources->asns);
+       if (resources->asns != NULL)
+               rasn_get(resources->asns);
        pr_debug("<Inherit ASN>");
        return 0;
 }

diff --git a/src/rsync/rsync.c b/src/rsync/rsync.c
index a0770ba15d90ac573ca4a4246e0d699fabce9900..38583265b685d8a43f161be33ba4c942c1fe9d89 100644 (file)
--- a/src/rsync/rsync.c
+++ b/src/rsync/rsync.c
@@ -86,8 +86,10 @@ do_rsync(char const *rsync_uri, char const *localuri)
        pr_debug("(%s) command = %s", __func__, command);
 
        /*
-        * TODO Improve the execution of the command, maybe using another
-        * function instead of system().
+        * TODO (next iteration) system(3): "Do not use system() from a
+        * privileged program"
+        * I don't think there's a reason to run this program with privileges,
+        * but consider using exec(3) instead.
         */
        error = system(command);
        if (error) {

diff --git a/src/state.c b/src/state.c
index a8b94650104a8bf5b259a1743abbb508ea16e3d2..7ee418377d4dc9c584393ace4940566f2530df04 100644 (file)
--- a/src/state.c
+++ b/src/state.c
@@ -375,7 +375,8 @@ validation_store_subject(struct validation *state, char *subject)
 
        ARRAYLIST_FOREACH(&cert->subjects, cursor)
                if (strcmp(*cursor, subject) == 0)
-                       return pr_err("Subject name is not unique.");
+                       return pr_err("Subject name '%s' is not unique.",
+                           subject);
 
        duplicate = strdup(subject);
        if (duplicate == NULL)

diff --git a/src/thread_var.h b/src/thread_var.h
index 868cc6ea43954af8582992bb72bac928ee608640..074148a7af8f2faac9a31e35150cc09f5d94fea5 100644 (file)
--- a/src/thread_var.h
+++ b/src/thread_var.h
@@ -13,7 +13,6 @@ void fnstack_push(char const *);
 char const *fnstack_peek(void);
 void fnstack_pop(void);
 
-/* TODO use these more, to print better error messages. */
 char const *v4addr2str(struct in_addr *addr);
 char const *v4addr2str2(struct in_addr *addr);
 char const *v6addr2str(struct in6_addr *addr);