Properly fix version of json-smart transitive dependency for third parties

[mstruk]

Proper fix for #265

[scholzj]

Should we keep it in the dependency management in the main pom and just reference it in the oauth-commoon pom?

[mstruk]

Should we keep it in the dependency management in the main pom and just reference it in the oauth-commoon pom?

I think that keeping it in the dependencyManagement of the main gives a false impression that this is part of the fix, when it is not. It would obfuscate the fix. oauth-common is the only place that pulls in json-path and transitively json-smart. If there were multiple users of json-path then maybe an alternative would be to move the json-path dependency as a oauth main project dependency (in dependencies rather than in managedDependencies). As we learned managedDependencies section is ineffective in overriding the versions of transitive dependencies.

[scholzj]

Should we keep it in the dependency management in the main pom and just reference it in the oauth-commoon pom?

I think that keeping it in the dependencyManagement of the main gives a false impression that this is part of the fix, when it is not. It would obfuscate the fix. oauth-common is the only place that pulls in json-path and transitively json-smart. If there were multiple users of json-path then maybe an alternative would be to move the json-path dependency as a oauth main project dependency (in dependencies rather than in managedDependencies). As we learned managedDependencies section is ineffective in overriding the versions of transitive dependencies.

I'm not sure it is ineffective. I think it will be effective, but you have to use the dependency defined in the dependency management in the right place. That would correspond to how we use it in the other projects.

I do not have strong feelings about the use of the dependency management. But I think you should either handle all dependencies through it or you should get rid of it completely.

[mstruk]

I take your point about the consistency - e.g. having things listed. But then you might argue that we list things to the extent that they have an effect. You expect for dependencies within dependencyManagement to have an effect down the whole tree and such an effect is there only for direct dependencies, not for transitive dependencies. By that logic json-smart doesn't belong there. We only made json-smart the direct dependency temporarily in order to provide this fix, and putting it also into dependencyManagement section is redundant for that purpose. As an interim measure I prefer to not put it in dependencyManagement.

Ideally there was some maven plugin that flagged a warning about having a dependency in dependencyManagement that is not present anywhere in the modules tree under dependencies as I don't see a case where such an inconsistency would be desirable, but that's a different issue - it might help us avoid this mess with inadequate fix.

[scholzj]

My understanding is that the dependency management defines the versions of the dependencies you use. And you have to define in the actual child poms where exactly you want it to be used. And you did not use this module anywhere directly in 0.16.1. That is why it had no effect.

So from my point of view, you do not need to change the main pom.xml. You just need to add this to the poms where you want it to be used:

        <!-- Transitive override to address CVE-2024-57699. Remove in the future. -->
        <dependency>
            <groupId>net.minidev</groupId>
            <artifactId>json-smart</artifactId>
        </dependency>

(Notice the version is defined only in the main pom.xml)

[mstruk]

If I understand correctly what you wrote is exactly the current solution 😄

We can add the dependency to the dependencyManagement section of the main pom.xml as you suggested, but it is operationally a noop - redundant. We can add it for consistency sake as you say. I'm just pointing out that it's redundant. But if you really hate it that the lack of it violates the consistency, then sure, we can also add the dependencyManagement section, it's no big deal.

[scholzj]

That is not the current solution - neither the one from 0.16.1 nor the one from this PR.

[mstruk]

If I understand correctly what you wrote is exactly the current solution 😄

No, I didn't understand it correctly. I see what you mean. Ufff we're running in circles here about an trivial thing. Let me update the PR as it's going to be faster.

[scholzj]

Right - with the last commit - that is what I meant and how I believe we do it for example in the operators repo.

[ppatierno]

So ... by adding the json-smart directly in the root pom.xml here https://github.com/strimzi/strimzi-kafka-oauth/blob/main/pom.xml#L212 has the effect to bring the desired json-smart version 2.5.2 but only when compiling the oauth project itself because everything is resolved correctly: if you do a mvn dependency tree you should see the json-smart 2.5.2 under the oauth common module. It works and its consistent if your project is multi module but will never be used as dependency of something else but it works on its own (i.e. operator).
The issue comes because oauth is used as dependency in another project and the dependency management doesn't work the way I was expected (for transitive deps as well).
In this case yes, we need json-smart added directly to oauth common as well. I would also leave in the dependency management root pom to have a central place where putting the desired version, otherwise you should declare the version within oauth common itself: it would work anyway. Both solutions would work.

[mstruk]

@scholzj Approved?

[scholzj]

Sure, approved assuming it actually works as well 😉

[ppatierno]

Sure, approved assuming it actually works as well 😉

I tested it already but checking out the Marko's branch and installing that oauth version (1.0.0-SNAPSHOT) then used within the bridge locally. It's bringing the right json-smart and the CVE is not detected.
I will re-do the test anyway with the 0.16.2 RC1 out.