Skip to content

Port number correction on client for SslStream sample (C#, VB) #4377

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.

Sign up for GitHub

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

Closed
wants to merge 3 commits into from
Closed

Port number correction on client for SslStream sample (C#, VB) #4377

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d8c7a9b
Unify ports & add a note
Jun 13, 2020
fc1c7bd
Address comments
Jul 27, 2020
df7338f
Some grammar fixes
gewarren Aug 3, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion samples/snippets/csharp/VS_Snippets_Remoting/NclSslClientSync/CS/clientsync.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ public static void RunClient(string machineName, string serverName)
//<snippet4>
// Create a TCP/IP client socket.
// machineName is the host running the server application.
TcpClient client = new TcpClient(machineName,443);
TcpClient client = new TcpClient(machineName,8080);
Copy link
Member

Choose a reason for hiding this comment

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

Why do we change the port? 443 is default for ssl/tls while 80 and 8080 is typically used plain HTTP.
If this is a problem, perhaps the example should take the port as parameter.

Copy link
Author

@DavidEshtehari DavidEshtehari Jul 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry for the late back. If you look at the server-side will see there is a port over there also. The point is these two ports must be the same. I didn't choose the 443 to avoid the misunderstanding of using the standard https port number.
Using a parameter will make it complicated. So, I'd rather the plain numbers.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we change the port? 443 is default for ssl/tls while 80 and 8080 is typically used plain HTTP.

If this is a problem, perhaps the example should take the port as parameter.

I do agree with the possible confusion using 80 plain HTTP related port.

As mentioned by @DavidEshtehari the server example is using 8080. Should we change both of them?

Using default port can also be good from my point of view.

We should only take care of that, perhaps those code snippets are used more than once and that kind of mismatch could be perhaps found somewhere else.

Copy link
Member

Choose a reason for hiding this comment

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

I like the idea of updating the server. Since the client uses the default port, it would just work and be representative of users actually need to do in most cases. And yes, it make sense that the examples work together e.g. use same port.

Console.WriteLine("Client connected.");
// Create an SSL stream that will close the client's stream.
SslStream sslStream = new SslStream(
Expand Down
2 changes: 1 addition & 1 deletion samples/snippets/visualbasic/VS_Snippets_Remoting/NclSslClientSync/VB/clientsync.vb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ Namespace Examples.System.Net
'<snippet4>
' Create a TCP/IP client socket.
' machineName is the host running the server application.
Dim client = New TcpClient(machineName, 443)
Dim client = New TcpClient(machineName, 8080)
Console.WriteLine("Client connected.")

' Create an SSL stream that will close the client's stream.
Expand Down
5 changes: 4 additions & 1 deletion xml/System.Net.Security/SslStream.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,10 @@
<summary>Provides a stream used for client-server communication that uses the Secure Socket Layer (SSL) security protocol to authenticate the server and optionally the client.</summary>
<remarks>
<format type="text/markdown"><![CDATA[


> [!NOTE]
> Be sure you have the private key included in the certificate; cause the server needs it to decrypt the received message otherwise you will get a decryption error on the server-side.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
> Be sure you have the private key included in the certificate; cause the server needs it to decrypt the received message otherwise you will get a decryption error on the server-side.
> Be sure to include the private key in the certificate. The server needs the private key to decrypt the received message. If you omit it, you'll get a decryption error on the server side.

Copy link
Member

Choose a reason for hiding this comment

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

While this is true in general, Is this remark attached to some specific property?
Also I think the "message" should be plural. Perhaps something like ... decrypt the received messages and complete handshake.
However, AFAIK we will not get that far and SslStream will throw PNSP or Authentication Exception if X509Certificate without key is provided. That should be documented on the property it self and delegates returning certificates.
That being said, there is also some fixup code to search through certificate stores and it can work even if given certificate instance does not have a key. But it may be better to guide users to use X509Certificate2 properly.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for your suggestion. Any description to clarify the sample and its related issues is welcome.
Please, commit your suggestions. I'll accept it.

Copy link
Author

Choose a reason for hiding this comment

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

How to using the certificates is not the subject of this context. And I have avoided describing it more here. Nonetheless, if you have useful links we can add it here as a reference.


## Remarks
SSL protocols help to provide confidentiality and integrity checking for messages transmitted using an <xref:System.Net.Security.SslStream>. An SSL connection, such as that provided by <xref:System.Net.Security.SslStream>, should be used when communicating sensitive information between a client and a server. Using an <xref:System.Net.Security.SslStream> helps to prevent anyone from reading and tampering with information while it is in transit on the network.

Expand Down