Improve fmt::Display for common error variants

[ctz]

Not completely sure if we should do this here, or in rustls, or indeed at all?

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.60%. Comparing base (2b1cf9b) to head (67637c4).

Additional details and impacted files

@@                    Coverage Diff                    @@
##           jbp-time-error-detail     #316      +/-   ##
=========================================================
+ Coverage                  97.46%   97.60%   +0.14%     
=========================================================
  Files                         20       20              
  Lines                       4345     4475     +130     
=========================================================
+ Hits                        4235     4368     +133     
+ Misses                       110      107       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[djc]

Probably more useful in rustls? At least IIRC we convert these before putting them in the rustls Error.

Maybe we should give UnixTime a useful Display impl, too.

[cpu]

Probably more useful in rustls?

I was leaning the other way. It seems best to me to describe the errors in the same codebase that produces them so that each downstream doesn't have to do it themselves, inconsistently.

[cpu]

maybe clearer to put the units in the message? current UNIX epoch time is: xxx

[djc]

Probably more useful in rustls?

I was leaning the other way. It seems best to me to describe the errors in the same codebase that produces them so that each downstream doesn't have to do it themselves, inconsistently.

Yes, but only if we somehow preserve the webpki errors in the rustls API, which we don't do IIRC?

[ctz]

I was leaning the other way. It seems best to me to describe the errors in the same codebase that produces them so that each downstream doesn't have to do it themselves, inconsistently.

I think we could expose all this stuff in rustls in one of two ways:

1. Add NotValidForNameDetail(NotValidForNameDetail) to rustls CertificateError, and make NotValidForNameDetail be unconstructable outside the crate, and wrap the underlying webpki error. That underlying error can be leveraged for fmt::Display.
2. Add NotValidForNameDetail { expected: ServerName<'static>, ... } to rustls CertificateError, and then any fancy fmt::Display work needs to happen there rather than here.

(1) has the downside that users using custom verifiers cannot construct those variants and take advantage of the better errors. Seems minor.

[cpu]

wrap the underlying webpki error. That underlying error can be leveraged for fmt::Display.

That's what I was thinking FWIW.

[djc]

Maybe we could have an Error::Verify(Box<dyn Error>) (naming to be bikeshedded) that wraps the webpki error? So we can (a) rely on pretty error contexts from this crate while (b) avoiding a direct dependency on the webpki crate in the public API.

[ctz]

Maybe we could have an Error::Verify(Box<dyn Error>) (naming to be bikeshedded) that wraps the webpki error? So we can (a) rely on pretty error contexts from this crate while (b) avoiding a direct dependency on the webpki crate in the public API.

I... don't really like that, I'm afraid, because it removes the ability for users to hook actions to different errors (other than string matching, yeurch).

[djc]

Okay, well, then we do one boxed webpki error per variant?

[cpu]

I... don't really like that, I'm afraid, because it removes the ability for users to hook actions to different errors (other than string matching, yeurch).

I thought they could take a webpki dep and then downcast but yeah.... very ugly :-/

[cpu]

I was leaning the other way. It seems best to me to describe the errors in the same codebase that produces them so that each downstream doesn't have to do it themselves, inconsistently.

If lifting the display logic to rustls makes for the easiest solution I'm not strictly opposed. I think in reality this crate has few meaningful downstream users outside of Rustls 🤷‍♂️

[djc]

I... don't really like that, I'm afraid, because it removes the ability for users to hook actions to different errors (other than string matching, yeurch).

I thought they could take a webpki dep and then downcast but yeah.... very ugly :-/

If we tell them to rely on downcasting we've reintroduced the semver hazard of semver-incompatible webpki upgrades in rustls.