Skip to content
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

fix: validate CLI using REST / instead of REST /info #3197

Merged
merged 1 commit into from Aug 14, 2019

Conversation

@spena
Copy link
Member

commented Aug 12, 2019

BREAKING CHANGE: Previously CLI startup permissions were based on whether the user has access to /info, but now it's based on whether the user has access to /. This change implies that if a user has permissions to / but not /info, they now have access to the CLI whereas previously they did not.

Description

The CLI currently makes a call to GET /info to validate if a client can connect to the KSQL server. If the server is not authorized to access the endpoint, then the CLI displays the correct access error (or any other access) on the console.

With the introduction of the KSQL security extensions, these REST endpoints might have different access restrictions. For instance, the GET /info call might be opened to any authenticated user to get information about the KSQL server (such as KSQL and Kafka clusters IDs) and send the info to the KSQL admins to request access this KSQL server. Because of this, then the CLI will never show if a user is authorized to access the server during the CLI initialization.

To improve this CLI message, we should validate the client using the GET / endpoint instead. This endpoint is not used and looks like a dummy endpoint.

Testing done

  • Modified the current unit tests to use the new endpoint
  • Verified the CLI messages manually:
 ./bin/ksql --user u1 --password u1 http://localhost:8088                 
...

CLI v5.4.0-SNAPSHOT, Server v5.4.0-SNAPSHOT located at http://localhost:8088

Having trouble? Type 'help' (case-insensitive) for a rundown of how things work!

Couldn't connect to the KSQL server: Access denied: Contribute permissions required to access "GET /"

ksql>

Reviewer checklist

  • Ensure docs are updated if necessary. (eg. if a user visible feature is being added or changed).
  • Ensure relevant issues are linked (description should include text like "Fixes #")

@spena spena requested review from vcrfxia and confluentinc/ksql Aug 12, 2019

@spena spena self-assigned this Aug 12, 2019

@vcrfxia

This comment has been minimized.

Copy link
Contributor

commented Aug 13, 2019

Hey @spena I'm having trouble understanding why this is different. Doesn't / simply redirect to /info? How could the two have different permissions?

@spena

This comment has been minimized.

Copy link
Member Author

commented Aug 13, 2019

@vcrfxia That line just returns a response to the caller about the new location the caller must be redirected. It does not do the redirection automatically unless your client manages that. For instance, a Web browser will look into the HTTP response to see if there is a new location that needs to be redirected.

Here's what CURL gets:

$ curl -v http://localhost:8088
....
> GET / HTTP/1.1
> Host: localhost:8088
> User-Agent: curl/7.64.0
> Accept: */*
> 
< HTTP/1.1 307 Temporary Redirect
< Date: Tue, 13 Aug 2019 14:37:18 GMT
< Location: http://localhost:8088/info
...
@vcrfxia

This comment has been minimized.

Copy link
Contributor

commented Aug 13, 2019

That line just returns a response to the caller about the new location the caller must be redirected. It does not do the redirection automatically unless your client manages that.

To check that I understand correctly, you're saying that if a user has access to /info but not /, then when the user tries to start the CLI the CLI will show that the user does not have permissions to access the KSQL server, despite the redirect response? If so, the changes here LGTM.

Can you call out the fact that this is a breaking change by adding BREAKING CHANGE: (followed by an explanation) into the PR description, and also ensure that the BREAKING CHANGE: line is propagated through to the commit body when the PR is merged? That way this change will be called out in our auto-generated changelogs. Here's the relevant section of our CONTRIBUTING.md: https://github.com/confluentinc/ksql/blob/master/CONTRIBUTING.md#breaking-changes

@spena

This comment has been minimized.

Copy link
Member Author

commented Aug 13, 2019

That's correct @vcrfxia

Regarding the breaking change, I'm not sure I'm breaking something here. The CLI will continue working as before. No new output is added or modified. If something is broken, that would be on the REST endpoint authorization which is handled by an external authorization library which is not exposed in this code.

Or what do you think would break?

@vcrfxia

This comment has been minimized.

Copy link
Contributor

commented Aug 13, 2019

"Breaking change" doesn't mean anything is broken, but rather that the PR introduces backwards-incompatible behavior. The behavior that's changed here is that previously CLI startup permissions were based on whether the user has access to /info, but now it's based on whether the user has access to /. This change implies that if a user has permissions to / but not /info, they now have access to the CLI whereas previously they did not. While I don't expect this to be a common case, I think it's worth calling out in our changelogs (and also generally good to get in the habit of calling out breaking changes, even if minor).

@spena

This comment has been minimized.

Copy link
Member Author

commented Aug 13, 2019

thanks @vcrfxia . I used your same comment in the breaking change in this PR. You used the right words to describe it :).

What do you think?

@vcrfxia
Copy link
Contributor

left a comment

Thanks @spena -- LGTM! Friendly reminder to propagate the "BREAKING CHANGE: [...]" text from the PR description into the commit message body when you merge the PR :)

@vcrfxia vcrfxia requested a review from confluentinc/ksql Aug 13, 2019

@spena spena force-pushed the spena:validate_client_on_slash branch from 8ddfa33 to 3339c7a Aug 14, 2019

@spena

This comment has been minimized.

Copy link
Member Author

commented Aug 14, 2019

I'll force the merge. Jenkins is failing on the Twislockscan phase, which seems flaky.

@spena spena merged commit 7935488 into confluentinc:5.3.x Aug 14, 2019

1 of 2 checks passed

continuous-integration/jenkins/pr-merge This commit cannot be built
Details
Semantic Pull Request ready to be squashed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.