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

Calling a non-TLS service running behing TLS enabled load balancer/proxy, says http2 frame too large #839

Closed
swagftw opened this issue Jun 27, 2024 · 4 comments

Comments

@swagftw
Copy link

swagftw commented Jun 27, 2024

I understand that the error makes sense, as my service (Let's name it A) will try to respond with http/1.1 when the centrifugo expects http/2 response and thus says frame too large.

But I also call this service A from another service B, which is able to communicate with service A, because my gRPC client configuration explicitly says insecureSkipVerify: true inside TLS configuration while initiating the gRPC client.

coreServiceConn, err = grpc.Dial(
			config.GetCoreServerURL(),
			grpc.WithTransportCredentials(credentials.NewTLS(&tls.Config{
				InsecureSkipVerify: true, //nolint:gosec
			})),
			grpc.WithStatsHandler(otelgrpc.NewClientHandler()),
		)

I was going through the Centrifuge code and saw that the gRPC client makes the following the request when certs are not provided explicitly in configuration. Reference

function reference:

func getDialOpts(p Config) ([]grpc.DialOption, error) {

exact line reference:

dialOpts = append(dialOpts, grpc.WithTransportCredentials(insecure.NewCredentials()))

The reason I asked for this question and not created an issue

Is there any reason (that I am unaware of) due to which the client does not support any configuration to allow the TLS config with insecureFlag=true?

So the client configuration will look like below

func getDialOpts(p Config) ([]grpc.DialOption, error) {
	var dialOpts []grpc.DialOption
	if p.GrpcCredentialsKey != "" {
		dialOpts = append(dialOpts, grpc.WithPerRPCCredentials(&rpcCredentials{
			key:   p.GrpcCredentialsKey,
			value: p.GrpcCredentialsValue,
		}))
	}
	if p.GrpcCertFile != "" {
		cred, err := credentials.NewClientTLSFromFile(p.GrpcCertFile, "")
		if err != nil {
			return nil, fmt.Errorf("failed to create TLS credentials %v", err)
		}
		dialOpts = append(dialOpts, grpc.WithTransportCredentials(cred))
	} else if p.SkipInsecureVerify {
		dialOpts = append(dialOpts, grpc.WithTransportCredentials(credentials.NewTLS(&tls.Config{
				InsecureSkipVerify: true, //nolint:gosec
			})))
	}else {
		dialOpts = append(dialOpts, grpc.WithTransportCredentials(insecure.NewCredentials()))
        }
	if p.GrpcCompression {
		dialOpts = append(dialOpts, grpc.WithDefaultCallOptions(grpc.UseCompressor(gzip.Name)))
	}

	if p.testGrpcDialer != nil {
		dialOpts = append(dialOpts, grpc.WithContextDialer(p.testGrpcDialer))
	}

	return dialOpts, nil
}
@FZambia
Copy link
Member

FZambia commented Jun 28, 2024

Hello @swagftw

Actually, we can support such option, we support InsecureSkipVerify option for other TLS configurations. This is btw related to #831 – would be great to have a common approach for all TLS configurations throughout Centrifugo, and specifically regarding this issue – allow configure the entire TLS config for GRPC client, so that it could be possible to pass not only server CA root cert, but support mTLS, etc.

UPD. I am now making relevant things, so will try to address this as some starting point for #831

@swagftw
Copy link
Author

swagftw commented Jun 28, 2024

Hey @FZambia

I saw that issue before raising this question here. I also thought that it was related, having a sense of common configuration related to TLS in this case makes much sense, but it has to be per transport, as you have suggested in the issue as well. I am using http proxy for this, so, I have two listeners in my server now, which I want to avoid.

I will wait till we have this merged. I would love to work on this, but I am quite caught in my own work.

@FZambia
Copy link
Member

FZambia commented Jul 15, 2024

@swagftw hello, this was addressed in Centrifugo v5.4.2 - please check it out, I am closing the issue, but feel free to continue discussion here or re-open in case of issues with the configuration.

@FZambia FZambia closed this as completed Jul 15, 2024
@swagftw
Copy link
Author

swagftw commented Jul 16, 2024

@swagftw hello, this was addressed in Centrifugo v5.4.2 - please check it out, I am closing the issue, but feel free to continue discussion here or re-open in case of issues with the configuration.

I will take a look :)

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

No branches or pull requests

2 participants