refactor: Generalize csr/crl signed_by to take &impl AsRef issuer

[audunhalland]

Completes #307 by applying the same treatment to csr and crl.

Also improves CertificateParams::signed_by issuer from AsRef<Self> to AsRef<CertificateParams>, because I think it's irrelevant that the issuer is the same type as the CertificateParams being signed, thus Self should not be used.

[djc]

Makes sense, thanks!

[djc]

Nit: let's stick to Self here?

[audunhalland]

Hmm, I tried to make an explanation of this change in the PR summary.

[cpu]

The argument you made seems reasonable to me in absence of a reason to prefer keeping it as-is.

[cpu]

Looks good, thank you :-)

[cpu]

Since we discovered this is a breaking change (#324) and reverted 0.13.3 we need to think about a path forward.

We could:

1. Revert this, and re-introduce it in a semver compatible way
2. Re-version main as 0.14 and document it as a breaking change
3. Something else?

I'm not sure I have a strong opinion. Initially I was thinking option 2, but perhaps it's best to wait until we have more breaking changes before incrementing that version component to avoid downstream churn?

@djc @est31 @audunhalland Do you folks have thoughts?

[est31]

So #324 was about AsRef creating problems with Arc<Certificate>, while &Certificate works well with it. I guess the limitation here is that the compiler doesn't attempt AsRef two times, and going from Arc<Certificate> to &Certificate to &CertificateParams takes two steps.

Then #307 is similarly a breaking change, it is not that different from this PR.

I think it's better to have a more generic API than a more restricted one, so the change itself makes sense. But looking at the releases page, the 0.13 release is from December 2024, so from three months ago. Making another breaking change now would be a little bit fast I think.

In the past I would have been "who cares, make a release", but rcgen has grown in size, and most of our downloads are still in the "other" category from before 0.13. So maybe we can make an issue with a wishlist of breaking changes and put the AsRef stuff there. Then in 3-6 months if there is nothing else, we can make a semver breaking release, even if there is no other breakage we want to do. One or two semver breaking releases per year are okay for libraries the kind of rcgen I think.

[audunhalland]

I consider AsRef an API "hack" and wasn't sure if semver was violated. Indeed it was. Sorry for not catching it upfront.

Ideally I'd prefer not having AsRef hacks, but instead take issuer: &CertificateParams directly, which is a simpler API than a generic one. That way you'd need to do cert.signed_by(public_key, issuer.params(), issuer_key) when the issuer is a Certificate.

The other option that works now and is backwards compatible is of course to add another set of methods: signed_by_params. But not very pretty.

[djc]

I don't like the idea of releasing 0.14 only for this change much -- too soon after the previous semver-incompatible release, for too little gain. @audunhalland if you'd like this API to become available sooner, adding it under a different method name could make sense for the 0.13.x line.

I also want to spend more time cleaning up the issuer API to make it clearer/safer for this scenario of parsing an existing CA, and when that happens the simple change here might not make sense anymore.

[cpu]

It sounds like we all agree that we don't want to make a 0.14 just for this change.

I propose we revert the two (?) AsRef changes for now, file issues for a semver breaking "proper" fix, and then review/merge a side-by-side new functions that offer this in the short term. We can cut a 0.13.1 after the revert, or after the new side-by-side fns are available.

Sounds good?

[audunhalland]

Reverting those two changes is fine by me, I can prepare another PR with alternative methods.