Issue 2815: Upgrade to log4j2 to get rid of CVE-2019-17571 #2816
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
eolivelli
requested changes
Oct 6, 2021
bookkeeper-common/src/main/java/org/apache/bookkeeper/common/util/MdcUtils.java
Outdated
Show resolved
Hide resolved
|
With current changes, log4j1.2 seems to be completely removed: |
dlg99
approved these changes
Oct 8, 2021
eolivelli
approved these changes
Oct 13, 2021
|
@RaulGracia can you please resolve the conflicts ? |
Signed-off-by: Raúl <raul.gracia@emc.com>
Signed-off-by: Raúl <raul.gracia@emc.com>
Signed-off-by: Raúl <raul.gracia@emc.com>
Signed-off-by: Raúl <raul.gracia@emc.com>
Signed-off-by: Raúl <raul.gracia@emc.com>
Signed-off-by: Raúl <raul.gracia@emc.com>
Signed-off-by: Raúl <raul.gracia@emc.com>
Signed-off-by: Raúl <raul.gracia@emc.com>
Signed-off-by: Raúl <raul.gracia@emc.com>
This reverts commit a01d257.
Signed-off-by: Raúl <raul.gracia@emc.com>
RaulGracia
force-pushed
the
issue-log4j2-migration
branch
from
1927589 to
48cbaed
Compare
This was referenced Oct 15, 2021
| @@ -85,8 +85,16 @@ | |||
|
|
|||
| <!-- slf4j binding --> | |||
| <dependency> | |||
| <groupId>org.slf4j</groupId> | |||
| <artifactId>slf4j-log4j12</artifactId> | |||
| <groupId>org.apache.logging.log4j</groupId> | |||
There was a problem hiding this comment.
I know I am late at the PR, apologies. But I am not sure this is the right thing to do. Bookkeeper should work with SLF4J instead of log4j. Allow me to explain. There are three elements to it.
- SLF4J this is the facade for logging framework. It does not provide logging implementation. But just facade API. Bookkeeper should only be using that, test code could be a possible exception. That way SLF4j can work with any type of logging infra, log4j2 or log4j1 being just few options out of many.
- SLF4j-LOG4J12: This is Binding for SLF4 facade with LOG4J1, "ideally" bookkeeper should not have any dependency on this. This should again be provided by application at runtime. Role of SLF4j-LOG4J12 is that If SLF4J sees SLF4j-LOG4J12 in the Runtime classpath, It will go on lookout for log4j12. And that will ensure that for actual logging log4j2 or log4j12 is used. In other words SLF4j-LOG4J12 works as bridge between facade and implementation.
- log4j12 or log4j2 or simplelog4j or foolog4j: This is actual logging library. Which does the actual logging. And certainly should be provided at runtime by application. In ideal case Bookkeeper should not provide this as dependency.
To me looks like we have removed slf4j and started using log4j directly. Which means we have created a hard binding with log4j, which I believe is not desirable.
There was a problem hiding this comment.
This is the dist package.
It is important that in BK code we use only slf4j
But this is the package, we have to provide an implementation
eolivelli
reviewed
Oct 17, 2021
There was a problem hiding this comment.
@pkumar-singh you are right.
I left a comment in the common package
| <dependency> | ||
| <groupId>org.apache.logging.log4j</groupId> | ||
| <artifactId>log4j-1.2-api</artifactId> | ||
| <version>${log4j.version}</version> |
There was a problem hiding this comment.
Here we should not add these deps in common.
The common and server packages should not bring in any implementation
|
@RaulGracia Would you like to address the new comment in another PR? |
|
@zymap @eolivelli @pkumar-singh let me try to send a draft PR with the suggested change, so we can continue the discussion there. |
hsaputra
pushed a commit
that referenced
this pull request
Oct 20, 2021
BP-44: USE metrics. A proposal for improving BookKeeper metrics so that operators can employ the USE method for diagnosing performance issues. Reviewers: Henry Saputra <hsaputra@apache.org>, Andrey Yegorov <None>, Enrico Olivelli <eolivelli@gmail.com> This closes #2835 from Vanlightly/BP-44-use-metrics and squashes the following commits: 8d9baab [Jack Vanlightly] Added link to USE method and listed each term of USE 5a0f67d [Jack Vanlightly] BP-44 USE metrics a9b576d [Yunze Xu] Release semaphore when addEntry accepts the same entries (#2832) 148bf22 [Yun Tang] Ensure to release cache during KeyValueStorageRocksDB#closec (#2821) 4dc4260 [gaozhangmin] Heap memory leak problem when ledger replication failed (#2794) a522fa3 [Raúl Gracia] Issue 2815: Upgrade to log4j2 to get rid of CVE-2019-17571 (#2816) 0465052 [Nicolò Boschi] Upgrade httpclient from 4.5.5 to 4.5.13 (#2793) 594a056 [Raúl Gracia] Issue 2795: Bookkeeper upgrade using Bookie ID may fail due to cookie mismatch (#2796) 354cf37 [Raúl Gracia] Upgraded dependencies with CVEs (#2792) e413c70 [Raúl Gracia] Issue 2728: Entry Log GC may get blocked when using entryLogPerLedgerEnabled option (#2779) 883231e [pradeepbn] Building bookkeeper with gradle on java11
hsaputra
pushed a commit
that referenced
this pull request
Oct 29, 2021
### Motivation
In order to complete migration to Gradle we must build all the subprojects.
### Changes
- Enabled `sh` integration tests with gradle, located in `tests/scripts/src/test/bash/gradle`
- Added these modules to the build
- `bookkeeper-http:servlet-http-server`
- `metadata-drivers:etcd`
- `tests:backward-compat:*`
- `tests:shaded:*`
- `stream:bk-grpc-name-resolver`
- DL shading process is now performed (before it didn't build any jar)
- Groovy tests (`tests:backward-compat:*`) now are triggered by the build/tests itself; with Maven, there is a "runner" project (`tests/integration-tests-base-groovy`); in Gradle is useless so it is skipped
### Test
- Both `bin/bookkeper standalone` and `bin/bookkeper_gradle standalone` work locally
- Tests are passing locally
Master Issue: #2849
Reviewers: Henry Saputra <hsaputra@apache.org>, Prashant Kumar <None>
This closes #2850 from nicoloboschi/fix/2849/gradle and squashes the following commits:
00b49f4 [Nicolò Boschi] Fix common_gradle.sh regex
bd739fd [Nicolò Boschi] fix sh tests
43230ba [Nicolò Boschi] revert sh files. Avoid to modify maven files, create gradle versions to faciltate migration
d1f95e4 [Nicolò Boschi] fix shaded deps
bcab40d [Nicolò Boschi] fix build
5fd0341 [Nicolò Boschi] fix build
0082e0e [Nicolò Boschi] fix build
2c32ac1 [Nicolò Boschi] fixes
3bc0b26 [Nicolò Boschi] bookkeeper-server-shaded-tests
ba89132 [Nicolò Boschi] shaded tests
6d39e33 [Nicolò Boschi] sh tests
e0032bc [Nicolò Boschi] actually run arquillian groovy tests
08dcc39 [Nicolò Boschi] backwards
2361f79 [Nicolò Boschi] hierarchical-ledger-manager
8388e11 [Nicolò Boschi] current-server-old-clients
6a24344 [Nicolò Boschi] bc-non-fips
2faca01 [Nicolò Boschi] bk-grpc-name-resolver
991bc11 [Nicolò Boschi] servlet-http-server
675ef7b [Nicolò Boschi] etcd
b1d5e14 [ZhangJian He] A empty implement in EtcdLedgerManagerFactory to let the project can compile (#2845)
bd5c50b [shustsud] Add error handling to readLedgerMetadata in over-replicated ledger GC (#2844)
746f9f6 [Matteo Merli] Remove direct ZK access for Auditor (#2842)
4117200 [ZhangJian He] the compare should be >= instead of > (#2782)
14ef56f [Prashant Kumar] BookieId can not be cast to BookieSocketAddress (#2843)
e10f3fe [ZhangJian He] Forget to close preAllocator log on shutdown (#2819)
53954ca [shustsud] Add ensemble check to over-replicated ledger GC (#2813)
919fdd3 [Prashant Kumar] Issue:2840 Create bookie shellscript for gradle (#2841)
031d168 [gaozhangmin] fix-npe-when-pulsar-ZkBookieRackAffinityMapping-getBookieAddressResolver (#2788)
3dd671c [Prashant Kumar] Migrate bookkeepr-server:test to gradle run unit tests excepts org.apache.bookkeeper.bookie. org.apache.bookkeeper.client org.apache.bookkeeper.replication org.apache.bookkeeper.tls. (#2812)
f6903b8 [Jack Vanlightly] BP-44 USE metrics
a4afaa4 [Matteo Merli] Eliminate direct ZK access in ScanAndCompareGarbageCollector (#2833)
a9b576d [Yunze Xu] Release semaphore when addEntry accepts the same entries (#2832)
148bf22 [Yun Tang] Ensure to release cache during KeyValueStorageRocksDB#closec (#2821)
4dc4260 [gaozhangmin] Heap memory leak problem when ledger replication failed (#2794)
a522fa3 [Raúl Gracia] Issue 2815: Upgrade to log4j2 to get rid of CVE-2019-17571 (#2816)
0465052 [Nicolò Boschi] Upgrade httpclient from 4.5.5 to 4.5.13 (#2793)
594a056 [Raúl Gracia] Issue 2795: Bookkeeper upgrade using Bookie ID may fail due to cookie mismatch (#2796)
354cf37 [Raúl Gracia] Upgraded dependencies with CVEs (#2792)
e413c70 [Raúl Gracia] Issue 2728: Entry Log GC may get blocked when using entryLogPerLedgerEnabled option (#2779)
883231e [pradeepbn] Building bookkeeper with gradle on java11
|
This change does not seem to have been released officially yet and many people are looking at ASF and wondering why we still have releases that have insecure logging in them. Could a new release be done? Latest released pom: |
|
I change the version of log4j from 2.14.1 to 2.17.1, and run bookie in local, it just print out few logs, and many logs are lost. When I roll back the log4j version to 2.14.1, and run bookies in local, the log is normal |
Ghatage
pushed a commit
to sijie/bookkeeper
that referenced
this pull request
Jul 12, 2024
### Motivation Upgrades to log4j2 to get rid of CVE-2019-17571. ### Changes The migration of log4j has been done mainly taking the official guidelines: https://logging.apache.org/log4j/2.x/manual/migration.html. In this PR, the following changes are included: - Replacement of `slf4j-log4j12` by `log4j-1.2-api`. Also included the `log4j-slf4j-impl` binding as well as the `log4j-core` library. - Changes in `pom`, `gradle` and license files to reflect the above library upgrade. - Test classes `TestOrderedExecutorDecorators`, `LoggerOutput`, `MdcContextTest`, as well as the class `FIleSystemUpgrade` made use of log4j1.2 API. This PR attempts to keep the same functionality with the new APIs. ### Verification - Existing tests are passing. - log4j1.2 is removed from project: apache#2816 (comment) - Using `localbookie`, we observe that logs are shown correctly: ``` 2021-10-07T16:04:23,757 - INFO - [main:GarbageCollectorThread@245] - Minor Compaction : enabled=true, threshold=0.20000000298023224, interval=3600000 2021-10-07T16:04:23,760 - INFO - [main:GarbageCollectorThread@247] - Major Compaction : enabled=true, threshold=0.800000011920929, interval=86400000 2021-10-07T16:04:23,952 - INFO - [main:BookieImpl@920] - Finished replaying journal in 2 ms. 2021-10-07T16:04:23,958 - INFO - [SyncThread-7-1:SyncThread@135] - Flush ledger storage at checkpoint CheckpointList{checkpoints=[LogMark: logFileId - 0 , logFileOffset - 0]}. 2021-10-07T16:04:23,980 - INFO - [main:BookieImpl@1010] - Finished reading journal, starting bookie 2021-10-07T16:04:24,011 - INFO - [BookieJournal-5000:Journal@919] - Starting journal on /tmp/localbookkeeper06554024139823286046test/current 2021-10-07T16:04:24,031 - INFO - [ForceWriteThread:Journal$ForceWriteThread@478] - ForceWrite Thread started 2021-10-07T16:04:24,048 - INFO - [BookieJournal-5000:JournalChannel@169] - Opening journal /tmp/localbookkeeper06554024139823286046test/current/17c5b11c65b.txn ``` In addition to that, if we change the `log4j.properties` file, the changes are reflected in the console output, meaning that the legacy configuration works and changes can be correctly applied: ``` Over Replicated Ledger Deletion : enabled=true, interval=86400000 Minor Compaction : enabled=true, threshold=0.20000000298023224, interval=3600000 Major Compaction : enabled=true, threshold=0.800000011920929, interval=86400000 Finished replaying journal in 5 ms. Flush ledger storage at checkpoint CheckpointList{checkpoints=[LogMark: logFileId - 0 , logFileOffset - 0]}. Finished reading journal, starting bookie Starting journal on /tmp/localbookkeeper015049859959001160726test/current ForceWrite Thread started Opening journal /tmp/localbookkeeper015049859959001160726test/current/17c5b143063.txn ``` More verifications that logging works properly related to other Bookkeeper sub-components impacted may be needed. Master Issue: apache#2815
Ghatage
pushed a commit
to sijie/bookkeeper
that referenced
this pull request
Jul 12, 2024
BP-44: USE metrics. A proposal for improving BookKeeper metrics so that operators can employ the USE method for diagnosing performance issues. Reviewers: Henry Saputra <hsaputra@apache.org>, Andrey Yegorov <None>, Enrico Olivelli <eolivelli@gmail.com> This closes apache#2835 from Vanlightly/BP-44-use-metrics and squashes the following commits: 8d9baab [Jack Vanlightly] Added link to USE method and listed each term of USE 5a0f67d [Jack Vanlightly] BP-44 USE metrics a9b576d [Yunze Xu] Release semaphore when addEntry accepts the same entries (apache#2832) 148bf22 [Yun Tang] Ensure to release cache during KeyValueStorageRocksDB#closec (apache#2821) 4dc4260 [gaozhangmin] Heap memory leak problem when ledger replication failed (apache#2794) a522fa3 [Raúl Gracia] Issue 2815: Upgrade to log4j2 to get rid of CVE-2019-17571 (apache#2816) 0465052 [Nicolò Boschi] Upgrade httpclient from 4.5.5 to 4.5.13 (apache#2793) 594a056 [Raúl Gracia] Issue 2795: Bookkeeper upgrade using Bookie ID may fail due to cookie mismatch (apache#2796) 354cf37 [Raúl Gracia] Upgraded dependencies with CVEs (apache#2792) e413c70 [Raúl Gracia] Issue 2728: Entry Log GC may get blocked when using entryLogPerLedgerEnabled option (apache#2779) 883231e [pradeepbn] Building bookkeeper with gradle on java11
Ghatage
pushed a commit
to sijie/bookkeeper
that referenced
this pull request
Jul 12, 2024
### Motivation
In order to complete migration to Gradle we must build all the subprojects.
### Changes
- Enabled `sh` integration tests with gradle, located in `tests/scripts/src/test/bash/gradle`
- Added these modules to the build
- `bookkeeper-http:servlet-http-server`
- `metadata-drivers:etcd`
- `tests:backward-compat:*`
- `tests:shaded:*`
- `stream:bk-grpc-name-resolver`
- DL shading process is now performed (before it didn't build any jar)
- Groovy tests (`tests:backward-compat:*`) now are triggered by the build/tests itself; with Maven, there is a "runner" project (`tests/integration-tests-base-groovy`); in Gradle is useless so it is skipped
### Test
- Both `bin/bookkeper standalone` and `bin/bookkeper_gradle standalone` work locally
- Tests are passing locally
Master Issue: apache#2849
Reviewers: Henry Saputra <hsaputra@apache.org>, Prashant Kumar <None>
This closes apache#2850 from nicoloboschi/fix/2849/gradle and squashes the following commits:
00b49f4 [Nicolò Boschi] Fix common_gradle.sh regex
bd739fd [Nicolò Boschi] fix sh tests
43230ba [Nicolò Boschi] revert sh files. Avoid to modify maven files, create gradle versions to faciltate migration
d1f95e4 [Nicolò Boschi] fix shaded deps
bcab40d [Nicolò Boschi] fix build
5fd0341 [Nicolò Boschi] fix build
0082e0e [Nicolò Boschi] fix build
2c32ac1 [Nicolò Boschi] fixes
3bc0b26 [Nicolò Boschi] bookkeeper-server-shaded-tests
ba89132 [Nicolò Boschi] shaded tests
6d39e33 [Nicolò Boschi] sh tests
e0032bc [Nicolò Boschi] actually run arquillian groovy tests
08dcc39 [Nicolò Boschi] backwards
2361f79 [Nicolò Boschi] hierarchical-ledger-manager
8388e11 [Nicolò Boschi] current-server-old-clients
6a24344 [Nicolò Boschi] bc-non-fips
2faca01 [Nicolò Boschi] bk-grpc-name-resolver
991bc11 [Nicolò Boschi] servlet-http-server
675ef7b [Nicolò Boschi] etcd
b1d5e14 [ZhangJian He] A empty implement in EtcdLedgerManagerFactory to let the project can compile (apache#2845)
bd5c50b [shustsud] Add error handling to readLedgerMetadata in over-replicated ledger GC (apache#2844)
746f9f6 [Matteo Merli] Remove direct ZK access for Auditor (apache#2842)
4117200 [ZhangJian He] the compare should be >= instead of > (apache#2782)
14ef56f [Prashant Kumar] BookieId can not be cast to BookieSocketAddress (apache#2843)
e10f3fe [ZhangJian He] Forget to close preAllocator log on shutdown (apache#2819)
53954ca [shustsud] Add ensemble check to over-replicated ledger GC (apache#2813)
919fdd3 [Prashant Kumar] Issue:2840 Create bookie shellscript for gradle (apache#2841)
031d168 [gaozhangmin] fix-npe-when-pulsar-ZkBookieRackAffinityMapping-getBookieAddressResolver (apache#2788)
3dd671c [Prashant Kumar] Migrate bookkeepr-server:test to gradle run unit tests excepts org.apache.bookkeeper.bookie. org.apache.bookkeeper.client org.apache.bookkeeper.replication org.apache.bookkeeper.tls. (apache#2812)
f6903b8 [Jack Vanlightly] BP-44 USE metrics
a4afaa4 [Matteo Merli] Eliminate direct ZK access in ScanAndCompareGarbageCollector (apache#2833)
a9b576d [Yunze Xu] Release semaphore when addEntry accepts the same entries (apache#2832)
148bf22 [Yun Tang] Ensure to release cache during KeyValueStorageRocksDB#closec (apache#2821)
4dc4260 [gaozhangmin] Heap memory leak problem when ledger replication failed (apache#2794)
a522fa3 [Raúl Gracia] Issue 2815: Upgrade to log4j2 to get rid of CVE-2019-17571 (apache#2816)
0465052 [Nicolò Boschi] Upgrade httpclient from 4.5.5 to 4.5.13 (apache#2793)
594a056 [Raúl Gracia] Issue 2795: Bookkeeper upgrade using Bookie ID may fail due to cookie mismatch (apache#2796)
354cf37 [Raúl Gracia] Upgraded dependencies with CVEs (apache#2792)
e413c70 [Raúl Gracia] Issue 2728: Entry Log GC may get blocked when using entryLogPerLedgerEnabled option (apache#2779)
883231e [pradeepbn] Building bookkeeper with gradle on java11
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
Upgrades to log4j2 to get rid of CVE-2019-17571.
Changes
The migration of log4j has been done mainly taking the official guidelines: https://logging.apache.org/log4j/2.x/manual/migration.html.
In this PR, the following changes are included:
slf4j-log4j12bylog4j-1.2-api. Also included thelog4j-slf4j-implbinding as well as thelog4j-corelibrary.pom,gradleand license files to reflect the above library upgrade.TestOrderedExecutorDecorators,LoggerOutput,MdcContextTest, as well as the classFIleSystemUpgrademade use of log4j1.2 API. This PR attempts to keep the same functionality with the new APIs.Verification
localbookie, we observe that logs are shown correctly:In addition to that, if we change the
log4j.propertiesfile, the changes are reflected in the console output, meaning that the legacy configuration works and changes can be correctly applied:More verifications that logging works properly related to other Bookkeeper sub-components impacted may be needed.
Master Issue: #2815