Support for Basic Authentication

[tonyfosterdev]

Added a simple implementation for basic authentication.

This implementation follows roughly how kubectl formerly handled basic auth (I believe basic auth is now deprecated in Kubernetes, but don't quote me on that). Credentials may be added directly to a context in .kcctl or set using the set-context command.

Documentation was added to README and tests added for some relevant areas.

I chose to keep this implementation very simple. Additional items should be able to be added via KafkaConnectClientHeadersFactory in the future.

[tonyfosterdev]

This is in reference to #17

[tonyfosterdev]

@gunnarmorling Just wanted to make sure you saw this.

[gunnarmorling]

Hey, yep, saw it, will look into it ASAP. Thanks a lot!

…

[tonyfosterdev]

Just checking in to see if anyone has been able to look at this

[tonyfosterdev]

@jmcristobal can you test this?

[tonyfosterdev]

@gunnarmorling if it makes this easier, I could put together a repo with a container to demonstrate.

[gunnarmorling]

@tonyfosterdev, thank you so much! If you could do that, that'd be awesome indeed. Most of the times, I'm using this docker-compose.yml file for some manual testing of kcctl. If you could provide one similar to that, but using authenticated Kafka Connect, that'd be phantastic. Having never used authenication with KC myself, I've struggled so far to find a simple set-up which would allow to play with it. Thanks again!

[tonyfosterdev]

@gunnarmorling

I added a compose file, similar to yours above, with etcd stuff removed. I've commented the meaningful variables there. Also see that I've mounted ./basic-auth into the /kafka/configs directory inside the container. This is the configuration necessary for supporting basic auth, which was enabled in the compose file.

Assuming the alias for kcctl is setup via the README.docs, you should be able to run (assuming a localhost:8083 context):

$ kcctl info

and get an error indicating authentication issues. Modifying .kcctl to this:

{
  "currentContext" : "local",
  "local" : {
    "cluster" : "http://localhost:8083",
    "bootstrapServers" : "localhost:9092",
    "offsetTopic" : "my-connect-offsets",
    "username" : "testuser",
    "password" : "testpassword"
  }
}

should show things resolving once again. Playing with username and password in the config should show the authentication working as expected.

[tonyfosterdev]

@gunnarmorling Forgot to mention, the compose file is named docker-compose.basic-auth.yml. Run with the following:

$ docker-compose -f ./docker-compose.basic-auth.yml up

Let me know how these items should be structured in the code (if they should remain).

[gunnarmorling]

Hey @tonyfosterdev, so this looks great overall; thanks for providing the Compose set-up, that helped a lot with verifying this change. I've pushed one commit for some smaller changes. My one remaining question is related to how auth failures are represented: currently, an exception is logged (I've adjusted the exception mapper to make clear it's about auth rather than the previously logged JSON marshalling exception):

dev.morling.kccli.service.KafkaConnectException: User cannot access the resource.
	at dev.morling.kccli.service.KafkaConnectResponseExceptionMapper.toThrowable(KafkaConnectResponseExceptionMapper.java:28)
...

I'd argue though that no exception should be shown at all, but rather just the error message. The reasoning being that an authentication failure isn't really an exceptional situation but rather an expected error. We do have similar cases elsewhere, e.g. here; but ideally we wouldn't have to add a try/catch block to handle auth failures within each individual command. Perhaps you could check with the PicoCLI docs whether there's an error handling mechanism which would allow to handle this kind of error in a generic way, within one central place?

Other than that, I think that's a great change. Thanks a lot!

[gunnarmorling]

Oh, and one other thing: could you rework your commit(s) using

• an e-mail address linked to your GH account?
• the issue key and a suitable emoji (e.g. the police officer from my commit) as a commit message prefix

Thanks!

[tonyfosterdev]

@gunnarmorling I'm struggling with reworking the commits... Unfortunately the interactive rebase seems to have added commits... Do I need to start the PR over again, or do you know of a simple way to fix this? commit --amend seems to only work locally

[gunnarmorling]

Ha, yeah, seems something weird happened. I've sorted it out, the result is here: #77. Can you take a second look at that one, so to make sure I haven't missed anything (I've squashed the commit with the license headers). I'm going to close this one here, let's continue the discussion over on the new PR; the one remaining question is that regarding how an auth failure should be presented to the user: exception vs. simple error message.