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

Support HTTPS in CliClient #9

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Conversation

bcuff
Copy link

@bcuff bcuff commented May 2, 2019

Hi, I hope you don't mind the unsolicited PR. I was exploring some HTTP2 options in C# and found your library. I wanted to get it working with SSL and managed to do so in your CliClient example.

Some Notes:

  • I pulled in a .gitignore from here.
  • I had to upgrade to netcoreapp2.2 because I needed support for ALPN in SslStream.

Thanks!

Copy link
Owner

@Matthias247 Matthias247 left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!
Generally looks very helpful! I left a few comments on some places.

// Create HTTP/2 stream abstraction on top of the socket
var wrappedStreams = tcpClient.Client.CreateStreams();
var upgradeReadStream = new UpgradeReadStream(wrappedStreams.ReadableStream);
var (readableStream, writeableStream) = await CreateStreams(scheme, host, port);
Copy link
Owner

Choose a reason for hiding this comment

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

If I understand the specification correctly then HTTP/2 via upgrade is only supported for not encrypted connections (h2c). So while this might work it seems to be against the spec.
Maybe just throw an exception that upgrade+TLS is not supported instead.

If this would work, then the ALPN modifier should also be not set - since the upgrade intent is signaled via HTTP upgrade instead of via ALPN for this case.

Copy link
Author

Choose a reason for hiding this comment

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

Ah yes, I will throw a NotSupportException in this case.

},
};
await stream.AuthenticateAsClientAsync(options, default(CancellationToken));
logger.LogInformation("SSL Authenticated");
Copy link
Owner

Choose a reason for hiding this comment

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

Does at this place a verification need to get inserted that checks whether the server accepted the proposed ALPN protocol?

Copy link
Author

Choose a reason for hiding this comment

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

I added a protocol verification.

Copy link
Owner

@Matthias247 Matthias247 left a comment

Choose a reason for hiding this comment

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

I added one more suggestions. Let's see if I can apply that myself.

Examples/CliClient/Program.cs Outdated Show resolved Hide resolved
@bcuff
Copy link
Author

bcuff commented May 10, 2019

oh wait, i think your suggestion had the protocol backwards. let me fix that and test.

@bcuff
Copy link
Author

bcuff commented May 10, 2019

Fixed & tested. Here's where it's referenced in the spec.

@Matthias247
Copy link
Owner

The last one should have been right?

The string "h2c" identifies the protocol where HTTP/2 is run over cleartext TCP. This identifier is used in the HTTP/1.1 Upgrade header field and in any place where HTTP/2 over TCP is identified.

We are doing an upgrade, and cleartext TCP (since TLS always uses ALPN). So h2c should have been right?

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.

2 participants