You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@bookkeeper.apache.org by GitBox <gi...@apache.org> on 2020/06/04 03:53:46 UTC
[GitHub] [bookkeeper] Ghatage opened a new pull request #2355: BP-41 Certificate role based authorization in Apache Bookkeeper.
Ghatage opened a new pull request #2355:
URL: https://github.com/apache/bookkeeper/pull/2355
This feature allows a predefined set of services to be 'whitelisted' to be able
to access bookkeeper based on their client certificates.
### Motivation
As BookKeeper and its supported services move to a cloud friendly service based architecture, it becomes of utmost importance to monitor and allow only certain qualified services to be able to access the data in BK.
We have TLS based authentication, however, any service with the rootCA can access Bookkeeper clusters which is not desirable.
### Changes
So this feature can be broken down into two parts:
1. Certificate and roles
2. Server configuration for authorized roles
Details:
1. Certificate and roles:
Here is an example of how the SUBJECT field of a final certificate for Apache Pulsar running in the cloud would look like:
CN=apache.bookkeeper.org
O=apache-pulsar
OU=0:pulsar-broker-role;1:cluster-1
L=San Francisco
S=CA
C=US
This shows that this bookkeeper client certificate is owned by the apache pulsar service has the role ‘pulsar-broker-role’ for entities in ‘cluster-1’.
Only those services with pulsar-broker-role should be able to access it.
We can add more fields separated by commas to increase the upstream application clusters to be able to access this bookkeeper cluster.
For example: OU=0:herddb-readonlyNode,herddb-readwriteNode;1:herddb-cluster2
Such separation of access based on services is paramount to keeping this secure as many upstream users of BookKeeper are financial institutions, databases and other services.
2. Server configuration for authorized roles
Once we have a certificate whose SUBJECT field has the OU attribute with the roles we want to authorize, on the Bookie side, we need to specify which roles are allowed.
We make this happen by introducing a server configuration option called ‘authorizedRoles’.
Since we have only static options, this will be set in stone as long as the bookie booted up with it.
If in case we need to change the allowed roles, we’ll need to stop the bookie, update the configuration file and then restart the bookie.
We can have multiple roles which are authorized as the OU field can have multiple comma separated values for roles.
Master Issue: #2354
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] Ghatage commented on a change in pull request #2355: Certificate role based authorization in Apache Bookkeeper.
Posted by GitBox <gi...@apache.org>.
Ghatage commented on a change in pull request #2355:
URL: https://github.com/apache/bookkeeper/pull/2355#discussion_r442949022
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/tls/BookieAuthZFactory.java
##########
@@ -0,0 +1,133 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.bookkeeper.tls;
+
+import com.google.common.base.Strings;
+
+import java.io.IOException;
+import java.security.cert.X509Certificate;
+import java.util.Collection;
+
+import lombok.extern.slf4j.Slf4j;
+
+import org.apache.bookkeeper.auth.AuthCallbacks;
+import org.apache.bookkeeper.auth.AuthToken;
+import org.apache.bookkeeper.auth.BookKeeperPrincipal;
+import org.apache.bookkeeper.auth.BookieAuthProvider;
+import org.apache.bookkeeper.client.BKException;
+import org.apache.bookkeeper.conf.ServerConfiguration;
+import org.apache.bookkeeper.proto.BookieConnectionPeer;
+import org.apache.bookkeeper.util.CertUtils;
+
+
+/**
+ * Authorization factory class.
+ */
+@Slf4j
+public class BookieAuthZFactory implements BookieAuthProvider.Factory {
Review comment:
I don't know about this class, I checked the code and it doesn't seem to be there..
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] rdhabalia commented on pull request #2355: BP-41 Certificate role based authorization in Apache Bookkeeper.
Posted by GitBox <gi...@apache.org>.
rdhabalia commented on pull request #2355:
URL: https://github.com/apache/bookkeeper/pull/2355#issuecomment-645832297
@Ghatage do we need this patch for 4.11?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] Ghatage closed pull request #2355: Certificate role based authorization in Apache Bookkeeper.
Posted by GitBox <gi...@apache.org>.
Ghatage closed pull request #2355:
URL: https://github.com/apache/bookkeeper/pull/2355
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] Ghatage commented on pull request #2355: Certificate role based authorization in Apache Bookkeeper.
Posted by GitBox <gi...@apache.org>.
Ghatage commented on pull request #2355:
URL: https://github.com/apache/bookkeeper/pull/2355#issuecomment-646489892
Hey @eolivelli,
I've made the following changes as requested:
* Added a test class for `BookieAuthZFactory` which doesn't spin up a bookie cluster.
* Added all negative test cases to the above new test class.
* Removed the 'heavy' negative test case from `TestTLS` class.
* Modified comment to explain the success case better.
* Converted `CertUtils` class to abstract with a private default constructor.
* Removed unrelated comments from `bk_server.conf`
* Added `isAuthenticated()` method to authProvider
Feature wise this is complete and switched off by default.
Lastly, once the CI tests pass, it's best to hold off on merging this before 4.11 is cut and released as discussed in the mailing list.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] Ghatage commented on a change in pull request #2355: BP-41 Certificate role based authorization in Apache Bookkeeper.
Posted by GitBox <gi...@apache.org>.
Ghatage commented on a change in pull request #2355:
URL: https://github.com/apache/bookkeeper/pull/2355#discussion_r436318219
##########
File path: bookkeeper-server/src/test/java/org/apache/bookkeeper/tls/TestTLS.java
##########
@@ -635,6 +635,47 @@ public void testBookieAuthPluginRequireClientTLSAuthenticationLocal() throws Exc
assertTrue(cert instanceof X509Certificate);
}
+ /**
+ * Verify that given role in client certificate is checked when BookieAuthZFactory is set.
+ * Positive test case where all given roles are present.
Review comment:
If `testClient()` fails, it will be because of an exception thrown during Authorization.
And hence we catch that and fail the test.
Otherwise, if the test passes, it never reaches the `catch` block and finishes, passing the test
Sorry for the confusing comment, I can change that in a follow up.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lamber-ken commented on pull request #2355: BP-41 Certificate role based authorization in Apache Bookkeeper.
Posted by GitBox <gi...@apache.org>.
lamber-ken commented on pull request #2355:
URL: https://github.com/apache/bookkeeper/pull/2355#issuecomment-640917504
rerun failure checks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] eolivelli commented on a change in pull request #2355: BP-41 Certificate role based authorization in Apache Bookkeeper.
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #2355:
URL: https://github.com/apache/bookkeeper/pull/2355#discussion_r435021238
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieRequestProcessor.java
##########
@@ -564,10 +564,19 @@ public void operationComplete(Future<Channel> future) throws Exception {
AuthHandler.ServerSideHandler authHandler = c.pipeline()
.get(AuthHandler.ServerSideHandler.class);
authHandler.authProvider.onProtocolUpgrade();
- if (future.isSuccess()) {
+
+ /*
+ * Success of the future doesn't guarantee success in authentication
+ * future.isSuccess() only checks if the result field is not null
+ */
+ if (future.isSuccess() && authHandler.authenticated) {
Review comment:
can we add a "isAuthenticated" method to authHandler ?
this way we want access directly the field
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] Ghatage commented on a change in pull request #2355: Certificate role based authorization in Apache Bookkeeper.
Posted by GitBox <gi...@apache.org>.
Ghatage commented on a change in pull request #2355:
URL: https://github.com/apache/bookkeeper/pull/2355#discussion_r442948240
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieRequestProcessor.java
##########
@@ -564,10 +564,19 @@ public void operationComplete(Future<Channel> future) throws Exception {
AuthHandler.ServerSideHandler authHandler = c.pipeline()
.get(AuthHandler.ServerSideHandler.class);
authHandler.authProvider.onProtocolUpgrade();
- if (future.isSuccess()) {
+
+ /*
+ * Success of the future doesn't guarantee success in authentication
+ * future.isSuccess() only checks if the result field is not null
+ */
+ if (future.isSuccess() && authHandler.authenticated) {
Review comment:
Fixed in [7276947](https://github.com/apache/bookkeeper/commit/72769477f52e4982081ae87a8e43da0a0e34e2d1)
##########
File path: conf/bk_server.conf
##########
@@ -226,6 +226,15 @@ httpServerClass=org.apache.bookkeeper.http.vertx.VertxHttpServer
#
# permittedStartupUsers=
+# Certificate role based authorization for Bookie
+# To enable this option, bookieAuthProviderFactoryClass should be set to org.apache.bookkeeper.tls.BookieAuthZFactory
+# comma separated values of roles from the certificates OU field which we want to authorize for access
+#
+# Example settings:
+# authorizedRoles=apache-pulsar-cluster-12,apache-kafka-cluster-2
Review comment:
Fixed in [7276947](https://github.com/apache/bookkeeper/commit/72769477f52e4982081ae87a8e43da0a0e34e2d1)
##########
File path: bookkeeper-server/src/test/java/org/apache/bookkeeper/tls/TestTLS.java
##########
@@ -635,6 +635,47 @@ public void testBookieAuthPluginRequireClientTLSAuthenticationLocal() throws Exc
assertTrue(cert instanceof X509Certificate);
}
+ /**
+ * Verify that given role in client certificate is checked when BookieAuthZFactory is set.
+ * Positive test case where all given roles are present.
Review comment:
Fixed in [7276947](https://github.com/apache/bookkeeper/commit/72769477f52e4982081ae87a8e43da0a0e34e2d1)
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/util/CertUtils.java
##########
@@ -0,0 +1,117 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.bookkeeper.util;
+
+import java.io.IOException;
+import java.security.cert.X509Certificate;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+
+import javax.naming.InvalidNameException;
+import javax.naming.ldap.LdapName;
+import javax.naming.ldap.Rdn;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Certificate parsing utilities.
+ */
+public class CertUtils {
Review comment:
Fixed in [7276947](https://github.com/apache/bookkeeper/commit/72769477f52e4982081ae87a8e43da0a0e34e2d1)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] Ghatage commented on pull request #2355: Certificate role based authorization in Apache Bookkeeper.
Posted by GitBox <gi...@apache.org>.
Ghatage commented on pull request #2355:
URL: https://github.com/apache/bookkeeper/pull/2355#issuecomment-703966200
Messed up the PR, closing this and reopening under new PR.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] rdhabalia commented on pull request #2355: BP-41 Certificate role based authorization in Apache Bookkeeper.
Posted by GitBox <gi...@apache.org>.
rdhabalia commented on pull request #2355:
URL: https://github.com/apache/bookkeeper/pull/2355#issuecomment-645832083
rerun failure checks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] Ghatage commented on pull request #2355: Certificate role based authorization in Apache Bookkeeper.
Posted by GitBox <gi...@apache.org>.
Ghatage commented on pull request #2355:
URL: https://github.com/apache/bookkeeper/pull/2355#issuecomment-646754244
rerun failure checks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] Ghatage commented on a change in pull request #2355: Certificate role based authorization in Apache Bookkeeper.
Posted by GitBox <gi...@apache.org>.
Ghatage commented on a change in pull request #2355:
URL: https://github.com/apache/bookkeeper/pull/2355#discussion_r442948570
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/tls/BookieAuthZFactory.java
##########
@@ -0,0 +1,133 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.bookkeeper.tls;
+
+import com.google.common.base.Strings;
+
+import java.io.IOException;
+import java.security.cert.X509Certificate;
+import java.util.Collection;
+
+import lombok.extern.slf4j.Slf4j;
+
+import org.apache.bookkeeper.auth.AuthCallbacks;
+import org.apache.bookkeeper.auth.AuthToken;
+import org.apache.bookkeeper.auth.BookKeeperPrincipal;
+import org.apache.bookkeeper.auth.BookieAuthProvider;
+import org.apache.bookkeeper.client.BKException;
+import org.apache.bookkeeper.conf.ServerConfiguration;
+import org.apache.bookkeeper.proto.BookieConnectionPeer;
+import org.apache.bookkeeper.util.CertUtils;
+
+
+/**
+ * Authorization factory class.
+ */
+@Slf4j
+public class BookieAuthZFactory implements BookieAuthProvider.Factory {
+
+ public String[] allowedRoles;
+
+ @Override
+ public String getPluginName() {
+ return "BookieAuthZFactory";
+ }
+
+ @Override
+ public void init(ServerConfiguration conf) throws IOException {
+ // Read from config
+ allowedRoles = conf.getAuthorizedRoles();
+
+ if (allowedRoles == null || allowedRoles.length == 0) {
+ throw new RuntimeException("Configuration option \'bookieAuthProviderFactoryClass\' is set to"
+ + " \'BookieAuthZFactory\' but no roles set for configuration field \'authorizedRoles\'.");
Review comment:
Gonna let this be, as the message is specific to this class.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] eolivelli commented on a change in pull request #2355: BP-41 Certificate role based authorization in Apache Bookkeeper.
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #2355:
URL: https://github.com/apache/bookkeeper/pull/2355#discussion_r435018656
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/tls/BookieAuthZFactory.java
##########
@@ -0,0 +1,133 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.bookkeeper.tls;
+
+import com.google.common.base.Strings;
+
+import java.io.IOException;
+import java.security.cert.X509Certificate;
+import java.util.Collection;
+
+import lombok.extern.slf4j.Slf4j;
+
+import org.apache.bookkeeper.auth.AuthCallbacks;
+import org.apache.bookkeeper.auth.AuthToken;
+import org.apache.bookkeeper.auth.BookKeeperPrincipal;
+import org.apache.bookkeeper.auth.BookieAuthProvider;
+import org.apache.bookkeeper.client.BKException;
+import org.apache.bookkeeper.conf.ServerConfiguration;
+import org.apache.bookkeeper.proto.BookieConnectionPeer;
+import org.apache.bookkeeper.util.CertUtils;
+
+
+/**
+ * Authorization factory class.
+ */
+@Slf4j
+public class BookieAuthZFactory implements BookieAuthProvider.Factory {
Review comment:
what about BookieX509CertificateAuthZFactory ?
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/util/CertUtils.java
##########
@@ -0,0 +1,117 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.bookkeeper.util;
+
+import java.io.IOException;
+import java.security.cert.X509Certificate;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+
+import javax.naming.InvalidNameException;
+import javax.naming.ldap.LdapName;
+import javax.naming.ldap.Rdn;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Certificate parsing utilities.
+ */
+public class CertUtils {
Review comment:
abstract ?
with a private default constructor
##########
File path: conf/bk_server.conf
##########
@@ -226,6 +226,15 @@ httpServerClass=org.apache.bookkeeper.http.vertx.VertxHttpServer
#
# permittedStartupUsers=
+# Certificate role based authorization for Bookie
+# To enable this option, bookieAuthProviderFactoryClass should be set to org.apache.bookkeeper.tls.BookieAuthZFactory
+# comma separated values of roles from the certificates OU field which we want to authorize for access
+#
+# Example settings:
+# authorizedRoles=apache-pulsar-cluster-12,apache-kafka-cluster-2
Review comment:
looks like 'kafka' is not useful in BK context :-)
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/tls/BookieAuthZFactory.java
##########
@@ -0,0 +1,133 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.bookkeeper.tls;
+
+import com.google.common.base.Strings;
+
+import java.io.IOException;
+import java.security.cert.X509Certificate;
+import java.util.Collection;
+
+import lombok.extern.slf4j.Slf4j;
+
+import org.apache.bookkeeper.auth.AuthCallbacks;
+import org.apache.bookkeeper.auth.AuthToken;
+import org.apache.bookkeeper.auth.BookKeeperPrincipal;
+import org.apache.bookkeeper.auth.BookieAuthProvider;
+import org.apache.bookkeeper.client.BKException;
+import org.apache.bookkeeper.conf.ServerConfiguration;
+import org.apache.bookkeeper.proto.BookieConnectionPeer;
+import org.apache.bookkeeper.util.CertUtils;
+
+
+/**
+ * Authorization factory class.
+ */
+@Slf4j
+public class BookieAuthZFactory implements BookieAuthProvider.Factory {
+
+ public String[] allowedRoles;
+
+ @Override
+ public String getPluginName() {
+ return "BookieAuthZFactory";
+ }
+
+ @Override
+ public void init(ServerConfiguration conf) throws IOException {
+ // Read from config
+ allowedRoles = conf.getAuthorizedRoles();
+
+ if (allowedRoles == null || allowedRoles.length == 0) {
+ throw new RuntimeException("Configuration option \'bookieAuthProviderFactoryClass\' is set to"
+ + " \'BookieAuthZFactory\' but no roles set for configuration field \'authorizedRoles\'.");
Review comment:
what about usiing this.class.getName() instead of an hardcoded "BookieAuthZFactory" ?
##########
File path: bookkeeper-server/src/test/java/org/apache/bookkeeper/tls/TestTLS.java
##########
@@ -635,6 +635,47 @@ public void testBookieAuthPluginRequireClientTLSAuthenticationLocal() throws Exc
assertTrue(cert instanceof X509Certificate);
}
+ /**
+ * Verify that given role in client certificate is checked when BookieAuthZFactory is set.
+ * Positive test case where all given roles are present.
Review comment:
this test does look like a "Positive test"
you are asserting that the client is not able to work
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] Ghatage commented on pull request #2355: Certificate role based authorization in Apache Bookkeeper.
Posted by GitBox <gi...@apache.org>.
Ghatage commented on pull request #2355:
URL: https://github.com/apache/bookkeeper/pull/2355#issuecomment-703966627
Reopened under https://github.com/apache/bookkeeper/pull/2429
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] Ghatage commented on pull request #2355: Certificate role based authorization in Apache Bookkeeper.
Posted by GitBox <gi...@apache.org>.
Ghatage commented on pull request #2355:
URL: https://github.com/apache/bookkeeper/pull/2355#issuecomment-647296541
rerun failure checks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] Ghatage commented on pull request #2355: Certificate role based authorization in Apache Bookkeeper.
Posted by GitBox <gi...@apache.org>.
Ghatage commented on pull request #2355:
URL: https://github.com/apache/bookkeeper/pull/2355#issuecomment-650627384
The test suite runs fine locally and the test I've written doesn't fail either, two other tests randomly fail with `Not Enough Bookies`.
The option set is only for that test and by default it's false.
@sijie, @eolivelli any idea why `TestTLS` keeps failing?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] eolivelli commented on a change in pull request #2355: BP-41 Certificate role based authorization in Apache Bookkeeper.
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #2355:
URL: https://github.com/apache/bookkeeper/pull/2355#discussion_r442005521
##########
File path: conf/bk_server.conf
##########
@@ -226,6 +226,15 @@ httpServerClass=org.apache.bookkeeper.http.vertx.VertxHttpServer
#
# permittedStartupUsers=
+# Certificate role based authorization for Bookie
+# To enable this option, bookieAuthProviderFactoryClass should be set to org.apache.bookkeeper.tls.BookieAuthZFactory
+# comma separated values of roles from the certificates OU field which we want to authorize for access
+#
+# Example settings:
+# authorizedRoles=apache-pulsar-cluster-12,apache-kafka-cluster-2
Review comment:
please remove it
##########
File path: bookkeeper-server/src/test/java/org/apache/bookkeeper/tls/TestTLS.java
##########
@@ -635,6 +635,47 @@ public void testBookieAuthPluginRequireClientTLSAuthenticationLocal() throws Exc
assertTrue(cert instanceof X509Certificate);
}
+ /**
+ * Verify that given role in client certificate is checked when BookieAuthZFactory is set.
+ * Positive test case where all given roles are present.
Review comment:
yes please update the comment
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] Ghatage commented on pull request #2355: Certificate role based authorization in Apache Bookkeeper.
Posted by GitBox <gi...@apache.org>.
Ghatage commented on pull request #2355:
URL: https://github.com/apache/bookkeeper/pull/2355#issuecomment-650622186
rerun failure checks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] Ghatage commented on pull request #2355: Certificate role based authorization in Apache Bookkeeper.
Posted by GitBox <gi...@apache.org>.
Ghatage commented on pull request #2355:
URL: https://github.com/apache/bookkeeper/pull/2355#issuecomment-646198462
> @Ghatage do we need this patch for 4.11?
@rdhabalia No, we can leave this for later.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org