Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

MIGRATIONS-1296 - Fix mend security issues #293

Merged
merged 14 commits into from
Sep 6, 2023
Merged

MIGRATIONS-1296 - Fix mend security issues #293

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
a3c342e
MIGRATIONS-1296 - Fix mend security issues - part 1
okhasawn Sep 1, 2023
235cd49
Update checkstyle version to latest
okhasawn Sep 1, 2023
0f97945
fix aws-msk-iam-auth related mend issue
okhasawn Sep 1, 2023
cf7711a
actually update spotbugs
okhasawn Sep 1, 2023
293bb8d
apache related mend fixes
okhasawn Sep 1, 2023
3dc7b38
upgrading aws-cdk-lib version in hopes of fixing mend issue
okhasawn Sep 1, 2023
076aa9c
upgrading aws-cdk-lib version in hopes of fixing mend issue
okhasawn Sep 1, 2023
70ec837
reverting aws-cdk-lib upgrade because it didn't fix mend issue
okhasawn Sep 1, 2023
f34a25e
removing unnecessary repo in gradle file
okhasawn Sep 1, 2023
423ae11
specify a minimum version for some transitive dependencies per @lewij…
okhasawn Sep 1, 2023
dc9ded4
add string parsing to compare version manually and accurately choose …
okhasawn Sep 5, 2023
a875b38
updated version comparison function
okhasawn Sep 5, 2023
fa29cef
Merge branch 'main' into mend-fixes
sumobrian Sep 5, 2023
a708c59
Merge branch 'mend-fixes' of github.com:okhasawn/opensearch-migration…
okhasawn Sep 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion TrafficCapture/buildSrc/src/main/groovy/org/opensearch/migrations/common/CommonUtils.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ class CommonConfigurations {
static void applyCommonConfigurations(Project project) {
project.configurations.all {
resolutionStrategy.dependencySubstitution {
substitute module('org.apache.xmlgraphics:batik-codec') using module('org.apache.xmlgraphics:batik-all:1.15')
substitute module('org.apache.xmlgraphics:batik-codec') using module('org.apache.xmlgraphics:batik-all:1.17')
}
}
}
Expand Down
4 changes: 2 additions & 2 deletions TrafficCapture/captureKafkaOffloader/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@ dependencies {
implementation project(':captureOffloader')
implementation 'org.projectlombok:lombok:1.18.26'
implementation 'com.google.protobuf:protobuf-java:3.22.2'
implementation 'org.apache.kafka:kafka-clients:3.5.0'
implementation 'software.amazon.msk:aws-msk-iam-auth:1.1.7'
implementation 'org.apache.kafka:kafka-clients:3.5.1'
implementation 'software.amazon.msk:aws-msk-iam-auth:1.1.9'
implementation 'org.slf4j:slf4j-api:2.0.7'
testImplementation project(':captureProtobufs')
testImplementation 'org.mockito:mockito-core:4.6.1'
Expand Down
8 changes: 5 additions & 3 deletions TrafficCapture/testUtilities/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ spotbugs {
}

checkstyle {
toolVersion = '10.9.3'
toolVersion = '10.12.3'
configFile = new File(rootDir, 'config/checkstyle/checkstyle.xml')
System.setProperty('checkstyle.cache.file', String.format('%s/%s',
buildDir, 'checkstyle.cachefile'))
Expand All @@ -40,12 +40,14 @@ repositories {
}

dependencies {
spotbugs 'com.github.spotbugs:spotbugs:4.7.3'

implementation group: 'com.fasterxml.jackson.core', name: 'jackson-databind', version: '2.15.0'
implementation group: 'com.google.guava', name: 'guava', version: '32.0.1-jre'
implementation group: 'io.netty', name: 'netty-all', version: '4.1.89.Final'
implementation group: 'org.apache.httpcomponents.client5', name: 'httpclient5', version: '5.2.1'
implementation group: 'org.bouncycastle', name: 'bcprov-jdk15on', version: '1.68'
implementation group: 'org.bouncycastle', name: 'bcpkix-jdk15on', version: '1.68'
implementation group: 'org.bouncycastle', name: 'bcprov-jdk18on', version: '1.74'
implementation group: 'org.bouncycastle', name: 'bcpkix-jdk18on', version: '1.74'
implementation group: 'org.projectlombok', name: 'lombok', version: '1.18.22'
implementation group: 'org.slf4j', name: 'slf4j-api', version: '2.0.7'

Expand Down
50 changes: 41 additions & 9 deletions TrafficCapture/trafficReplayer/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ buildscript {
plugins {
id 'org.opensearch.migrations.java-application-conventions'
id "com.github.spotbugs" version "4.7.3"
// id 'checkstyle'
id 'checkstyle'
id 'org.owasp.dependencycheck' version '8.2.1'
id "io.freefair.lombok" version "8.0.1"
}
Expand All @@ -28,18 +28,20 @@ spotbugs {
includeFilter = new File(rootDir, 'config/spotbugs/spotbugs-include.xml')
}

//checkstyle {
// toolVersion = '10.9.3'
// configFile = new File(rootDir, 'config/checkstyle/checkstyle.xml')
// System.setProperty('checkstyle.cache.file', String.format('%s/%s',
// buildDir, 'checkstyle.cachefile'))
//}
checkstyle {
toolVersion = '10.12.3'
configFile = new File(rootDir, 'config/checkstyle/checkstyle.xml')
System.setProperty('checkstyle.cache.file', String.format('%s/%s',
buildDir, 'checkstyle.cachefile'))
}

repositories {
mavenCentral()
}

dependencies {
spotbugs 'com.github.spotbugs:spotbugs:4.7.3'

implementation project(':captureProtobufs')

implementation 'software.amazon.awssdk:sdk-core:2.20.102'
Expand All @@ -55,19 +57,49 @@ dependencies {
implementation group: 'org.json', name: 'json', version: '20230227'
implementation group: 'org.projectlombok', name: 'lombok', version: '1.18.22'

implementation group: 'org.apache.kafka', name: 'kafka-clients', version: '3.5.0'
implementation group: 'org.apache.kafka', name: 'kafka-clients', version: '3.5.1'
implementation group: 'org.apache.logging.log4j', name: 'log4j-api', version: '2.20.0'
implementation group: 'org.apache.logging.log4j', name: 'log4j-core', version: '2.20.0'
implementation group: 'org.apache.logging.log4j', name: 'log4j-slf4j2-impl', version: '2.20.0'
implementation group: 'org.slf4j', name: 'slf4j-api', version: '2.0.7'
implementation group: 'software.amazon.msk', name: 'aws-msk-iam-auth', version: '1.1.7'
implementation group: 'software.amazon.msk', name: 'aws-msk-iam-auth', version: '1.1.9'

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'
}

def isRequestedVersionOlder(String requested, String target) {
def requestedParts = requested.split('\\.')*.toInteger()
def targetParts = target.split('\\.')*.toInteger()

for (int i = 0; i < 3; i++) {
if (requestedParts[i] < targetParts[i]) {
return false
} else if (requestedParts[i] > targetParts[i]) {
return true
}
}
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few additional comments to capture in another JIRA:

  1. You can make your new function static
  2. 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
  3. With the previous change, can you make your if statement simply, without the boolean check
    if (isRequestedVersionOlder(details.requested.version, targetVersion))

def targetVersion = '1.10.0'
if (isRequestedVersionOlder(details.requested.version, targetVersion) != true) {
details.useVersion targetVersion
}
}
if (details.requested.group == 'org.apache.bcel' && details.requested.name == 'bcel') {
def targetVersion = '6.7.0'
if (isRequestedVersionOlder(details.requested.version, targetVersion) != true) {
details.useVersion targetVersion
}
}
}
}

application {
mainClass = 'org.opensearch.migrations.replay.TrafficReplayer'
}
Expand Down
2 changes: 1 addition & 1 deletion test/requirements.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
certifi==2023.5.7
certifi==2023.7.22
charset-normalizer==3.1.0
idna==3.4
iniconfig==2.0.0
Expand Down
Loading