Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add client certificate chain support and SSL context generation #454
Add client certificate chain support and SSL context generation #454
Changes from 5 commits
11a178f
43e5c82
2db37bd
0e91569
b6ed851
d0b47f1
d5961c7
75e7040
e664883
6818656
f785f0e
6ac8e8f
5ce3fb5
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should necessarily pluralize
LoadClientCerts
- it is still just 1 cert plus intermediates. Plural to me indicates the list of Client Certs will be tried based off some property of the cert such as common name or thumbprint. Should expand the doc here to explain why it'sX509Certificate2Collection
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also - should this callback return a SslStreamCertificateContext instead of an
X509Certificate2Collection
, since it appears that is the correct API for building a client certificate chain with intermediatesThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about
X509ChainPolicy
it needs that as wellThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SslClientAuthenticationOptions.CertificateChainPolicy applies to remote certificate validation, I think that could be implemented similar to
NatsTlsOpts.CertificateRevocationCheckMode
and is not needed in this callback?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hard for me to weigh in on this with my limited vision of customers beyond my organization (and what they need). The most extreme and simple would be to expose the hook for
SslClientAuthenticationOptions
or for aFunc<SslClientAuthenticationOptions,SslClientAuthenticationOptions>
which allows for customizable overrides of the default built inSslStreamConnection
. Between that and the current state just seems like different levels of tradeoff between flexibility and consistency.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, trying the tests without
CertificateChainPolicy
doesn't seem to make any difference. Also looks like it might mess up the OCPS (I might've remembered the acronym wrong) feature:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the only problem is that
ClientCertificateContext
option (which gets assigned) is not available in net6.0There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that API popped up in net8, there is a mega thread about it here: dotnet/runtime#26323