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

Find a way to use grpc-go >= 1.67.0 with ALPN disabled #9463

Open
DmitriyMV opened this issue Oct 7, 2024 · 4 comments · May be fixed by #9486
Open

Find a way to use grpc-go >= 1.67.0 with ALPN disabled #9463

DmitriyMV opened this issue Oct 7, 2024 · 4 comments · May be fixed by #9486
Assignees

Comments

@DmitriyMV
Copy link
Member

Starting with v1.67.0 Go gRPC implementation no longer work with TLS connections that don't support ALPN. There is a special header GRPC_ENFORCE_ALPN_ENABLED which, when set to false, should disable this behavior. But setting it after the process started (even in init block) has no effect, because relevant variable initialization happens before any init blocks.

@DmitriyMV DmitriyMV self-assigned this Oct 7, 2024
@DmitriyMV DmitriyMV changed the title Find a way to use grpc-go >= 1.67.0 with ALPN disabled. Find a way to use grpc-go >= 1.67.0 with ALPN disabled Oct 7, 2024
@smira
Copy link
Member

smira commented Oct 7, 2024

My first question - does Talos set up TLS wrong way so that it's not ALPN-enabled? We should fix it in the first place.

@howardjohn
Copy link

@smira just stumbled upon this while working on another project. I found if you use GetConfigForClient you may not get the ALPN set properly. See cert-manager/istio-csr#422

@smira
Copy link
Member

smira commented Oct 8, 2024

Thank you, that should be the case for us, as we do live cert reload.

DmitriyMV added a commit to DmitriyMV/talos that referenced this issue Oct 10, 2024
Bump grpc library to 1.67.1 and ensure that we set proper HTTP/2 ALPN value.

Closes siderolabs#9463

Depends on siderolabs/crypto#34

Signed-off-by: Dmitriy Matrenichev <[email protected]>
@DmitriyMV DmitriyMV linked a pull request Oct 10, 2024 that will close this issue
DmitriyMV added a commit to DmitriyMV/talos that referenced this issue Oct 10, 2024
Bump grpc library to 1.67.1 and ensure that we set proper HTTP/2 ALPN value.

Closes siderolabs#9463

Depends on siderolabs/crypto#34

Signed-off-by: Dmitriy Matrenichev <[email protected]>
@DmitriyMV
Copy link
Member Author

DmitriyMV commented Oct 10, 2024

So this is actually interesting: basically all relevant code in Talos uses credentials.NewTLS which internally uses AppendH2ToNextProtos which ensures that we have "h2" in TLS config. Actually all clients and servers, use it. Except in our crypto library we indeed use GetConfigForClient which drops doesn't carry those changes to tls.Config since gRPC NewTLS operate on copy.

What is good, is that even before gRPC 1.67.0 the client would send the proper "h2" tag during negotiation. It should have warned us about incorrect ALPN settings even on v1.65.0 gRPC but I guess we are silencing that log somewhere. I also have no idea how that works in the first place, since Go TLS negotiateALPN should revert to http/1.1 if there is no proper ALPN.

Edit: look like in h2_bundle.go (http2ConfigureServer and http2ConfigureTransports and newTLSConfig method) it does setup NextProtos field.

DmitriyMV added a commit to DmitriyMV/talos that referenced this issue Oct 10, 2024
Bump grpc library to 1.67.1 and ensure that we set proper HTTP/2 ALPN value.

Closes siderolabs#9463

Depends on siderolabs/crypto#34

Signed-off-by: Dmitriy Matrenichev <[email protected]>
DmitriyMV added a commit to DmitriyMV/talos that referenced this issue Oct 11, 2024
Bump grpc library to 1.67.1 and ensure that we set proper HTTP/2 ALPN value.

Closes siderolabs#9463

Depends on siderolabs/crypto#34

Signed-off-by: Dmitriy Matrenichev <[email protected]>
DmitriyMV added a commit to DmitriyMV/talos that referenced this issue Oct 11, 2024
Bump grpc library to 1.67.1 and ensure that we set proper HTTP/2 ALPN value.

Closes siderolabs#9463

Depends on siderolabs/crypto#34

Signed-off-by: Dmitriy Matrenichev <[email protected]>
DmitriyMV added a commit to DmitriyMV/talos that referenced this issue Oct 11, 2024
Bump grpc library to 1.67.1 and ensure that we set proper HTTP/2 ALPN value.

Closes siderolabs#9463

Depends on siderolabs/crypto#34

Signed-off-by: Dmitriy Matrenichev <[email protected]>
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 a pull request may close this issue.

3 participants