add Java / JDK image scan option #115

tchughesiv · 2023-09-13T21:26:08Z

% sudo ./check-payload scan java -h               
Usage:
  check-payload scan java-image [image pull spec] [flags]

Aliases:
  java-image, java

Flags:
      --disabled-algorithms strings   additional algorithms that java should be disabling in FIPS mode
  -h, --help                          help for java-image
      --rpm-scan                      use RPM scan (same as during node scan)
      --spec string                   java payload url

Requirements to run -

FIPS enabled host. This is required in order to verify java is properly configured for FIPS at runtime.
Scans images w/ jdk 1.8+ installed

example printer outputs -

---- Failure Report
+----------------------------------------------+--------------------------------------------+
| STATUS                                       | IMAGE                                      |
+----------------------------------------------+--------------------------------------------+
| java scan must be run on a FIPS enabled host | registry.access.redhat.com/ubi8/openjdk-11 |
|                                              |                                            |
+----------------------------------------------+--------------------------------------------+

---- Failure Report
+-----------------------------------------------------------------------------+--------------------------------------------+
| STATUS                                                                      | IMAGE                                      |
+-----------------------------------------------------------------------------+--------------------------------------------+
| java property 'security.useSystemPropertiesFile' should not be set to false | registry.access.redhat.com/ubi8/openjdk-11 |
| java property 'com.redhat.fips' should not be set to false                  |                                            |
|                                                                             |                                            |
+-----------------------------------------------------------------------------+--------------------------------------------+

---- Failure Report
+----------------------------------------------------------------+----------------------+
| STATUS                                                         | IMAGE                |
+----------------------------------------------------------------+----------------------+
| algorithm 'DH keySize < 2048' should be disabled               | docker.io/openjdk:10 |
| algorithm 'TLSv1.1' should be disabled                         |                      |
| algorithm 'TLSv1' should be disabled                           |                      |
| algorithm 'SSLv2' should be disabled                           |                      |
| algorithm 'TLS_RSA_WITH_AES_256_CBC_SHA256' should be disabled |                      |
| algorithm 'TLS_RSA_WITH_AES_256_CBC_SHA' should be disabled    |                      |
| algorithm 'TLS_RSA_WITH_AES_128_CBC_SHA256' should be disabled |                      |
| algorithm 'TLS_RSA_WITH_AES_128_CBC_SHA' should be disabled    |                      |
| algorithm 'TLS_RSA_WITH_AES_256_GCM_SHA384' should be disabled |                      |
| algorithm 'TLS_RSA_WITH_AES_128_GCM_SHA256' should be disabled |                      |
| algorithm 'DHE_DSS' should be disabled                         |                      |
| algorithm 'RSA_EXPORT' should be disabled                      |                      |
| algorithm 'DHE_DSS_EXPORT' should be disabled                  |                      |
| algorithm 'DHE_RSA_EXPORT' should be disabled                  |                      |
| algorithm 'DH_DSS_EXPORT' should be disabled                   |                      |
| algorithm 'DH_RSA_EXPORT' should be disabled                   |                      |
| algorithm 'DH_anon' should be disabled                         |                      |
| algorithm 'ECDH_anon' should be disabled                       |                      |
| algorithm 'DH_RSA' should be disabled                          |                      |
| algorithm 'DH_DSS' should be disabled                          |                      |
| algorithm 'ECDH' should be disabled                            |                      |
| algorithm 'DES_CBC' should be disabled                         |                      |
| algorithm 'RC4_128' should be disabled                         |                      |
| algorithm 'RC2' should be disabled                             |                      |
| algorithm 'HmacMD5' should be disabled                         |                      |
|                                                                |                      |
+----------------------------------------------------------------+----------------------+

$ sudo ./check-payload scan java --spec registry.access.redhat.com/ubi8/openjdk-17 --disabled-algorithms TESTING,THIS
........
---- Failure Report
+----------------------------------------+--------------------------------------------+
| STATUS                                 | IMAGE                                      |
+----------------------------------------+--------------------------------------------+
| algorithm 'TESTING' should be disabled | registry.access.redhat.com/ubi8/openjdk-17 |
| algorithm 'THIS' should be disabled    |                                            |
|                                        |                                            |
+----------------------------------------+--------------------------------------------+

