Skip to content

Conversation

@aik-jahoda
Copy link
Contributor

@aik-jahoda aik-jahoda commented Jun 11, 2020

Summary

Collect all changes from #3916 related to SslClientAuthenticationOptions

@dotnet-bot dotnet-bot added this to the June 2020 milestone Jun 11, 2020
@aik-jahoda aik-jahoda requested review from mairaw and wfurt June 11, 2020 13:39
@aik-jahoda aik-jahoda changed the title NegotiateStream - update documentation SslClientAuthenticationOptions - update documentation Jun 11, 2020
@aik-jahoda
Copy link
Contributor Author

Copy link
Member

@wfurt wfurt left a comment

Choose a reason for hiding this comment

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

generally looks good to me. I added few comments.

Copy link
Member

@wfurt wfurt left a comment

Choose a reason for hiding this comment

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

LGTM with few more comments.

</ReturnValue>
<Docs>
<summary>To be added.</summary>
<summary>List of the client certificates offered to the server.</summary>
Copy link
Member

Choose a reason for hiding this comment

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

We should make sure to be clear we will pick just one from the collection and it must have private key.
There are some weird rules hard to describe and there were cases when we did not pick anything from the collection - sometimes even if there is exactly one certificate.

<Docs>
<summary>To be added.</summary>
<value>To be added.</value>
<summary>Gets or sets the <see cref="T:System.Security.Authentication.SslProtocols" /> value that represents protocols used for authentication.</summary>
Copy link
Member

Choose a reason for hiding this comment

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

We should make sure IMHO that this is set of protocols client wants to use. One of them may be used or negotiation may fail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

You will get that only if you follow the link but not oof you joust read the property decryption, right?
how about

Gets or sets the value that represents protocol versions offered by client to the server during authentication.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated this summary also in SslServerAuthenticationOptions.xml

@aik-jahoda aik-jahoda requested a review from gewarren June 30, 2020 09:45
@aik-jahoda
Copy link
Contributor Author

I applied all comments from @wfurt. @gewarren, can you take a look please?

Copy link
Contributor

@gewarren gewarren left a comment

Choose a reason for hiding this comment

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

Left a few grammatical suggestions, otherwise looks good.

aik-jahoda and others added 2 commits July 7, 2020 11:02
Co-authored-by: Genevieve Warren <24882762+gewarren@users.noreply.github.com>
@aik-jahoda aik-jahoda merged commit ae470de into dotnet:master Jul 7, 2020
@aik-jahoda aik-jahoda deleted the jajahoda/SslClientAuthenticationOptions branch July 7, 2020 11:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants