Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove client certificate config #994

Merged
merged 2 commits into from
May 14, 2021
Merged

Remove client certificate config #994

merged 2 commits into from
May 14, 2021

Conversation

Tratcher
Copy link
Member

@Tratcher Tratcher commented May 13, 2021

Contributes to #352

There have been a number of concerns raised with this approach so we're going to remove it for now and reconsider it in the future.

  • It's different between the json config model and the code config model
  • it requires passwords in the json config
  • There's no rotation support, you have to modify the cluster config to point at a new cert and trigger a reload
  • Multiple config provider implementations are using System.Text.Json to serialize the code model across processes, but that doesn't make sense for X509Certificate2.
  • Cert lifetime issues across config reloads. The current model requires a complex weak reference cache.

Recommended alternatives:

  • Use the improved ProxyHttpClientFactory support to specify the client certificate in code. This way the app controls the cert lifetimes, credentials, etc..
  • Certificate information can be stored in the cluster metadata collection and consumed in the ProxyHttpClientFactory.

Future proposals:

  • We may add an indirection model where a cluster references a certificate by a string id that is used to resolve the certificate from a separate service that manages them. I'd expect ProxyHttpClientFactory to consume that service.

@Tratcher Tratcher added this to the YARP 1.0.0-preview12 milestone May 13, 2021
@Tratcher Tratcher requested a review from alnikola as a code owner May 13, 2021 18:54
@Tratcher Tratcher self-assigned this May 13, 2021
@Tratcher Tratcher requested a review from MihaZupan as a code owner May 13, 2021 18:54
Copy link
Contributor

@alnikola alnikola left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Tratcher Tratcher merged commit 8f64351 into main May 14, 2021
@Tratcher Tratcher deleted the tratcher/lessconfig branch May 14, 2021 16:38
@samsp-msft
Copy link
Contributor

@Tratcher - Do we need to document how to do this now that we have removed the capability from config, or provide it in a sample (potentially the auth sample)?

@Tratcher
Copy link
Member Author

The workaround for directly configuring the SocketHttpHandler is here:
https://github.com/microsoft/reverse-proxy/blob/4f9e59dc6a196a42de6e6b5e34880328a3abef7f/docs/docfx/articles/proxyhttpclientconfig.md#configuring-the-http-client

Want me to update that doc to show setting a client certificate instead of 100-continue?

MihaZupan added a commit that referenced this pull request Sep 9, 2022
This isn't needed anymore after #994
MihaZupan added a commit that referenced this pull request Sep 9, 2022
This isn't needed anymore after #994
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants