[artifact-manager] (#331) Fix CRITICAL and HIGH level vulnerabilities #420

bcpmihail · 2024-09-17T17:17:47Z

Description

Trivy identified critical and high level vulnerabilities in the spring framework and snakeyaml dependencies for artifact-manager.
These were resolved by using the latest versions of com.fasterxml.jackson (of which snakeyaml is a transitive dependency) and org.springframework; this required upgrading org.apache.httpcomponents to 5.3, affecting:

HttpClient initialization
usage of HttpStatus
BasicAuthorizationInterceptor (replaced by BasicAuthenticationInterceptor)
handling of URISyntaxException and ConfigurationException

After the upgrade, no more critical or high level vulnerabilities were detected by trivy.

Checklist

I have added relevant error handling and logging messages to help troubleshooting
I have added tests that prove my fix is effective or that my feature works
I have added necessary documentation, relevant usage information (if applicable)
I have updated the PR title with affected component, related issue number and a short summary of the changes introduced
I have added labels for implementation kind (kind/) and version type (version/)
I have tested against live environment, if applicable
I have synced any structure and/or content vRA-NG improvements with vra-ng and ts-vra-ng archetypes (if applicable)
I have my changes rebased and squashed to the minimal number of relevant commits. Notice: don't squash all commits
I have added a descriptive commit message with a short title, including a Fixed #XXX - or Closed #XXX - prefix to auto-close the issue

Testing

Builds and deploys test project successfully to environment.

Related issues and PRs

#331

common/artifact-manager/src/main/java/com/vmware/pscoe/iac/artifact/rest/RestClientVrops.java

vmwclabot · 2024-09-17T17:17:51Z

@bcpmihail, you must sign every commit in this pull request acknowledging our Developer Certificate of Origin before your changes are merged. This can be done by adding Signed-off-by: John Doe <[email protected]> to the last line of each Git commit message. The e-mail address used to sign must match the e-mail address of the Git author. Click here to view the Developer Certificate of Origin agreement.

Commit 71cfc57 must be signed by [email protected]
Commit bb7bca0 must be signed by [email protected]

vmwclabot · 2024-09-17T17:17:51Z

@bcpmihail, you must sign every commit in this pull request acknowledging our Developer Certificate of Origin before your changes are merged. This can be done by adding Signed-off-by: John Doe <[email protected]> to the last line of each Git commit message. The e-mail address used to sign must match the e-mail address of the Git author. Click here to view the Developer Certificate of Origin agreement.

Commit 71cfc57 must be signed by [email protected]
Commit bb7bca0 must be signed by [email protected]

Michaelpalacce · 2024-09-18T12:03:27Z

Wow, so this is great effort and a great job. Honestly, The only problem is that there are so many irrelevant changes in the form of convention and formatting, that the important ones get easily lost. I am not sure I can do a good review of this :/

Tests are passing, and they are unchanged, outside some imports, so that is good and pretty much good enough. Not sure why but again no matter what IDE I use Java dev on BTVA is.... something else... Trying to resolve that and will continue my efforts

bcpmihail · 2024-09-18T12:12:40Z

Wow, so this is great effort and a great job. Honestly, The only problem is that there are so many irrelevant changes in the form of convention and formatting, that the important ones get easily lost. I am not sure I can do a good review of this :/

Tests are passing, and they are unchanged, outside some imports, so that is good and pretty much good enough. Not sure why but again no matter what IDE I use Java dev on BTVA is.... something else... Trying to resolve that and will continue my efforts

Shall I overwrite the git history to separate the style changes (formatting) from the rest for better readability? You'll need to review commit by commit afterwards.

bcpmihail · 2024-09-18T12:12:44Z

Currently addressing the codeql issues (mainly adding Javadoc comments)

Michaelpalacce

Haven't looked fully at the primitives and the clients, but let's fix the Config part, and we'll worry about that later. If you agree, of course.

Michaelpalacce · 2024-09-18T12:31:13Z

maven/plugins/vrealize/src/main/java/com/vmware/pscoe/maven/plugins/PushMojo.java

-		Map<String, List<Artifact>> artifactsByType = allArtifacts.stream().collect(Collectors.groupingBy(Artifact::getType));
-
+	private void importArtifacts(Collection<Artifact> allArtifacts)
+			throws MojoExecutionException, ConfigurationException {


God, this ConfigurationException seems like a complete mess... Can't we do ConfigurationException extends RuntimException and be done with it?

Michaelpalacce · 2024-09-18T12:32:32Z

maven/plugins/vrealize/src/main/java/com/vmware/pscoe/maven/plugins/PushMojo.java

 				PackageStore<?> store = getConfigurationForType(PackageType.fromExtension(type.getKey()))
-						.flatMap(configuration -> Optional.of(PackageStoreFactory.getInstance(configuration))).orElseThrow(() -> new ConfigurationException(
-								"Unable to find PackageStore based on configuration. Make sure there is configuration for type: " + pkgType.name()));
+						.flatMap(configuration -> PackageStoreFactory.getInstanceWrapped(configuration))


I get it, the whole idea behind getInstanceWrapped. TBF, I prefer it (using RuntimeException), :D Can't we do it everywhere?

I am just desperate to clean up this "Error Propagation" or whatever you want to call it

Michaelpalacce · 2024-09-18T12:42:13Z

common/artifact-manager/src/main/java/com/vmware/pscoe/iac/artifact/PackageStoreFactory.java

+	 * @param <T>           The configuration type.
+	 * @return The PackageStore instance as Optional.
+	 */
+	public static <T extends Configuration> Optional<PackageStore<?>> getInstanceWrapped(T configuration) {


Cool change!

Michaelpalacce · 2024-09-18T12:44:51Z

Also forgot to mention. Let's ignore the linter. Let it complain. Last thing we want is more to this PR.

Signed-off-by: Mihail Penchev (c) <[email protected]>

Using latest springframework, jackson versions in artifact-manager. Upgraded apache httcomponents to httpcore5 5.3 and httpclient5 5.3. Signed-off-by: Mihail Penchev (c) <[email protected]>

Signed-off-by: Mihail Penchev (c) <[email protected]>

…3 and httpclient5 5.3.1 Minor changes to accomodate the new APIs. Signed-off-by: Mihail Penchev (c) <[email protected]>

Signed-off-by: Mihail Penchev (c) <[email protected]>

bcpmihail · 2024-09-18T17:53:09Z

Also forgot to mention. Let's ignore the linter. Let it complain. Last thing we want is more to this PR.

Changed ConfigurationException to inherit from RuntimeException.
Removed unnecessary changes in error handling.
Edited git history to format all affected files in a single commit - easy to see actual changes in subsequent commits.
Temporarily disabled linter checks.
Will need help to temporarily comment out last 2 rows in codeql-java.yml.
Will open additional PR after merging this one to revert the temporary changes.

Ignored checks for Javadoc comments, DesignForExtension Signed-off-by: Mihail Penchev (c) <[email protected]>

bcpmihail · 2024-09-19T13:50:33Z

Addressed linter issues* and reverted changes to sun_checks.xml.
* except Javadocs/DesignForExtension - they are unrelated to the changes and too many. As discussed with @Michaelpalacce, the PR can be force-merged without fixing them.

Michaelpalacce · 2024-09-19T13:59:54Z

Can you sync with master @bcpmihail.

vmwclabot · 2024-09-19T14:28:34Z

@bcpmihail, you must sign every commit in this pull request acknowledging our Developer Certificate of Origin before your changes are merged. This can be done by adding Signed-off-by: John Doe <[email protected]> to the last line of each Git commit message. The e-mail address used to sign must match the e-mail address of the Git author. Click here to view the Developer Certificate of Origin agreement.

Commit b279c10 must be signed by [email protected]

vmwclabot · 2024-09-19T14:28:34Z

@bcpmihail, you must sign every commit in this pull request acknowledging our Developer Certificate of Origin before your changes are merged. This can be done by adding Signed-off-by: John Doe <[email protected]> to the last line of each Git commit message. The e-mail address used to sign must match the e-mail address of the Git author. Click here to view the Developer Certificate of Origin agreement.

Commit b279c10 must be signed by [email protected]

Signed-off-by: Mihail Penchev (c) <[email protected]>

bcpmihail · 2024-09-19T14:32:49Z

Can you sync with master @bcpmihail.

@Michaelpalacce Done

bcpmihail added lang/java Related to Java Code area/artifact-manager Relates to the `artifact-manager` maven module version/minor Introduces a non-breaking feature or change kind/feature New Feature to the project kind/security Security changes labels Sep 17, 2024

bcpmihail requested review from joroaf, Michaelpalacce and VenelinBakalov September 17, 2024 17:17

bcpmihail self-assigned this Sep 17, 2024

bcpmihail requested a review from a team as a code owner September 17, 2024 17:17

bcpmihail linked an issue Sep 17, 2024 that may be closed by this pull request

Fix Critical And High vulnerabilities that are resolved for artifact-manager #331

Closed

github-advanced-security bot found potential problems Sep 17, 2024

View reviewed changes

common/artifact-manager/src/main/java/com/vmware/pscoe/iac/artifact/rest/RestClientVrops.java Dismissed Show resolved Hide resolved

vmwclabot added the dco-required label Sep 17, 2024

github-actions bot added the kind/bug Something isn't working label Sep 17, 2024

bcpmihail force-pushed the feature/331-fix-trivy-vulnerabilities branch from 6419fc2 to f7a845e Compare September 17, 2024 17:23

vmwclabot removed the dco-required label Sep 17, 2024

Michaelpalacce requested changes Sep 18, 2024

View reviewed changes

bcpmihail added 6 commits September 18, 2024 20:03

style: [331] Formatting of affected files

a83727d

Signed-off-by: Mihail Penchev (c) <[email protected]>

chore: [331] upgraded artifact-manager pom.xml

98ffc0e

Using latest springframework, jackson versions in artifact-manager. Upgraded apache httcomponents to httpcore5 5.3 and httpclient5 5.3. Signed-off-by: Mihail Penchev (c) <[email protected]>

feat: [331] ConfigurationException to extend RuntimeException

9134544

Signed-off-by: Mihail Penchev (c) <[email protected]>

feat: [331] upgraded imports for apache httcomponents to httpcore5 5.…

02e360e

…3 and httpclient5 5.3.1 Minor changes to accomodate the new APIs. Signed-off-by: Mihail Penchev (c) <[email protected]>

feat: [331] RestClientFactory httpclient5

3d4a36d

Signed-off-by: Mihail Penchev (c) <[email protected]>

docs: [331] Updated release.md

5ea09b4

Signed-off-by: Mihail Penchev (c) <[email protected]>

bcpmihail force-pushed the feature/331-fix-trivy-vulnerabilities branch from f7a845e to 0a5efee Compare September 18, 2024 17:46

bcpmihail force-pushed the feature/331-fix-trivy-vulnerabilities branch from 0a5efee to 6dde3d1 Compare September 18, 2024 17:51

bcpmihail force-pushed the feature/331-fix-trivy-vulnerabilities branch from 6dde3d1 to 7c050b1 Compare September 19, 2024 06:26

fix: [331] Addressed linter issues

a6b49cb

Ignored checks for Javadoc comments, DesignForExtension Signed-off-by: Mihail Penchev (c) <[email protected]>

bcpmihail force-pushed the feature/331-fix-trivy-vulnerabilities branch from 2c648cc to a6b49cb Compare September 19, 2024 07:32

vmwclabot added dco-required labels Sep 19, 2024

chore: merged origin/main into feature/331-fix-trivy-vulnerabilities

42dea9d

Signed-off-by: Mihail Penchev (c) <[email protected]>

bcpmihail force-pushed the feature/331-fix-trivy-vulnerabilities branch from b279c10 to 42dea9d Compare September 19, 2024 14:29

vmwclabot removed the dco-required label Sep 19, 2024

Michaelpalacce merged commit 044b59a into main Sep 20, 2024
13 of 17 checks passed

VenelinBakalov added kind/enhancement Enhancement or improvement of existing features and removed kind/bug Something isn't working kind/feature New Feature to the project labels Sep 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[artifact-manager] (#331) Fix CRITICAL and HIGH level vulnerabilities #420

[artifact-manager] (#331) Fix CRITICAL and HIGH level vulnerabilities #420

bcpmihail commented Sep 17, 2024

vmwclabot commented Sep 17, 2024

vmwclabot commented Sep 17, 2024

Michaelpalacce commented Sep 18, 2024

bcpmihail commented Sep 18, 2024

bcpmihail commented Sep 18, 2024

Michaelpalacce left a comment

Michaelpalacce Sep 18, 2024

Michaelpalacce Sep 18, 2024

Michaelpalacce Sep 18, 2024

Michaelpalacce Sep 18, 2024

Michaelpalacce commented Sep 18, 2024

bcpmihail commented Sep 18, 2024

bcpmihail commented Sep 19, 2024 •

edited

Loading

Michaelpalacce commented Sep 19, 2024

vmwclabot commented Sep 19, 2024

vmwclabot commented Sep 19, 2024

bcpmihail commented Sep 19, 2024

[artifact-manager] (#331) Fix CRITICAL and HIGH level vulnerabilities #420

[artifact-manager] (#331) Fix CRITICAL and HIGH level vulnerabilities #420

Conversation

bcpmihail commented Sep 17, 2024

Description

Checklist

Testing

Related issues and PRs

vmwclabot commented Sep 17, 2024

vmwclabot commented Sep 17, 2024

Michaelpalacce commented Sep 18, 2024

bcpmihail commented Sep 18, 2024

bcpmihail commented Sep 18, 2024

Michaelpalacce left a comment

Choose a reason for hiding this comment

Michaelpalacce Sep 18, 2024

Choose a reason for hiding this comment

Michaelpalacce Sep 18, 2024

Choose a reason for hiding this comment

Michaelpalacce Sep 18, 2024

Choose a reason for hiding this comment

Michaelpalacce Sep 18, 2024

Choose a reason for hiding this comment

Michaelpalacce commented Sep 18, 2024

bcpmihail commented Sep 18, 2024

bcpmihail commented Sep 19, 2024 • edited Loading

Michaelpalacce commented Sep 19, 2024

vmwclabot commented Sep 19, 2024

vmwclabot commented Sep 19, 2024

bcpmihail commented Sep 19, 2024

bcpmihail commented Sep 19, 2024 •

edited

Loading