search

hyperium / tonic

A native gRPC client & server implementation with async/await support.
https://docs.rs/tonic
MIT License
9.8k stars 998 forks source link

chore(tls): Refactor parsing certificate and identity #1763

Open tottoto opened 3 months ago

tottoto commented 3 months ago

Refactors parsing certificate and identity.

djc commented 3 months ago

I think this and #1748 need a more holistic design discussion of what we're trying to achieve here and how to balance the trade-offs. As is, I don't think the changes here are a clear improvement. I think the goals are:

  1. Hide rustls API surface so we can semver-compatibly bump rustls
  2. Be easy to use
  3. Provide type safety

Given the developments in upstream rustls with the migration to rustls-pki-types, personally I think it could be worth simplifying so that our public API fully relies on rustls-pki-types directly (which was not the case before rustls adopted rustls-pki-types). We could still provide wrapper around the use of rustls-pemfile but I'm personally not convinced the benefit on (2) is worth the costs on (3), and some of the ways in which it might make the API worse.

tottoto commented 3 months ago

As is, I don't think the changes here are a clear improvement.

Can you elaborate what points in the existing implementations are clear than this changes?

Regarding to #1748, this pull request is not related to it. Since it contains difficult problem, this is completely separated patch to make easier implementation at the moment. This change reduces the duplicate logic implementation and reimplementing of rustls providing.

djc commented 3 months ago

Can you elaborate what points in the existing implementations are clear than this changes?

The previous implementation had a higher-level interface (adding certificates to the root store). This implementation ends up deduplicating the certificate parsing (good) but actually duplicates how certificates get added to the root store (even though this doesn't result in much extra code).

I'm also not a fan of the impl blocks living in a separate file from the struct definition they're linked to, and I don't think parse() is a great name for the methods -- parse() is usually associated with FromStr which generally provided more like a 1:1 conversion whereas at least for certificates this is more like a 1:n conversion.

tottoto commented 3 months ago

Thanks for the explanation.

The previous implementation had a higher-level interface (adding certificates to the root store). This implementation ends up deduplicating the certificate parsing (good) but actually duplicates how certificates get added to the root store (even though this doesn't result in much extra code).

Given this opinion, I feel it sounds this implementation is better than the previous one regarding this point. What I aimed to do are removing the higher level interface, and resolves the asymmetry between what add_certs_from_pem and load_identity do by separating the responsibilities of add_certs_from_pem, and eliminate the distribution of the actual implementations of the process of registering certificates to the root store.

I'm also not a fan of the impl blocks living in a separate file from the struct definition they're linked to

I understand what you concern. I think it is an acceptable drawback thinking of reducing importing the previous functions and expressing that they are related to certificate or identity type, but I also understand the cost of not being located at one place. I will change them to independent functions.

I don't think parse() is a great name for the methods -- parse() is usually associated with FromStr which generally provided more like a 1:1 conversion whereas at least for certificates this is more like a 1:n conversion.

In this case, I would think this can be thought as one set of certificates, so it can be regarded as 1:1 conversion. But I think we can name more descriptively as well.