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.

Sign up for GitHub

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

Jump to bottom

[Go][FlightRPC] flightsql gRPC driver does not respect flightsql's DriverConfig.TLSEnabled field #40097

Closed
waynr opened this issue Feb 15, 2024 · 0 comments · Fixed by #40098
Closed

[Go][FlightRPC] flightsql gRPC driver does not respect flightsql's DriverConfig.TLSEnabled field #40097

waynr opened this issue Feb 15, 2024 · 0 comments · Fixed by #40098
Assignees
@waynr
Labels
Component: FlightRPC Component: Go Type: bug
Milestone
16.0.0

Comments

@waynr
Copy link
Contributor

waynr commented Feb 15, 2024 •
edited
Loading

Describe the bug, including details regarding any error messages, version, and platform.

I am working with an internal tool that establishes a flightsql gRPC connection to a thing. The tool sets DriverConfig.TLSEnabled to false but I still see this error when the connection is being created:

grpc: the credentials require transport level security (use grpc.WithTransportCredentials() to set)

This error is coming from grpc-go's ClientConn.validateTransportCredential but only after calling the RequireTransportSecurity() method of the PerRPCCredentials trait.

The flightsql driver's grpcCredentials type implements this interface, but it doesn't seem to have any way to respect the aforementioned DriverConfig.TLSEnabled field.

I am about to open a PR to propose threading the config parameter through and have flightsql's grpcCredentials.RequireTransportSecurity() implementation respect it (definitely open to alternative approaches). I am writing up this issue because the CONTRIBUTING doc suggested I do so before opening a PR.

Component(s)

