ASA 8404 (#218) #220

vishalhcl-5960 · 2024-06-25T16:33:59Z

Include-SCA implementation

mattmurp

I understand that a lot of the changes here were made in order to run 2 scans at once (i.e. call the perform method twice). However, there is a lot of if/else/if/else logic that's hard to follow and there are a lot of classes (e.g. AppScanBuildStep, ResultsRetriever) that now have specific knowledge of different types of scans.
One solution to these problems would be to create a new type of scan that will run both a SAST and SCA scan.

mattmurp · 2024-06-25T16:48:25Z

src/main/java/com/hcl/appscan/jenkins/plugin/scanners/Scanner.java

+	public boolean hasOptions() {
+		return m_hasOptions;
+	}
+


The methods getHasOptionsUploadDirect() and hasOptionsUploadDirect() do the same thing. Why do we have 2 methods? Also, upload direct is specific to SAST scans, so why are these methods being added to the Scanner base class that is used by DAST and SCA, as well?

Okay sure, added the "hasOptionsUploadDirect()" methods in the StaticAnalyzer class.

mattmurp · 2024-06-25T16:49:05Z

src/main/java/com/hcl/appscan/jenkins/plugin/scanners/Scanner.java

 	}

 	public boolean getHasOptions() {
 		return m_hasOptions;
 	}
