redhat-openshift-ecosystem/openshift-preflight

Operator and container preflight certification tests

openshift-preflight's Issues

Need to catch Warnings in bundle validate output

openshift-preflight/certification/internal/shell/validate_operator_bundle.go

Line 23 in 6a626e2

if strings.Contains(lines[len(lines)-1], "All validation tests have completed successfully") {

The operator-sdk bundle validate command outputs a string when it finishes "All validation tests have completed successfully". However, we should catch the Warnings too and pass those on to the user.

Add unit test for max layer check

Default log level should be lower than trace

Trace is the most verbose log level. The default should be something like Warning. Developers should only need debug and higher.

Using customised issue template for the repository

https://docs.github.com/en/communities/using-templates-to-encourage-useful-issues-and-pull-requests/configuring-issue-templates-for-your-repository

While filing bugs/RFE if we have customised template it will be easier for Getting all the required information.
This will be helpful for identifying root cause and saves time for getting required information for debugging

Need an OWNERS file for openshift-ci

We should add an OWNERS file to comply with reqs of using openshift-ci.

Add unit tests for HasUniqueTag check

Refactoring the check code and unit testing

preflight.log is created with an executable permission

The default log file is created with 0700 permissions and probably just needs 0600 (RW-) https://github.com/redhat-openshift-ecosystem/openshift-preflight/blob/main/cmd/log.go#L14

Identify and build quay namespace for test container and operator assets

When we decide to build and enable e2e tests, we will need to have test assets available in a publicly accessible location so that we can run those tests against known-good and known-bad assets.

The Quay namespace where this will be hosted needs to be identified so that we can start looking at automated build and publication of those assets.

The default output format should be JSON and JUnitXML support can be removed

Initial design required our junitxml be the default output format for test cases, but this requirement has changed and JSON is now the preferred output format. Schema definition is TBD, but we can use our placeholder/generic implementation for now.

Create abstraction for Operator-sdk

Operator-sdk needs an abstraction like Podman and Skopeo.

preflight check container registry_url should handle authorised image gracefully when not logged into registry

podman run -it localhost/viksss_preflight check container registry.connect.redhat.com/hpe3parcinder/hpe3parcinder16-1:latest
time="2021-07-15T13:48:56Z" level=error msg="unable to pull the container from the registry: failed to pull remote container: exit status 125"
time="2021-07-15T13:48:56Z" level=error msg="unable to pull the container from the registry: failed to pull remote container: exit status 125"
time="2021-07-15T13:48:56Z" level=error msg="unable to pull the container from the registry: failed to pull remote container: exit status 125"
time="2021-07-15T13:48:56Z" level=error msg="unable to pull the container from the registry: failed to pull remote container: exit status 125"
time="2021-07-15T13:48:56Z" level=error msg="unable to pull the container from the registry: failed to pull remote container: exit status 125"
time="2021-07-15T13:48:56Z" level=error msg="unable to pull the container from the registry: failed to pull remote container: exit status 125"
time="2021-07-15T13:48:56Z" level=error msg="unable to pull the container from the registry: failed to pull remote container: exit status 125"
time="2021-07-15T13:48:56Z" level=error msg="unable to pull the container from the registry: failed to pull remote container: exit status 125"
{
    "image": "registry.connect.redhat.com/hpe3parcinder/hpe3parcinder16-1:latest",
    "validation_lib_version": {
        "version": "0.0.0",
        "commit": "36053928678a4f901c1b0530418fd20419ce3754"
    },
    "results": {
        "Passed": [],
        "Failed": [],
        "Errors": [
            {
                "message": "Components in the container image cannot contain any critical or important vulnerabilities, as defined at https://access.redhat.com/security/updates/classification",
                "suggestion": "Update your UBI image to the latest version or update the packages in your image to the latest versions distrubuted by Red Hat."
            },
            {
                "message": "Containers should have a tag other than latest, so that the image can be uniquely identfied.",
                "suggestion": "Add a tag to your image. Consider using Semantic Versioning. https://semver.org/"
            },
            {
                "message": "The container image should not include Red Hat Enterprise Linux (RHEL) kernel packages.",
                "suggestion": "Remove any RHEL packages that are not distributable outside of UBI"
            },
            {
                "message": "A container that does not specify a non-root user will fail the automatic certification, and will be subject to a manual review before the container can be approved for publication",
                "suggestion": "Indicate a specific USER in the dockerfile or containerfile"
            },
            {
                "message": "Uncompressed container images should have less than 40 layers. Too many layers within the container images can degrade container performance.",
                "suggestion": "Optimize your Dockerfile to consolidate and minimize the number of layers. Each RUN command will produce a new layer. Try combining RUN commands using \u0026\u0026 where possible."
            },
            {
                "message": "Container images must include the following metadata: name, vendor, version, release, summary, description",
                "suggestion": "Add the following labels to your Dockerfile or Containerfile: name, vendor, version, release, summary, description"
            },
            {
                "message": "It is recommened that your image be based upon the Red Hat Universal Base Image (UBI)",
                "suggestion": "Change the FROM directive in your Dockerfile or Containerfile to FROM registry.access.redhat.com/ubi8/ubi"
            },
            {
                "message": "Container images must include terms and conditions applicable to the software including open source licensing information. The license must be at /licenses",
                "suggestion": "Create a directory named /licenses and include all relevant licensing and/or terms and conditions as text file(s) in that directory."
            }
        ]
    }

When not logged into registry(podman login) can we just print the not authorised please check if you are logged in to the registry and avoid Actual container checking

PodmanEngine should be an internal interface

Currently, the PodmanEngine interface is a public, exported interface. It really should be an internal interface.

Therefore, it should move to internal/cli as a package.

Add [make image-build] and [make image-push] into Workflow

[proposed] create them as a new yaml e.g. build.yml, to separate build steps from go/unit steps.

Write unit tests for the formatters package

[Check] Operator Images Schema Version

The preflight tool should generate hashes based on the bundle directory

https://docs.google.com/document/d/1AVSIJCdXvBk2LzvnkXZ1PTtJudVDxYQuJ-YmT7cF2_U/edit describes how a set of hashes can be generated to identify a specific bundle.

[Check] Operator Image Source

Skopeo tag listing implementation does not properly strip digests from images

The skopeo command does not want tags or digests when doing inspections. Our current implementation will strip a tag, but does not handle digest stripping which causes the check to fail.

The line that needs an update is here: https://github.com/redhat-openshift-ecosystem/openshift-preflight/blob/main/certification/internal/shell/skopeo.go#L16

We need to handle cases where the user provided an image that looks like this:

registry.access.redhat.com/ubi8/ubi-micro@sha256:d389fa0cfee2091b3dd75a446fb982714db67bfb0916369f48985923890e2597

cc @samira-barouti (to make sure I parsed the implementation details correctly)

[ Check ] Operator Catalog initialization test

Reference: https://github.com/redhat-openshift-ecosystem/operator-test-playbooks/blob/master/roles/operator_catalog_initialization_test/tasks/main.yml

Proposed implementation:

• Add a user flag for providing KUBECONFIG (in case it wasn't added yet).
• Define the check as runnable for Bundle Images
• Use ExtractContainerTar to obtain manifests
• Run catalog initializer and add RC and output into results.

Consider writing check results to a file

As a user, I may prefer for the results of my preflight execution to be written to a file instead of to STDOUT. Note that this refers to the test output that is passed through the implemented formatters itself, and not the log output the that preflight provides to the user at execution time.

Implementation Note:

We write our formatted text in one place: https://github.com/redhat-openshift-ecosystem/openshift-preflight/blob/ae9b2c7a5e78c092d574de7425392ce1a6faea89/cmd/root.go#L56 

This should just be a matter of choosing where to write.

[Check] Operator Deployment with OLM

Take the operator bundle image, include it in a custom catalog and confirm that it can be installed on a cluster running OLM. I think CodeReady Containers or KinD with OLM and SDK installed would work. We will need to add a way to pass in the kubeconfig to preflight so the tool knows which cluster to use.

Add unit tests for checks HasLicense, HasNoProhibitedPackages, RunAsNonRoot

Add unit tests for the the checks that use podman run. This include checks HasLicense, HasNoProhibitedPackages, and RunAsNonRoot.

Implement deliverables for multiple architectures

Reference:

• https://www.digitalocean.com/community/tutorials/how-to-build-go-executables-for-multiple-platforms-on-ubuntu-16-04

• https://gist.github.com/asukakenji/f15ba7e588ac42795f421b48b8aede63

• make build for: linux-amd64, darwin-amd64, zos-s390x, linux-ppc64, darwin-arm64 (Apple M1) ?, windows-amd64 ?

• make image-build for: linux-amd64, darwin-amd64, zos-s390x, linux-ppc64, darwin-arm64 (Apple M1) ?, windows-amd64 ?

• add a test to run each binary on the appropriate architecture into CI [proposed Label] kind/design

Ensure checks fall back to "failing" to avoid inadvertently passing a check

In a given check's Validate() function, we should ensure that the logic checks for explicit cases indicating we've passed (and falling back to failing) instead of explicit cases indicating we've failed (and falling back to passing).

So for example, say we have this simplified method (pseudo-code)

func (s *SomeCheck) Validate() (bool, error) {
	data := s.runSomeCheckAndGetSomeData()
	
	hasFailed := s.evaluateDataForFailure(data) // returnings a bool
	
	if hasFailed {
		return false, nil
	}

	return true, nil
}

In this case, we check to see if we've failed. If we have, then we return false and fail the check. If, for whatever reason, s.evaluateDataForFailure(data) returns a false positive due to some error in our parsing (i.e. `hasFailed == false), then we'd pass the check by default.

Instead, I'm thinking we should encourage the inverse:

func (s *SomeCheck) Validate() (bool, error) {
	data := s.runSomeCheckAndGetSomeData()
	
	hasPassed := s.evaluateDataForFailure(data) // returnings a bool
	
	if hasPassed {
		return true, nil
	}

	return false, nil
}

Implementation Notes:
Ex. where this is done according to the above example:
https://github.com/redhat-openshift-ecosystem/openshift-preflight/blob/ae9b2c7a5e78c092d574de7425392ce1a6faea89/certification/internal/shell/base_on_ubi.go#L47https://github.com/redhat-openshift-ecosystem/openshift-preflight/blob/main/certification/internal/shell/base_on_ubi.go#L47

We would want to replace checks that return true as their final line, as seen here:

openshift-preflight/certification/internal/shell/has_license.go

Line 32 in ae9b2c7

return true, nil

To fix this, we would have to go through and update existing checks to make sure that we look for explicit indicators of success and then return true in those cases (as opposed to looking for explicit indicators of failure and then returning false.

 

Preflight warns the user if they reference non-Marketplace images in a Red Hat Marketplace operator

Users might unintentionally reference their published images in the connect registry and not realize it. There should be very obvious warnings when this occurs.

[Check] Operator Scorecard

HasLicenseCheck should be more robust

Right now, when the /licenses directory exists, it is actually passing not because of the "No such file or directory" check. It's because the podman run exits with an error code. It never actually gets to the Validate method. Even if it did, "No such file or directory" would be on stderr, not stdout. So, stderr should in fact be checked for that string, rather than stdout.

If might make more sense to change this into a couple of different tests, that might yield more granular results, and allow us to be more prescriptive in the help suggestion.

First step would be to run 'stat /licenses'. And rather than parse the output, we should be checking the return code. Then, only if that succeeds (exit code of 0) should we then check the output of 'ls -1 /licenses | wc -l'. This will yield a number, that can then be checked against 0, rather than relying on the empty string.

[Check] Operator Bundle Data Linting

Run operator-sdk bundle validate -b podman --select-optional name=operatorhub --select-optional name=community registry.redhat.io/quay/quay-operator-bundle
Check should fail on errors and print to stdout and the log
Check should pass on warnings but print warnings to stdout and the log

The preflight.log file is written even when simply asking for `--help`.

This is because the initial implementation is called in an init() function[1] which is always executed. Instead, we want to explicitly initialize logging functionality once we've parsed the user provided configuration.

Was previously a part of #15 and has been separated into its own issue due to descoping the rest of the work in that issue.

Improve BasedOnUBI check to ensure reliable validation of UBI vs. RHEL images

The BasedOnUBi check currently checks to ensure that the /etc/os-release file contains the following keys:

ID="rhel"
NAME="Red Hat Enterprise Linux"

Technically, this passes with RHEL images:

podman run \
    -it --rm \
    --entrypoint cat \
    registry.access.redhat.com/rhel7@sha256:d7de4fd7b3850d03d2d432a3dce209e99db31d6a4a3d0017a195363208ade4f5 \
    /etc/os-release \
    -- | egrep "^ID=|^NAME="

NAME="Red Hat Enterprise Linux Server"
ID="rhel"

We'll need to decide:

• Should we be more explicit in identifying something is UBI, and how we do that, or if we want to pass RHEL-based images
• How we identify that something is specifically UBI and not RHEL

If we want to be more specific with this check, we'll need to

• Update certification/internal/shell/based_on_ubi.go to factor in the additional validation
• Update certification/internal/shell/based_on_ubi_test.go to test that validation

Extract common unit test logic into helper functions

There is a lot of boilerplate in the tests for each of the checks. This can be extracted into helper functions located on the suite.

Create method for operator-sdk bundle validate

The output file for preflight itself should be configurable (default: preflight.log)

We currently write a logfile to preflight.log as well as STDOUT. We should:

• Accept an environment variable/flag that indicates where we should write this file, and default to preflight.log if none is provided.
• Accept an environment variable/flag that indicates whether we should NOT also write to STDOUT/ERR (e.g. a --quiet flag).
  There is also a current bug that should be fixed by this enhancement. Currently. all calls to preflight will write a log file on disk. This includes calls to -help or -version which we don't need.

This is because the initial implementation is called in an init() function[1] which is always executed. Instead, we want to explicitly initialize logging functionality once we've parsed the user provided configuration. 

[1]https://github.com/redhat-openshift-ecosystem/openshift-preflight/blob/ae9b2c7a5e78c092d574de7425392ce1a6faea89/cmd/log.go

HasMinimalVulnerabilitiesCheck OVAL definitions may need to change based on the input image

Mentioned in the 2021-07-15 community meeting, the OVAL file used for the HasMinimalVulnerabilitiesCheck will need to changed to match the underlying base image.

Currently we utilize the RHEL-8 definition (see

openshift-preflight/certification/internal/shell/podman.go

Lines 20 to 21 in f1a21ac

	ovalFilename = "rhel-8.oval.xml.bz2"
	ovalUrl = "https://www.redhat.com/security/data/oval/v2/RHEL8/"

) in the current implementation, and that applies to all images. This may not be the desired outcome for all submitted/tested images.

Add unit test for HasRequiredLabelsCheck

PR #2

Write unit tests for HasMinimalVulnerabilityCheck

Print distinct message for check errors

Right now if a check errors out we print the help text but we should print a different message to distinguish between a Check Failure and an error. We can see say something as simple as, "This check encountered an error. Please review the logs for more information."

[Docs] Document how to install Preflight from source

Document how to install Preflight from source.

Document process for new contributors

Create a document that shows people how to contribute to this project.

HasNoProhibitedPackagesCheck throws an error when the image does not have the `rpm` binary

When running check container against UBI Micro (registry.access.redhat.com/ubi8/ubi-micro@sha256:d389fa0cfee2091b3dd75a446fb982714db67bfb0916369f48985923890e2597), the HasProhibitedPackages will fail because it cannot find rpm. We probably need to support cases where UBI Micro is used, and potentially need to fail gracefully in cases where an image that isn't RH*-based is used (though an error in those cases may be appropriate).

The error log was longer, but here's the gist:

level=error msg=\"container_linux.go:370: starting container process caused: exec: \\\"rpm\\\": executable file not found in $PATH\": OCI not found\n

For context, I believe this is because Micro UBI doesn't have any package tooling installed at all in an effort to make it smaller, but I wouldn't quote me on that fact.

Properly parse `operator-sdk scorecard` return code to determine if a test failed

In #92 we implemented a workaround that allows us to ensure we aren't prematurely terminating an operator-sdk scorecard check because we got a non-zero exit code from operator-sdk. The full discussion is in that issue.

Here is the corresponding RFE for operator-sdk such that we can remove this workaround and instead parse the return code.

operator-framework/operator-sdk#5061

When this merges, we should remove this workaround.

Add initial behavior testing

I think we can programmatically test behaviors of the application as a whole without necessarily calling out to the built binary by performing similar functions to what's done in the cobra Run commands.

The Plan

• I'll find a "known good" and "known bad" container image and operator bundle
• build a runtime.Config using that image and our known defaults
• get the default engine
• run the checks
• parse the results

Similar to what we do here: https://github.com/redhat-openshift-ecosystem/openshift-preflight/blob/main/cmd/check_container.go#L24-L64

The Test Case

The "known good" configuration should have no failed/errored entries in our results
The "known bad" configuration should have no successful/errored entries in our results

Down the road, we could expand this to be more granular, where we point to images that have known, specific failures and confirm that the results reflect that failure.

Logistically, I'd probably put this in a top-level test directory.

Expose the checks used by the `check container` and `check operator` actions in their help outputs

It may be useful for the user to know exactly what tests are going to be executed when they call container/operator check commands.

Reduce elevated privileges required to run preflight successfully when testing against a container image

When testing against a container image, the HasMinimalVulnerabilitiesCheck check makes a call out to oscap-podman, and it requires the ability to run a container as root. This test will fail if the runtime user cannot run a pod as root from what I can see.

We see this error if we run without elevated privileges

time="2021-07-09T13:36:57-05:00" level=error msg="unable to execute oscap-podman on the image: This script cannot run in rootless mode.\nvuln.html"

We're currently unable to control what checks are executed when calling preflight, which means we have to run the entire policy called with sudo.

We need to identify a way to satisfy the HasMinimalVulnerabilitiesCheck's need for elevated privileges while still maintaining the ability to run the rest of the checks without elevated privileges.

Rely on sane defaults for initial release version of preflight tooling

We have a few configurables such as enabled checks and output formats currently implemented. For initial releases and integration with the pipelines as a whole, we should disable those configurations and look to enable them as we need them down the line.

[Check] Operator Images Schema Version

Need to confirm that all the related images for this Operator are using FAT manifest (schemaVersion 2)

Possible implementation
podman pull the bundle image
podman image mount the bundle image
Load /manifest/<crd.yaml> into a variable
Use gojq to search for any values in the relatedImages field
Iterate the list of relatedImages and run podman manifest inspect <link to related image>
confirm that the JSOn returned has "schemaVersion": 2 for each related image

If all related images have a manifest schemaVersion: 2 then this check should pass
If any related image does not have schemaVersion: 2 then the check should fail and print the offending images to the log

[Check] Operator Package Name Uniqueness

This check will require an API from the backend teams. The API should accept a package name and return true if the name is unique (no other packages have that name) or false if the name is a duplicate of an existing package name in any of the catalogs

Need to work with the backend teams (CVP?) to have them create the API for us.

Implement `certify` subcommand to submit results for certification

The preflight tool should allow a user to take their results on-disk and submit them to Red Hat for review. The API endpoint is TBD.

Introduce viper for configuration and refactor logging init

Introduce viper to handle our configuration and refactor the logging configuration.

Modified cmd/root.go to initialize viper
Supports Env Vars

export PFLT_LOGFILE="foo.log"
export PFLT_LOGLEVEL="debug"

Supports an optional config.yaml file in the same directory as the binary

---
 logfile: "foo.log"
 loglevel: "Trace"

New subcommands `operator` and `container` do not properly expose runtime flags

Previously, we used ../preflight <image> to invoke tests, and the following subcommands existed:

Flags:
  -c, --enabled-checks string   Which checks to apply to the image to ensure compliance.
                                (Env) PREFLIGHT_ENABLED_CHECKS
  -h, --help                    help for preflight
  -l, --log-file string         Where to write cli log output.
                                (Env) PREFLIGHT_CLI_LOG_FILE (Default) preflight.log
  -o, --output-format string    The format for the check test results.
                                (Env) PREFLIGHT_OUTPUT_FORMAT (Default) json

Use "preflight [command] --help" for more information about a command.

These however are not exposed for the check subcommands check operator and check container:

This command will allow you to execute the Red Hat Certificaiton tests for an operator or a container.

Usage:
  preflight check [command]

Available Commands:
  container   Run checks for a container
  operator    Run checks for an Operator

Flags:
  -h, --help   help for check

Use "preflight check [command] --help" for more information about a command.