Set APC correctly for community updates, reject routes for non-APC comms

When an APC community update is received, the "apcs" list is empty. The
APC for any realms described by that update should be the APC community
itself.

Also, the trust router previously accepted any realm/community pairs for
routing. That should have been APC communities only.

Finally, this also prevents configuring multiple communities with the
same ID.

diff --git a/common/tr_comm.c b/common/tr_comm.c
index e915b70..56578b9 100644 (file)
--- a/common/tr_comm.c
+++ b/common/tr_comm.c
@@ -1365,11 +1365,24 @@ TR_COMM *tr_comm_table_find_comm(TR_COMM_TABLE *ctab, TR_NAME *comm_id)
   return tr_comm_lookup(ctab->comms, comm_id);
 }
 
-void tr_comm_table_add_comm(TR_COMM_TABLE *ctab, TR_COMM *new)
+/**
+ * Add a community to the table.
+ *
+ * Does not allow duplicate community ids.
+ *
+ * @param ctab
+ * @param new
+ * @return 0 on success, -1 on failure
+ */
+int tr_comm_table_add_comm(TR_COMM_TABLE *ctab, TR_COMM *new)
 {
+  if (tr_comm_table_find_comm(ctab, tr_comm_get_id(new)) != NULL)
+    return -1;
+
   tr_comm_add(ctab->comms, new);
   if (ctab->comms!=NULL)
     talloc_steal(ctab, ctab->comms); /* make sure it's in the right context */
+  return 0;
 }
 
 void tr_comm_table_remove_comm(TR_COMM_TABLE *ctab, TR_COMM *comm)

diff --git a/common/tr_config_comms.c b/common/tr_config_comms.c
index 60b5f93..99e2070 100644 (file)
--- a/common/tr_config_comms.c
+++ b/common/tr_config_comms.c
@@ -323,10 +323,13 @@ TR_CFG_RC tr_cfg_parse_comms (TR_CFG *trc, json_t *jcfg)
                                                &rc))) {
         return rc;
       }
+      if (tr_comm_table_add_comm(trc->ctable, comm) != 0) {
+        tr_debug("tr_cfg_parse_comms: Duplicate community %s.", tr_comm_get_id(comm)->buf);
+        return TR_CFG_NOPARSE;
+      }
+
       tr_debug("tr_cfg_parse_comms: Community configured: %s.",
                tr_comm_get_id(comm)->buf);
-
-      tr_comm_table_add_comm(trc->ctable, comm);
     }
   }
   tr_debug("tr_cfg_parse_comms: Finished (rc=%d)", rc);

diff --git a/include/tr_comm.h b/include/tr_comm.h
index 6228c1f..4004346 100644 (file)
--- a/include/tr_comm.h
+++ b/include/tr_comm.h
@@ -113,7 +113,7 @@ void tr_comm_table_free(TR_COMM_TABLE *ctab);
 TR_COMM_TABLE *tr_comm_table_new(TALLOC_CTX *mem_ctx);
 void tr_comm_table_free(TR_COMM_TABLE *ctab);
 void tr_comm_table_sweep(TR_COMM_TABLE *ctab);
-void tr_comm_table_add_comm(TR_COMM_TABLE *ctab, TR_COMM *new);
+int tr_comm_table_add_comm(TR_COMM_TABLE *ctab, TR_COMM *new);
 void tr_comm_table_remove_comm(TR_COMM_TABLE *ctab, TR_COMM *comm);
 TR_RP_REALM *tr_comm_table_find_rp_realm(TR_COMM_TABLE *ctab, TR_NAME *realm_id);
 void tr_comm_table_add_rp_realm(TR_COMM_TABLE *ctab, TR_RP_REALM *new);