FlightRPC, Go
@waynr waynr mentioned this issue Feb 15, 2024
Merged
zeroshade pushed a commit that referenced this issue Feb 20, 2024
@waynr
GH-40097: [Go][FlightRPC] Enable disabling TLS (#40098)
a690088 
See #40097 for more in-depth description
about the problem that led me to file this PR.

### Rationale for this change

Because it's annoying to not be able to connect to a non-TLS flightsql endpoint
in my development environment just because my development environment happens
to still use token authentication.

### What changes are included in this PR?

Thread the flightsql `DriverConfig.TLSEnabled` parameter into the
`grpcCredentials` type so that `grpcCredentials.RequireTransportSecurity` can
return false if TLS is not enabled on the driver config.

One thing that occurred to me about the `DriverConfig.TLSEnabled` field is that
its semantics seem very mildly dangerous since golang `bool` types are `false`
by default and golang doesn't require fields on structs to be explicitly
initialized. It seems to me that `DriverConfig.TLSDisabled` would be better (semantically speaking)
because then the API user doesn't have to explicitly enable TLS. But I suppose
it's probably undesirable to change the name of a public field on a public type.

### Are these changes tested?

I haven't written any tests, mostly because there weren't already any tests for
the `grpcCredentials` type but I have manually verified this fixes the problem
I described in #40097 by rebuilding my
tool and running it against the non-TLS listening thing in my development
environment.

### Are there any user-facing changes?

* Closes: #40097

Authored-by: wayne warren <wayne.warren.s@gmail.com>
Signed-off-by: Matt Topol <zotthewizard@gmail.com>
@zeroshade zeroshade added this to the 16.0.0 milestone Feb 20, 2024
@kou kou changed the title flightsql gRPC driver does not respect flightsql's DriverConfig.TLSEnabled field [Go][FlightRPC] flightsql gRPC driver does not respect flightsql's DriverConfig.TLSEnabled field Feb 20, 2024
zanmato1984 pushed a commit to zanmato1984/arrow that referenced this issue Feb 28, 2024
@waynr @zanmato1984
apacheGH-40097: [Go][FlightRPC] Enable disabling TLS (apache#40098)
3120428 
See apache#40097 for more in-depth description
about the problem that led me to file this PR.

### Rationale for this change

Because it's annoying to not be able to connect to a non-TLS flightsql endpoint
in my development environment just because my development environment happens
to still use token authentication.

### What changes are included in this PR?

Thread the flightsql `DriverConfig.TLSEnabled` parameter into the
`grpcCredentials` type so that `grpcCredentials.RequireTransportSecurity` can
return false if TLS is not enabled on the driver config.

One thing that occurred to me about the `DriverConfig.TLSEnabled` field is that
its semantics seem very mildly dangerous since golang `bool` types are `false`
by default and golang doesn't require fields on structs to be explicitly
initialized. It seems to me that `DriverConfig.TLSDisabled` would be better (semantically speaking)
because then the API user doesn't have to explicitly enable TLS. But I suppose
it's probably undesirable to change the name of a public field on a public type.

### Are these changes tested?

I haven't written any tests, mostly because there weren't already any tests for
the `grpcCredentials` type but I have manually verified this fixes the problem
I described in apache#40097 by rebuilding my
tool and running it against the non-TLS listening thing in my development
environment.

### Are there any user-facing changes?

* Closes: apache#40097

Authored-by: wayne warren <wayne.warren.s@gmail.com>
Signed-off-by: Matt Topol <zotthewizard@gmail.com>
thisisnic pushed a commit to thisisnic/arrow that referenced this issue Mar 8, 2024
@waynr @thisisnic
apacheGH-40097: [Go][FlightRPC] Enable disabling TLS (apache#40098)
1d735e8 
See apache#40097 for more in-depth description
about the problem that led me to file this PR.

### Rationale for this change

Because it's annoying to not be able to connect to a non-TLS flightsql endpoint
in my development environment just because my development environment happens
to still use token authentication.

### What changes are included in this PR?

Thread the flightsql `DriverConfig.TLSEnabled` parameter into the
`grpcCredentials` type so that `grpcCredentials.RequireTransportSecurity` can
return false if TLS is not enabled on the driver config.

One thing that occurred to me about the `DriverConfig.TLSEnabled` field is that
its semantics seem very mildly dangerous since golang `bool` types are `false`
by default and golang doesn't require fields on structs to be explicitly
initialized. It seems to me that `DriverConfig.TLSDisabled` would be better (semantically speaking)
because then the API user doesn't have to explicitly enable TLS. But I suppose
it's probably undesirable to change the name of a public field on a public type.

### Are these changes tested?

I haven't written any tests, mostly because there weren't already any tests for
the `grpcCredentials` type but I have manually verified this fixes the problem
I described in apache#40097 by rebuilding my
tool and running it against the non-TLS listening thing in my development
environment.

### Are there any user-facing changes?

* Closes: apache#40097

Authored-by: wayne warren <wayne.warren.s@gmail.com>
Signed-off-by: Matt Topol <zotthewizard@gmail.com>
kou pushed a commit to apache/arrow-go that referenced this issue Aug 30, 2024
@waynr @kou
GH-40097: [Go][FlightRPC] Enable disabling TLS (#40098)
f098f11 
See apache/arrow#40097 for more in-depth description
about the problem that led me to file this PR.

### Rationale for this change

Because it's annoying to not be able to connect to a non-TLS flightsql endpoint
in my development environment just because my development environment happens
to still use token authentication.

### What changes are included in this PR?

Thread the flightsql `DriverConfig.TLSEnabled` parameter into the
`grpcCredentials` type so that `grpcCredentials.RequireTransportSecurity` can
return false if TLS is not enabled on the driver config.

One thing that occurred to me about the `DriverConfig.TLSEnabled` field is that
its semantics seem very mildly dangerous since golang `bool` types are `false`
by default and golang doesn't require fields on structs to be explicitly
initialized. It seems to me that `DriverConfig.TLSDisabled` would be better (semantically speaking)
because then the API user doesn't have to explicitly enable TLS. But I suppose
it's probably undesirable to change the name of a public field on a public type.

### Are these changes tested?

I haven't written any tests, mostly because there weren't already any tests for
the `grpcCredentials` type but I have manually verified this fixes the problem
I described in apache/arrow#40097 by rebuilding my
tool and running it against the non-TLS listening thing in my development
environment.

### Are there any user-facing changes?

* Closes: #40097

Authored-by: wayne warren <wayne.warren.s@gmail.com>
Signed-off-by: Matt Topol <zotthewizard@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@waynr waynr

Labels
Component: FlightRPC Component: Go Type: bug
Projects
None yet
Milestone
16.0.0
Development

Successfully merging a pull request may close this issue.

GH-40097: [Go][FlightRPC] Enable disabling TLS waynr/arrow
2 participants
@zeroshade @waynr