Add support for basic authentication #7
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 [email protected]. 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 |
|
Trino does NOT have a preconfigured admin user or anything like that so we should not hardcode a username in this class. Ideally we should try to add a test for basic auth. Commit message should be changed to "Add support for basic authentication" it should be suitable for any usage .. not just testing.. |
|
The default config from the trino helm chart creates a user named admin without a password. This was meant for this. The reason i say it is only for testing is that you don't want a authorization without passwords in production. That is why i want to discourage this. I will remove the default user and will add a simple unit test. |
|
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 [email protected]. 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 |
|
The code looks similar to the LDAP auth implementation |
|
I think that you want to keep these 2 separate. The BasicAuth is very insecure and I would recommend no one to use in production and only for prototyping. I can change this code but is this really something that you want? |
zelda1link3
changed the title
|
Why do you think BasicAuth is very insecure @zelda1link3 ... its just the usual username and password auth over HTTPS .. which then can be using different backends on the Trino side .. but thats kinda irrelevant. HTTPS is required for any auth in Trino. And if you are just talking about the default mode in which Trino can run on HTTP without any authentication but requires a username and no password .. yes thats insecure and if we need implementation for that it could be separate from BasicAuth .. or be the same with some override .. probably best if we do the same thing the JDBC driver does.. maybe @electrum or @wendigo can chime in here |
|
The reason is that you only provide a username. So for that reason i think it is insecure to use this in production. |
Well.. that is the INSECURE configuration of the Trino cluster .. that does not make BasicAuthentication as such in the client library insecure .. as long as it can be used with password as well thats fine from my point of view. |
|
BasicAuth should at least have an optional password field to make it complete (see https://www.rfc-editor.org/rfc/rfc7617). LDAPAuth can extend BasicAuth or not, but given that it uses the same username:password approach it probably should. |
|
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 [email protected]. 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 |
|
I updated the code to reflect the reviews better. I also already send an email with the CLA but i think it still needs to be reviewed. |
For testing purposes trino uses default only admin as user. This allows this.