Repository navigation
Allow clients to select ALPN protocols instead of forcing HTTP/2 - #159
Closed
sruthi-talluri wants to merge 1 commit into
Closed
sruthi-talluri wants to merge 1 commit into
sruthi-talluri wants to merge 1 commit into
Conversation
GetTLSConfigurationForClient has hardcoded NextProtos to {"h2"} since google#72,
which was added in 2022 when s2a-go was only used for gRPC. The HTTP entry
points added later, NewTLSClientConfigFactory and NewS2ADialTLSContextFunc,
inherited that default, so an HTTP/1.1 client gets a connection that has
negotiated h2 via ALPN. net/http then writes HTTP/1.1 onto it and the first
response fails to parse:
net/http: HTTP/1.x transport connection broken: malformed HTTP response
"\x00\x00$\x04\x00\x00\x00\x00\x00\x00\x05\x00\x10..."
which is an HTTP/2 SETTINGS frame. There is currently no way to avoid this
other than rebuilding the tls.Config by hand after Build returns.
This adds ClientOptions.NextProtos, applied whenever a client tls.Config is
built, and TLSClientConfigOptions.NextProtos as a per-Build override.
NewS2ADialTLSContextFunc goes through NewTLSClientConfigFactory, so it picks
the option up as well.
Behavior is unchanged when neither is set: nextProtosOrDefault falls back to
{"h2"}. The gRPC ClientHandshake path in internal/v2/s2av2.go passes nil
explicitly, since that transport requires HTTP/2 and must not be overridable.
The returned slice is copied so a later mutation by the caller cannot alter a
live tls.Config.
sruthi-talluri
marked this pull request as draft
September 18, 2026 04:04
sruthi-talluri
marked this pull request as ready for review
September 18, 2026 04:04
sruthi-talluri
added a commit
to sruthi-talluri/gcsfuse
that referenced
this pull request
Sep 18, 2026
v0.1.10 includes 489047f, "Client TLS configs should plumb identity to the
S2A server", which fixes LocalIdentity being dropped on the client TLS config
path. GCSFuse builds its http1 dialer through s2a.NewTLSClientConfigFactory,
so on v0.1.9 a delegated SPIFFE identity never reached the S2A server and the
handshake used the host identity instead.
The ALPN workaround in storageutil.newS2ADialTLSContextForHTTP1 is still
required. v0.1.10 continues to hardcode the client TLS config's NextProtos to
{"h2"}; google/s2a-go#159 makes that selectable and
the workaround can be removed once it ships.
Verified: go build ./... and go test -count=1 ./internal/storage/... ./cfg/...
both pass.
rmehta19
reviewed
Sep 18, 2026
Comment on lines
+46
to
+48
| // handshake. S2A has historically offered HTTP/2 only, which is required by | ||
| // the gRPC transport, so that remains the default when no protocols are | ||
| // requested. |
Collaborator
There was a problem hiding this comment.
nit: We can remove this second sentence
rmehta19
reviewed
Sep 18, 2026
| if len(nextProtos) == 0 { | ||
| return []string{h2} | ||
| } | ||
| return append([]string(nil), nextProtos...) |
Collaborator
There was a problem hiding this comment.
Can this be replaced with slices.Clone(nextProtos)?
rmehta19
reviewed
Sep 18, 2026
| // NextProtos is the list of ALPN protocols offered during the TLS | ||
| // handshake. If empty, ClientOptions.NextProtos is used; if that is also | ||
| // empty, HTTP/2 ("h2") is offered. | ||
| NextProtos []string |
Collaborator
There was a problem hiding this comment.
NextProtos param is added in TLSClientConfigOptions, s2aTLSClientConfigFactory and ClientOptions (in s2a_options.go).
Can it just be in TLSClientConfigOptions?
Contributor
There was a problem hiding this comment.
We need NextProtos in ClientOptions because NewS2ADialTLSContextFunc takes *ClientOptions and invokes factory.Build(ctx, &TLSClientConfigOptions{ServerName: serverName}) internally on every dial in the background.
Callers configuring http.Transport via NewS2ADialTLSContextFunc (such as GCSFuse) do not call Build() directly, so without ClientOptions.NextProtos, dialer users would have no way to configure ALPN.
Having NextProtos in both places gives us the best of both worlds:
ClientOptions.NextProtos: Sets the default for the factory and makesNewS2ADialTLSContextFuncwork out of the box for HTTP/1.1 clients.TLSClientConfigOptions.NextProtos: Provides an optional per-Build()override when interacting with the low-level factory directly.
Following up in #161
rmehta19
reviewed
Sep 18, 2026
| nextProtos []string | ||
| } | ||
|
|
||
| func (f *s2aTLSClientConfigFactory) Build( |
Collaborator
There was a problem hiding this comment.
Can we update s2a_test.go and s2a_e2e_test.go?
|
***@***.***
El vie., 18 sept. 2026 18:00, Riya Mehta ***@***.***>
escribió:
… ***@***.**** commented on this pull request.
------------------------------
In s2a.go
<#159 (comment)>:
> @@ -359,6 +366,7 @@ type s2aTLSClientConfigFactory struct {
getStream stream.GetS2AStream
// localIdentity should only be used by the client.
localIdentity *commonpb.Identity
+ nextProtos []string
}
func (f *s2aTLSClientConfigFactory) Build(
Can we update s2a_test.go and s2a_e2e_test.go?
—
Reply to this email directly, view it on GitHub
<#159?email_source=notifications&email_token=B3IDZZTLUPLQQ2OPL54EN3T5PVL3NA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTKMRUHE4DQNJZGE2KM4TFMFZW63VKON2WE43DOJUWEZLEUVSXMZLOOSWGM33PORSXEX3DNRUWG2Y#pullrequestreview-5249885914>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/B3IDZZXPLYU5WGFVXBNXGCD5PVL3NAVCNFSNUABFKJSXA33TNF2G64TZHM2DANBRGQZTAMZUHNEXG43VMU5TKNBZGU2DSNZWGY42C5QC>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
anushka567
added a commit
to GoogleCloudPlatform/gcsfuse
that referenced
this pull request
Sep 28, 2026
…rts (#5112) * feat(cfg): add s2a-address and s2a-spiffe-id flags and configuration mappings Add configuration fields and CLI flags for direct S2A authentication: - s2a-address: daemon address for direct S2A mTLS authentication - s2a-spiffe-id: local SPIFFE ID to assume for direct S2A authentication - update config-gen parser to support alphanumeric tokens in flag names - map flags to StorageClientConfig in cmd/legacy_main.go and add flag parsing unit tests * feat(storage): support direct S2A authentication for HTTP storage client Implement S2A mTLS transport dialer for HTTP storage client: - construct S2A DialTLSContext using s2a.NewClientOptions when S2AAddress is configured - support both HTTP/1 and HTTP/2 client protocols with S2A transport - bypass token acquisition and TokenSource in CreateHttpClient when S2A is active - add unit tests verifying S2A transport creation and mock S2A daemon dialer connection * feat(storage): support direct S2A authentication for gRPC storage and control clients Configure S2A transport credentials for gRPC storage and control clients: - use s2av2.NewClientCredentials in createClientOptionForGRPCClient when S2AAddress is configured - bypass standard token authentication when S2A credentials are used - add unit tests verifying client options assembly for gRPC and HTTP handles with S2A * fix(storage): refactor HTTP client creation and add S2A comments * fix: correct ALPN and peer verification for the S2A transports Two issues prevented an S2A-authenticated mount from working. First, http1 could not talk to GCS at all. s2a-go hardcodes the client TLS config's NextProtos to {"h2"}, and NewS2ADialTLSContextFunc returns a *tls.Conn, so http.Transport records a negotiated protocol of "h2". The http1 branch deliberately sets TLSNextProto to an empty map to disable HTTP/2, so the h2 handler lookup misses and the transport ends up speaking HTTP/1.1 over an h2-negotiated connection. Every request then fails while parsing the server's first HTTP/2 frame. Build the TLS config for the http1 branch via s2a.NewTLSClientConfigFactory and pin NextProtos to http/1.1 before dialing. The http2 branch continues to use s2a.NewS2ADialTLSContextFunc unchanged. Second, neither transport set VerificationMode on s2a.ClientOptions, so S2A received UNSPECIFIED. The legacy S2A certificate verifier treats UNSPECIFIED the same as CONNECT_TO_GOOGLE, so this went unnoticed, but the verifier used on the delegated-SPIFFE path falls through to SPIFFE peer verification instead and rejects the GCS server certificate with "Leaf SVID must have exactly one URI SAN representing the SPIFFE ID". GCS is a Google endpoint serving a WebPKI certificate, so ask for CONNECT_TO_GOOGLE explicitly on both the HTTP and gRPC clients, which is what s2a.DefaultClientOptions already does. Verified end to end against s2a-go's fake S2A v2 server: http1 negotiates HTTP/1.1 and succeeds, http2 still negotiates HTTP/2.0, and the previous http1 path reproducibly fails. Also verified on an ACE staging host against the local S2A daemon with a delegated SPIFFE identity: without the verification mode the handshake fails with the SVID error above, and with it the handshake completes and requests reach GCS. * fix(storage): send an OAuth token alongside S2A S2A secures the channel but conveys no IAM principal to GCS, so an access token is still required. Both the HTTP and the gRPC path treated S2A and the token as mutually exclusive, so an S2A mount sent no caller identity at all and GCS rejected every request with: Unauthenticated: Anonymous caller does not have storage.objects.list access storageutil/client.go: the oauth2.Transport wrapper was gated on `!AnonymousAccess && S2AAddress == ""`. Drop the S2A term so the token is attached whenever access is not anonymous. storage_handle.go: createClientOptionForGRPCClient built the S2A transport credentials and then appended option.WithoutAuthentication(), which suppressed the per-RPC token. Split the function into two independent blocks, one that selects the transport (S2A or default) and one that selects the identity (anonymous, google lib auth, or token source), and drop WithoutAuthentication() from the S2A path. The two compose correctly in google-api-go-client: transport/grpc/dial.go installs the default transport credentials first and appends the caller's GRPCDialOpts last, so the S2A credentials win, while the token source is installed separately as per-RPC credentials. Tests: TestCreateHttpClientWithS2A_DialVerification now supplies a static token source, since it previously failed the token fetch before reaching the dial. Adds TestCreateHttpClientWithS2A_AttachesOAuthTransport and TestCreateHttpClientWithAnonymousAccess_NoOAuthTransport, which assert the RoundTripper chain directly rather than only checking for a nil error. * fix(auth): send Metadata-Flavor: Google when fetching a token proxyTokenSource.Token() issued a bare client.Get with no headers. The token URL points at a GCE-metadata-compatible endpoint -- the metadata server itself, or a proxy in front of it such as ACE's MDS Proxy -- and those servers require the Metadata-Flavor: Google header. Requiring a non-standard header is what prevents a browser or other naive HTTP client from being coerced into fetching a token on an attacker's behalf, since a cross-origin request cannot set one. Because gcsfuse did not send it, ACE had to exempt the token endpoint from the check, leaving an SSRF/CSRF hole open (b/553201361). Sending the header unconditionally lets that exemption be removed, and is harmless against endpoints that do not inspect it. Build the request explicitly and set the header rather than using the client.Get shorthand. Tests: adds Test_NewTokenSourceFromURL_SendsMetadataFlavorHeader and Test_NewTokenSourceFromURL_UnixSocket_SendsMetadataFlavorHeader, covering both the TCP and the unix-socket transports, since the UDS path rewrites the request URL and is worth asserting separately. * chore(deps): upgrade github.com/google/s2a-go to v0.1.10 v0.1.10 includes 489047f, "Client TLS configs should plumb identity to the S2A server", which fixes LocalIdentity being dropped on the client TLS config path. GCSFuse builds its http1 dialer through s2a.NewTLSClientConfigFactory, so on v0.1.9 a delegated SPIFFE identity never reached the S2A server and the handshake used the host identity instead. The ALPN workaround in storageutil.newS2ADialTLSContextForHTTP1 is still required. v0.1.10 continues to hardcode the client TLS config's NextProtos to {"h2"}; google/s2a-go#159 makes that selectable and the workaround can be removed once it ships. Verified: go build ./... and go test -count=1 ./internal/storage/... ./cfg/... both pass. * fix(cfg): update proto tags and fix proto Go field CamelCase mapping * fix: pass baseDialer to newS2ADialTLSContextForHTTP1 to preserve custom dialer settings * fix(storage): align anonymous transport behavior and remove redundant WithoutAuthentication from S2A --------- Co-authored-by: Sruthi Talluri <sruthitalluri@google.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
GetTLSConfigurationForClient has hardcoded
NextProtos to {"h2"}, which was added before when s2a-go was only used for gRPC. The HTTP entry points added later,NewTLSClientConfigFactoryandNewS2ADialTLSContextFunc, inherited that default, so an HTTP/1.1 client gets a connection that has negotiated h2 via ALPN. net/http then writes HTTP/1.1 onto it and the first response fails to parse:which is an HTTP/2 SETTINGS frame. There is currently no way to avoid this other than rebuilding the tls.Config by hand after Build returns.
This adds
ClientOptions.NextProtos, applied whenever a clienttls.Configis built, andTLSClientConfigOptions.NextProtosas a per-Build override.NewS2ADialTLSContextFuncgoes throughNewTLSClientConfigFactory, so it picks the option.Behavior is unchanged when neither is
set: nextProtosOrDefaultfalls back to{"h2"}. The gRPC ClientHandshake path ininternal/v2/s2av2.gopassesnilexplicitly, since that transport requires HTTP/2 and must not be overridable. The returned slice is copied so a later mutation by the caller cannot alter a livetls.Config.