Thanks for the review. Let’s talk about the patch in more detail on GitHub.
> On Aug 13, 2025, at 13:21, Enrico Olivelli <eolive...@gmail.com> wrote: > > Il Mer 13 Ago 2025, 19:56 Andor Molnar <an...@apache.org> ha scritto: > >> I’ve created a PR to attempt to replace logback with slf4j-simple. We do >> have some test code relying on logback’s appenders, but we might be able to >> replace it with a basic output stream. >> >> https://github.com/apache/zookeeper/pull/2293 >> >> >> ptal. >> > > Overall I agree with the approach. I left a couple of questions about the > impact on client users and server system admins > > I am +1 to release 3.10 and send 3.8 to EOL > > > > Thanks > Enrico > > >> >> Andor >> >> >> >>> On Aug 12, 2025, at 15:58, Christopher <ctubb...@apache.org> wrote: >>> >>> I would NOT ship with reload4j in new releases. That exists only as a >>> temporary workaround for the lack of patch support for log4j 1.2, >>> until one is able to update to log4j2 or something else. ZK is already >>> using something else (logback). Neither log4j 1.2 nor reload4j are >>> appropriate for use with slf4j2. If a log4j implementation is desired, >>> a log4j 2.x version should be used. Using reload4j would be a step in >>> the wrong direction. >>> >>> However, since ZK uses slf4j-api in code, I would ship *only* with >>> slf4j-simple as the trivial reference implementation, and let users >>> determine their own runtime logging dependency and configuration, >>> which can be trivially swapped in place of slf4j-simple. This is >>> basically what I argued when logback was being considered to replace >>> log4j/reload4j a few years ago, but others preferred logback. >>> >>> One thing to consider is that, if I remember correctly, logback is >>> used as a test dependency for some build tests. It is easier to run >>> those tests with logback than it is to do it with another runtime >>> dependency. It is possible to have the tests run using logback, but >>> still ship with slf4j-simple instead of logback, but it may take some >>> careful attention to the Maven pom.xml files to ensure the desired >>> result (let's call this the "split dependency" option). It is >>> certainly easier to stay with logback ("single dependency option"), >>> but just update it to a newer version, than to modify the Maven build >>> to ship something different than what is used in testing, but both >>> options are technically possible. >>> >>> >>> On Fri, Aug 8, 2025 at 5:19 PM Andor Molnar <an...@apache.org> wrote: >>>> >>>> Yeah, I totally agree. >>>> >>>> The plan to go forward after getting 3.9.4 out of the door is to either >> remove logback from branch-3.8 and replace it with something simpler like >> slf4j-simple or reload4j since we ship the logback dependency as an >> example. Though I’m not sure if slf4j-simple is an option for us, I have to >> try it out in action. >>>> >>>> The other option is to announce EoL on branch-3.8 and encourage users >> to upgrade to 3.9.4. At the same time we have to create a 3.10.0 release >> off the main branch or maybe 4.0.0. I don't have a strong opinion here >> either, but I’m pretty confident that we should drop Java 8 support in the >> next “current” release. >>>> >>>> Andor >>>> >>>> >>>> >>>> >>>>> On Aug 8, 2025, at 15:05, Christopher <ctubb...@apache.org> wrote: >>>>> >>>>> I don't think the upgrade to slf4j 2 is purely a semantic one. I think >>>>> there are genuine incompatibilities, but probably not too many. For >>>>> example, slf4j2 drops support for Java 7 (probably not a problem for >>>>> ZK, since I think Java 8 is already a requirement), and it switches >>>>> the binding mechanisms, so that if somebody was using a different >>>>> sfl4j runtime other than logback, then it probably won't work anymore >>>>> without additional changes on the user's part. >>>>> >>>>> I think the switch on 3.9 is probably okay, but users should be warned >>>>> that their logging will probably break if they had used a different >>>>> runtime binding for slf4j other than the logback version that ships >>>>> with ZK. >>>>> >>>>> As for the differences between logback 1.2 and 1.3, I have no idea... >>>>> it's probably fine, since ZK is mainly just using it via slf4j-api >>>>> anyway, rather than using it directly, but I'd start getting concerned >>>>> that 1.3 is also no longer being developed. ZK should probably get >>>>> ahead of that on the master branch by requiring Java 11, and using >>>>> logback 1.5 there, if it hasn't been done already. Or else this >>>>> question of switching from logback 1.2 to 1.3 is going to come up >>>>> again soon when there's a CVE found against 1.3 and you have to switch >>>>> to 1.5 and Java 11.... certainly don't want to do that in a bugfix >>>>> release from the ZK 3.9 branch. >>>>> >>>>> On Thu, Aug 7, 2025 at 9:41 AM Andor Molnar <an...@apache.org> wrote: >>>>>> >>>>>> Considering all of this I’ll upgrade logback + slf4j to 1.3/2.0 on >> the 3.9 branch today and proceed with the release. 3.9 is the current >> release line and I think this step is still acceptable at this stage. I >> won’t do the same on the stable (3.8) branch and we should talk about >> EoL’ing soon in a separate thread. >>>>>> >>>>>> Andor >>>>>> >>>>>> >>>>>> >>>>>> >>>>>>> On Aug 6, 2025, at 19:56, Andor Molnar <an...@apache.org> wrote: >>>>>>> >>>>>>> "The 1.2.x series for logback-core and logback-classic has been >> deprecated for several years and is no longer maintained. As such, use of >> the 1.2.x series is discouraged.” >>>>>>> >>>>>>> "Logback version 1.3.15 is the latest in the 1.3.x series. It >> requires SLF4J version 2.0.x and JDK 8. Please note that the 1.3.x series >> is no loger actively developed.” >>>>>>> >>>>>>> "The current actively developed version of logback-core and >> logback-classic is 1.5.18. It requires JDK 11 and SLF4J version 2.0.1 at >> runtime.” >>>>>>> >>>>>>> Looks like our only option is 1.3.x, but once we drop JDK 8 support >> (3.10.x maybe?), we’ll be able to upgrade to 1.5. >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>>> On Aug 6, 2025, at 19:52, Andor Molnar <an...@apache.org> wrote: >>>>>>>> >>>>>>>> I cannot upgrade logback without upgrading slf4j as well. Build >> fails. >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>>> On Aug 6, 2025, at 17:07, Patrick Hunt <ph...@apache.org> wrote: >>>>>>>>> >>>>>>>>> Is slf4j really needed for security? >>>>>>>>> >>>>>>>>> Only cve I see here is from 2018... >>>>>>>>> https://www.slf4j.org/news.html >>>>>>>>> >>>>>>>>> Should we revert the slf4j change in its entirety/all branches >> until it can >>>>>>>>> be made in a b/w compatible way? >>>>>>>>> >>>>>>>>> Patrick >>>>>>>>> >>>>>>>>> On Wed, Aug 6, 2025 at 2:59 PM Andor Molnar <an...@apache.org> >> wrote: >>>>>>>>> >>>>>>>>>> Maybe the slf4j upgrade (1.7.30 -> 2.0.13) has higher impact, >> because it’s >>>>>>>>>> a major upgrade. Logback is just an example of how to do logging >> with >>>>>>>>>> ZooKeeper real life setups probably replace it with something >> else like >>>>>>>>>> log4j2. The logging facade (slf4j) could have bw incompatible >> changes that >>>>>>>>>> will force users to make changes related to logging on their >> classpath. >>>>>>>>>> >>>>>>>>>> I’m speculating and haven’t checked slf4j for details. >>>>>>>>>> >>>>>>>>>> Andor >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>>> On Aug 6, 2025, at 16:46, Patrick Hunt <ph...@apache.org> wrote: >>>>>>>>>>> >>>>>>>>>>> Is the only problem the minor "semantic" upgrade of logback in a >> fix >>>>>>>>>>> release of zk? That should be stable (contract wise) on the >> dependency, >>>>>>>>>>> right? Or is there some real impact, eg b/w incompat change >> visible to ZK >>>>>>>>>>> users? If the former that seems fine, if the latter then we have >> a harder >>>>>>>>>>> problem to address. (security issue breaking b/w compat) >>>>>>>>>>> >>>>>>>>>>> Patrick >>>>>>>>>>> >>>>>>>>>>> On Wed, Aug 6, 2025 at 2:36 PM Andor Molnar <an...@apache.org> >> wrote: >>>>>>>>>>> >>>>>>>>>>>> Sorry for the confusion, I picked 3.8 as an example, but >> logback/slf4j >>>>>>>>>>>> upgrades haven’t been backported to 3.9 either. Therefore I >> created the >>>>>>>>>>>> following backport PR: >>>>>>>>>>>> >>>>>>>>>>>> https://github.com/apache/zookeeper/pull/2290 >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>>> "Why would they be applied to master and not to any active >> (release) >>>>>>>>>>>> line? >>>>>>>>>>>> >>>>>>>>>>>> Since we’ve already released 3.9.3 with logback 1.2.13 and >> don’t want >>>>>>>>>>>> users to realize 1.2->1.3 logback upgrade in a 3.9.3->3.9.4 >> ZooKeeper >>>>>>>>>>>> upgrade process, although this upgrade is necessary anyways to >> address >>>>>>>>>> the >>>>>>>>>>>> CVE in question. >>>>>>>>>>>> >>>>>>>>>>>> (in my understanding) >>>>>>>>>>>> >>>>>>>>>>>> Andor >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>>> On Aug 6, 2025, at 15:34, Patrick Hunt <ph...@apache.org> >> wrote: >>>>>>>>>>>>> >>>>>>>>>>>>> I'm confused - this thread started with "OWASP reports CVEs on >> the 3.8 >>>>>>>>>>>>> branch and noticed in the PRs that we should only upgrade >> logback on >>>>>>>>>> the >>>>>>>>>>>>> master branch" - I read that as "some fixes on 3.9 are not >> backported >>>>>>>>>> to >>>>>>>>>>>>> 3.8". But you are saying that this is not fixed (still owasp >> warnings) >>>>>>>>>> on >>>>>>>>>>>>> 3.9 which is separate from master? Why would they be applied >> to master >>>>>>>>>>>> and >>>>>>>>>>>>> not to any active (release) line? What is the impact of the >> changes on >>>>>>>>>>>>> master and 3.9? iiuc there are backward incompatible changes >> if applied >>>>>>>>>>>> to >>>>>>>>>>>>> 3.8? There should not be b/w incompatible changes applied to >> any 3.x >>>>>>>>>>>> (incl >>>>>>>>>>>>> master, a future 3.x...) release. >>>>>>>>>>>>> >>>>>>>>>>>>> Patrick >>>>>>>>>>>>> >>>>>>>>>>>>> On Wed, Aug 6, 2025 at 1:16 PM Andor Molnar <an...@apache.org> >> wrote: >>>>>>>>>>>>> >>>>>>>>>>>>>> Yeah, that would remove the burden of maintaining the 3.8 >> version >>>>>>>>>> line, >>>>>>>>>>>>>> but 3.9.x versions still don’t have logback and slf4j >> upgraded, still >>>>>>>>>>>>>> flagged by the Owasp build and users will probably still >> complain >>>>>>>>>> about >>>>>>>>>>>>>> CVEs. >>>>>>>>>>>>>> >>>>>>>>>>>>>> My question is what should we do on branches other than the >> master? >>>>>>>>>>>>>> >>>>>>>>>>>>>> 1. Backport logback and slf4j upgrades from master, or >>>>>>>>>>>>>> 2. Add Owasp suppression rule to skip checking these libraries >>>>>>>>>>>> completely. >>>>>>>>>>>>>> >>>>>>>>>>>>>> I need to answer this question before going forward with the >> 3.9.4 >>>>>>>>>>>> release. >>>>>>>>>>>>>> >>>>>>>>>>>>>> Regards, >>>>>>>>>>>>>> Andor >>>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>>>>> On Aug 6, 2025, at 13:39, Christopher <ctubb...@apache.org> >> wrote: >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> +1 to that idea. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> The releases page[1] says "Apache ZooKeeper 3.9.3 is our >> current >>>>>>>>>>>>>>> release, and 3.8.4 our latest stable release". Is 3.9 >> sufficiently >>>>>>>>>>>>>>> stable to replace 3.8 as the current "stable"? If the answer >> is yes, >>>>>>>>>>>>>>> then I think it makes sense to EOL 3.8. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> [1]: https://zookeeper.apache.org/releases.html#download >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> On Mon, Aug 4, 2025 at 2:52 PM Patrick Hunt < >> ph...@apache.org> >>>>>>>>>> wrote: >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> Should we sunset that minor release due to the "unfixable" >> security >>>>>>>>>>>>>> issue >>>>>>>>>>>>>>>> and EOL of dependenc(ies)? >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> Patrick >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> On Mon, Aug 4, 2025 at 10:03 AM Andor Molnar < >> an...@apache.org> >>>>>>>>>>>> wrote: >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> Yeah, I agree with that, but we can’t leave things here >> just like >>>>>>>>>>>> that. >>>>>>>>>>>>>>>>> Either we should keep updating the logging libraries on >> all active >>>>>>>>>>>>>> branches >>>>>>>>>>>>>>>>> or add the necessary suppression to Owasp. Otherwise the >> report >>>>>>>>>>>> result >>>>>>>>>>>>>> will >>>>>>>>>>>>>>>>> be completely meaningless. >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> Andor >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> On Aug 4, 2025, at 08:21, Christopher < >> ctubb...@apache.org> >>>>>>>>>> wrote: >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> Yes, that is basically my concern. I commented at >>>>>>>>>>>>>>>>>> >>>>>>>>>>>> >> https://github.com/apache/zookeeper/pull/2290#issuecomment-3145955665 >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> On Fri, Aug 1, 2025, 18:43 Andor Molnar <an...@apache.org> >> wrote: >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>> Christopher raised concern about it in >>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>> >> https://github.com/apache/zookeeper/pull/2162#pullrequestreview-2037135095 >>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>> I suspect because SLF4j has to be major upgraded with >> logback 1.x >>>>>>>>>>>> -> >>>>>>>>>>>>>> 2.x >>>>>>>>>>>>>>>>>>> which should not be done in bugfix releases. >>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>> I’m not sure. Maybe we should just add another Owasp >> suppression, >>>>>>>>>>>> but >>>>>>>>>>>>>>>>> that >>>>>>>>>>>>>>>>>>> wouldn’t be appropriate either. >>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>> Andor >>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>> On Jul 30, 2025, at 18:39, Andor Molnar < >> an...@apache.org> >>>>>>>>>> wrote: >>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>> That’s my understanding too, but looks like folks >> skipped even >>>>>>>>>> the >>>>>>>>>>>>>> 3.9 >>>>>>>>>>>>>>>>>>> backport in the case of logback. >>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>> Andor >>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> On Jul 30, 2025, at 16:36, Patrick Hunt < >> ph...@apache.org> >>>>>>>>>>>> wrote: >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> My understanding, I thought the rule was to backport >> any patch >>>>>>>>>> to >>>>>>>>>>>>>> all >>>>>>>>>>>>>>>>> of >>>>>>>>>>>>>>>>>>>>> the active releases unless it's a new feature. Perhaps >> ask the >>>>>>>>>>>>>> folks >>>>>>>>>>>>>>>>> who >>>>>>>>>>>>>>>>>>>>> committed? >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> Patrick >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> On Wed, Jul 30, 2025 at 2:06 PM Andor Molnar < >> an...@apache.org >>>>>>>>>>> >>>>>>>>>>>>>>>>> wrote: >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>> Hi folks, >>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>> Currently I’m working on some backports, because >> OWASP reports >>>>>>>>>>>>>> CVEs >>>>>>>>>>>>>>>>> on >>>>>>>>>>>>>>>>>>> the >>>>>>>>>>>>>>>>>>>>>> 3.8 branch and noticed in the PRs that we should only >> upgrade >>>>>>>>>>>>>> logback >>>>>>>>>>>>>>>>>>> on >>>>>>>>>>>>>>>>>>>>>> the master branch. Why is that? >>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>> logback-core-1.2.13.jar >>>>>>>>>>>>>> (pkg:maven/ch.qos.logback/logback-core@1.2.13 >>>>>>>>>>>>>>>>> , >>>>>>>>>>>>>>>>>>>>>> cpe:2.3:a:qos:logback:1.2.13:*:*:*:*:*:*:*) : >> CVE-2024-12798, >>>>>>>>>>>>>>>>>>> CVE-2024-12801 >>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>> Regards, >>>>>>>>>>>>>>>>>>>>>> Andor >>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>> >> >>
