Unify Access Description management

diff --git a/src/object/certificate.c b/src/object/certificate.c
index dc553471b1ca8a57bebb0f54bae2ae42a4ecd603..a952798994c325d37a80f10d16d661d76a33a03b 100644 (file)
--- a/src/object/certificate.c
+++ b/src/object/certificate.c
@@ -601,46 +601,30 @@ is_rsync_uri(GENERAL_NAME *name)
 }
 
 static int
-handle_rpkiManifest(ACCESS_DESCRIPTION *ad, struct rpki_uri *mft)
+handle_rpkiManifest(struct rpki_uri *uri, void *arg)
 {
-       return uri_init_ad(mft, ad);
+       struct rpki_uri *mft = arg;
+       *mft = *uri;
+       uri->global = NULL;
+       uri->local = NULL;
+       return 0;
 }
 
 static int
-handle_caRepository(ACCESS_DESCRIPTION *ad)
+handle_caRepository(struct rpki_uri *uri, void *arg)
 {
-       struct rpki_uri uri;
-       int error;
-
-       error = uri_init_ad(&uri, ad);
-       if (error)
-               return error;
-
-       pr_debug("caRepository: %s", uri.global);
-       error = download_files(&uri);
-
-       uri_cleanup(&uri);
-       return error;
+       pr_debug("caRepository: %s", uri->global);
+       return download_files(uri);
 }
 
 static int
-handle_signedObject(ACCESS_DESCRIPTION *ad, struct certificate_refs *refs)
+handle_signedObject(struct rpki_uri *uri, void *arg)
 {
-       struct rpki_uri uri;
-       int error;
-
-       /* TODO all three of them probably need this treatment. */
-       error = uri_init_ad(&uri, ad);
-       if (error)
-               return error;
-
-       pr_debug("signedObject: %s", uri.global);
-
-       refs->signedObject = uri.global;
-       uri.global = NULL;
-
-       uri_cleanup(&uri);
-       return error;
+       struct certificate_refs *refs = arg;
+       pr_debug("signedObject: %s", uri->global);
+       refs->signedObject = uri->global;
+       uri->global = NULL;
+       return 0;
 }
 
 static int
@@ -825,7 +809,7 @@ handle_aki_ta(X509_EXTENSION *aki, void *arg)
                        if (extension_equals(aki, other))
                                return 0;
 
-                       return pr_err("The %s is not equal to the %s.",
+                       return pr_err("The '%s' does not equal the '%s'.",
                            AKI.name, SKI.name);
                }
        }
@@ -1012,55 +996,99 @@ end:
        return error;
 }
 
+/**
+ * The RFC does not explain AD validation very well. This is personal
+ * interpretation, influenced by Tim Bruijnzeels's response
+ * (https://mailarchive.ietf.org/arch/msg/sidr/4ycmff9jEU4VU9gGK5RyhZ7JYsQ)
+ * (I'm being a bit more lax than he suggested.)
+ *
+ * 1. Only one NID needs to be searched at a time. (This is currently somewhat
+ *    of a coincidence, and will probably be superseded at some point. But I'm
+ *    not going to complicate this until it's necessary.)
+ * 2. The NID MUST be found, otherwise the certificate is invalid.
+ * 3. The NID can be found more than once.
+ * 4. All access descriptions that match the NID must be URLs.
+ * 5. Precisely one of those matches will be an RSYNC URL, and it's the only one
+ *    we are required to support.
+ *    (I would have gone with "at least one of those matches", but I don't know
+ *    what to do with the other ones.)
+ * 6. Other access descriptions that do not match the NID are allowed and
+ *    supposed to be ignored.
+ * 7. Other access descriptions that match the NID but do not have RSYNC URIs
+ *    are also allowed, and also supposed to be ignored.
+ */
 static int
