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.

Sign up for GitHub

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

Jump to bottom

Added Keycloak Authorization configuration #2432

Merged
merged 15 commits into from
Jan 30, 2020
Merged

Added Keycloak Authorization configuration #2432

merged 15 commits into from
Jan 30, 2020

Conversation

ppatierno
Copy link
Member

Signed-off-by: Paolo Patierno [email protected]

Type of change

  • Enhancement / new feature

Description

This PR fixes #2428.
It's still missing tests and including the kafka-oauth-keycloak-authorizer JAR coming from this other PR strimzi/strimzi-kafka-oauth#24 still in progress.

Checklist

Please go through this checklist and make sure all applicable tasks have been done

  • Update/write design documentation in ./design
  • Write tests
  • Make sure all tests pass
  • Update documentation
  • Check RBAC rights for Kubernetes / OpenShift roles
  • Try your changes from Pod inside your Kubernetes and OpenShift cluster, not just locally
  • Reference relevant issue(s) and close them after merging
  • Update CHANGELOG.md

@ppatierno ppatierno requested review from mstruk and scholzj January 20, 2020 21:28
@ppatierno ppatierno added this to the 0.17.0 milestone Jan 20, 2020
@ppatierno
Copy link
Member Author

@scholzj @mstruk even if WIP, a first review would be really appreciated to have it ready as soon as possible for the next 0.17.0 release.

scholzj
scholzj reviewed Jan 21, 2020
View reviewed changes
...perator/src/main/java/io/strimzi/operator/cluster/model/KafkaBrokerConfigurationBuilder.java
* @param superUsers Super users list who have all the rights on the cluster
* @param authorization The authorization configuration from the Kafka CR
*/
private void configureAuthorization(String clusterName, List<String> superUsers, KafkaAuthorization authorization) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we maybe split this into two methods? configureSimpleAuthorization and configureKeycloakAuthorization?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already have a similar pattern for configuring authentication on which I agree. The configuraAuthentication method is just one and we have the logic for different authentication mechanisms inside. I am not sure about the advantage of having different methods here.

...perator/src/main/java/io/strimzi/operator/cluster/model/KafkaBrokerConfigurationBuilder.java
} else if (KafkaAuthorizationKeycloak.TYPE_KEYCLOAK.equals(authorization.getType())) {
KafkaAuthorizationKeycloak keycloakAuthz = (KafkaAuthorizationKeycloak) authorization;
writer.println("authorizer.class.name=" + KafkaAuthorizationKeycloak.AUTHORIZER_CLASS_NAME);
writer.println("principal.builder.class=" + KafkaAuthorizationKeycloak.PRINCIPAL_BUILDER_CLASS_NAME);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you double check what impact does this have on the internal super users? Do they still keey their own names?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They should keep their names, yes.

The JwtKafkaPrincipalBuilder only does something different when user was authenticated over SASL_OAUTHBEARER as seen here. An internal user would be handled by the same logic as before.

@scholzj scholzj mentioned this pull request Jan 23, 2020
Merged
7 tasks
@ppatierno ppatierno changed the title WIP: Added Keycloak Authorization configuration Added Keycloak Authorization configuration Jan 24, 2020
@ppatierno
Copy link
Member Author

This is not WIP anymore but we don't have to merge until strimzi/strimzi-kafka-oauth#24 is merged. Anyway can you have another review pass @scholzj @mstruk please? Thanks!

tombentley
tombentley approved these changes Jan 27, 2020
View reviewed changes
Copy link
Member

@tombentley tombentley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nits only. LGTM.

@ppatierno
Copy link
Member Author

@tombentley @scholzj @mstruk FYI I tried this with the keycloak library 0.3.0 (from the staging repository) and the integration seems to work fine.

@ppatierno
Added Keycloak Authorization configuration
70468c8 
Signed-off-by: Paolo Patierno <[email protected]>
@ppatierno
Update derived resources
6bf09a9 
Signed-off-by: Paolo Patierno <[email protected]>
@ppatierno
Fixed adding certificates volume
b6feb15 
Signed-off-by: Paolo Patierno <[email protected]>
@ppatierno
Added Keycloak authorization tests
936521e 
Signed-off-by: Paolo Patierno <[email protected]>
@ppatierno
Set required properties for Keycloak authorization
6b7e134 
Fixed a minor bug

Signed-off-by: Paolo Patierno <[email protected]>
@ppatierno
Added kafka-oauth-keycloak-authorizer third party dependency
51615d1 
Signed-off-by: Paolo Patierno <[email protected]>
@ppatierno
Review comments
7be7e59 
Signed-off-by: Paolo Patierno <[email protected]>
@ppatierno
Updated keycloack authorizer library parameters prefix
c8cffa3 
Signed-off-by: Paolo Patierno <[email protected]>
@ppatierno
Updated OAuth library version
c8c63d4 
Signed-off-by: Paolo Patierno <[email protected]>
@ppatierno
Updated oauth library version for Kafka thirdparty libs
cdadec1 
Signed-off-by: Paolo Patierno <[email protected]>
@ppatierno
Fixed certificates generation for Keycloak authorization
4e50402 
Signed-off-by: Paolo Patierno <[email protected]>
@ppatierno
Fixed CRD problem with required field now checked by CO
48c7eb6 
Signed-off-by: Paolo Patierno <[email protected]>
@ppatierno
Added missing file
bca7039 
Signed-off-by: Paolo Patierno <[email protected]>
@ppatierno
Copy link
Member Author

@strimzi-ci run tests

@strimzi-ci
Copy link

❗ Build Failed ❗

@ppatierno
Copy link
Member Author

@strimzi-ci run tests

@strimzi-ci
Copy link

❌ Test Summary ❌

TEST_PROFILE: acceptance
TEST_CASE: *ST
TOTAL: 20
PASS: 17
FAIL: 3
SKIP: 0

❗ Test Failures ❗

  • testLoadBalancerTls in io.strimzi.systemtest.KafkaST
  • testKafkaAndZookeeperScaleUpScaleDown in io.strimzi.systemtest.RollingUpdateST
  • testKafkaConnectWithFileSinkPlugin in io.strimzi.systemtest.ConnectST

Re-run command:
@strimzi-ci run tests profile=acceptance testcase=io.strimzi.systemtest.KafkaST#testLoadBalancerTls,io.strimzi.systemtest.RollingUpdateST#testKafkaAndZookeeperScaleUpScaleDown,io.strimzi.systemtest.ConnectST#testKafkaConnectWithFileSinkPlugin

scholzj
scholzj approved these changes Jan 30, 2020
View reviewed changes
Copy link
Member

@scholzj scholzj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some nits, but LGTM otherwise.

@ppatierno ppatierno merged commit 4f533bf into strimzi:master Jan 30, 2020
@ppatierno ppatierno deleted the keycloak-authz branch January 30, 2020 17:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tombentley tombentley tombentley approved these changes

@scholzj scholzj scholzj approved these changes

@mstruk mstruk Awaiting requested review from mstruk

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.17.0
Development

Successfully merging this pull request may close these issues.

Integrate OAuth Authorization into Strimzi
5 participants
@ppatierno @strimzi-ci @mstruk @tombentley @scholzj