+
+	public boolean hasOptions() {


This method returns the same thing as the existing method getHasOptions(). Why do we need it?

"hasOptions" was used as a checker method in the jelly files. Now, using the getHasOptions() itself in the jelly files too.

mattmurp · 2024-06-25T16:49:27Z

src/main/java/com/hcl/appscan/jenkins/plugin/scanners/Scanner.java

@@ -23,15 +23,29 @@ public abstract class Scanner extends AbstractDescribableImpl<Scanner> implement

 	private String m_target;
 	private boolean m_hasOptions;
+	private boolean m_hasOptionsUploadDirect;


Why are we adding a SAST specific property to the base Scanner class?

Moving this property to the StaticAnalyzer class.

mattmurp · 2024-06-28T13:14:29Z

src/main/java/com/hcl/appscan/jenkins/plugin/builders/AppScanBuildStep.java

@@ -235,11 +236,33 @@ public boolean getPersonalScan() {
    public DescriptorImpl getDescriptor() {
        return (DescriptorImpl)super.getDescriptor();
    }
+
+    //to execute SAST & SCA scans consecutively, 1st perform method call to execute SAST then after changing the scan type to SCA
+    private void includeSCAImplementation(AbstractBuild<?,?> build, Launcher launcher, BuildListener listener) throws IOException, InterruptedException {


These checks are all specific to StaticAnalyzer to see if we should only run SCA or if we should include SCA. I suggest moving all of these checks to the StaticAnalyzer.getProperties() method. If necessary, the getProperties() method can be overloaded with a version that takes in an IAuthenticationProvider for doing any checks that require it.

Moved the validations in the respective scan classes by adding an "validations()" method in the abstract class.

mattmurp · 2024-06-29T10:19:51Z

src/main/java/com/hcl/appscan/jenkins/plugin/builders/AppScanBuildStep.java

@@ -309,6 +338,8 @@ private void validations(boolean isAppScan360, Map<String, String> properties, I
        if(isAppScan360) {
            if (m_type.equals(Scanner.DYNAMIC_ANALYZER) && properties.containsKey(Scanner.PRESENCE_ID)) {
                throw new AbortException(Messages.error_presence_AppScan360());
+            } if(properties.containsKey(CoreConstants.INCLUDE_SCA)) {


Can all of these validations be moved to the getProperties() method of the individual scanners?

Moved the validations in the respective scan classes by adding an "validations()" method in the abstract class.

mattmurp · 2024-06-29T10:38:05Z

src/main/java/com/hcl/appscan/jenkins/plugin/results/ResultsInspector.java

@@ -48,4 +61,55 @@ private boolean exceedsThreshold(String type, int threshold) {
 			return false;
 		}
 	}
+


The additional code in this method requires the ResultsInspector to be aware of different types of scans. I suggest doing this differently. Some options would be:

Subclassing the ResultsInspector (e.g. create a new CombinedResultsInspector).

Using 2 ResultsInpector instances.

Creating a new type of IResultsProvider that can handle 2 sets of results.

Updating the existing NonCompliantIssuesResultProvider to handle 2 sets of results.

I have created a new class "CombinedResultsInspector" which is being extended from the base "ResultsInspector" class.

mattmurp · 2024-06-29T10:46:16Z

src/main/java/com/hcl/appscan/jenkins/plugin/builders/AppScanBuildStep.java

@@ -328,89 +364,122 @@ private void validations(boolean isAppScan360, Map<String, String> properties, I
            throw new AbortException(Messages.error_url_validation(target));
        }
    }
+
+    private String updatedScanTypesForBuildSummaryName(String scanType) {


Rather than having the AppScanBuildStep figure out this logic, I suggest adding a new method to the Scanner class. Something like getShortName() that returns these values.

If i add this method in the Scanner class from which i have to call this method in respective scan class. During the inlcudeSCA(SAST+SCA) execution, I have to call this method during runtime as i am utilizing the same properties in the SCA scan runtime.

I can define this method in the Utils class of the SDK.

mattmurp · 2024-06-29T10:52:28Z

src/main/java/com/hcl/appscan/jenkins/plugin/scanners/StaticAnalyzer.java

 		super(target, hasOptions);
                m_openSourceOnly=false;
                m_sourceCodeOnly=false;
                m_scanMethod=CoreConstants.CREATE_IRX;
                m_scanSpeed="";
-	}
+                m_includeSCAGenerateIRX="true";


Is there a reason we need both m_includeSCAGenerateIRX and m_includeSCAUploadDirect? Shouldn't there just be m_includeSCA and m_uploadDirect?

I think it is better to have two different names for these checkbox as the position of these checks are different in jelly file, it will hamper the value of the checkbox if one of the check is false.

mattmurp · 2024-07-11T17:27:40Z

src/main/java/com/hcl/appscan/jenkins/plugin/scanners/Scanner.java

@@ -38,7 +40,9 @@ public String getTarget() {
 	}

 	public abstract Map<String, String> getProperties(VariableResolver<String> resolver) throws AbortException;
-
+
+	public abstract void validations(IAuthenticationProvider authProvider, Map<String, String> properties, IProgress progress) throws AbortException;


The function name should be a verb, like "validate" or "validateSettings".

Okay, Now, Using the "validateSettings()" method name.

mattmurp · 2024-07-11T17:34:36Z

src/main/java/com/hcl/appscan/jenkins/plugin/scanners/Scanner.java

@@ -38,7 +40,9 @@ public String getTarget() {
 	}

 	public abstract Map<String, String> getProperties(VariableResolver<String> resolver) throws AbortException;
-
+
+	public abstract void validations(IAuthenticationProvider authProvider, Map<String, String> properties, IProgress progress) throws AbortException;


In all instances, the IAuthenticationProvider is being passed in and then cast to a JenkinsAuthenticationProvider just to check if this is AppScan 360. Why not just pass a boolean here instead?

In DynamicAnalyzer class we are doing the URL validity check & for that we need to call the getProxy() method from authProvider class. Instead of passing the IAuthenticationProvider, I will pass the JenkinsAuthenaticationProvider itself.

mattmurp · 2024-07-11T17:37:25Z

src/main/java/com/hcl/appscan/jenkins/plugin/scanners/DynamicAnalyzer.java

@@ -196,8 +198,18 @@ public String upgradeLoginScenario(){
 		}
 	}

+	public void validations(IAuthenticationProvider authProvider, Map<String, String> properties, IProgress progress) throws AbortException {
+		if (((JenkinsAuthenticationProvider) authProvider).isAppScan360()) {


The 2 "if" conditions should be "anded" together rather than nesting them.

okay, sure.

mattmurp · 2024-07-11T17:39:33Z

src/main/java/com/hcl/appscan/jenkins/plugin/results/ResultsInspector.java

@@ -10,14 +10,13 @@

 import com.hcl.appscan.sdk.results.IResultsProvider;

-public class ResultsInspector {
+public class ResultsInspector extends CombinedResultsInspector {


Why does the ResultsInspector extend the CombinedResultsInseptor? It seems this should be the other way around, where the CombinedResultsInspector adds to the logic of the ResultsInspector.

Removed the CombinedResultsInspector class as we are now handling the results in the SDK.

mattmurp · 2024-07-11T17:41:15Z

src/main/java/com/hcl/appscan/jenkins/plugin/results/CombinedResultsInspector.java

+import java.util.List;
+import java.util.Map;
+
+public class CombinedResultsInspector {


The results inspector should not require knowledge of the scan type or issue types (there are mentions of totalCountSAST for example).

Removed the CombinedResultsInspector class as we are now handling the results in the SDK.

mattmurp · 2024-07-12T15:08:51Z

src/main/java/com/hcl/appscan/jenkins/plugin/builders/AppScanBuildStep.java

-	perform((Run<?,?>)build, launcher, listener);
-	        return true;
+
+    //to execute SAST & SCA scans consecutively, 1st perform method call to execute SAST then after changing the scan type to SCA


Rather than having all of these checks for SCA and SAST and rather than trying to execute the perform() method twice, we should create a new scanner type in the SDK that will execute both a SAST scan and an SCA scan. That would eliminate the need for most, if not all, of these changes.

SAST_SCA class had been made in the SDK to handle the SAST+SCA scans execution.

mattmurp · 2024-07-22T19:42:15Z

src/main/java/com/hcl/appscan/jenkins/plugin/actions/ScanResults.java

@@ -151,7 +151,7 @@ public File getReport() {
 	}

 	private String getReportName() {
-		String name = (getScanType() + FileUtil.getValidFilename(getName())).replaceAll(" ", ""); //$NON-NLS-1$ //$NON-NLS-2$
+		String name = (FileUtil.getValidFilename(getName())).replaceAll(" ", ""); //$NON-NLS-1$ //$NON-NLS-2$


The extra ( ) should be removed around the FileUtil.getValidFileName() call.

sure, Matt.

mattmurp · 2024-07-22T19:49:18Z

src/main/resources/com/hcl/appscan/jenkins/plugin/Messages.properties

-warning.sca= Note: AppScan on Cloud (ASoC) now performs SAST and SCA analysis as separate scans. To execute an open-source only scan, use the Software Composition Analysis (SCA) scan type. The open-source only option will be removed from SAST scans in a future release.
-error.url.dynamic.unsupported= Either your A360 instance does not support dynamic scans or the starting URL is invalid: {0}.
+warning.sca= AppScan on Cloud (ASoC) now performs SAST and SCA analysis as separate scans. To execute an open-source only scan, use the Software Composition Analysis (SCA) scan type.
+warning.include.sca.AppScan360= Only SAST scan execution takes place as SCA is applicable for AppScan on Cloud only.


Can you run these new messages by Jen?

I have reviewed the attached statement with Binoy, updating the warning message to "Only the SAST scan is being executed. SCA is only available with AppScan on Cloud and is not included in this job"

mattmurp · 2024-07-22T20:01:54Z

src/main/java/com/hcl/appscan/jenkins/plugin/scanners/StaticAnalyzer.java

@@ -28,31 +32,40 @@ public class StaticAnalyzer extends Scanner {
 	private static final String STATIC_ANALYZER = "Static Analyzer"; //$NON-NLS-1$

        private boolean m_openSourceOnly;
+        private String m_includeSCAGenerateIRX;
+        private boolean m_additionalOptionsUploadDirect;
+        private boolean m_includeSCAUploadDirect;


I think we can simplify this with shorter variable names and we only need 2 new member variables:

private boolean m_includeSCA; private boolean m_uploadDirect;

The value of m_includeSCAGenerateIRX is just m_includeSCA && !m_uploadDirect. Likewise, the value of m_includeSCAUploadDirect is just m_includeSCA && m_uploadDirect. So I don't think we need a third variable here.

Matt, Are you referring that we should use the same name for both the checkbox(under "generateIRX" & "upload Files & folders" scan menthod) in config file?

There is difference in the UI names for the both the checkboxes.

Third variable is for the value of the optionalBlock checkbox under the "upload files & folders" scan method. We can remove that optionalBlock as it contains only 1 element into it.

I have changed the variable name for the additional option under "upload Files & folders" scan method. New variable name would be "m_hasOptionsUploadDirect".
Also, changed the variable type of the "m_includeSCAUploadDirect" to String so that we have a parity between the both checkboxes of "includeSCA".

mattmurp

There are too many hard coded strings for "true" and "false". When checking for boolean values from strings, you should use Boolean.parseBoolean(stringValue) and for setting a string to "true" or "false" you should use Boolean.toString(booleanValue).

mattmurp · 2024-07-29T11:34:43Z

src/main/java/com/hcl/appscan/jenkins/plugin/scanners/StaticAnalyzer.java

+
+        @DataBoundSetter
+        public void setIncludeSCAGenerateIRX(String includeSCAGenerateIRX) {
+            if(getHasOptions() && includeSCAGenerateIRX.equals("true")) {


This can be simplified to:
return getHashOptions() && Boolean.parseBoolean(includeSCAGenerateIRX);

mattmurp · 2024-07-29T11:37:57Z

src/main/java/com/hcl/appscan/jenkins/plugin/scanners/StaticAnalyzer.java

+
+        @DataBoundSetter
+        public void setIncludeSCAUploadDirect(String includeSCAUploadDirect) {
+            if(m_hasOptionsUploadDirect && includeSCAUploadDirect.equals("true")) {


Should use Boolean.parseBoolean(string).

Okay, sure.

mattmurp

The StaticAnalyzer class needs to be cleaned up, but approving in order to get the release done.

src/main/java/com/hcl/appscan/jenkins/plugin/scanners/StaticAnalyzer.java

ASA 8404 (#218)

5a957ff

Include-SCA implementation

vishalhcl-5960 requested a review from mattmurp June 25, 2024 16:33

vishalhcl-5960 and others added 6 commits June 25, 2024 22:08

Merge branch 'master' into ASA-8404

a610cb5

Merge branch 'master' into ASA-8404

300cac0

modified the checker method

c60f1c4

Update AppScanBuildStep.java

408cf84

suggested changes

0bf9eac

Update AppScanBuildStep.java

c5883be

mattmurp reviewed Jun 29, 2024

View reviewed changes

vishalhcl-5960 added 6 commits July 5, 2024 12:15

suggested changes

a61f127

Update AppScanBuildStep.java

96e802a

handled the SCO & OSO upgradation use-cases

41ad20c

include SCA

bd2616b

bug fixes

236f7f2

indentation changes

dc400a0

mattmurp reviewed Jul 12, 2024

View reviewed changes

vishalhcl-5960 added 5 commits July 15, 2024 18:44

suggested changes

a2d52b1

migrated the changes for the include SCA to SDK

b1bcb55

removed the unwanted void spaces

10fa048

required changes

57ce2b8

copyright years update

14859aa

mattmurp reviewed Jul 22, 2024

View reviewed changes

vishalhcl-5960 added 2 commits July 24, 2024 14:30

suggested changes

bb5900b

ASA-8724

2a17e9f

mattmurp reviewed Jul 29, 2024

View reviewed changes

vishalhcl-5960 and others added 4 commits July 29, 2024 23:52

Addessing PR comments

1d9332f

Update StaticAnalyzer.java

ed7fc17

Update pom.xml

8d33f1d

Delete .idea/libraries/Maven__com_hcl_appscan_sdk_1_1_3_SNAPSHOT.xml

db9021c

mattmurp approved these changes Jul 29, 2024

View reviewed changes

github-advanced-security bot found potential problems Jul 29, 2024

View reviewed changes

vishalhcl-5960 merged commit 06a676d into master Jul 30, 2024
3 checks passed

vishalhcl-5960 deleted the ASA-8404 branch July 30, 2024 04:32

ASA 8404 (#218) #220

ASA 8404 (#218) #220

Conversation

vishalhcl-5960 commented Jun 25, 2024

mattmurp left a 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

vishalhcl-5960 Jul 3, 2024 • edited Loading

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

vishalhcl-5960 Jul 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vishalhcl-5960 Jul 4, 2024 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vishalhcl-5960 Jul 22, 2024 • edited Loading

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

mattmurp left a 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

mattmurp left a comment

Choose a reason for hiding this comment

vishalhcl-5960 Jul 3, 2024 •

edited

Loading

vishalhcl-5960 Jul 2, 2024 •

edited

Loading

vishalhcl-5960 Jul 4, 2024 •

edited

Loading

vishalhcl-5960 Jul 22, 2024 •

edited

Loading