On Sat, Sep 06, 2025 at 10:19:38AM +0200, Theo Buehler wrote:
> > Here is a variant that is more consistent with the other *State fields:
> 
> I like this better

thank you for your feedback!

> ok tb
> 
> One bug and some minor notes:
> 
> > +IMPLEMENT_ASN1_FUNCTIONS(SubjectKeyIdentifier);
> 
> This is justified because the abstraction in libcrypto is very error
> prone and quite difficult to use.
> 
> > +
> > +ASN1_SEQUENCE(TrustAnchorState) = {
> > +	ASN1_SEQUENCE_OF(TrustAnchorState, taskis, ASN1_OCTET_STRING),
> 
> This should be
> 
> 	ASN1_SEQUENCE_OF(TrustAnchorState, taskis, SubjectKeyIdentifier),

That's what I had initially, but using SubjectKeyIdentifier here
(instead of ASN1_OCTET_STRING) seems to lead to different DER encoding:

    $ cat /var/db/rpki-client/rpki.ccr | der2ascii | tail -n 16 | head -n 12
        [4] {
          SEQUENCE {
            SEQUENCE {
              [UNIVERSAL 0 PRIMITIVE] { `0b9cca90dd0d7a8a37666b19217fe0d84037b7a2` }
              [UNIVERSAL 0 PRIMITIVE] { `13d4f24f9a9fcd98db36f930631808c88f3974bc` }
              [UNIVERSAL 0 PRIMITIVE] { `e8552b1fd6d1a4f7e404c6d8e5680d1ebc163fc3` }
              [UNIVERSAL 0 PRIMITIVE] { `eb680f38f5d6c71bb4b106b8bd06585012da31b6` }
              [UNIVERSAL 0 PRIMITIVE] { `fc8a9cb3ed184e17d30eea1e0fa7615ce4b1af47` }
            }
            OCTET_STRING { `b9ba66b2bcd54e4812249f60ed2de9357670cc48ff848f1bc35f5986703de71f` }
          }
        }

> then this matches what you do later:
> 
> > +	if ((ski = SubjectKeyIdentifier_new()) == NULL)
> > +		errx(1, "SubjectKeyIdentifier_new");
> > +
> > +	if (!ASN1_OCTET_STRING_set(ski, taski->keyid, sizeof(taski->keyid)))
> 
> Might warrant a SubjectKeyIdentifier_set() wrapper that also checks the
> length, but it might be overdoing it and I'm ok with this as it is (the
> sizeof already ensures the correct size).

checks the length of what exactly?

> > +	assert(cert->aki == NULL && cert->aia == NULL);
> 
> The "cert->aki == NULL &&" bit of the assert needs to be removed:
> The AKI is allowed for TAs (but it needs to be in a form matching the
> SKI). It is in fact used by the NRO TA, so it hits this assert because
> we send p->aki from the parser to the main process in cert_{write,read}().

yup, thanks, removed the AKI condition within the assert() in my tree.