Provide an iterator for CertificateChain

[kim]

Trying to integrate #644 I realise I'm kind of back to square one, because I can't get at the actual certificate :)

[djc]

Seems fine to me.

[djc]

Maybe we should also try to upstream some of the more friendly interfaces from our wrappers into rustls? The current solution feels a bit kludgey.

[Ralith]

AFAIK the main benefit of these interfaces as they stand is allowing people to avoid exposure to rustls's unstable API, so I'm not sure that would be a win.

[djc]

AFAIK the main benefit of these interfaces as they stand is allowing people to avoid exposure to rustls's unstable API, so I'm not sure that would be a win.

Well, my point is whether we can make a similar API stable in rustls. I filed rustls/rustls#348, feel free to comment there if you have any ideas around this.

[djc]

@kim, both of your versions implemented iterators that yield owned Certificates. Is there a reason you're staying away from just yielding &Certificates instead? I think that would be preferable here (for one thing, so the AuthenticationData can stay intact).

[kim]

@djc Because CertificateChain stores rustls::Certificates internally, but that field is also pub(crate), I was afraid of having to introduce a lot of conversion boilerplate if I changed that to store quinn's Certificate instead (which is the only way I can see implementing impl Iterator<Item = &Certificate> can work).

It turns out that only one callsite is affected (or even exists?), so it's not too bad.

I'm not really familiar with the impl<'a> Move for &'a Ref idiom, and also find it confusing when a simple fn iter(&self) -> impl Iterator<Item = &Ref> would do, so I implemented the latter in 015c498

[djc]

I'm sorry, I meant to distinguish between iterating over references vs iterating over owned types. I think the current changes probably contain too much stuff. Can we just have an impl IntoIterator<Item = &rustls::Certificate> for &CertificateChain? Would that solve your use case?

[kim]

Sure. You prefer impl IntoIterator over just fn iter()?

[Ralith]

Well, my point is whether we can make a similar API stable in rustls.

That would still lead to confusing breakage on each rustls update due to crate version mismatches, unless we reexport rustls per #300 and encourage people to use it that way.

Sure. You prefer impl IntoIterator over just fn iter()?

Idiomatically you'd have both.

[kim]

I feel like we're talking past each other. To summarise:

• Allow connection to return peer certificates #567 requested to provide access to data negotiated during the cryptographic handshake
• Expose the underlying TLS session #594 implements this, but only on the quinn-proto layer
• QUINN: Allow retrieving the peer's certificate chain from a connection #638 proposes to expose access to peer certificates on the quinn layer as well
• Create generic API for crypto protocol data #644 generalises this, the intention is to not only abstract over the cryptographic protocol, but also hide the implementation detail of rustls
• Unfortunately, this actually breaks the original motivation, because the CertificateChain newtype doesn't provide access to the underlying certificate data. Not being too familiar with the codebase, I missed this during review, sorry about that. This PR is meant to rectify the situation.
• My assumption about the newtypes in quinn-proto::crypto::types was that they are there to hide rustls. This seems to be wrong, their purpose is to add convenience methods. I do agree with @burdges that this might be better achieved using "Ext" traits.
• Alright, so it's ok to provide an iterator over rustls::Certificates, and not having to wrap it in the newtype again.
• That's cool, but then I don't get how impl IntoIterator for &CertificateChain is useful or idiomatic -- it's the same thing as self.certs.iter(), only gives the compiler more work. My understanding is that Into/into is a convention to indicate that the receiver will be consumed. But maybe I'm missing something.

Alright. So if I provide a impl IntoIterator for CertificateChain, would you be willing to accept this patch?

[kim]

So if I provide a impl IntoIterator for CertificateChain, would you be willing to accept this patch?

Sorry, to clarify: fn iter(&self) -> impl Iterator<Item = &rustls::Certificate> and impl IntoIterator for CertificateChain where type Item = rustls::Certificate.

[Ralith]

I think me and @djc have somewhat different perspectives here, and that's produced some confusion. I apologize for that.

My assumption about the newtypes in quinn-proto::crypto::types was that they are there to hide rustls.

This was my intention in adding those newtypes to begin with, though the specific goal was to allow callers to be ignorant of rustls, not necessarily to force them to--so exposing the details is acceptable so long as it's not required for simple cases.

I don't get how impl IntoIterator for &CertificateChain is useful or idiomatic -- it's the same thing as self.certs.iter(), only gives the compiler more work.

IntoIterator on references is the a common idiom because it makes for x in &xs work, which is a bit more pleasant than for x in xs.iter(). You'll see this pattern in every std collection, e.g. for slices and Vec.

So if I provide a impl IntoIterator for CertificateChain, would you be willing to accept this patch?

I'd be happy to merge either/both of those, though I'd consider an approach which doesn't expose rustls to be better still. An easier solution than wrapping/unwrapping the newtypes might be to fall back on Vec<u8> for owning iterators and &[u8] for borrowing ones.

[Ralith]

The current state looks good to me.

[kim]

IntoIterator on references is the a common idiom because it makes for x in &xs work

Ok, thanks. Please forgive my ignorance, I'm coming from a programming language whose main compiler can auto-derive every typeclass through a newtype 🤷‍♂

[Ralith]

No worries, always happy to help someone get up to speed!

[djc]

Yeah, sorry for the confusion and mixed messages here, and thanks for sticking with it!