MIGRATIONS-1296 - Fix mend security issues #293

okhasawn · 2023-09-01T15:17:51Z

Description

This PR attempts to fix some of the currently existing mend security issues in the repo.
These are the issues it attempts to fix at the moment (more will follow soon):
-#251
-#247
-#246
-#226
-#227

Issues Resolved

Migrations-1296

Check List

New functionality includes testing
- All tests pass, including unit test, integration test and doctest
New functionality has been documented
Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Omar Khasawneh <[email protected]>

codecov · 2023-09-01T15:23:27Z

Codecov Report

Merging #293 (a708c59) into main (d5444e5) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##               main     #293   +/-   ##
=========================================
  Coverage     62.14%   62.14%           
  Complexity      619      619           
=========================================
  Files            82       82           
  Lines          3141     3141           
  Branches        292      292           
=========================================
  Hits           1952     1952           
  Misses         1013     1013           
  Partials        176      176

Flag	Coverage Δ
unittests	`62.14% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Signed-off-by: Omar Khasawneh <[email protected]>

okhasawn · 2023-09-01T18:06:33Z

This also fixes 2 of 3 vulnerabilities from this issue: #205.

The one left (jtidy) is still not fixed (at least still shows up as a CVE) in the latest version, as mentioned here: jtidy/jtidy#63

okhasawn · 2023-09-01T19:00:24Z

Also fixes 3 out of 4 vulns from issue #126
The only one left is because we're using bcel-6.5.0.jar, I've updated that to use bcel-6.7.0.jar, which is the suggested fix, and what fixed the CVE locally. I ran ./gradlew trafficReplayer:dependencyCheckAnalyze and it was no longer showing up compared to version 6.5.

Signed-off-by: Omar Khasawneh <[email protected]>

lewijacn · 2023-09-01T21:49:32Z

TrafficCapture/trafficReplayer/build.gradle


    testImplementation project(':testUtilities')
    testImplementation group: 'org.apache.httpcomponents.client5', name: 'httpclient5', version: '5.2.1'
    testImplementation 'org.mockito:mockito-core:4.6.1'
    testImplementation 'org.mockito:mockito-junit-jupiter:4.6.1'
 }

+configurations.all {
+    resolutionStrategy.eachDependency { DependencyResolveDetails details ->
+        if (details.requested.group == 'org.apache.commons' && details.requested.name == 'commons-text') {


From mine and Omar's conversation earlier: I have a slight concern here about maintaining this in the future, say for instance the dependency which has this transitive dependency gets updated and in turn updates this transitive dependency to a later version than we have listed here (1.10.0). I don't want to actually prevent a later version from being used. If its a small fix we should add a conditional check here that the dependency is less than the version we want to insert, or otherwise don't override the version. If not a small fix let's raise an issue around this

Thanks Tanner, that's a very valid concern and I agree that it wouldn't be dev friendly to have to maintain that.
Fortunately, I just pushed a change where I'm specifying a minimum version and check if the dependency wants to pull a higher version of the transitive dependency. If that's the case, then the higher version would be used. In case the dependency was requesting a version lower than the one we're "targeting", then the targeted version would be used.

I was playing around with this a little bit and it seems like it is just doing string comparison and not actual version comparison. For instance if I set the target version to 1.9.0 and the incoming requested is 1.10.0, it will try to set the incoming requested to 1.9.0. Lmk if you see different results with this use case.

Oh interesting.
What I tested before pushing that was with the apache.bcel dependency. I know that the requested version is 6.5, If I set the target to 6.4 then the 6.5 one is pulled.
I'll look into that and see if there's a different method of comparison for this.

I'm thinking of doing something with string parsing and compare the versions manually, I think it can be helpful in case we have more situations with transitive dependency versions in the future. What do you think?

Please let me know what you think of the proposed method I just pushed.

Yeah I think this in more in line with what we need to do. I would change the function you created to return a boolean and call it something like isRequestedVersionOlder

Few additional comments to capture in another JIRA:

You can make your new function static

Your logic for the function seems to be reversed of what I think of from the method name, I would expect if the requested part is less than the target part that it is an older version. Maybe this name was a bit misleading too, a bit wordy but we could add some clarity with wasRequestedVersionReleasedBeforeTargetVersion

With the previous change, can you make your if statement simply, without the boolean check
if (isRequestedVersionOlder(details.requested.version, targetVersion))

recommendation Signed-off-by: Omar Khasawneh <[email protected]>

…the right version of the dependency Signed-off-by: Omar Khasawneh <[email protected]>

Signed-off-by: Omar Khasawneh <[email protected]>

…s into mend-fixes

lewijacn · 2023-09-06T01:27:49Z

TrafficCapture/trafficReplayer/build.gradle


    testImplementation project(':testUtilities')
    testImplementation group: 'org.apache.httpcomponents.client5', name: 'httpclient5', version: '5.2.1'
    testImplementation 'org.mockito:mockito-core:4.6.1'
    testImplementation 'org.mockito:mockito-junit-jupiter:4.6.1'
 }

+configurations.all {
+    resolutionStrategy.eachDependency { DependencyResolveDetails details ->
+        if (details.requested.group == 'org.apache.commons' && details.requested.name == 'commons-text') {


Few additional comments to capture in another JIRA:

You can make your new function static

Your logic for the function seems to be reversed of what I think of from the method name, I would expect if the requested part is less than the target part that it is an older version. Maybe this name was a bit misleading too, a bit wordy but we could add some clarity with wasRequestedVersionReleasedBeforeTargetVersion

With the previous change, can you make your if statement simply, without the boolean check
if (isRequestedVersionOlder(details.requested.version, targetVersion))

MIGRATIONS-1296 - Fix mend security issues - part 1

a3c342e

Signed-off-by: Omar Khasawneh <[email protected]>

okhasawn requested review from chelma, gregschohn, kartg, lewijacn, mikaylathompson and sumobrian as code owners September 1, 2023 15:17

okhasawn added 4 commits September 1, 2023 10:28

Update checkstyle version to latest

235cd49

Signed-off-by: Omar Khasawneh <[email protected]>

fix aws-msk-iam-auth related mend issue

0f97945

Signed-off-by: Omar Khasawneh <[email protected]>

actually update spotbugs

cf7711a

Signed-off-by: Omar Khasawneh <[email protected]>

apache related mend fixes

293bb8d

Signed-off-by: Omar Khasawneh <[email protected]>

okhasawn added 4 commits September 1, 2023 15:53

upgrading aws-cdk-lib version in hopes of fixing mend issue

3dc7b38

Signed-off-by: Omar Khasawneh <[email protected]>

upgrading aws-cdk-lib version in hopes of fixing mend issue

076aa9c

Signed-off-by: Omar Khasawneh <[email protected]>

reverting aws-cdk-lib upgrade because it didn't fix mend issue

70ec837

Signed-off-by: Omar Khasawneh <[email protected]>

removing unnecessary repo in gradle file

f34a25e

Signed-off-by: Omar Khasawneh <[email protected]>

lewijacn reviewed Sep 1, 2023

View reviewed changes

specify a minimum version for some transitive dependencies per @lewijacn

423ae11

recommendation Signed-off-by: Omar Khasawneh <[email protected]>

okhasawn changed the title ~~MIGRATIONS-1296 - Fix mend security issues - part 1~~ MIGRATIONS-1296 - Fix mend security issues Sep 1, 2023

okhasawn and others added 4 commits September 4, 2023 22:12

add string parsing to compare version manually and accurately choose …

dc9ded4

…the right version of the dependency Signed-off-by: Omar Khasawneh <[email protected]>

updated version comparison function

a875b38

Signed-off-by: Omar Khasawneh <[email protected]>

Merge branch 'main' into mend-fixes

fa29cef

Merge branch 'mend-fixes' of github.com:okhasawn/opensearch-migration…

a708c59

…s into mend-fixes

lewijacn approved these changes Sep 6, 2023

View reviewed changes

sumobrian merged commit bcc4998 into opensearch-project:main Sep 6, 2023
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MIGRATIONS-1296 - Fix mend security issues #293

MIGRATIONS-1296 - Fix mend security issues #293

okhasawn commented Sep 1, 2023 •

edited

Loading

codecov bot commented Sep 1, 2023 •

edited

Loading

okhasawn commented Sep 1, 2023

okhasawn commented Sep 1, 2023

lewijacn Sep 1, 2023

okhasawn Sep 1, 2023

lewijacn Sep 2, 2023

okhasawn Sep 2, 2023

okhasawn Sep 2, 2023

okhasawn Sep 5, 2023

lewijacn Sep 5, 2023

lewijacn Sep 6, 2023

lewijacn Sep 6, 2023

MIGRATIONS-1296 - Fix mend security issues #293

MIGRATIONS-1296 - Fix mend security issues #293

Conversation

okhasawn commented Sep 1, 2023 • edited Loading

Description

Issues Resolved

Check List

codecov bot commented Sep 1, 2023 • edited Loading

Codecov Report

okhasawn commented Sep 1, 2023

okhasawn commented Sep 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

okhasawn commented Sep 1, 2023 •

edited

Loading

codecov bot commented Sep 1, 2023 •

edited

Loading