rustls / rustls

A modern TLS library in Rust
Other
5.96k stars 636 forks source link

Document transitions between types of config builders #759

Open jsha opened 3 years ago

jsha commented 3 years ago

In going from a ConfigBuilder to a ServerConfig or ClientConfig, there are a number of other possible builder types along the way, and it's important to go from one to the next in the right order. It would be great if one of the doc pages (probably ConfigBuilder) had a diagram of the paths that building can take and what is yielded at each step; then each of the Builder types could link back to that explanation.

briansmith commented 3 years ago

In going from a ConfigBuilder to a ServerConfig or ClientConfig, there are a number of other possible builder types along the way, and it's important to go from one to the next in the right order. It would be great if one of the doc pages (probably ConfigBuilder) had a diagram of the paths that building can take and what is yielded at each step; then each of the Builder types could link back to that explanation.

It would get out of date quickly. I propose instead we have some doctests that best exemplify what people need.

djc commented 3 years ago

We already have a bunch of doctests. @jsha do you have specific suggestions of things that could use more documentation?

jsha commented 3 years ago

As an example: https://hoffman-andrews.com/rustls/rustls/struct.ConfigBuilder.html

ConfigBuilder::with_safe_default_cipher_suites()
    .with_safe_default_kx_groups()
    .with_safe_default_protocol_versions()
    .for_server()
    .unwrap();
ConfigBuilder::with_safe_defaults()
    .for_server()
    .unwrap();

These are nice for copy-paste coding, but they don't tell me what type I receive after each call in the chain, and what other possibilities I have. It actually quite common for builder objects to return Self throughout the chain, so my first assumption when reading these was that each call returns a ConfigBuilder (which is wrong).

As a concrete example: if I don't want with_safe_default_protocol_versions(), but instead want to choose different protocol versions, what type should I go to to find those choices?

Looking again at the docs, I see this is documented like so:

The types used here fit together like this: You must make a decision on which cipher suites to use, typically by calling ConfigBuilder::with_safe_default_cipher_suites(). You now have a ConfigBuilderWithSuites. You must make a decision on key exchange groups: typically by calling ConfigBuilderWithSuites::with_safe_default_kx_groups(). You now have a ConfigBuilderWithKxGroups. You must make a decision on which protocol versions to support, typically by calling ConfigBuilderWithKxGroups::with_safe_default_protocol_versions(). You now have a ConfigBuilderWithVersions and need to decide whether to make a ServerConfig or ClientConfig – call ConfigBuilderWithVersions::for_server() or ConfigBuilderWithVersions::for_client() respectively. Now see ServerConfigBuilder or ClientConfigBuilder for further steps.

But on first read this didn't really click. It feels like a different flow from the usual rustdoc documentation.

One thing that would be nice: If the examples had comments annotating the type at each step and linking to the corresponding page:

ConfigBuilder::with_safe_default_cipher_suites() // ConfigBuilderWithSuites
    .with_safe_default_kx_groups() // ConfigBuilderWithKxGroups
    .with_safe_default_protocol_versions() // ConfigBuilderWithVersions
    .for_server() // Result<ServerConfigBuilder, _>
    .unwrap(); // ServerConfigBuilder

I don't think it's possible to linkify like that from a doctest, but it could be simulated by putting in a <pre> tag.

Also: The types enforce a specific order of setting that is arbitrary and somewhat hard to remember. It would be nice to have a line that says:

Configs are built in this order: cipher suites; key exchange groups; TLS versions; server or client; peer certificate verifier; certificate provider for this instance.

jsha commented 3 years ago

It would also be useful to have forward / back signposting in the documentation. For instance, ConfigBuilderWithKxGroups should say "This is created by a ConfigBuilderWithKxGroups, and emits a ConfigBuilderWithVersions.

Also we might be able to avoid some redundancy in type names by moving all the builders into a module rustls::config_builder. So we'd have e.g. rustls::config_builder::WithSuites, rustls::config_builder::WithKxGroups, and code or documentation that needs to talk about those types when the context is clear could refer to them simply as WithSuites and WithKxGroups. I think the reduction in repetitive prefixes would make the docs a lot more readable.

djc commented 3 years ago

I don't think your use case (where you essentially need to deeply understand this API because you have to recreate/wrap over it) is very common, so I'm not sure how much documentation we should spend on the finer details of this (given the maintenance efforts of keeping everything in sync with the code). I also don't think it's all that unreasonable to just read the source code given this use case.

While builder chains that return some kind of Self are more common, type-state builder patterns are not that uncommon in the Rust world, so I don't feel like this is as big of a departure of idiomatic Rust as you're making it out to be.

That said, I could see how it would be reasonable to:

jsha commented 3 years ago

I don't think your use case (where you essentially need to deeply understand this API because you have to recreate/wrap over it) is very common

I think my needs here aren't specific to needing to wrap the API. In general people like to understand the APIs they are using, rather than copy-paste examples. And I think this sort of signposting between structs helps a lot with that.

how much documentation we should spend on the finer details of this (given the maintenance efforts of keeping everything in sync with the code).

Do you expect that the order of construction of builders is likely to change significantly in future releases?