Support Basic Authorization With UserName and Password#33
Conversation
|
Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to cla@trino.io. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla |
rileymcdowell
left a comment
rileymcdowell
left a comment
There was a problem hiding this comment.
I left a few comments. Let's see if we can work through the details and get this compiling. This PR can also update the README to say password auth is supported.
I'll plan to test the actual uname/pass auth as well as replacing some an in-use driver to make sure the existing auth flows still work like they did before. If it does all that it'll be good to merge.
Nice work and nice looking code!
| AM_EXTERNAL_AUTH = 1, | ||
| AM_CLIENT_CRED_AUTH = 2, | ||
| AM_DEVICE_FLOW = 3 | ||
| AM_DEVICE_CODE = 3, |
There was a problem hiding this comment.
The rename from AM_DEVICE_FLOW to AM_DEVICE_CODE is causing compile errors for me. Looks like there are references to it in src/driver/config/driverConfig.cpp. I'm in favor of the rename, but you may need to include at least one more file in this PR to take care of the full rename.
There was a problem hiding this comment.
This base64 code is pretty terse. It also is eerily similar to this implementation, which means we probably need to include a copyright notice and the MIT license text. We also have src/util/b64decoder.cpp already in the source tree which does a similar thing.
I see two paths forward here that make sense.
- Bring it in a dependency/library to offload managing the code but keep the functionality.
- Leverage the CryptoAPI on windows. We already had to take a dependency on it for managing OIDC Client Secret encryption/decryption. It also supports base64, so using that would eliminate this code as well as the b64decoder.cpp already in the repo. The downside is it's another functional dependency on the windows api.
Thoughts?
|
|
||
| std::string const ConnectionConfig::getStatementUrl() { | ||
| return (this->hostname) + ":" + std::to_string(port) + "/v1/statement"; | ||
| std::string scheme = port == 443 || port == 8443 ? "https://" : "http://"; |
There was a problem hiding this comment.
This looks like it might be a breaking change for how the hostname is passed to the driver. Currently it expects the scheme to be prefixed on the hostname. Once this PR is able to be compiled, I'll give it a test to confirm or deny that it's a breaking change.
I'm not opposed to the opinionated "*443 means HTTPS" approach, but it's a decision to adopt with intention.
| #if DEBUG | ||
| //disallow peer cert verification in debug builds | ||
| curl_easy_setopt(this->curl, CURLOPT_SSL_VERIFYPEER, 0); | ||
| #endif |
There was a problem hiding this comment.
I'm not sure how I feel about disabling cert verification in debug builds. This almost feels like a better fit for a driver-level setting that defaults to enforcement but can be disabled by users for a specific DSN if they want.
I would be in favor of keeping this in and adding an issue to track removing on a feature request for a DSN-level setting.
Support Basic Authorization With UserName and Password
dprophet
force-pushed
the
SupportBasicAuthUserNameAndPassword
branch
from
95f1c25 to
64bde13
Compare
|
Thank you for implementing this! In case it may help here is my commit: I didn't handle proper storage of the password in the registry (no encryption), got confused between AM_DEVICE_FLOW and AM_DEVICE_CODE, and commented out some lines about oidc, but i'm now querying data to my basic auth trino. Oh, and the ODBC DSN form still needs work related to password handling and auth types. |
3 participants
Support Basic Authorization With UserName and Password