StackExchange / StackExchange.Redis

General purpose redis client
https://stackexchange.github.io/StackExchange.Redis/
Other
5.84k stars 1.5k forks source link

CheckTrustedIssuer: Fixes for invalid chains #2665

Closed NickCraver closed 3 months ago

NickCraver commented 4 months ago

This issue was brought to my attention last night (thanks to Badrish Chandramouli): https://github.com/dotnet/dotnet-api-docs/pull/6660

This changeset ensures that we do not honor self-signed certs or partial/broken chains as a result of X509VerificationFlags.AllowUnknownCertificateAuthority downstream and adds a few tests and utilities to generate test certificates (currently valid for ~9000 days). Instead we are checking that the certificate we're being told to trust is explicitly in the chain, given that the result of .Build() cannot be trusted for this case.

This also resolves an issue where TrustIssuer could be called but we'd error when no errors were detected (due to requiring chain errors in our validator), this means users couldn't temporarily trust a cert while getting it installed on the machine for instance and migrating between the 2 setups was difficult.

This needs careful eyes, please scrutinize heavily. It's possible this breaks an existing user, but...it should be broken if so unless there's a case I'm not seeing.

mgravell commented 4 months ago

Conceptually I'm :+1: here, however: I do not consider myself a certificates expert. Locally, have we tried connecting to (say) azure redis after this change?

NickCraver commented 4 months ago

Same here - reaching out for more test scenarios to be careful here.

NickCraver commented 3 months ago

@bartonjs @vcsjones Latest comments appreciated - if you have time for final eyes would appreciate it, want to do a release with this and other fixes today if we're good.