On Wed, Nov 16, 2022 at 02:52:59PM +0100, Theo Buehler wrote:
> On Wed, Nov 16, 2022 at 12:47:46PM +0100, Claudio Jeker wrote:
> > ASPA sets are the 2nd thing that will be generated out of rpki (and
> > rpki-client). ASPA is used to further prevent leaks by providing customer
> > provider attestation (and blocking paths which violate these attestations).
> >
> > I started to implement this and while this code does no checks yet it is
> > enough plumbing to parse and "merge" the ASPA sets. Similar to the roa-set
> > an aspa-set in the config will be parsed and passed to the rtr process
> > where it will be merged with the data from RTR sessions.
> > What is missing is all the code needed in the RDE to do the lookups.
> >
> > A aspa-set is defined like this:
> > aspa-set {
> > source-as 1 transit-as { 5 }
> > source-as 2 expires 1668181648 transit-as { 3 4 }
> > source-as 5 transit-as { 1 2 allow inet 7 allow inet6 }
> > }
>
> Adding this to the end of the default bgpd.conf I get
>
> /tmp/bgpd.conf:134: syntax error, expires not allowed
> /tmp/bgpd.conf:136: syntax error
>
> Deleting the expire time, it parses fine. I suspect the duplicate
> aspa_set rules in parse.y don't work as intended.
No this is actually the roa-set code that triggers this. It changes
noexpires to 1 and then after that aspa-set fails.
I changed to code to set noexpires in the origin-set case (where expires
is not allowed) and then set it back to 0 when leaving that node.
> >
> > As usual more than one aspa-set can be used and these sets will all be
> > merged into one table.
> >
> > Since this is already a lot of code to fiddle with various rb trees and
> > doing various realloc games I would like to get this reviewed and
> > committed before adding a lot more code.
> >
> > Once this is in rpki-client can be adjusted to produce an aspa-set in the
> > openbgpd output.
>
> Generally looks good. The bitmask is a bit of a hack, but I guess we
> can live with it.
My plan is to compress the bitmask before sending it over but because of
the memmove()s it is easier to work with bytes initially.
> A few comments and questions below.
See below
> > @@ -586,6 +597,55 @@ roa_set_l : prefixset_item SOURCEAS as4n
> > }
> > ;
> >
> > +aspa_set : ASPASET '{' optnl aspa_set_l optnl '}'
> > + | ASPASET '{' optnl '}'
> > + ;
> > +
> > +aspa_set_l : aspa_set
> > + | aspa_set_l comma aspa_set
> > + ;
> > +
> > +aspa_set : SOURCEAS as4number expires TRANSITAS '{' optnl
>
> aspa_set was already defined a few lines up. Is this legal yacc? As
> mentioned above, it doesn't seem to work.
Magic. I guess it works because the one look-ahead is able to distinguish
the two. Need to fix this though.
> > +static int
> > +merge_aspa_set(uint32_t as, struct aspa_tas_l *tas, time_t expires)
> > +{
> > + struct aspa_set *aspa, needle = { .as = as };
> > + uint32_t i, num, *newtas;
> > + uint8_t *newtasaid = NULL;
> > +
> > + aspa = RB_FIND(aspa_tree, &conf->aspa, &needle);
> > + if (aspa == NULL) {
> > + if ((aspa = calloc(1, sizeof(*aspa))) == NULL) {
> > + yyerror("out of memory");
> > + return -1;
> > + }
> > + aspa->as = as;
> > + RB_INSERT(aspa_tree, &conf->aspa, aspa);
> > + }
> > +
> > + num = aspa->num + tas->num;
>
> Are these numbers guaranteed to be small enough that we don't overflow
> the 32-bit num? Also, it's not clear to me that num > 0 is guaranteed
> (which seems to be assumed in the for loop below).
ASnumers are 32-bit only. So without duplicates no overflow should happen.
Now if one builds a dumb config it would be possible to overflow this.
I think the avage num here will be around 3-4 and the max will be < 1000.
Adding an extra check here may be appropriate.
> > + newtas = recallocarray(aspa->tas, aspa->num, num, sizeof(uint32_t));
> > + if (newtas == NULL) {
> > + yyerror("out of memory");
> > + return -1;
> > + }
> > + if (aspa->tas_aid) {
>
> I don't really understand the reason for this conditional. Are aspa
> sets with all tas->aid == AFI_UNSPEC common enough to warrant such extra
> dances? Doing this recallocarray() unconditionally would allow us to
> remove the calloc() in the somewhat tricky loop below.
I may drop this here and do the compression in the RTR code.
I expect that 99% of all ASPA entries to be AFI_UNSPEC (as in both IPv4
and IPv6). But since this is both done here and in the RTR code it may be
sensible to just compress at the end.
> > + newtasaid = recallocarray(aspa->tas_aid, aspa->num, num, 1);
>
> extra space after =
fixed
> > + if (newtasaid == NULL) {
> > + free(newtas);
> > + yyerror("out of memory");
> > + return -1;
> > + }
> > + }
> > +
> > + /* fill starting at the end since the tas list is reversed */
> > + for (i = num - 1; tas; tas = tas->next) {
>
> I'd move the i-- here:
>
> for (i = num - 1; tas; tas = tas->next, i--) {
Sure.
> > + newtas[i] = tas->as;
> > + if (tas->aid != AFI_UNSPEC) {
>
> typo: AID_UNSPEC?
Yes, result is the same.
> > @@ -274,6 +296,41 @@ rtr_dispatch_imsg_parent(struct imsgbuf
> > fatalx("IMSG_RECONF_ROA_ITEM bad len");
> > roa_insert(&nconf->roa, imsg.data);
> > break;
> > + case IMSG_RECONF_ASPA:
> > + if (imsg.hdr.len - IMSG_HEADER_SIZE !=
> > + offsetof(struct aspa_set, tas))
> > + fatalx("IMSG_RECONF_ASPA bad len");
> > + if (aspa != NULL)
> > + fatalx("unexpected IMSG_RECONF_ASPA");
> > + if ((aspa = calloc(1, sizeof(*aspa))) == NULL)
> > + fatal("aspa alloc");
>
> Worth switching to reallocarray(NULL, ...) to avoid zeroing and immediately
> overwriting all of it?
It is not overwriting all. Only the bytes up to the tas.
memcpy uses offsetof(struct aspa_set, tas) not sizeof(*aspa).
> > + memcpy(aspa, imsg.data, offsetof(struct aspa_set, tas));
> > + break;
> > + case IMSG_RECONF_ASPA_TAS:
> > + if (imsg.hdr.len - IMSG_HEADER_SIZE !=
> > + aspa->num * sizeof(*aspa->tas))
> > + fatalx("IMSG_RECONF_ASPA_TAS bad len");
> > + aspa->tas = calloc(aspa->num, sizeof(*aspa->tas));
>
> ditto
Here reallocarray(NULL, ...) makes sense.
> > + if (aspa->tas == NULL)
> > + fatal("aspa tas alloc");
> > + memcpy(aspa->tas, imsg.data,
> > + aspa->num * sizeof(*aspa->tas));
> > + break;
> > + case IMSG_RECONF_ASPA_TAS_AID:
> > + if (imsg.hdr.len - IMSG_HEADER_SIZE != aspa->num)
> > + fatalx("IMSG_RECONF_ASPA_TAS_AID bad len");
> > + aspa->tas_aid = malloc(aspa->num);
>
> here you use malloc.
>
> > + if (aspa->tas_aid == NULL)
> > + fatal("aspa tas aid alloc");
> > + memcpy(aspa->tas_aid, imsg.data, aspa->num);
> > + break;
> > +static void
> > +asap_set_entry(struct aspa_set *aspa, uint32_t asnum, uint8_t aid)
>
> typo: aspa_set_entry()
Grrrr. I constanly type asap instead of aspa.
> > +{
> > + uint32_t i, num, *newtas;
> > + uint8_t *newtasaid;
> > +
> > + switch (aid) {
> > + case AID_INET:
> > + aid = 0x1;
> > + break;
> > + case AID_INET6:
> > + aid = 0x2;
> > + break;
> > + case AID_UNSPEC:
> > + aid = 0x3;
>
> This is not yet untangled, right? Or did I miss it?
Yeah, up to here the table could have two entries (one with AID_INET and
one with AID_INET6) but we want to combine them into one entry allowing
both.
I do this here so I can use a simple |= to combine equal entries.
> > + break;
> > + default:
> > + fatalx("aspa_set bad AID");
> > + }
> > +
> > + for (i = 0; i < aspa->num; i++) {
> > + if (asnum < aspa->tas[i] || aspa->tas[i] == 0)
> > + break;
> > + if (asnum == aspa->tas[i]) {
> > + aspa->tas_aid[i] |= aid;
> > + return;
> > + }
> > + }
> > +
> > + num = aspa->num + 1;
> > + newtas = recallocarray(aspa->tas, aspa->num, num, sizeof(uint32_t));
> > + newtasaid = recallocarray(aspa->tas_aid, aspa->num, num, 1);
> > + if (newtas == NULL || newtasaid == NULL)
> > + fatal("aspa_set merge");
> > +
> > + if (i < aspa->num) {
> > + memmove(newtas + i + 1, newtas + i,
> > + (aspa->num - i) * sizeof(uint32_t));
> > + memmove(newtasaid + i + 1, newtasaid + i, (aspa->num - i));
> > + }
>
> Phew. looks correct to me.
yeah, not happy. The ASPA spec is overengineered to make the implementors
lives hard.
> > + newtas[i] = asnum;
> > + newtasaid[i] = aid;
> > +
> > + aspa->num = num;
> > + aspa->tas = newtas;
> > + aspa->tas_aid = newtasaid;
> > +}
> > +
> > +static void
> > +rtr_aspa_merge_set(struct aspa_tree *a, struct aspa_set *mergeset)
> > +{
> > + struct aspa_set *aspa, needle = { .as = mergeset->as };
> > + uint32_t i;
> > +
> > + aspa = RB_FIND(aspa_tree, a, &needle);
> > + if (aspa == NULL) {
> > + if ((aspa = calloc(1, sizeof(*aspa))) == NULL)
> > + fatal("aspa insert");
> > + aspa->as = mergeset->as;
> > + RB_INSERT(aspa_tree, a, aspa);
> > + }
> > +
> > + for (i = 0; i < mergeset->num; i++) {
> > + uint8_t aid = AID_UNSPEC;
> > + if (mergeset->tas_aid != NULL)
> > + aid = mergeset->tas_aid[i];
>
> Again, I'm not quite sure what allowing mergeset->tas_aid == NULL buys us.
> We rely on the fact that AFI_UNSPEC == AID_UNSPEC == 0 when not setting
> them in parse.y.
As mentioned above I will do the reduction in the last step.
--
:wq Claudio