diff --git a/trp/trps.c b/trp/trps.c
index 83461d9..a0aff37 100644 (file)
--- a/trp/trps.c
+++ b/trp/trps.c
@@ -656,36 +656,51 @@ static TRP_RC trps_accept_update(TRPS_INSTANCE *trps, TRP_UPD *upd, TRP_INFOREC
 static TRP_RC trps_handle_inforec_route(TRPS_INSTANCE *trps, TRP_UPD *upd, TRP_INFOREC *rec)
 {
   TRP_ROUTE *route=NULL;
+  TR_COMM *comm = NULL;
   unsigned int feas=0;
 
   /* determine feasibility */
   feas=trps_check_feasibility(trps, trp_upd_get_realm(upd), trp_upd_get_comm(upd), rec);
   tr_debug("trps_handle_update: record feasibility=%d", feas);
 
-  /* do we have an existing route? */
-  route=trps_get_route(trps,
-                       trp_upd_get_comm(upd),
-                       trp_upd_get_realm(upd),
-                       trp_upd_get_peer(upd));
-  if (route!=NULL) {
-    /* there was a route table entry already */
-    tr_debug("trps_handle_updates: route entry already exists.");
-    if (feas) {
-      /* Update is feasible. Accept it. */
-      trps_accept_update(trps, upd, rec);
-    } else {
-      /* Update is infeasible. Ignore it unless the trust router has changed. */
-      if (0!=tr_name_cmp(trp_route_get_trust_router(route),
-                         trp_inforec_get_trust_router(rec))) {
-        /* the trust router associated with the route has changed, treat update as a retraction */
-        trps_retract_route(trps, route);
+  /* verify that the community is an APC */
+  comm = tr_comm_table_find_comm(trps->ctable, trp_upd_get_comm(upd));
+  if (comm == NULL) {
+    /* We don't know this community. Reject the route. */
+    tr_debug("trps_handle_updates: community %.*s unknown, ignoring route for %.*s",
+             trp_upd_get_comm(upd)->len, trp_upd_get_comm(upd)->buf,
+             trp_upd_get_realm(upd)->len, trp_upd_get_realm(upd)->buf);
+  } else if (tr_comm_get_type(comm) != TR_COMM_APC) {
+    /* The community in a route request *must* be an APC. This was not - ignore it. */
+    tr_debug("trps_handle_updates: community %.*s is not an APC, ignoring route for %.*s",
+             trp_upd_get_comm(upd)->len, trp_upd_get_comm(upd)->buf,
+             trp_upd_get_realm(upd)->len, trp_upd_get_realm(upd)->buf);
+  } else {
+    /* do we have an existing route? */
+    route=trps_get_route(trps,
+                         trp_upd_get_comm(upd),
+                         trp_upd_get_realm(upd),
+                         trp_upd_get_peer(upd));
+    if (route!=NULL) {
+      /* there was a route table entry already */
+      tr_debug("trps_handle_updates: route entry already exists.");
+      if (feas) {
+        /* Update is feasible. Accept it. */
+        trps_accept_update(trps, upd, rec);
+      } else {
+        /* Update is infeasible. Ignore it unless the trust router has changed. */
+        if (0!=tr_name_cmp(trp_route_get_trust_router(route),
+                           trp_inforec_get_trust_router(rec))) {
+          /* the trust router associated with the route has changed, treat update as a retraction */
+          trps_retract_route(trps, route);
+        }
       }
+    } else {
+      /* No existing route table entry. Ignore it unless it is feasible and not a retraction. */
+      tr_debug("trps_handle_update: no route entry exists yet.");
+      if (feas && trp_metric_is_finite(trp_inforec_get_metric(rec)))
+        trps_accept_update(trps, upd, rec);
     }
-  } else {
-    /* No existing route table entry. Ignore it unless it is feasible and not a retraction. */
-    tr_debug("trps_handle_update: no route entry exists yet.");
-    if (feas && trp_metric_is_finite(trp_inforec_get_metric(rec)))
-      trps_accept_update(trps, upd, rec);
   }
 
   return TRP_SUCCESS;
@@ -771,7 +786,7 @@ cleanup:
   return comm;
 }
 
-static TR_RP_REALM *trps_create_new_rp_realm(TALLOC_CTX *mem_ctx, TR_NAME *realm_id, TRP_INFOREC *rec)
+static TR_RP_REALM *trps_create_new_rp_realm(TALLOC_CTX *mem_ctx, TR_NAME *comm, TR_NAME *realm_id, TRP_INFOREC *rec)
 {
   TALLOC_CTX *tmp_ctx=talloc_new(NULL);
   TR_RP_REALM *rp=tr_rp_realm_new(tmp_ctx);
@@ -794,11 +809,15 @@ cleanup:
   return rp;
 }
 
-static TR_IDP_REALM *trps_create_new_idp_realm(TALLOC_CTX *mem_ctx, TR_NAME *realm_id, TRP_INFOREC *rec)
+static TR_IDP_REALM *trps_create_new_idp_realm(TALLOC_CTX *mem_ctx,
+                                               TR_NAME *comm_id,
+                                               TR_NAME *realm_id,
+                                               TRP_INFOREC *rec)
 {
   TALLOC_CTX *tmp_ctx=talloc_new(NULL);
   TR_IDP_REALM *idp=tr_idp_realm_new(tmp_ctx);
-  
+  TR_APC *realm_apcs = NULL;
+
   if (idp==NULL) {
     tr_debug("trps_create_new_idp_realm: unable to allocate new realm.");
     goto cleanup;
@@ -810,14 +829,52 @@ static TR_IDP_REALM *trps_create_new_idp_realm(TALLOC_CTX *mem_ctx, TR_NAME *rea
     idp=NULL;
     goto cleanup;
   }
-  if (trp_inforec_get_apcs(rec)!=NULL) {
-    tr_idp_realm_set_apcs(idp, tr_apc_dup(tmp_ctx, trp_inforec_get_apcs(rec)));
-    if (tr_idp_realm_get_apcs(idp)==NULL) {
-      tr_debug("trps_create_new_idp_realm: unable to allocate APC list.");
-      idp=NULL;
+
+  /* Set the APCs. If the community is a CoI, copy its APCs. If it is an APC, then
+   * that community itself is the APC for the realm. */
+  if (trp_inforec_get_comm_type(rec) == TR_COMM_APC) {
+    /* the community is an APC for this realm */
+    realm_apcs = tr_apc_new(tmp_ctx);
+    if (realm_apcs == NULL) {
+      tr_debug("trps_create_new_idp_realm: unable to allocate new APC list.");
+      idp = NULL;
+      goto cleanup;
+    }
+
+    tr_apc_set_id(realm_apcs, tr_dup_name(comm_id));
+    if (tr_apc_get_id(realm_apcs) == NULL) {
+      tr_debug("trps_create_new_idp_realm: unable to allocate new APC name.");
+      idp = NULL;
+      goto cleanup;
+    }
+  } else {
+    /* the community is not an APC for this realm */
+    realm_apcs = trp_inforec_get_apcs(rec);
+    if (realm_apcs == NULL) {
+      tr_debug("trps_create_new_idp_realm: no APCs for realm %.*s/%.*s, cannot add.",
+               realm_id->len, realm_id->buf,
+               comm_id->len, comm_id->buf);
+      idp = NULL;
+      goto cleanup;
+    }
+
+    /* we have APCs, make our own copy */
+    realm_apcs = tr_apc_dup(tmp_ctx, realm_apcs);
+    if (realm_apcs == NULL) {
+      tr_debug("trps_create_new_idp_realm: unable to duplicate APC list.");
+      idp = NULL;
       goto cleanup;
     }
   }
+
+  /* Whether the community is an APC or CoI, the APCs for the realm are in realm_apcs */
+  tr_idp_realm_set_apcs(idp, realm_apcs); /* takes realm_apcs out of tmp_ctx on success */
+  if (tr_idp_realm_get_apcs(idp) == NULL) {
+    tr_debug("trps_create_new_idp_realm: unable to set APC list for new realm.");
+    idp=NULL;
+    goto cleanup;
+  }
+
   idp->origin=TR_REALM_DISCOVERED;
   
   talloc_steal(mem_ctx, idp);
@@ -867,7 +924,11 @@ static TRP_RC trps_handle_inforec_comm(TRPS_INSTANCE *trps, TRP_UPD *upd, TRP_IN
         tr_debug("trps_handle_inforec_comm: unable to create new community.");
         goto cleanup;
       }
-      tr_comm_table_add_comm(trps->ctable, comm);
+      if (tr_comm_table_add_comm(trps->ctable, comm) != 0)
+      {
+        tr_debug("trps_handle_inforec_comm: unable to add community to community table.");
+        goto cleanup;
+      }
     }
     /* TODO: see if other comm data match the new inforec and update or complain */
 
@@ -881,7 +942,7 @@ static TRP_RC trps_handle_inforec_comm(TRPS_INSTANCE *trps, TRP_UPD *upd, TRP_IN
       if (rp_realm==NULL) {
         tr_debug("trps_handle_inforec_comm: unknown RP realm %.*s in inforec, creating it.",
                  realm_id->len, realm_id->buf);
-        rp_realm=trps_create_new_rp_realm(tmp_ctx, realm_id, rec);
+        rp_realm= trps_create_new_rp_realm(tmp_ctx, tr_comm_get_id(comm), realm_id, rec);
         if (rp_realm==NULL) {
           tr_debug("trps_handle_inforec_comm: unable to create new RP realm.");
           /* we may leave an unused community in the table, but it will only last until
@@ -902,7 +963,7 @@ static TRP_RC trps_handle_inforec_comm(TRPS_INSTANCE *trps, TRP_UPD *upd, TRP_IN
       if (idp_realm==NULL) {
         tr_debug("trps_handle_inforec_comm: unknown IDP realm %.*s in inforec, creating it.",
                  realm_id->len, realm_id->buf);
-        idp_realm=trps_create_new_idp_realm(tmp_ctx, realm_id, rec);
+        idp_realm= trps_create_new_idp_realm(tmp_ctx, tr_comm_get_id(comm), realm_id, rec);
         if (idp_realm==NULL) {
           tr_debug("trps_handle_inforec_comm: unable to create new IDP realm.");
           /* we may leave an unused community in the table, but it will only last until