Skip to content

Commit

Permalink
Reject submit if api token or cert id are empty
Browse files Browse the repository at this point in the history 
Just setting the flags to required on submit doesn't test for whether
the flags are actually provided. It only checks for their existence.
This code checks that the data is actually provided, and that another
flag isn't accidentally accepted as the value.

Fixes #754

Signed-off-by: Brad P. Crochet <brad@redhat.com>
  • Loading branch information
@bcrochet
bcrochet committed Aug 16, 2022
1 parent 5941395 commit 8a685ae
Show file tree
Hide file tree
Showing 5 changed files with 148 additions and 62 deletions.
33 changes: 29 additions & 4 deletions cmd/check_container.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package cmd
import (
"context"
"fmt"
"strings"

"github.com/redhat-openshift-ecosystem/openshift-preflight/certification"
"github.com/redhat-openshift-ecosystem/openshift-preflight/certification/engine"
Expand All @@ -12,6 +13,7 @@ import (
"github.com/redhat-openshift-ecosystem/openshift-preflight/version"
log "github.com/sirupsen/logrus"
"github.com/spf13/cobra"
"github.com/spf13/pflag"
"github.com/spf13/viper"
)

Expand Down Expand Up @@ -171,14 +173,37 @@ func checkContainerPositionalArgs(cmd *cobra.Command, args []string) error {
return fmt.Errorf("a container image positional argument is required")
}

cmd.Flags().VisitAll(func(f *pflag.Flag) {
if f.Changed && strings.Contains(f.Value.String(), "--submit") {
// We have --submit in one of the flags. That's a problem.
// We will set the submit flag to true so that the next block functions properly
submit = true
}
})

// --submit was specified
if submit {
if !viper.IsSet("certification_project_id") {
cmd.MarkFlagRequired("certification-project-id")
// If the flag is not marked as changed AND viper hasn't gotten it from environment, it's an error
if !cmd.Flag("certification-project-id").Changed && !viper.IsSet("certification_project_id") {
return fmt.Errorf("certification Project ID must be specified when --submit is present")
}
if !cmd.Flag("pyxis-api-token").Changed && !viper.IsSet("pyxis_api_token") {
return fmt.Errorf("pyxis API Token must be specified when --submit is present")
}

if !viper.IsSet("pyxis_api_token") {
cmd.MarkFlagRequired("pyxis-api-token")
// If the flag is marked as changed AND it's still empty, it's an error
if cmd.Flag("certification-project-id").Changed && viper.GetString("certification_project_id") == "" {
return fmt.Errorf("certification Project ID cannot be empty when --submit is present")
}
if cmd.Flag("pyxis-api-token").Changed && viper.GetString("pyxis_api_token") == "" {
return fmt.Errorf("pyxis API Token cannot be empty when --submit is present")
}

// Finally, if either certification project id or pyxis api token start with '--', it's an error
if strings.HasPrefix(viper.GetString("pyxis_api_token"), "--") || strings.HasPrefix(viper.GetString("certification_project_id"), "--") {
return fmt.Errorf("pyxis API token and certification ID are required when --submit is present")
}

}

return nil
Expand Down
58 changes: 54 additions & 4 deletions cmd/check_container_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"encoding/json"
"os"
"path"
"path/filepath"

. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
Expand All @@ -16,6 +17,7 @@ import (
"github.com/redhat-openshift-ecosystem/openshift-preflight/certification/policy"
"github.com/redhat-openshift-ecosystem/openshift-preflight/certification/pyxis"
"github.com/redhat-openshift-ecosystem/openshift-preflight/certification/runtime"
"github.com/spf13/viper"

"github.com/sirupsen/logrus"
)
Expand Down Expand Up @@ -469,11 +471,59 @@ var _ = Describe("Check Container Command", func() {
})
})

Context("and the user has enabled the submit flag", func() {
It("should cause the certification-project-id and pyxis-api-token flag to be required", func() {
out, err := executeCommand(checkContainerCmd(), "--submit", "foo")
DescribeTable("and the user has enabled the submit flag",
func(errString string, args []string) {
out, err := executeCommand(checkContainerCmd(), args...)
Expect(err).To(HaveOccurred())
Expect(out).To(ContainSubstring("required flag(s) \"%s\", \"%s\" not set", "certification-project-id", "pyxis-api-token"))
Expect(out).To(ContainSubstring(errString))
},
Entry("certification-project-id and pyxis-api-token are not supplied", "certification Project ID must be specified when --submit is present", []string{"--submit", "foo"}),
Entry("pyxis-api-token is not supplied", "pyxis API Token must be specified when --submit is present", []string{"foo", "--submit", "--certification-project-id=fooid"}),
Entry("certification-project-id is not supplied", "certification Project ID must be specified when --submit is present", []string{"--submit", "foo", "--pyxis-api-token=footoken"}),
Entry("pyxis-api-token flag is present but empty because of '='", "cannot be empty when --submit is present", []string{"foo", "--submit", "--certification-project-id=fooid", "--pyxis-api-token="}),
Entry("certification-project-id flag is present but empty because of '='", "cannot be empty when --submit is present", []string{"foo", "--submit", "--certification-project-id=", "--pyxis-api-token=footoken"}),
Entry("submit is passed after empty api token", "pyxis API token and certification ID are required when --submit is present", []string{"foo", "--certification-project-id=fooid", "--pyxis-api-token", "--submit"}),
Entry("submit is passed with explicit value after empty api token", "pyxis API token and certification ID are required when --submit is present", []string{"foo", "--certification-project-id=fooid", "--pyxis-api-token", "--submit=true"}),
)

When("the user enables the submit flag", func() {
When("environment variables are used for certification ID and api token", func() {
BeforeEach(func() {
os.Setenv("PFLT_CERTIFICATION_PROJECT_ID", "certid")
os.Setenv("PFLT_PYXIS_API_TOKEN", "tokenid")
DeferCleanup(os.Unsetenv, "PFLT_CERTIFICATION_PROJECT_ID")
DeferCleanup(os.Unsetenv, "PFLT_PYXIS_API_TOKEN")
})
It("should still execute with no error", func() {
submit = true

err := checkContainerPositionalArgs(checkContainerCmd(), []string{"foo"})
Expect(err).ToNot(HaveOccurred())
Expect(viper.GetString("pyxis_api_token")).To(Equal("tokenid"))
Expect(viper.GetString("certification_project_id")).To(Equal("certid"))
})
})
When("a config file is used", func() {
BeforeEach(func() {
config := `pyxis_api_token: mytoken
certification_project_id: mycertid`
tempDir, err := os.MkdirTemp("", "check-container-submit-*")
Expect(err).ToNot(HaveOccurred())
err = os.WriteFile(filepath.Join(tempDir, "config.yaml"), bytes.NewBufferString(config).Bytes(), 0o644)
Expect(err).ToNot(HaveOccurred())
viper.AddConfigPath(tempDir)
DeferCleanup(os.RemoveAll, tempDir)
})
It("should still execute with no error", func() {
// Make sure that we've read the config file
initConfig()
submit = true

err := checkContainerPositionalArgs(checkContainerCmd(), []string{"foo"})
Expect(err).ToNot(HaveOccurred())
Expect(viper.GetString("pyxis_api_token")).To(Equal("mytoken"))
Expect(viper.GetString("certification_project_id")).To(Equal("mycertid"))
})
})
})
})
Expand Down
2 changes: 2 additions & 0 deletions cmd/types.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,8 @@ type noopSubmitter struct {
log *log.Logger
}

var _ resultSubmitter = &noopSubmitter{}

func (s *noopSubmitter) Submit(ctx context.Context) error {
if s.emitLog {
msg := "Results are not being sent for submission."
Expand Down
30 changes: 15 additions & 15 deletions go.mod
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,16 +7,17 @@ require (
github.com/docker/cli v20.10.16+incompatible
github.com/google/go-containerregistry v0.10.0
github.com/knqyf263/go-rpmdb v0.0.0-20220629110411-9a3bd2ebb923
github.com/onsi/ginkgo/v2 v2.1.3
github.com/onsi/gomega v1.18.1
github.com/onsi/ginkgo/v2 v2.1.4
github.com/onsi/gomega v1.20.0
github.com/openshift/api v0.0.0-20210910062324-a41d3573a3ba
github.com/openshift/client-go v0.0.0-20210521082421-73d9475a9142
github.com/operator-framework/api v0.15.0
github.com/operator-framework/operator-manifest-tools v0.2.1
github.com/shurcooL/graphql v0.0.0-20220606043923-3cf50f8a0a29
github.com/sirupsen/logrus v1.8.1
github.com/spf13/cobra v1.4.0
github.com/spf13/viper v1.10.1
github.com/spf13/cobra v1.5.0
github.com/spf13/pflag v1.0.5
github.com/spf13/viper v1.12.0
gotest.tools/v3 v3.0.3
k8s.io/api v0.24.0
k8s.io/apimachinery v0.24.0
Expand All @@ -37,7 +38,7 @@ require (
github.com/docker/docker-credential-helpers v0.6.4 // indirect
github.com/emicklei/go-restful v2.9.5+incompatible // indirect
github.com/evanphx/json-patch v4.12.0+incompatible // indirect
github.com/fsnotify/fsnotify v1.5.1 // indirect
github.com/fsnotify/fsnotify v1.5.4 // indirect
github.com/go-logr/logr v1.2.0 // indirect
github.com/go-openapi/jsonpointer v0.19.5 // indirect
github.com/go-openapi/jsonreference v0.19.5 // indirect
Expand All @@ -55,29 +56,28 @@ require (
github.com/josharian/intern v1.0.0 // indirect
github.com/json-iterator/go v1.1.12 // indirect
github.com/klauspost/compress v1.15.4 // indirect
github.com/kr/pretty v0.2.1 // indirect
github.com/magiconair/properties v1.8.5 // indirect
github.com/magiconair/properties v1.8.6 // indirect
github.com/mailru/easyjson v0.7.6 // indirect
github.com/mattn/go-sqlite3 v1.14.12 // indirect
github.com/matttproud/golang_protobuf_extensions v1.0.2-0.20181231171920-c182affec369 // indirect
github.com/mitchellh/go-homedir v1.1.0 // indirect
github.com/mitchellh/mapstructure v1.4.3 // indirect
github.com/mitchellh/mapstructure v1.5.0 // indirect
github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd // indirect
github.com/modern-go/reflect2 v1.0.2 // indirect
github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 // indirect
github.com/opencontainers/go-digest v1.0.0 // indirect
github.com/opencontainers/image-spec v1.0.3-0.20220114050600-8b9d41f48198 // indirect
github.com/pelletier/go-toml v1.9.4 // indirect
github.com/pelletier/go-toml v1.9.5 // indirect
github.com/pelletier/go-toml/v2 v2.0.1 // indirect
github.com/pkg/errors v0.9.1 // indirect
github.com/prometheus/client_golang v1.12.1 // indirect
github.com/prometheus/client_model v0.2.0 // indirect
github.com/prometheus/common v0.32.1 // indirect
github.com/prometheus/procfs v0.7.3 // indirect
github.com/spf13/afero v1.6.0 // indirect
github.com/spf13/cast v1.4.1 // indirect
github.com/spf13/afero v1.8.2 // indirect
github.com/spf13/cast v1.5.0 // indirect
github.com/spf13/jwalterweatherman v1.1.0 // indirect
github.com/spf13/pflag v1.0.5 // indirect
github.com/subosito/gotenv v1.2.0 // indirect
github.com/subosito/gotenv v1.3.0 // indirect
github.com/vbatts/tar-split v0.11.2 // indirect
golang.org/x/net v0.0.0-20220524220425-1d687d428aca // indirect
golang.org/x/oauth2 v0.0.0-20220524215830-622c5d57e401 // indirect
Expand All @@ -91,9 +91,9 @@ require (
google.golang.org/appengine v1.6.7 // indirect
google.golang.org/protobuf v1.28.0 // indirect
gopkg.in/inf.v0 v0.9.1 // indirect
gopkg.in/ini.v1 v1.66.2 // indirect
gopkg.in/ini.v1 v1.66.4 // indirect
gopkg.in/yaml.v2 v2.4.0 // indirect
gopkg.in/yaml.v3 v3.0.0-20210107192922-496545a6307b // indirect
gopkg.in/yaml.v3 v3.0.1 // indirect
k8s.io/apiextensions-apiserver v0.24.0 // indirect
k8s.io/client-go v0.24.0 // indirect
k8s.io/component-base v0.24.0 // indirect
Expand Down

0 comments on commit 8a685ae

Please sign in to comment.