Skip to content

Use ALPN to determine what sort of connection we've accepted #1055

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

Merged
merged 1 commit into from
Nov 26, 2020

Conversation

glbrntt
Copy link
Collaborator

@glbrntt glbrntt commented Nov 24, 2020

Motivation:

To determine whether we're dealing with gRPC or gRPC Web (i.e. HTTP/2 or
HTTP/1) the first bytes received on a new connection are parsed. When
TLS is enabled and ALPN is supported there's no need to do this: we
will be informed about the protocol we negotiated with our peer and
therefore how to configure the channel. We also never enforced that the
protocol negotiated via ALPN was used!

Modifications:

  • Add a 'GRPCServerPipelineConfigurator' handler to supersede the
    'HTTPProtocolSwitcher'. The new handler will use either ALPN and fall
    back to parsing the incoming bytes.
  • The parsing behavior is slightly different to that of
    'HTTPProtocolSwitcher' in that we only read off enough bytes for the
    HTTP/2 connection preface, or enough bytes to parse an HTTP/1 request
    line (rather than reading the whole buffer as a string and processing
    that).
  • Added a server configuration option to disable ALPN being required.
  • Added tests.

Result:

@glbrntt glbrntt requested a review from Lukasa November 24, 2020 13:10
@glbrntt glbrntt added the 🆕 semver/minor Adds new public API. label Nov 24, 2020
Motivation:

To determine whether we're dealing with gRPC or gRPC Web (i.e. HTTP/2 or
HTTP/1) the first bytes received on a new connection are parsed. When
TLS is enabled and ALPN is supported there's no need to do this: we
will be informed about the protocol we negotiated with our peer and
therefore how to configure the channel. We also never enforced that the
protocol negotiated via ALPN was used!

Modifications:

- Add a 'GRPCServerPipelineConfigurator' handler to supersede the
  'HTTPProtocolSwitcher'. The new handler will use either ALPN and fall
  back to parsing the incoming bytes.
- The parsing behavior is slightly different to that of
  'HTTPProtocolSwitcher' in that we only read off enough bytes for the
  HTTP/2 connection preface, or enough bytes to parse an HTTP/1 request
  line (rather than reading the whole buffer as a string and processing
  that).
- Added a server configuration option to disable ALPN being required.
- Added tests.

Result:

- Server pipeline configuration is driven by ALPN, falling back to
  parsing.
- ALPN is enforced on the server (partially resolving grpc#1042)
@glbrntt glbrntt force-pushed the gb-protocol-switcher branch from 0167ae7 to ce5c80e Compare November 25, 2020 11:24
@glbrntt glbrntt requested a review from Lukasa November 25, 2020 11:24
Copy link
Collaborator

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

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

Nice one, looks great.

@glbrntt glbrntt merged commit b81b8f2 into grpc:main Nov 26, 2020
@glbrntt glbrntt deleted the gb-protocol-switcher branch November 26, 2020 08:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🆕 semver/minor Adds new public API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants