Skip to content

credentials/tls: default GRPC_ENFORCE_ALPN_ENABLED to true#7535

Merged
arjan-bal merged 3 commits intogrpc:masterfrom
arjan-bal:alpn
Sep 4, 2024
Merged

credentials/tls: default GRPC_ENFORCE_ALPN_ENABLED to true#7535
arjan-bal merged 3 commits intogrpc:masterfrom
arjan-bal:alpn

Conversation

@arjan-bal
Copy link
Copy Markdown
Contributor

@arjan-bal arjan-bal commented Aug 19, 2024 •
edited
Loading

Related issue: #434

RELEASE NOTES:

  • Clients and servers will reject TLS connections that don't support ALPN. This can be disabled by setting environment variable GRPC_ENFORCE_ALPN_ENABLED to false (case insensitive). Please file a bug if you encounter any issues with this behaviour. The environment variable to revert this behaviour will be removed in an upcoming release.

@arjan-bal arjan-bal added Type: Bug Type: Behavior Change Behavior changes not categorized as bugs and removed Type: Bug labels Aug 19, 2024
@arjan-bal arjan-bal added this to the 1.66 Release milestone Aug 19, 2024
@arjan-bal arjan-bal requested a review from dfawley August 19, 2024 09:55
@arjan-bal arjan-bal assigned dfawley and arjan-bal and unassigned dfawley Aug 19, 2024
@arjan-bal arjan-bal assigned dfawley and unassigned arjan-bal Aug 19, 2024
@codecov
Copy link
Copy Markdown

codecov Bot commented Aug 19, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.75%. Comparing base (005b092) to head (1b94310).
Report is 12 commits behind head on master.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #7535      +/-   ##
==========================================
- Coverage   81.91%   81.75%   -0.17%     
==========================================
  Files         361      361              
  Lines       27825    27825              
==========================================
- Hits        22794    22748      -46     
- Misses       3841     3869      +28     
- Partials     1190     1208      +18
Files with missing lines Coverage Δ
internal/envconfig/envconfig.go 100.00% <ø> (ø)

... and 19 files with indirect coverage changes

@dfawley
Copy link
Copy Markdown
Member

dfawley commented Aug 20, 2024

Let's wait a few weeks for this since we have a lot of other semi-risky things in flight right now.

@arjan-bal arjan-bal modified the milestones: 1.66 Release, 1.67 Release Aug 20, 2024
@dfawley dfawley assigned arjan-bal and unassigned dfawley Aug 29, 2024
@arjan-bal
Copy link
Copy Markdown
Contributor Author

arjan-bal commented Aug 30, 2024 •
edited
Loading

Waiting to fix internal users before submitting this PR.

@arjan-bal arjan-bal merged commit 70f19ee into grpc:master Sep 4, 2024
@arjan-bal arjan-bal deleted the alpn branch September 4, 2024 11:25
@atollena
Copy link
Copy Markdown
Collaborator

atollena commented Sep 5, 2024

Small note on the release note:

Clients and servers will reject TLS connections that don't support ALPN. This can be disabled by setting environment variable GRPC_ENFORCE_ALPN_ENABLED to false (case insensitive). Please file a bug if you encounter any issues with this behaviour. This will become the default behaviour in an upcoming release.

Is the last sentence correct? Shouldn't it just read "The environment variable to revert this behavior will be removed in an upcoming release."?

@arjan-bal
Copy link
Copy Markdown
Contributor Author

arjan-bal commented Sep 5, 2024

Small note on the release note:

Clients and servers will reject TLS connections that don't support ALPN. This can be disabled by setting environment variable GRPC_ENFORCE_ALPN_ENABLED to false (case insensitive). Please file a bug if you encounter any issues with this behaviour. This will become the default behaviour in an upcoming release.

Is the last sentence correct? Shouldn't it just read "The environment variable to revert this behavior will be removed in an upcoming release."?

You're correct. It was a copy/paste error. Updated the release note

@skonto skonto mentioned this pull request Sep 24, 2024
Merged
This was referenced Oct 5, 2024
Closed
Merged
Merged
@DmitriyMV DmitriyMV mentioned this pull request Oct 22, 2024
Closed
@nr-tdeluna nr-tdeluna mentioned this pull request Nov 22, 2024
Closed
3 tasks
@tbg tbg mentioned this pull request Nov 27, 2024
Closed
16 tasks
tbg added a commit to cockroachdb/grpc-go that referenced this pull request Nov 28, 2024
@tbg
envconfig: default GRPC_ENFORCE_ALPN_ENABLED to false
3093b97 
CRDB at v1.68.0 fails to communicate with CRDB at v1.56.3 due to this
check. This is strange, since CRDB uses gRPC throughout, and in both
v1.56.3 and v1.68.0 uses `tls.NewConfig` which ensures that `NextProtos`
always contains `h2`.

gRPC introduced this check in grpc#7535.

See: cockroachdb/cockroach#136278 (comment)
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Mar 5, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@dfawley dfawley dfawley approved these changes

Assignees

@arjan-bal arjan-bal

Labels

Type: Behavior Change Behavior changes not categorized as bugs

Projects

None yet

Milestone

1.67 Release

Development

Successfully merging this pull request may close these issues.

3 participants

@arjan-bal @dfawley @atollena