cloudflare / tls-tris

crypto/tls, now with 100% more 1.3. THE API IS NOT STABLE AND DOCUMENTATION IS NOT GUARANTEED.
Other
292 stars 50 forks source link

Create package crypto/tls/constants #92

Closed cjpatton closed 6 years ago

cjpatton commented 6 years ago

Note: This PR is one commit ahead of #91. Review that first.

The DC extension has its own package (crypto/tls/delegated_credential). This is useful because its API is not only invoked by the TLS client and server, but also by the backend delegator (the entity that signs delegated credentials for use by the TLS server). Thus, if we wanted to implement DC in the crypto/tls package, we would need to change its exported API. It seems cleaner to leave this API alone.

The problem is that the DC extension needs to import some constants from crypto/tls: in particular the identifiers for signature schemes and the protocol version. But since crypto/tls imports crypto/tls/delegated_credential, there is a circular dependency. This PR removes this circular dependency by defining these constants in a new package crypto/tls/constants.

This solution is somewhat inelegant in that we still need the names of the constants in the tls package. In fact, there are external packages in golang that use names like tls.VersionTLS12.

kriskwiatkowski commented 6 years ago

I don't think introducing new package in TLS lib in order to break dependency cycle isn't right thing to do.

In C you would simply do " #ifndef #define #endif" to solve this particular problem. In go, "interface" is a first thing which comes to my mind. Something like:

This way you break dependency and don't need to change "tls". It's also DIP from SOLID. There is maybe one more thing which is bottering me, but I need to read DC RFC first.

cjpatton commented 6 years ago

Thanks for the comments, @henrydcase! I'm gonna dump this PR and start anew. I'll ping you when it's ready.