Fix regression in i2d_re_X509_REQ_tbs()

[t8m]

This fixes regression from commit 8e39049.

Fixes #19297

[DDvO]

LGTM

[DDvO]

Just found in crypto/x509/x509cset.c:

int i2d_re_X509_CRL_tbs(X509_CRL *crl, unsigned char **pp)
{
    crl->crl.enc.modified = 1;
    return i2d_X509_CRL_INFO(&crl->crl, pp);
}

that at least for some related data types the modified flag is set before calling i2d_*, apparently to be on the safe side.
Maybe such apparent workarounds are no more needed after the adaptations I added yesterday to #19271 ?

[davidben]

Should this have a unit test?

[t8m]

Should this have a unit test?

It should. However IMO this falls under much bigger effort to write the test than the actual fix. We do not have any unit test for i2d_re_* functions nor for the i2d_X509_REQ() function. I could squeeze it in x509_check_cert_pkey_test.c easily, on the other hand it is a little bit weird to piggy-back on quite unrelated test.

[no6v]

Thanks for your quick works.

Just found in crypto/x509/x509cset.c:

int i2d_re_X509_CRL_tbs(X509_CRL *crl, unsigned char **pp)
{
    crl->crl.enc.modified = 1;
    return i2d_X509_CRL_INFO(&crl->crl, pp);
}

that at least for some related data types the modified flag is set before calling i2d_*, apparently to be on the safe side.

The "re" in i2d_re_X509_tbs stands for "re-encode", and ensures that a
fresh encoding is generated in case the object has been modified after
creation (..snip..)

If, after modification, the X509 object is re-signed with X509_sign(),
the encoding is automatically renewed. Otherwise, the encoding of the
TBSCertificate portion of the X509 can be manually renewed by calling
i2d_re_X509_tbs().

My understanding from reading the above part of manpage is that setting the modified flag before calling relevant functions tells them to invalidate the internal cache, so the renewed TBS portion will be encoded.

Maybe such apparent workarounds are no more needed after the adaptations I added yesterday to #19271 ?

If your work resolves this situation, that will be great, thanks.

[davidben]

More reason to write a test, since it seems 8e39049 regressed more than just the return code! :-) (It used to be in the right order, but then 8e39049 broke it.)

[t8m]

More reason to write a test, since it seems 8e39049 regressed more than just the return code! :-) (It used to be in the right order, but then 8e39049 broke it.)

Feel free to write one.

[no6v]

de54030 looks good to me, thanks!

[t8m]

@DDvO still OK?
ping @openssl/otc for otc review.

[DDvO]

@DDvO still OK?

Yep

[openssl-machine]

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually.

[t8m]

Merged to master and 3.0 branches. Thank you for the reviews.