-
Notifications
You must be signed in to change notification settings - Fork 702
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
Update dashboard's validateToken not to use k8s api server. #4043
Conversation
Signed-off-by: Michael Nelson <minelson@vmware.com>
Signed-off-by: Michael Nelson <minelson@vmware.com>
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.
Thanks for the changes, +1 anyway, but have a look at the TokenReview
comment
@@ -60,21 +65,27 @@ export class Auth { | |||
// Throws an error if the token is invalid | |||
public static async validateToken(cluster: string, token: string) { | |||
try { | |||
await Axios.get(url.api.k8s.base(cluster) + "/", { | |||
headers: { Authorization: `Bearer ${token}` }, | |||
await this.resourcesClient(token).CheckNamespaceExists({ |
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.
+1 to this approach; however, I wonder whether we should use an alternative function here. I mean, at first glance, calling CheckNamespaceExists
for the validateToken
function seems a bit misleading.
Perhaps we could use the can-i
operation there or, even better, directly use the k8s object aimed at validating tokens, the TokenReview
one (more info):
{
"apiVersion": "authentication.k8s.io/v1",
"kind": "TokenReview",
"spec": {
"token": "014fbff9a07c...",
}
}
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.
Yeah, I'd initially gone along this direction, adding a new endpoint to the resources plugin (didn't think of using the TokenReview - that would have been a good match), but the reason I stopped was that this functionality (for validateToken
) is relevant only for the "demo-only" functionality of using service account tokens to authenticate with Kubeapps.
The existing functionality just tries to get the root URL of the k8s API server and I realised that the endpoint is actually arbitrary - the check just needs to see a 200 or 403 - either is fine.
+1 to switch it over to a can-i
endpoint, once we have one available in the resources plugin.
dashboard/src/shared/Auth.test.ts
Outdated
// TODO(absoludity): tried using `expect(fn()).rejects.toThrow()` but it seems we need | ||
// to upgrade jest for `toThrow()` to work with async. |
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.
Just wondering if this ToDo still applies after having applied several upgrades to the jest deps.
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.
Nope, it works with the .toThrow
now in an async function. Thanks for the nudge, updated.
// TODO(minelson): Move to a shared helper for plugins interacting | ||
// with the k8s cluster. |
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.
+1, we are using this function also in the flux and carvel plugins
Signed-off-by: Michael Nelson <minelson@vmware.com>
Signed-off-by: Michael Nelson minelson@vmware.com
Description of the change
Part one of removing
shared/Auth
's use of the k8s API server, this PR updates thevalidateToken
check, used when Kubeapps is configured with token authentication, so that it hits the resources API (checkNamespace
), rather than hitting the k8s API server directly.To do so, I had to correct the error handling in the resources plugin to differentiate between 401s and 403s (and will later follow up as per the TODO there).
I also fixed an issue where the dashboard was checking cookie auth based only on the loginURL being set, which it is by default anyway (which resulted in an extra error being displayed when token login failed).
Benefits
One more action no longer hitting k8s API.
Possible drawbacks
Applicable issues
Additional information