Yield all the errors

[djc]

Alternative to #126 that is a bit more principled maybe?

Release notes:

• load_native_certs() now returns a CertificateResult value containing all the certificates that were successfully found as well as any errors encountered. Changes made in 0.7.1 in order to find certificates in more locations resulted in new errors in scenarios that previously worked for most users. This change allows callers to determine the error handling strategy most appropriate to their application.

[cpu]

Are you thinking this would be a 0.8 release since it includes a breaking API change? I kicked around some ideas about how to maintain API compat but maybe it's simplest to just release a breaking change and let the ecosystem catch up when it can.

[djc]

Yeah, I think a breaking release for this crate is relatively low impact.

[ctz]

Yeah, I think a breaking release for this crate is relatively low impact.

Fair enough. I think I was trying to avoid that, since some downstream users seemed to have broken applications and I think that warranted a semver-compatible change.

I was also working on the assumption that if the "best effort" approach works for go's (larger) user base, it probably works for ours too.

[ctz]

I think this looks good in substance.

Do we want to include more context in the Error type to reflect what the error was in response to? For example, considering macos.rs, we could end up with two errors and some certificates. Is it useful to be able to record "this error came from asking about Domain::User, this error came from Domain::System"?

edit: er, disregard? it's not our error type. i guess we could wrap errors in strings explaining what was happening, but stringy errors are quite ick.

[djc]

I think this looks good in substance.

Do we want to include more context in the Error type to reflect what the error was in response to? For example, considering macos.rs, we could end up with two errors and some certificates. Is it useful to be able to record "this error came from asking about Domain::User, this error came from Domain::System"?

edit: er, disregard? it's not our error type. i guess we could wrap errors in strings explaining what was happening, but stringy errors are quite ick.

We can trivially add our own Error type? I think the recent issues that led to this PR made it clear that clear/specific errors work for us, in the sense that they give users more context leading to fewer "support" issues.

[djc]

Added our own Error type, which can wrap io::Error and the other error types from the OS trust stores. At +257/-159 this is a fair bit of extra complexity (especially relative to the ~750 LoC size of the library), but I feel like it (a) enables applications to decide on their sensitivity to errors, (b) has a nicer error handling story, and (c) the additions are pretty boring code that will not require a lot of maintenance.

Fair enough. I think I was trying to avoid that, since some downstream users seemed to have broken applications and I think that warranted a semver-compatible change.

That's true -- my impression is that there are relatively few downstream users who are broken in a bad way by the current state of the branch (especially now that 0.7.2 is released which clarifies the source of the problem).

I was also working on the assumption that if the "best effort" approach works for go's (larger) user base, it probably works for ours too.

Fair, though I think in a number of places Rust (and the ecosystem) aim higher, not least in terms of good error handling.

[cpu]

Fair enough. I think I was trying to avoid that, since some downstream users seemed to have broken applications and I think that warranted a semver-compatible change.

Can we land on a compromise? Maybe merge #126, cut a 0.7.3 and then use this for a 0.8? That doesn't seem like much extra work and might make life easier for some users.

Fair, though I think in a number of places Rust (and the ecosystem) aim higher, not least in terms of good error handling.

I agree and like the approach in this branch. Everyone swallowing these errors just perpetuates the misconfigurations, making it harder for the next project that wants to do the right thing :'(

I don't think there's a good story for why you would intentionally install roots without broad permissions into a location for system-wide usage and I wager that a "can't read this cert file" error will be a lot easier to triage for the average non-TLS-savvy user than "why does this connection fail with an unknown issuer error when I've already installed the root".

[cpu]

Thanks for all the up-front tidying along the way. Appreciated 🙇

[djc]

Fair enough. I think I was trying to avoid that, since some downstream users seemed to have broken applications and I think that warranted a semver-compatible change.

Can we land on a compromise? Maybe merge #126, cut a 0.7.3 and then use this for a 0.8? That doesn't seem like much extra work and might make life easier for some users.

Sure, I agree that could make sense! (Seems less like a compromise and more like a "best of both worlds"...)

[djc]

@ctz thoughts on the plan proposed by @cpu?

[ctz]

Yes, sorry, I agree. Will make a minor follow-up comment on #126 ...

[cpu]

@djc Would you mind drafting some release notes for 0.8 in the PR desc?

[djc]

Wrote something up. Maybe too long/detailed? Open to feedback.

[cpu]

I think it might help to mention the change up front, and then dig a bit into the background. I think it might make sense to also suggest that the errors are a misconfiguration of the underlying system. WDYT about something like:

load_native_certs() now returns a CertificateResult type containing all the certificates that were successfully found and any errors encountered.
In 0.7.1, we started looking in more locations for user-installed root certificates. Unfortunately, we found that some users had misconfigured systems that yielded permission errors loading some certificates. The 0.7.3 release restored previous behavior, swallowing errors and returning the certificates that could be loaded. The new CertificateResult type moves this decision to callers, allowing applications to decide whether or not to surface any encountered errors.

[djc]

Addressed comments and rewrote the proposed release notes.

[cpu]

Updated notes LGTM, thanks!

[djc]

• Published rustls-native-certs v0.8.0 at registry crates-io
• [new tag] v/0.8.0 -> v/0.8.0
• Release notes