---- Failure Report
+-----------------------------------+---------------------+
| STATUS                            | IMAGE               |
+-----------------------------------+---------------------+
| this scan tool supports java 1.8+ | docker.io/openjdk:7 |
+-----------------------------------+---------------------+

---- Warning Report
+-----------------------------+---------------------------+
| STATUS                      | IMAGE                     |
+-----------------------------+---------------------------+
| openssl library not present | quay.io/keycloak/keycloak |
+-----------------------------+---------------------------+
---- Successful run with warnings

Future enhancements?

Handle/parse entrypoint wrapper scripts. We currently ignore them if set.
Support JRE image scans. This would require precompiled java code (per java version), which we could embed within the check-payload binary.

openshift-ci · 2023-09-13T21:26:25Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

tchughesiv · 2023-09-13T21:56:23Z

/test all

tchughesiv · 2023-09-14T14:39:03Z

/test all

tchughesiv · 2023-09-14T14:54:33Z

/test all

rphillips · 2023-09-19T20:27:35Z

internal/podman/podman.go

 		return stdout, fmt.Errorf("podman error (args=%v) (stderr=%v) (error=%w)", args, stderr.String(), err)
 	}
 	return stdout, nil
 }

+func ScanJava(ctx context.Context, image string, javaDisabledAlgorithms []string) (string, error) {
+	javaFilePath, _, err := releases.GetJavaFile()
+	defer os.Remove(javaFilePath)


why is there a remove of this file?

The defer here needs cleaned up... It should be after the err check.

rphillips · 2023-09-19T20:37:52Z

dist/releases/embeds.go

+	if err != nil {
+		return "", "", err
+	}
+	fileModeRO := os.FileMode(syscall.S_IRUSR) | os.FileMode(syscall.S_IRGRP) | os.FileMode(syscall.S_IROTH)


If any of the if statements here error the temp file isn't cleaned up. This function should defer cleanup the tempfile.

rphillips · 2023-09-19T20:39:14Z

internal/podman/podman.go

+	}
+
+	algFilePath, algFile, err := releases.GetAlgorithmFile(javaDisabledAlgorithms)
+	defer os.Remove(algFilePath)


rphillips · 2023-09-19T20:40:17Z

This looks good after the few fixes requested! Thanks!

tchughesiv · 2023-09-19T21:07:47Z

tks @rphillips ... fixes have been applied

kolyshkin · 2023-09-20T02:26:10Z

dist/releases/embeds.go

