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

auto-set ALPN when using GetConfigForClient #7709

Closed
howardjohn opened this issue Oct 8, 2024 · 4 comments · Fixed by #7754
Closed

auto-set ALPN when using GetConfigForClient #7709

howardjohn opened this issue Oct 8, 2024 · 4 comments · Fixed by #7754
Assignees
Labels
Area: Server Includes Server, Streams and Server Options. Type: Feature New features or improvements in behavior

Comments

@howardjohn
Copy link
Contributor

Use case(s) - what problem will this feature solve?

In grpc-go v67, the client will reject servers not advertising ALPN=h2.

Typically, gRPC will automatically set this on the tls.Config passed in on the serverside. However, when GetConfigForClient is used, it will not.

Proposed Solution

Make GetConfigForClient usage automatically set alpn=h2

Alternatives Considered

  • Change application to explicitly set ALPN
  • Deploy all clients with GRPC_ENFORCE_ALPN_ENABLED=false. This is not great since it will probably go away(?) and you don't always control the clients.

Additional Context

Previous issue, stale-closed: #5814. Likely not a priority since it didn't hurt most users until the client's started denying

Popular projects broken by this:

@howardjohn howardjohn added the Type: Feature New features or improvements in behavior label Oct 8, 2024
@purnesh42H
Copy link
Contributor

@howardjohn could you clarify what did you expect and what did you see instead? We currently by default Enforce ALPN so clients and servers will now reject TLS connections that don't support ALPN. However, this can be disabled by setting the environment variable GRPC_ENFORCE_ALPN_ENABLED to false (case insensitive)

@purnesh42H purnesh42H added Status: Requires Reporter Clarification Area: Transport Includes HTTP/2 client/server and HTTP server handler transports and advanced transport features. labels Oct 9, 2024
@howardjohn
Copy link
Contributor Author

When i setup a gRPC server with something like &tls.Config{}, and pass this to gRPC, it automatically adds NextProtos=[]string{h2}. This means that ALPN works and will not be rejected.

If I instead pass &tls.Config{GetConfigForClient: func() { ... }, the tls.Config will be used directly from the result of the function, and NOT get the NextProtos set, meaning there will be no ALPN, and clients will reject things.

A server can workaround this by manually setting Nextprotos on the resulting tls.config, of course, but its something many projects are not doing since they didn't need to prior to 0.67.

@purnesh42H
Copy link
Contributor

@howardjohn you are correct. We need to do the same modifications to config returned by GetConfigForClient as well

@purnesh42H purnesh42H added Type: Bug and removed Type: Feature New features or improvements in behavior labels Oct 14, 2024
@arjan-bal arjan-bal assigned arjan-bal and unassigned purnesh42H Oct 17, 2024
@arjan-bal arjan-bal added Type: Feature New features or improvements in behavior Area: Server Includes Server, Streams and Server Options. and removed Type: Bug Area: Transport Includes HTTP/2 client/server and HTTP server handler transports and advanced transport features. labels Oct 17, 2024
@arjan-bal
Copy link
Contributor

The fix is expected be released as part of grpc-go v1.69 since we've already cut the branch for 1.68.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Server Includes Server, Streams and Server Options. Type: Feature New features or improvements in behavior
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants