try with extra roots when root is untrusted

[cstkingkey]

Fixes #214

[djc]

LGTM. @complexspaces want to take a look?

[ctz]

For me this change underlines a significant lack of testing in this crate.

[complexspaces]

For me this change underlines a significant lack of testing in this crate.

Do you mind elaborating @ctz? To be upfront I don't have a reason to disagree but I would like more detail if you have thought about it. Enthusiastic testing has been hindered by platform behaviors making debugging hard, as shown by the constant Android issues, and the testing originally written by 1Password definitely focused more on making sure a specific set of behaviors worked and others were rejected leaning towards failing closed.

To be honest, a more detailed thought process would go a long way because I don't know what is "good enough" in this space yet due to lack of experience. Maybe trying to get all of the BetterTLS data passing on every platform or at least running with every failure exemption documented? I don't know if that's right because this crate is just a TLS client wrapper (or that's the wrong way to think about this?)

With all that said the way I was thinking about it tonight used two buckets: "edge case behavior in base functionality" and "we've added a lot of new code without accompanying tests".

Things like #195 fall under those edge cases that could just be rejected because the original use case (supporting TLS proxies in enterprise environments for the desktop app) meant it was not going to get exposed to nearly as much weird data.

I definitely think the new_with_extra_roots extensions fall can be classified as incorrectly missing tests. That might just be my bad for not being stricter during PR review; people wanted the functionality fast. We really do need to add coverage for it so we have minimum the common cases are covered (self signed root also acting as the certificate, self-issued root somewhere in the chain, public root provided as an extra that's not in the platform store, etc).

...want to take a look?

@djc The change itself looks fine to me and pretty straightforward. But after reading the concern about testing I spent some time tonight putting together tests for those first two cases though. Without this PR the 1st fails and the 2nd passes and then applying the changes allows both to pass. I haven't gotten a chance to run them on macOS though so its not known yet if its an easy thing to land alongside this small improvement.

[complexspaces]

I've opened #216 to complement this PR and ensure the changed code doesn't regress at minimum.

[nab138]

+1, any updates on this?

[ctz]

Do you mind elaborating @ctz? To be upfront I don't have a reason to disagree but I would like more detail if you have thought about it.

There's not a lot to it, really. I am just remarking that the existing tests that call new_with_extra_roots() should be extended/altered to cover this case (which, in so much as I understand the change in this PR, is a basic level of workingness of the extra roots function.)