-	names := make([]string, len(dirs))
-	for i, d := range dirs {
-		names[i] = d.Name()
+	names := []string{}


I think you can still benefit from the optimization (that this commit removes) of knowing the slice length. You just use capacity rather than size for make.

kolyshkin · 2023-09-20T02:27:05Z

dist/releases/embeds.go

-		names[i] = d.Name()
+	names := []string{}
+	for _, d := range dirs {
+		if d.Name() != "java" {


if name := d.Name(); name != "java" { names = append(names, name) }

(so that we don't call d.Name() twice).

kolyshkin · 2023-09-20T02:27:23Z

dist/releases/embeds.go


 	semver "github.com/Masterminds/semver/v3"
 )

 //go:embed */*
 var configs embed.FS
+var JavaFips = "FIPS.java"


is this supposed to be a constant?

kolyshkin · 2023-09-20T02:28:09Z

dist/releases/embeds.go

+	return createTempFile(r.Bytes(), "disabledAlgorithms")
+}
+
+// createTempFile returns a temporary file with specified content and prefix, created in the OS's default temp folder


nit: missing period.

kolyshkin · 2023-09-20T02:28:14Z

dist/releases/embeds.go

+}
+
+// createTempFile returns a temporary file with specified content and prefix, created in the OS's default temp folder
+// Closing of the file (defer os.Remove(file.Name())) is the caller's responsibility


kolyshkin · 2023-09-20T02:31:43Z

dist/releases/embeds.go

+	r := bytes.NewBuffer([]byte{})
+	for i := range javaDisabledAlgorithms {
+		r.WriteString(javaDisabledAlgorithms[i] + "\n")
+	}
+
+	return createTempFile(r.Bytes(), "disabledAlgorithms")


I think you can replace this with something like

return createTempFile([]byte(strings.Join(javaDisabledAlgorithms, "\n")), "disabledAlgorithms")

(with an intermediate variable, if you prefer it that way)

kolyshkin · 2023-09-20T02:33:04Z

dist/releases/embeds.go

+// createTempFile returns a temporary file with specified content and prefix, created in the OS's default temp folder
+// Closing of the file (defer os.Remove(file.Name())) is the caller's responsibility
+func createTempFile(content []byte, prefix string) (fileFullPath, fileName string, err error) {
+	file, err := os.CreateTemp(os.TempDir(), prefix)


s/os.TempDir()/""/

because

If dir is the empty string, CreateTemp uses the default directory for temporary files, as returned by TempDir.

kolyshkin · 2023-09-20T02:46:32Z

internal/podman/podman.go

+	}
+
+	if validations.JavaClassLessThan52.Check(javaClassVersion) {
+		return "", fmt.Errorf("this scan tool supports java 1.8+")


nit: s/fmt.Errorf/errors.New/

kolyshkin · 2023-09-20T02:49:18Z

internal/podman/podman.go

+		return "", err
+	}
+
+	cmdArgs2 := []string{"run", "--rm", "--entrypoint", "", "-v", javaFilePath + ":" + jInfo.WorkingDir + "/" + releases.JavaFips + ":z", "-v", algFilePath + ":" + jInfo.WorkingDir + "/" + algFile + ":z", image}


maybe use better names than cmdArgs1 and cmdArgs2. Or just reuse cmdArgs variable.

kolyshkin · 2023-09-20T02:49:33Z

internal/podman/podman.go

+	} else {
+		cmdArgs2 = append(cmdArgs2, []string{"java", releases.JavaFips, algFile}...)
+	}
+	stdout2, err := runPodman(ctx, cmdArgs2...)


you can definitely reuse stdout variable here.

kolyshkin · 2023-09-20T02:50:04Z

internal/podman/podman.go

+	}
+	stdout2, err := runPodman(ctx, cmdArgs2...)
+	if err != nil {
+		fmt.Println(stdout2.String())


Maybe use logging here?

kolyshkin · 2023-09-20T02:50:27Z

internal/scan/scan.go

@@ -231,6 +231,25 @@ func validateTag(ctx context.Context, tag *v1.TagReference, cfg *types.Config) *
 		//  - skip per-tag and per-component config rules.
 		return rpmRootScan(ctx, cfg, mountPath)
 	}
+
+	// java


IMO this comment adds no value.

kolyshkin · 2023-09-20T02:55:01Z

internal/scan/scan.go

+	// openssl related results should be a warning for java.
+	// java has its own native implementation of SSL


nit: please use complete sentences.

tchughesiv · 2023-09-20T15:07:58Z

@kolyshkin thanks for the review! i've made those fixes. sorry for the sloppy code :)

mrunalp · 2023-09-21T00:40:30Z

internal/podman/podman.go