-handle_aia(X509_EXTENSION *ext, void *arg)
+handle_ad(char *ia_name, SIGNATURE_INFO_ACCESS *ia, char *ad_name, int ad_nid,
+    int (*cb)(struct rpki_uri *, void *), void *arg)
 {
-       struct certificate_refs *refs = arg;
-       AUTHORITY_INFO_ACCESS *aia;
        ACCESS_DESCRIPTION *ad;
+       struct rpki_uri uri;
+       bool found = false;
        int i;
        int error;
 
-       aia = X509V3_EXT_d2i(ext);
-       if (aia == NULL)
-               return cannot_decode(&AIA);
+       for (i = 0; i < sk_ACCESS_DESCRIPTION_num(ia); i++) {
+               ad = sk_ACCESS_DESCRIPTION_value(ia, i);
+               if (OBJ_obj2nid(ad->method) == ad_nid) {
+                       error = uri_init_ad(&uri, ad);
+                       if (error == ENOTRSYNC)
+                               continue;
+                       if (error)
+                               return error;
 
-       /*
-        * RFC 6487:
-        *     an rsync URI [RFC5781] MUST be specified with an accessMethod
-        *     value of id-ad-caIssuers. (...) Other accessMethod URIs
-        *     referencing the same object MAY also be included in the value
-        *     sequence of this extension.
-        *
-        * It doesn't technically say that id-ad-caIssuers is reserved for RSYNC
-        * URIs, but it sure feels like it's the actual intention.
-        *
-        * But let's commit to the literal wording, which I think equals "there
-        * must be a caIssuers RSYNC URI, which must be correct (ie. equal the
-        * manifest's), and there can also be anything else."
-        */
-       for (i = 0; i < sk_ACCESS_DESCRIPTION_num(aia); i++) {
-               ad = sk_ACCESS_DESCRIPTION_value(aia, i);
-               /*
-                * TODO open the call hierarchy of location.
-                * All GEN_URIs should probably be handled the same.
-                */
-               if (OBJ_obj2nid(ad->method) == NID_ad_ca_issuers
-                   && is_rsync_uri(ad->location)) {
-                       /*
-                        * Bringing the parent certificate's URI all the way
-                        * over here is too much trouble, so do the handle_cdp()
-                        * hack.
-                        */
-                       error = ia5s2string(ad->location->d.GN_URI,
-                           &refs->caIssuers);
-                       goto end;
+                       if (found) {
+                               uri_cleanup(&uri);
+                               return pr_err("Extension '%s' has multiple '%s' RSYNC URIs.",
+                                   ia_name, ad_name);
+                       }
+
+                       error = cb(&uri, arg);
+                       if (error) {
+                               uri_cleanup(&uri);
+                               return error;
+                       }
+
+                       uri_cleanup(&uri);
+                       found = true;
                }
        }
 
-       error = pr_err("The certificate lacks a caIssuers RSYNC URI.");
+       if (!found) {
+               pr_err("Extension '%s' lacks a '%s' RSYNC URI.", ia_name,
+                   ad_name);
+               return -ESRCH;
+       }
+
+       return 0;
+}
+
+static int
+handle_caIssuers(struct rpki_uri *uri, void *arg)
+{
+       struct certificate_refs *refs = arg;
+       /*
+        * Bringing the parent certificate's URI all the way
+        * over here is too much trouble, so do the handle_cdp()
+        * hack.
+        */
+       refs->caIssuers = uri->global;
+       uri->global = NULL;
+       return 0;
+}
+
+static int
+handle_aia(X509_EXTENSION *ext, void *arg)
+{
+       AUTHORITY_INFO_ACCESS *aia;
+       int error;
+
+       aia = X509V3_EXT_d2i(ext);
+       if (aia == NULL)
+               return cannot_decode(&AIA);
+
+       error = handle_ad("AIA", aia, "caIssuers", NID_ad_ca_issuers,
+           handle_caIssuers, arg);
 
-end:
        AUTHORITY_INFO_ACCESS_free(aia);
        return error;
 }
@@ -1069,10 +1097,6 @@ static int
 handle_sia_ca(X509_EXTENSION *ext, void *arg)
 {
        SIGNATURE_INFO_ACCESS *sia;
-       ACCESS_DESCRIPTION *ad;
-       bool rsync_found = false;
-       bool manifest_found = false;
-       int i;
        int error;
 
        sia = X509V3_EXT_d2i(ext);
@@ -1080,58 +1104,18 @@ handle_sia_ca(X509_EXTENSION *ext, void *arg)
                return cannot_decode(&SIA);
 
        /* rsync */
-       for (i = 0; i < sk_ACCESS_DESCRIPTION_num(sia); i++) {
-               ad = sk_ACCESS_DESCRIPTION_value(sia, i);
-               if (OBJ_obj2nid(ad->method) == NID_caRepository) {
-                       error = handle_caRepository(ad);
-                       if (error == ENOTRSYNC)
-                               continue;
-                       if (error)
-                               goto end;
-                       rsync_found = true;
-                       break;
-               }
-       }
-
-       if (!rsync_found) {
-               pr_err("SIA extension lacks a caRepository RSYNC URI.");
-               error = -ESRCH;
+       error = handle_ad("SIA", sia, "caRepository", NID_caRepository,
+           handle_caRepository, NULL);
+       if (error)
                goto end;
-       }
 
        /*
         * Store the manifest URI in @mft.
         * (We won't actually touch the manifest until we know the certificate
         * is fully valid.)
         */
-       for (i = 0; i < sk_ACCESS_DESCRIPTION_num(sia); i++) {
-               ad = sk_ACCESS_DESCRIPTION_value(sia, i);
-               if (OBJ_obj2nid(ad->method) == NID_rpkiManifest) {
-                       /*
-                        * rfc6481#section-2.2
-                        * This validation is naive.
-                        * The fact that only one manifest is listed in the SIA
-                        * does not mean that there aren't more or them in the
-                        * publication point.
-                        * But it's all we can do methinks.
-                        */
-                       if (manifest_found) {
-                               error = pr_err("SIA defines more than one manifest.");
-                               goto end;
-                       }
-
-                       error = handle_rpkiManifest(ad, arg);
-                       if (error)
-                               goto end;
-                       manifest_found = true;
-               }
-       }
-
-       /* rfc6481#section-2 */
-       if (!manifest_found) {
-               pr_err("SIA extension lacks an rpkiManifest access description.");
-               error = -ESRCH;
-       }
+       error = handle_ad("SIA", sia, "rpkiManifest",NID_rpkiManifest,
+           handle_rpkiManifest, arg);
 
 end:
        AUTHORITY_INFO_ACCESS_free(sia);
@@ -1142,50 +1126,15 @@ static int
 handle_sia_ee(X509_EXTENSION *ext, void *arg)
 {
        SIGNATURE_INFO_ACCESS *sia;
-       ACCESS_DESCRIPTION *ad;
-       bool signedObject_found = false;
-       int nid;
-       int i;
-       int error = 0;
-
-       /* TODO this is getting convoluted. Make a generic AD foreach. */
+       int error;
 
        sia = X509V3_EXT_d2i(ext);
        if (sia == NULL)
                return cannot_decode(&SIA);
 
-       for (i = 0; i < sk_ACCESS_DESCRIPTION_num(sia); i++) {
-               ad = sk_ACCESS_DESCRIPTION_value(sia, i);
-               nid = OBJ_obj2nid(ad->method);
-
-               if (nid == NID_signedObject) {
-                       error = handle_signedObject(ad, arg);
-                       /* TODO Unknown signedObjects are allowed. */
-                       if (error)
-                               goto end;
-                       signedObject_found = true;
+       error = handle_ad("SIA", sia, "signedObject", NID_signedObject,
+           handle_signedObject, arg);
 
-               } else {
-                       /*
-                        * rfc6487#section-4.8.8.2 says that non-signedObject
-                        * ADs are prohibited, but then 8182 contradicts this.
-                        * And so there's nothing stopping future RFCs from
-                        * defining even more ADs. So shrug them off.
-                        *
-                        * rpkiNotify is known to fall through here, but we
-                        * don't support it yet.
-                        *
-                        * This should be level INFO.
-                        */
-                       pr_debug("EE Certificate has an non-signedObject access description. (NID: %d)",
-                           nid);
-               }
-       }
-
-       if (!signedObject_found)
-               error = pr_err("EE Certificate's SIA lacks a signedObject access description.");
-
-end:
        AUTHORITY_INFO_ACCESS_free(sia);
        return error;
 }

diff --git a/src/object/manifest.c b/src/object/manifest.c
index 5cdf34fe53a670f7d18de0cae20e87d583af4c70..8bd796f47b40b6f5a5e91202e2018148de1f82d6 100644 (file)
--- a/src/object/manifest.c
+++ b/src/object/manifest.c
@@ -118,6 +118,11 @@ __handle_manifest(struct manifest *mft, struct rpp **pp)
                fah = mft->obj->fileList.list.array[i];
 
                error = uri_init_mft(&uri, mft->file_path, &fah->file);
+               /*
+                * Not handling ENOTRSYNC is fine because the manifest URL
+                * should have been RSYNC. Something went wrong if an RSYNC URL
+                * plus a relative path is not RSYNC.
+                */
                if (error)
                        goto fail;
 

diff --git a/src/object/tal.c b/src/object/tal.c
index 14b97193985971d4234896f968e62082cd65b819..429113477d05037f3ceb917dde9c0b1997757d3a 100644 (file)
--- a/src/object/tal.c
+++ b/src/object/tal.c
@@ -9,6 +9,7 @@
 #include <sys/stat.h>
 #include <openssl/evp.h>
 
+#include "common.h"
 #include "line_file.h"
 #include "log.h"
 #include "random.h"
@@ -204,6 +205,11 @@ foreach_uri(struct tal *tal, foreach_uri_cb cb)
 
        for (i = 0; i < tal->uris.count; i++) {
                error = uri_init_str(&uri, tal->uris.array[i]);
+               if (error == ENOTRSYNC) {
+                       /* Log level should probably be INFO. */
+                       pr_debug("TAL has non-RSYNC URI; ignoring.");
+                       continue;
+               }
                if (error)
                        return error;
 

diff --git a/src/uri.c b/src/uri.c
index 8fa68e13984a5bb14711797e9006cfd3952cd2e2..ce593e786ae41da3345f2e3ec1fd2b7f4c2614cc 100644 (file)
--- a/src/uri.c
+++ b/src/uri.c
@@ -69,7 +69,6 @@ succeed:
  *
  * By contract, if @guri is not RSYNC, this will return ENOTRSYNC.
  * This often should not be treated as an error; please handle gracefully.
- * TODO open call hirarchy.
  */
 static int
 g2l(char const *global, size_t global_len, char **result)

diff --git a/src/uri.h b/src/uri.h
index e5e6b7c838a0d7ff4ecc517fc6dede45381aef9d..de2bc476d7052a7a63e3a363d470c33c511971a4 100644 (file)
--- a/src/uri.h
+++ b/src/uri.h
@@ -7,6 +7,8 @@
 
 /**
  * These are expected to live on the stack, or as part of other objects.
+ *
+ * All rpki_uris are guaranteed to be RSYNC URLs right now.
  */
 struct rpki_uri {
        /**