-
Notifications
You must be signed in to change notification settings - Fork 59
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
Support for JWT Authentication #110
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 |
trino/trino.go
Outdated
@@ -249,6 +252,7 @@ func (c *Config) FormatDSN() (string, error) { | |||
"session_properties": strings.Join(sessionkv, ","), | |||
"extra_credentials": strings.Join(credkv, ","), | |||
"custom_client": c.CustomClientName, | |||
"bearer_auth": c.BearerAuth, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be called accessToken
, as in the JDBC driver? Also rename the config struct property.
Reference: https://trino.io/docs/current/client/jdbc.html#parameter-reference
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nineinchnick - Sure, I can fix that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nineinchnick - Fixed it. Please check.
trino/etc/secrets/generate_jwt.py
Outdated
# Generate JWT token | ||
token = jwt.encode(payload, private_key, algorithm='RS256') | ||
|
||
print(token) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When this should be used? Maybe mention it in the README or test docs. Can this be a Go program?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nineinchnick - This is the code I used to generate the token. I can change to Go and I will add README.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Added README
- Removed not used secret files
- Added Go based JWT token generator
- Increase expiry time for certificate/token
trino/etc/secrets/jwt_token
Outdated
@@ -0,0 +1 @@ | |||
eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiJ0ZXN0IiwiZXhwIjo0ODY3MjQ4MzY1fQ.a8O5YhYSn1WNedLqAYq8e5lXSwOkabZjhvKIAXPEGBc5juYBgnWcfad4kXn4PCoD6ykcmGACRuMiBNGbQMxPy6iX5vNie8tX-P-GuXhgHuRIcf6deVOibGb9RHaS0VwtwsgBZREhxF9-410db0k5qwipVJKu-t8mttvgIRhx2hyRaXigH2IT1W3Q06K3o81Olq-c_b4qF1oVpVEuQ7uzPUXb5LVK30YVBcUtFHtlYQX0ZJ_5hCTI37l5krBGIt--WOGs368tVrUPDLyhdEW2V7dN_NnPwfGuxzBVRTgC8oqGNlbOWSouUYrfW3gN6LYEckBfXGbk5daNQ5u0OmYkgg |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How long is this valid for? Can it be valid for a really long time, so we never have to regenerate it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nineinchnick - Valid for 100 years.
Repository: "trinodb/trino", | ||
Tag: *trinoImageTagFlag, | ||
Mounts: []string{wd + "/etc:/etc/trino"}, | ||
ExposedPorts: exposedPorts, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to expose ports? This will cause conflicts if anything already uses these ports (other containers).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nineinchnick - These are container ports and mapped to some random ports on host m/c, they wont't conflict. I have setup 2 ports, 8080 is without authentication and 8081 is with jwt authentication.
@@ -106,6 +113,8 @@ func TestMain(m *testing.M) { | |||
log.Fatalf("Timed out waiting for container to get ready: %s", err) | |||
} | |||
*integrationServerFlag = "http://test@localhost:" + resource.GetPort("8080/tcp") | |||
|
|||
http.DefaultTransport.(*http.Transport).TLSClientConfig = &tls.Config{InsecureSkipVerify: true} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we test without InsecureSkipVerify
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nineinchnick - We can not easily test without InsecureSkipVerify as I have self signed certificate. The process of verifying certificate is more complicated, I think not worth for integration testing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you have self-signed certificates, you should have the CA too and set it here to be able to verify the server certificate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nineinchnick - Still working on this. I will have this by EOD.
trino/integration_test.go
Outdated
@@ -751,6 +760,28 @@ func TestIntegrationQueryContextCancellation(t *testing.T) { | |||
} | |||
} | |||
|
|||
func TestIntegrationBearerAuth(t *testing.T) { | |||
bearerAuth := "eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiJ0ZXN0IiwiZXhwIjo0ODY3MjQ4MzY1fQ.a8O5YhYSn1WNedLqAYq8e5lXSwOkabZjhvKIAXPEGBc5juYBgnWcfad4kXn4PCoD6ykcmGACRuMiBNGbQMxPy6iX5vNie8tX-P-GuXhgHuRIcf6deVOibGb9RHaS0VwtwsgBZREhxF9-410db0k5qwipVJKu-t8mttvgIRhx2hyRaXigH2IT1W3Q06K3o81Olq-c_b4qF1oVpVEuQ7uzPUXb5LVK30YVBcUtFHtlYQX0ZJ_5hCTI37l5krBGIt--WOGs368tVrUPDLyhdEW2V7dN_NnPwfGuxzBVRTgC8oqGNlbOWSouUYrfW3gN6LYEckBfXGbk5daNQ5u0OmYkgg" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this token different from trino/etc/secrets/jwt_token
? Do we need both? How long it's valid for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @nineinchnick - Thanks for your feedback.
The JWT token has a validity of 100 years and I can remove all the files in the directory that are not used (just added for completeness). Also, I will add steps in Readme file in case we need to generate the secrets again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, please remove unused files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nineinchnick - Removed unused files
trino/trino.go
Outdated
@@ -362,9 +366,14 @@ func newConn(dsn string) (*Conn, error) { | |||
trinoSchemaHeader: query.Get("schema"), | |||
trinoSessionHeader: query.Get("session_properties"), | |||
trinoExtraCredentialHeader: query.Get("extra_credentials"), | |||
authorization: query.Get("bearer_auth"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a method that would prefix the value of query.Get("bearer_auth")
if it's not empty to avoid the condition in the loop below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nineinchnick - Fixed. Please check.
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 |
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 |
BTW this will fix #59 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost there, you can resolve my comments that were already addressed (I can't).
@@ -751,6 +760,28 @@ func TestIntegrationQueryContextCancellation(t *testing.T) { | |||
} | |||
} | |||
|
|||
func TestIntegrationAccessToken(t *testing.T) { | |||
accessToken := "eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnb3RyaW5vIiwic3ViIjoidGVzdCIsImV4cCI6NDg2NzQzMTgzMH0.qKwLIsAxg6O2RfonRcxMatXuQ8Prw_lLqnl7cCt1zRgL_zcZBAm4rdv8PFu2QpWshIG7675n0VE0y4c5q1-Q-ifuCbfAAExD8SXEi5_aa2299xiQH8ADmrycizzebGNIwwK1qzH6u3jNWwSkABbDaJ58Z7vnQDPE5yir_Fap4mE7-OXWjuKF5CNL9CbtfBMEsnMtSZ6kRTvlBQ84dNOaYwcHPhQQuwKaIFFzrvN_nathFc5r1qjLkSvxA6mPLGw-dqXt2k0QD0DEppyu80b0BL3pPLXXZ99VfGR2iW0j2n34D_3PF4gUOTSh8jpfzrDPC-qqxlfUb_5yTA-LBDImsw" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you ported the code that generates this to Go, you could move it into a method and generate a short-lived token every time in this test. It should be quick enough, right?
if count < 1 { | ||
t.Fatal("not enough rows returned:", count) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of counting rows, I think we should use a more deterministic query, like SELECT 1
and test for the return value. This will help to avoid breaking this test if we add some more test catalogs.
@kamalkang-hpe did you close this my mistake or intentionally? I can finish this in another PR, preserving your authorship of the commits. |
Hi Jan,
Sorry, I used wrong GitHub account. Please do what is appropriate, and no need to preserve authorship.
Sorry about that.
Kamal
From: Jan Waś ***@***.***>
Date: Wednesday, April 24, 2024 at 1:40 PM
To: trinodb/trino-go-client ***@***.***>
Cc: Kang, Kamal ***@***.***>, Mention ***@***.***>
Subject: Re: [trinodb/trino-go-client] Support for JWT Authentication (PR #110)
@kamalkang-hpe<https://github.com/kamalkang-hpe> did you close this my mistake or intentionally? I can finish this in another PR, preserving your authorship of the commits.
—
Reply to this email directly, view it on GitHub<#110 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AX7UBBCYM4VLLNJRLB5ZOGLY67U55AVCNFSM6AAAAABGRXD5ACVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANZVGQ4TCNJZGQ>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
Added support for JWT Authentication