Certificate role based authorization in Apache Bookkeeper.#2355
Certificate role based authorization in Apache Bookkeeper.#2355Ghatage wants to merge 44 commits into
Conversation
eolivelli
left a comment
There was a problem hiding this comment.
great work.
I left a couple of comments.
If you did not follow the BP process I won't call this PR a "BP", a BP needs an additional design document on the website.
The change is not an API change and the impact is low, so there is no need for a BP
Please also consider adding unit tests about the new AuthProvider class, without the need of starting the cluster and a full client
|
rerun failure checks |
|
rerun failure checks |
|
@Ghatage do we need this patch for 4.11? |
@rdhabalia No, we can leave this for later. |
This feature allows a predefined set of services to be 'whitelisted' to be able to access bookkeeper based on their client certificates.
baaca81 to
7276947
Compare
|
Hey @eolivelli, I've made the following changes as requested:
Feature wise this is complete and switched off by default. |
|
rerun failure checks |
1 similar comment
|
rerun failure checks |
|
rerun failure checks |
|
The test suite runs fine locally and the test I've written doesn't fail either, two other tests randomly fail with |
### Motivation static code blocks are always invoked when the class is initialized, when client connect to bookkeeper server, output repeat init log ``` 2020-06-04 10:10:24,751 INFO org.apache.bookkeeper.meta.MetadataDrivers - BookKeeper metadata driver manager initialized 2020-06-04 10:10:24,753 INFO org.apache.bookkeeper.meta.MetadataDrivers - BookKeeper metadata driver manager initialized 2020-06-04 10:10:24,753 INFO org.apache.bookkeeper.meta.MetadataDrivers - BookKeeper metadata driver manager initialized ``` ### Changes Remove redundant initialization code. Reviewers: Jia Zhai <zhaijia@apache.org>, Matteo Minardi <minardi.matteo@hotmail.it>, Enrico Olivelli <eolivelli@gmail.com> This closes apache#2356 from lamber-ken/bk-init-repeat
### Motivation `GitHub Action` is deprecated! Implement ci bot using `actions/github-script`, instead. https://github.com/actions/github https://github.com/actions/github-script ### Changes Implement ci using `actions/github-script`, based on https://github.com/zymap/bot/tree/v1.0.1 Reviewers: Jia Zhai <zhaijia@apache.org>, Enrico Olivelli <eolivelli@gmail.com>, Yong Zhang <zhangyong1025.zy@gmail.com>, Sijie Guo <None> This closes apache#2352 from lamber-ken/bot-v2
### Motivation Fix redundant statistics, run 5 bookies `bin/bookkeeper localbookie 10`, then show bookieinfo ``` ./bin/bookkeeper shell bookieinfo ``` **Wrong result** ``` Free disk space info: 10.101.52.18(10.101.52.18):5004: Free: 34930151424(34.93GB) Total: 255850758144(255.85GB) 10.101.52.18(10.101.52.18):5003: Free: 34930151424(34.93GB) Total: 255850758144(255.85GB) 10.101.52.18(10.101.52.18):5002: Free: 34930151424(34.93GB) Total: 255850758144(255.85GB) 10.101.52.18(10.101.52.18):5000: Free: 34930151424(34.93GB) Total: 255850758144(255.85GB) 10.101.52.18(10.101.52.18):5005: Free: 34930151424(34.93GB) Total: 255850758144(255.85GB) Total free disk space in the cluster: 174650757120(174.65GB) Total disk capacity in the cluster: 1279253790720(1.279TB) ``` **Right result** ``` Free disk space info: 10.101.52.18(10.101.52.18):5004: Free: 33172185088(33.172GB) Total: 255850758144(255.85GB) 10.101.52.18(10.101.52.18):5003: Free: 33172185088(33.172GB) Total: 255850758144(255.85GB) 10.101.52.18(10.101.52.18):5002: Free: 33172185088(33.172GB) Total: 255850758144(255.85GB) 10.101.52.18(10.101.52.18):5006: Free: 33172185088(33.172GB) Total: 255850758144(255.85GB) 10.101.52.18(10.101.52.18):5005: Free: 33172185088(33.172GB) Total: 255850758144(255.85GB) Total free disk space in the cluster: 33172185088(33.172GB) Total disk capacity in the cluster: 255850758144(255.85GB) ``` Reviewers: Jia Zhai <zhaijia@apache.org>, Sijie Guo <None>, Enrico Olivelli <eolivelli@gmail.com> This closes apache#2348 from lamber-ken/bk-info-command
…cceeded Descriptions of the changes in this PR: - in ZkLedgerUnderreplicationManager.releaseUnderreplicatedLedger remove ‘lock’ from ‘heldLocks’ only if lock znode deletion has succeeded. - This is needed because, if RW.logBKExceptionAndReleaseLedger fails to delete the lock znode, then it needs to be tried once more in the finally block of 'rereplicate(long ledgerIdToReplicate)’ before giving up and shutting down RW. Reviewers: Enrico Olivelli <eolivelli@gmail.com> This closes apache#2206 from reddycharan/fixzklossissue
Descriptions of the changes in this PR: - in Auditor.ReadLedgerMetadataCallbackForReplicasCheck, fixing logic for empty segments - added test for validating areEntriesOfLedgerStoredInTheBookie for empty middle segment - added testcase for ledger with more empty segments. Reviewers: Enrico Olivelli <eolivelli@gmail.com> This closes apache#2205 from reddycharan/fixreplicascheck
### Motivation In a large bookie cluster environment, we frequently require a utility to change bookie-ip with a different bookie-ip in ledger's metadata in different scenarios such as: 1. Host Re-IP: requires updating ledger metadata and reuse the host without losing/copying data 2. Backup-restore usecase while doing cluster level data migration. Therefore, we frequently need a tool to update bookie-ip with a new bookie-ip in existing ledgers' ensemble metadata. ### Modification Add CLI command to update bookie-id in ledger metadata. Reviewers: Enrico Olivelli <eolivelli@gmail.com>, Sijie Guo <None>, Jia Zhai <zhaijia@apache.org> This closes apache#2321 from rdhabalia/reip
According to the release guide, before cutting a release we have to remove the 'alpha' qualifier from Python client version Reviewers: Enrico Olivelli <eolivelli@gmail.com> This closes apache#2362 from rdhabalia/python_client
### Motivation After generating website using [script](http://bookkeeper.apache.org/community/release_guide/#update-website), it seems somhow it has updated site with incorrect version. I still have to find out where was the issue while generating the website. But fixing it for now to correct the version on website. Reviewers: Enrico Olivelli <eolivelli@gmail.com> This closes apache#2372 from rdhabalia/fix_site
…pted EntryLogger file - in case of corrupted entry log file the bookie cannot perform GC - handle the case of an unexpected negative entrysize ### Changes Stop scanning the file Master Issue: apache#2264 Reviewers: Jia Zhai <zhaijia@apache.org>, Rajan Dhabalia <rdhabalia@apache.org> This closes apache#2373 from eolivelli/fix/issue2264-corruptedfile
### Motivation Improve the user experience of the site.  ### Changes **Compare:** https://lamber-ken.github.io/docs/4.10.0/getting-started/installation/ https://bookkeeper.apache.org/docs/4.10.0/getting-started/installation/ Reviewers: Enrico Olivelli <eolivelli@gmail.com>, Matteo Minardi <minardi.matteo@hotmail.it> This closes apache#2344 from lamber-ken/improve-site
These code blocks will not be executed: dbLedgerStorageStats.getReadAheadBatchCountStats().registerSuccessfulValue(count); dbLedgerStorageStats.getReadAheadBatchSizeStats().registerSuccessfulValue(size); ### Motivation fix fillReadAheadCache stat bug ### Changes Replace return with break to exit the while loop Reviewers: Enrico Olivelli <eolivelli@gmail.com>, Rajan Dhabalia <rdhabalia@apache.org> This closes apache#2383 from holmes07/master
Descriptions of the changes in this PR:
### Motivation
Every time a problem with ZK arises you don't see it in the exception chain of the BKException (in this case ZKException) and you end up with errors like:
```
org.apache.bookkeeper.client.BKException$ZKException: Error while using ZooKeeper
at org.apache.bookkeeper.client.SyncCallbackUtils.finish(SyncCallbackUtils.java:83)
at org.apache.bookkeeper.client.SyncCallbackUtils$SyncAddCallback.addComplete(SyncCallbackUtils.java:251)
at org.apache.bookkeeper.client.AsyncCallback$AddCallback.addCompleteWithLatency(AsyncCallback.java:91)
at org.apache.bookkeeper.client.PendingAddOp.submitCallback(PendingAddOp.java:430)
at org.apache.bookkeeper.client.LedgerHandle.errorOutPendingAdds(LedgerHandle.java:1784)
at org.apache.bookkeeper.client.LedgerHandle$5.safeRun(LedgerHandle.java:574)
```
### Changes
Add a "cause" to every ZKException.
### Notes
There are very few places that cannot be fixed because they are still using the old callback based mechanism without CompletableFuture. Those points are not changed in order to make the patch simple but still useful.
Reviewers: Jia Zhai <zhaijia@apache.org>
This closes apache#2384 from eolivelli/fix/zkexception-chain
This is the design document of "BP-41 Separate BookieId from Bookie Network Address" Master Issue: apache#2396
…ons execution with 3.6.1 Fix for apache#2385 Now we avoid to send out multiple operations request to ZooKeeper when there are not operations to execute Reviewers: Enrico Olivelli <eolivelli@gmail.com>, Sijie Guo <None> This closes apache#2390 from nicoloboschi/fix/2385/zk-npe, closes apache#2385
Fix apache#2197 Create "logs" directory before run bkctl cli Reviewers: Enrico Olivelli <eolivelli@gmail.com>, Sijie Guo <None> This closes apache#2389 from nicoloboschi/fix/2197/bkclt-logs, closes apache#2197
### Motivation Found 'responsbility' typo Reviewers: Sijie Guo <None> This closes apache#2392 from nicoloboschi/fix-doc-typo
…rflow when we do a large scale benchmark test - When` latency.size` is is more than `20,000,000`(e.g, `25000000`), `(size * percentile) / 100` will be a negative number due to Integer overflow and the latency will be wrong. - For example: `size `= 25000000, `percentile `= 99, `sampleSize `= -18199672 Reviewers: Enrico Olivelli <eolivelli@gmail.com>, Sijie Guo <None> This closes apache#2381 from maoling/BenchThroughputLatency-latency-wrong
Descriptions of the changes in this PR: ### Motivation NP check for print BookieSocketAddress and a better format to fix issues apache#2376 and apache#2371 ### Changes * NP check for print BookieSocketAddress on listbookies command * A better format mentioned on apache#2376 Master Issue: apache#2376 apache#2371 Reviewers: Enrico Olivelli <eolivelli@gmail.com>, Sijie Guo <None>, Rajan Dhabalia <rdhabalia@apache.org> This closes apache#2380 from rudy2steiner/list_bookie
Descriptions of the changes in this PR: ### Motivation For troubleshooting bookkeeper disk usage issues, having a tool to list the ledgers that are still alive in the entry log file is super useful. ### Changes * Add ListActiveLedgerCommand on BookieShell Master Issue: apache#2358 Reviewers: Enrico Olivelli <eolivelli@gmail.com>, Sijie Guo <None> This closes apache#2369 from rudy2steiner/active_ledger_command
### Motivation When bookie client read/write data from/to bookie servers, it will check the health of each connected server in sepecific interval. Once the amount of errors reached the threshold, the bookie server will be quarantined for server miniutes (configurated by `bookieQuarantineTimeSeconds`) by the bookie client. In most circumstance, there are large amount of bookie clients connected to one bookie server, like pulsar broker. Once the bookie server runs in heavy load, most of bookie clients will receive errors and trigger quarantine in the same time, and then quarantine the server for several miniutes. After a few miniutes passed by, the quarantined server will be put back in the same time for most bookie clients, which will lead to periodic oscillation of in/out throughput of the server. It is the obstacle of tunning the throughput of the bookkeeper cluster. ### Changes I introduce a quarantine probability to determine whether to quarantine the server for the client, avoiding quaraninte the heavy load server in the same time for most of bookie client. I also expose the quarantine stats to prometheus. Reviewers: Jia Zhai <zhaijia@apache.org>, Sijie Guo <None> This closes apache#2327 from hangc0276/bookieClient_Quarantine_ratio
- rename self job seed configuration in order to disable it temporary
Under heavy load, the write cache can fill up faster than it can be flushed. When this occurs, something needs to trigger the flush. Previously we've triggered the flush and then gone into a loop waiting to be able to add to the write cache. However, if the currently active write cache fills while we are waiting for the flushing write cache to be flushed, we can end up in a situation where nothing is able to flush because all I/O threads are waiting for the write cache to empty. The solution is to move the flush into the loop so that if we fail to add to the cache, we flush the write cache if it is needed. Co-authored-by: Ivan Kelly <ikelly@splunk.com>
### Motivation This is the client side implementation of BP-41, see the design doc here https://github.com/apache/bookkeeper/blob/master/site/bps/BP-41-bookieid.md ### Changes - Introduce BookeId concept - Separated BookieId from BookieSocketAddress - in LedgerMetadata we use now BookieId and not BookieSocketAddress - a BookieId is basically a wrapper around a string - BookieId is compatible with BookieSocketAddress - In order to *resolve" a BookieId to a BookieSocketAddress we use a BookieAddressResolver - the DefaultBookieAddressResolver implementation uses BookieServiceInfo metadata, read from ZooKeeper in order to discover the current network address of the Bookie (see BP-38) Unfortunately the patch is huge due to the changes in the client API and LedgerMetadata. This patch tries to be the smallest set of changes, see apache#2396 for the list of the follow up patches that will be contributed. Notes: - The new client and the bookie is 100% compatible with previous Bookies and Clients (and integration tests were not touched) - We are going to introduce, in a follow up patch, a flag to not use the metadata on ZooKeeper for the resolution of the BookieSocketAddress, in order to save resources - The ability to configure a custom BookieId, different from the network address will come in a separate patch Master Issue: apache#2396 Co-authored-by: Enrico Olivelli <eolivelli@apache.org> Co-authored-by: Enrico Olivelli <enrico.olivelli@diennea.com>
Signed-off-by: xiaolong.ran <rxlapache.org> ### Motivation As the apache/pulsar#7931 said, CVE-2018-12540 has been raised on vertx 3.4.1 which is found in the pulsar dependencies. ### Changes - Upgrade the vertx version from 3.4.1 to 3.5.3 - Update license files Reviewers: Enrico Olivelli <eolivelli@gmail.com>, Sijie Guo <None> This closes apache#2410 from wolfstudy/xiaolong/upgrade-vertx-version
Descriptions of the changes in this PR: change the doclint configuration in pom.xml ### Motivation when you clone the project there is always warning in IntelliJ IDEA ### Changes change the doclint configuration in pom.xml according to https://stackoverflow.com/questions/52547306/maven-javadoc-plugin-not-accepting-additionalparam-xdoclintnone-additionalpa Reviewers: Enrico Olivelli <eolivelli@gmail.com>, Sijie Guo <None> This closes apache#2409 from WJL3333/fix_maven_pom_doclint_config
### Descriptions of the changes in this PR: -Updated netty,netty-boringssl and rocksdb to latest version for aarch64 support -Updated deprecated methods of rocksdb to remove compilation warning and resolve build failure of bookkeeper-server on amd64 and aarch64 platforms as Werror flag is enabled. ### Motivation Build of the bookkeeper-server package was failing on amd64 and aarch64 platforms. ### Changes There are many methods that are marked deprecated in rocksdb, but in use by bookkeeper package. So updated those methods in bookkeeper package according to current rocksdb implementation. Also updated netty,netty-boringssl and rocksdb to latest version as they are having aarch64 support. Master Issue: apache#2378 Reviewers: Enrico Olivelli <eolivelli@gmail.com>, Sijie Guo <None> This closes apache#2379 from odidev/bookkeeper_aarch
### Motivation This is the bookie side implementation of BP-41 ### Changes - add new `bookieId` configuration option - implement Bookie#getBookieId - add test cases - make all BookKeeperClusterTestCase to use a random UUID as bookieId in order to increase code coverage - add useUUIDasBookieId flag to BookKeeperClusterTestCase for test cases that require using hostname:port - add a default BookieServiceInfo provider in tests - enable Auditor to use BookieId and not BookieSocketAddress.toBookieId() - use BookieId for LocalTransport addresses - in Cookie.java rename BookieHost to BookieId (only in Java code, no data format changes) Master Issue: apache#2396 Co-authored-by: Enrico Olivelli <enrico.olivelli@diennea.com>
Descriptions of the changes in this PR: ### Motivation Resolves apache#2285 ### Changes We write "bookie server unavailable log" every 30 seconds per bookie client Reviewers: Enrico Olivelli <eolivelli@apache.org, Sijie Guo <sijie@apache.org> This closes apache#2393 from nicoloboschi/fix/2285/spammy-logs, closes apache#2285
Descriptions of the changes in this PR: - Fix potential NullPointerException when local book keeper stop on exit. Reviewers: Enrico Olivelli <eolivelli@gmail.com>, Sijie Guo <sijie@apache.org> This closes apache#2414 from gaoxinge/fix
the web site rendering error in section reference/cli/ updateBookieInLedger Options
c891774 to
691ddb0
Compare
|
Messed up the PR, closing this and reopening under new PR. |
|
Reopened under #2429 |
This feature allows a predefined set of services to be 'whitelisted' to be able to access bookkeeper based on their client certificates. _This feature is disabled by default._
**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**
To enable if, we have to set 2 configuration options in server config.
1. Set `bookieAuthProviderFactoryClass` config option to use BookieAuthZFactory
`bookieAuthProviderFactoryClass=org.apache.bookkeeper.tls.BookieAuthZFactory`
2. Set `authorizedRoles` to a comma separated list of roles present in client certificates' OU field.
`authorizedRoles=pulsar-broker-1,pulsar-broker-2`
Read further for details on how to implement these in your client certificates and how to wire it up.
So this feature can be broken down into two parts:
Certificate and roles
Server configuration for authorized roles
**Details:**
_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.
_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.
This is a redo of stale PR #2355
Master Issue: #2354
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:
Details:
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.
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