feat(go/adbc/driver/flightsql): Add OAuth Support to Flight Client - #2651
Conversation
|
I would suggest to update |
howareyouman
left a comment
There was a problem hiding this comment.
Thank you, Helder, for this PR!
The only concerns I have are around thread safety.
| }, nil | ||
| } | ||
|
|
||
| func (f *tokenExchange) GetToken(ctx context.Context) (*oauth2.Token, error) { |
There was a problem hiding this comment.
Could you please add RWLock here to make this method thread safe?
| }, nil | ||
| } | ||
|
|
||
| func (c *clientCredentials) GetToken(ctx context.Context) (*oauth2.Token, error) { |
There was a problem hiding this comment.
The same question is here - should it be thread safe? If so, could you please add rwlock here?
zeroshade
left a comment
There was a problem hiding this comment.
maybe we can use https://pkg.go.dev/golang.org/x/oauth2@v0.28.0/clientcredentials#Config instead and just call https://pkg.go.dev/golang.org/x/oauth2@v0.28.0/clientcredentials#Config.TokenSource
If we do that, then most of this code can be entirely removed in favor of just using https://pkg.go.dev/google.golang.org/grpc/credentials/oauth#TokenSource and https://pkg.go.dev/google.golang.org/grpc#WithPerRPCCredentials which would manage the oauth flow for us entirely as long as we can construct the token source which performs refreshes as necessary (as per the clientcredentials config above)
Essentially, SetOptions can create the TokenSource and then getFlightClient just adds the token source as a dialoption via WithPerRPCCredentials. That would be much more convenient and less work/code for us to maintain
@zeroshade I was not aware of that, thank you for bringing this up. While I was testing this proposal I realized that TokenSource requires transport security. I think this conflicts with development and how to run tests making it mandatory to use tls. What alternatives do I have here? The options I see:
Do you have any suggestion? |
|
We have a TLS suite set up already for tests here: you can reuse that |
|
Also TokenSource on it's own shouldn't require transport security, it just has a method that returns a bool letting the caller know whether or not it requires it |
Replaced extra structs for OAuth in favor of grpc's TokenSource and grpc.DialOption WithPerRPCCredentials
* Changed OAuth tests to use TLS since it is a requirement from grpc's TokenSource and WithPerRPCCredentials * Split DoSetupSuit to setupFlightServer and setupDatabase so they can be used independently
* Removed adbc.flight.sql.token in favour of adbc.flight.sql.authorization_header when the client wants to pass a token or adbc.flight.sql.oauth.exchange.subject_token when client wants to do token exchange * Simplified options to set oauth flow. Now client can set client_credentials or token_exchange instead of integers
| header will then be sent back as the ``authorization`` header on all | ||
| future requests. | ||
|
|
||
| - (Go only) OAuth 2.0 authentication flows. |
There was a problem hiding this comment.
isn't it not Go only? Anything that uses the flightsql driver should be able to use the options that are being added. (We should add constants to the python adbc_driver_flightsql package)
There was a problem hiding this comment.
Does it make sense to create a separate PR for this?
There was a problem hiding this comment.
It makes sense to make a separate PR to add the option constants, but I would still say that the "Go only" should be removed as nothing would prevent any other binding from using these options.
|
Just to add to this (and this could be a follow-on PR) - what happens if https://github.com/apache/arrow-adbc/blob/a187ead78afebe85c75b466a06ad6e01ae4ac8c6/go/adbc/driver/flightsql/flightsql_statement.go#L194C21-L194C25 is part of a long running operation and the token expires? How does the token get refreshed and then continue the poll operation? By the way, this is a concern for all ADBC drivers in general, as we don't seem to have a standard way of failing "mid stream" and then "resume" the long running operation without starting it over. |
Or if runs long. |
And for context, #2655 is making an attempt to demonstrate this capability for BigQuery in the .NET driver when using Entra authentication. The Databricks driver also needs to do this. Essentially, any OAuth protected data source could support this type of retry behavior. |
|
@davidhcoe I think you can refer to this comment. |
zeroshade
left a comment
There was a problem hiding this comment.
just a couple nits, but otherwise this looks good to me! Thanks for this!
…pache#2651) ## Description This pull request introduces OAuth support to the Flight client in the GO driver. The changes include the addition of OAuth access token support, implementation of token exchange and client credentials OAuth flows. ## Related Issues - Closes #[2650](apache#2650) ## Changes Made 1. Added `token` as a database option 1. Added support for [Token Exchange](https://datatracker.ietf.org/doc/html/rfc8693). If configured, `token` gets exchanged and the result is added to the `Authorization` header as a `Bearer` token 1. Added support for [Client Credentials](https://datatracker.ietf.org/doc/html/rfc6749#section-4.4). If configured, `client_id` and `client_secret` are used to obtain a access token that is added to the `Authorization` header as a `Bearer` token 1. Added new driver options to allow third-party applications to configure oauth flows: 1. Added tests Here's the markdown code for the OAuth 2.0 configuration options table: markdown# OAuth 2.0 Configuration Options | Option | Description | |--------|-------------| | `adbc.flight.sql.oauth.flow` | Specifies the OAuth 2.0 flow type to use. Possible values: `client_credentials`, `token_exchange` | | `adbc.flight.sql.oauth.client_id` | Unique identifier issued to the client application by the authorization server | | `adbc.flight.sql.oauth.client_secret` | Secret associated to the client_id. Used to authenticate the client application to the authorization server | | `adbc.flight.sql.oauth.token_uri` | The endpoint URL where the client application requests tokens from the authorization server | | `adbc.flight.sql.oauth.scope` | Space-separated list of permissions that the client is requesting access to (e.g `"read.all offline_access"`) | | `adbc.flight.sql.oauth.exchange.subject_token` | The security token that the client application wants to exchange | | `adbc.flight.sql.oauth.exchange.subject_token_type` | Identifier for the type of the subject token. Check list below for supported token types. | | `adbc.flight.sql.oauth.exchange.actor_token` | A security token that represents the identity of the acting party | | `adbc.flight.sql.oauth.exchange.actor_token_type` | Identifier for the type of the actor token. Check list below for supported token types. | | `adbc.flight.sql.oauth.exchange.aud` | The intended audience for the requested security token | | `adbc.flight.sql.oauth.exchange.resource` | The resource server where the client intends to use the requested security token | | `adbc.flight.sql.oauth.exchange.scope` | Specific permissions requested for the new token | | `adbc.flight.sql.oauth.exchange.requested_token_type` | The type of token the client wants to receive in exchange. Check list below for supported token types. | **Supported token types:** * `urn:ietf:params:oauth:token-type:access_token` * `urn:ietf:params:oauth:token-type:refresh_token` * `urn:ietf:params:oauth:token-type:id_token` * `urn:ietf:params:oauth:token-type:saml1` * `urn:ietf:params:oauth:token-type:saml2` * `urn:ietf:params:oauth:token-type:jwt`
…pache#3849) This pull request adds OAuth options introduced in apache#2651 to Python. ## Changes Made - Added enums in python with available OAuth options - Updated documentation - Added python recipes with working mock oauth server ## Related Issues Closes apache#2714.
Description
This pull request introduces OAuth support to the Flight client in the GO driver. The changes include the addition of OAuth access token support, implementation of token exchange and client credentials OAuth flows.
Related Issues
Changes Made
tokenas a database optiontokengets exchanged and the result is added to theAuthorizationheader as aBearertokenclient_idandclient_secretare used to obtain a access token that is added to theAuthorizationheader as aBearertokenHere's the markdown code for the OAuth 2.0 configuration options table:
markdown# OAuth 2.0 Configuration Options
adbc.flight.sql.oauth.flowclient_credentials,token_exchangeadbc.flight.sql.oauth.client_idadbc.flight.sql.oauth.client_secretadbc.flight.sql.oauth.token_uriadbc.flight.sql.oauth.scope"read.all offline_access")adbc.flight.sql.oauth.exchange.subject_tokenadbc.flight.sql.oauth.exchange.subject_token_typeadbc.flight.sql.oauth.exchange.actor_tokenadbc.flight.sql.oauth.exchange.actor_token_typeadbc.flight.sql.oauth.exchange.audadbc.flight.sql.oauth.exchange.resourceadbc.flight.sql.oauth.exchange.scopeadbc.flight.sql.oauth.exchange.requested_token_typeSupported token types:
urn:ietf:params:oauth:token-type:access_tokenurn:ietf:params:oauth:token-type:refresh_tokenurn:ietf:params:oauth:token-type:id_tokenurn:ietf:params:oauth:token-type:saml1urn:ietf:params:oauth:token-type:saml2urn:ietf:params:oauth:token-type:jwt