@@ -58,11 +64,82 @@ func runPodman(ctx context.Context, args ...string) (bytes.Buffer, error) {
 	cmd.Stdout = &stdout
 	cmd.Stderr = &stderr
 	if err := cmd.Run(); err != nil {
+		// Exit code 8 is used to differentiate valid java scan returns from other execution errors.
+		var exiterr *exec.ExitError
+		if errors.As(err, &exiterr); exiterr.ExitCode() == 8 {


Let's define a constant for the 8.

done, thanks

rphillips · 2023-10-11T19:06:03Z

/lgtm
/assign @kolyshkin

kolyshkin · 2023-10-11T19:11:14Z

@tchughesiv can you please squash the commits and I will re-review?

tchughesiv · 2023-10-11T19:14:20Z

@kolyshkin done

kolyshkin · 2023-10-11T21:17:03Z

dist/releases/embeds.go

-	names := make([]string, len(dirs))
-	for i, d := range dirs {
-		names[i] = d.Name()
+	names := make([]string, 0, cap(dirs))


nit: I think it should be len(dirs) here (because the cap could be more than len, and we won't add more elements than len).

thanks... done

Signed-off-by: Tommy Hughes <tohughes@redhat.com>

openshift-ci · 2023-10-11T21:48:41Z

@tchughesiv: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

kolyshkin

Couple of nits but overall LGTM

openshift-ci · 2023-10-12T00:46:01Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kolyshkin, tchughesiv

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [kolyshkin]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 13, 2023

tchughesiv force-pushed the java branch from aba594f to 7595281 Compare September 14, 2023 14:38

tchughesiv force-pushed the java branch from 3d933aa to 4c3ef39 Compare September 14, 2023 14:54

tchughesiv force-pushed the java branch 4 times, most recently from 5a24bcd to f8022c1 Compare September 14, 2023 19:34

tchughesiv marked this pull request as ready for review September 14, 2023 19:38

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 14, 2023

openshift-ci bot requested review from mrunalp and rphillips September 14, 2023 19:38

rphillips reviewed Sep 19, 2023

View reviewed changes

tchughesiv force-pushed the java branch 2 times, most recently from d5a7aaa to 6cf1f76 Compare September 19, 2023 21:05

tchughesiv force-pushed the java branch from 6cf1f76 to f27b669 Compare September 19, 2023 21:11

kolyshkin reviewed Sep 20, 2023

View reviewed changes

tchughesiv force-pushed the java branch 2 times, most recently from 7deac77 to 6143d5b Compare September 20, 2023 15:06

tchughesiv force-pushed the java branch from 6143d5b to 0f6515f Compare September 20, 2023 19:32

mrunalp reviewed Sep 21, 2023

View reviewed changes

tchughesiv force-pushed the java branch 2 times, most recently from a7cc253 to 061b1d1 Compare September 25, 2023 18:12

openshift-ci bot assigned kolyshkin and rphillips Oct 11, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 11, 2023

tchughesiv force-pushed the java branch from 061b1d1 to 6a6a62c Compare October 11, 2023 19:13

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Oct 11, 2023

kolyshkin reviewed Oct 11, 2023

View reviewed changes

java payload

64bd1b4

Signed-off-by: Tommy Hughes <tohughes@redhat.com>

tchughesiv force-pushed the java branch from 6a6a62c to 64bd1b4 Compare October 11, 2023 21:45

kolyshkin approved these changes Oct 12, 2023

View reviewed changes

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 12, 2023

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 12, 2023

openshift-ci bot merged commit 27df6ad into openshift:main Oct 12, 2023

		// openssl related results should be a warning for java.
		// java has its own native implementation of SSL

add Java / JDK image scan option #115

add Java / JDK image scan option #115

Conversation

tchughesiv commented Sep 13, 2023 • edited Loading

openshift-ci bot commented Sep 13, 2023

tchughesiv commented Sep 13, 2023

tchughesiv commented Sep 14, 2023

tchughesiv commented Sep 14, 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

rphillips commented Sep 19, 2023

tchughesiv commented Sep 19, 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

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

tchughesiv commented Sep 20, 2023 • edited Loading

Choose a reason for hiding this comment

tchughesiv Sep 21, 2023 • edited Loading

Choose a reason for hiding this comment

rphillips commented Oct 11, 2023

kolyshkin commented Oct 11, 2023

tchughesiv commented Oct 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci bot commented Oct 11, 2023

kolyshkin left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Oct 12, 2023

tchughesiv commented Sep 13, 2023 •

edited

Loading

tchughesiv commented Sep 20, 2023 •

edited

Loading

tchughesiv Sep 21, 2023 •

edited

Loading