deps: rcgen 0.12 -> 0.13

[cpu]

• updates rcgen dev dependency to 0.13
• fixes breaking upstream changes
• also fixes an unrelated nightly clippy err in a separate commit

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.22%. Comparing base (97f0364) to head (9a55677).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #239      +/-   ##
==========================================
+ Coverage   97.19%   97.22%   +0.02%     
==========================================
  Files          19       19              
  Lines        4065     4104      +39     
==========================================
+ Hits         3951     3990      +39     
  Misses        114      114              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[djc]

I think you can take more advantage of the impl From<Certificate> for CertificateDer<'static> that we have in rcgen... called out a bunch of places but it looks like there are a lot more.

[djc]

Oh, so we don't have ownership of the Certificate in all those places? Seems surprising.

[cpu]

Oh, so we don't have ownership of the Certificate in all those places?

We have ownership. I'll try again 🤔

[cpu]

I'll try again 🤔

I don't think there's any way to do this without clone or using to_owned instead of into_owned. Here's a minimal reproducer annotated with types. Everything is owned up until the DER we borrow from Certificate::der().

Code and error:

    #[test]
    fn test_it() {
        let cert_and_key: rcgen::CertifiedKey = make_issuer("Bogus Subject");
        let cert: rcgen::Certificate = cert_and_key.cert;
        let cert_der_borrowed: &CertificateDer<'static> = cert.der();
        let cert_der_owned: CertificateDer<'static> = cert_der_borrowed.into_owned();
    }

error[E0507]: cannot move out of `*cert_der_borrowed` which is behind a shared reference
   --> src/verify_cert.rs:789:50
    |
789 |         let cert_der_owned: CertificateDer<'_> = cert_der_borrowed.into_owned();
    |                                                  ^^^^^^^^^^^^^^^^^ ------------ `*cert_der_borrowed` moved due to this method call
    |                                                  |
    |                                                  move occurs because `*cert_der_borrowed` has type `rustls_pki_types::CertificateDer<'_>`, which does not implement the `Copy` trait
    |
note: `rustls_pki_types::CertificateDer::<'_>::into_owned` takes ownership of the receiver `self`, which moves `*cert_der_borrowed`
   --> /home/daniel/.cargo/registry/src/index.crates.io-6f17d22bba15001f/rustls-pki-types-1.4.1/src/lib.rs:498:23
    |
498 |     pub fn into_owned(self) -> CertificateDer<'static> {
    |                       ^^^^
help: you can `clone` the value and consume it, but this might not be your desired behavior
    |
789 |         let cert_der_owned: CertificateDer<'_> = <rustls_pki_types::CertificateDer<'_> as Clone>::clone(&cert_der_borrowed).into_owned();
    |                                                  ++++++++++++++++++++++++++++++++++++++++++++++++++++++++                 +

[cpu]

Maybe in rcgen Certificate::der() should be returning CertificateDer<'static> by cloning its owned CertificateDer<'static> member instead of returning a &CertificateDer<'static> borrow? That would probably be more ergonomic since it seems like most uses are going to need to clone it.

[cpu]

Maybe in rcgen Certificate::der() should be returning CertificateDer<'static> by cloning its owned CertificateDer<'static> member instead of returning a &CertificateDer<'static> borrow?

Or, maybe what we should have done was to return a CertificateDer<'a> from Certificate's owned CertificateDer<'static> copy:

pub fn der<'a>(&'a self) -> CertificateDer<'a> {
    CertificateDer::from(self.der.as_ref())
}

Edit: Something like cpu/rcgen@2069697 (but probably added as a second fn since we don't want to make a 0.14...... 😭)

Edit 2: It ends up looking like this on our side: dc2d224 I think it's a big improvement. I just wish we noticed this before 0.13 😭 😭

[djc]

That looks like you are not using the impl From<rcgen::Certificate> for CertificateDer<'static>...

[djc]

(It's late here so my brain is too fried to go deep on this.)

[cpu]

That looks like you are not using the impl Fromrcgen::Certificate for CertificateDer<'static>...

I'm not sure what you mean, but in either case this would be separate from the problem I was trying to solve w.r.t der() since I can't get a CertificateDer<'static> from Certificate::der()'s &CertificateDer<'static> return as written without clone(), right?

How would you adjust the smaller snippet I shared if you don't think clone() is appropriate, and don't think the upstream Certificate should offer a CertificateDer<'a> return in addition to (or replacing) der() -> &CertificateDer<'static>?

[djc]

Like this?

#[test]
fn test_it() {
    use pki_types::CertificateDer;
    let cert_and_key: rcgen::CertifiedKey = make_issuer("Bogus Subject");
    let cert: rcgen::Certificate = cert_and_key.cert;
    let cert_der_borrowed: &CertificateDer<'static> = cert.der();
    let cert_der_owned = CertificateDer::from(cert);
}

[cpu]

That looks like you are not using the impl Fromrcgen::Certificate for CertificateDer<'static>...
..
Like this?

Ohhhhh! I see, not using der() at all 💡 I'm not sure why this was so hard for me to get into my head but in either case thanks for making it explicit 😆

I'll rework this & rustls/rustls#1852 with my new understanding.

[djc]

When I added the From impl in rcgen I debated whether it could/should (also?) be an into_der() method...

[cpu]

I'll rework this

Updated to remove almost all of the clunky clone()'s. There are a couple that seem unavoidable I left comments justifying. I also had to rejig some of the test helpers to make the lifetimes work out nicer. It was a useful exercise because I managed to reduce a bit of duplication at the same time.

[djc]

Have some thoughts on further improvements, but let's get this in.

[cpu]

@ctz Is this a branch you'd like to review before it lands?

[cpu]

Since this is only touching test & support code I'm going to merge and if there's any more feedback I'll look at follow-up PRs.

Thanks!