On Sat, Sep 06, 2025 at 07:38:08AM +0000, Job Snijders wrote:
> Here is a variant that is more consistent with the other *State fields:

I like this better

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),

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).

> +	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}().