From d5920d88d9baa24807968b73220a9b0434e91932 Mon Sep 17 00:00:00 2001 From: Junjie Gao Date: Wed, 19 Jul 2023 10:53:49 +0800 Subject: [PATCH 1/6] fix: unset NOTATION_USERNAME and NOTATION_PASSWORD to avoid leaking credentials to plugin Signed-off-by: Junjie Gao --- cmd/notation/common.go | 4 ++++ cmd/notation/common_test.go | 21 +++++++++++++++++++++ 2 files changed, 25 insertions(+) diff --git a/cmd/notation/common.go b/cmd/notation/common.go index 809384ff0..5037de0cb 100644 --- a/cmd/notation/common.go +++ b/cmd/notation/common.go @@ -68,6 +68,10 @@ func (opts *SecureFlagOpts) ApplyFlags(fs *pflag.FlagSet) { setFlagInsecureRegistry(fs, &opts.InsecureRegistry) opts.Username = os.Getenv(defaultUsernameEnv) opts.Password = os.Getenv(defaultPasswordEnv) + + // unset to avoid leaking credentials + os.Unsetenv(defaultUsernameEnv) + os.Unsetenv(defaultPasswordEnv) } // Credential returns an auth.Credential from opts.Username and opts.Password. diff --git a/cmd/notation/common_test.go b/cmd/notation/common_test.go index 3910c7812..8651e79d9 100644 --- a/cmd/notation/common_test.go +++ b/cmd/notation/common_test.go @@ -14,9 +14,11 @@ package main import ( + "os" "reflect" "testing" + "github.com/spf13/pflag" "oras.land/oras-go/v2/registry/remote/auth" ) @@ -72,3 +74,22 @@ func TestSecureFlagOpts_Credential(t *testing.T) { }) } } + +func TestCredentialsUnset(t *testing.T) { + // Set environment variables for testing + os.Setenv(defaultUsernameEnv, "testuser") + os.Setenv(defaultPasswordEnv, "testpassword") + + secureFlagOpts := &SecureFlagOpts{} + fs := pflag.NewFlagSet("test", pflag.ContinueOnError) + secureFlagOpts.ApplyFlags(fs) + + // check credentials environment variables are unset + if os.Getenv(defaultUsernameEnv) != "" { + t.Errorf("expected %s to be unset", defaultUsernameEnv) + } + + if os.Getenv(defaultPasswordEnv) != "" { + t.Errorf("expected %s to be unset", defaultPasswordEnv) + } +} From fcc456bd9e13abb7882a4d11c3efc76c02241878 Mon Sep 17 00:00:00 2001 From: Junjie Gao Date: Wed, 19 Jul 2023 11:00:30 +0800 Subject: [PATCH 2/6] test: update test Signed-off-by: Junjie Gao --- cmd/notation/common_test.go | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/cmd/notation/common_test.go b/cmd/notation/common_test.go index 8651e79d9..ed92da4ae 100644 --- a/cmd/notation/common_test.go +++ b/cmd/notation/common_test.go @@ -76,20 +76,22 @@ func TestSecureFlagOpts_Credential(t *testing.T) { } func TestCredentialsUnset(t *testing.T) { + const notationUsername = "NOTATION_USERNAME" + const notationPassword = "NOTATION_PASSWORD" // Set environment variables for testing - os.Setenv(defaultUsernameEnv, "testuser") - os.Setenv(defaultPasswordEnv, "testpassword") + os.Setenv(notationUsername, "testuser") + os.Setenv(notationPassword, "testpassword") secureFlagOpts := &SecureFlagOpts{} fs := pflag.NewFlagSet("test", pflag.ContinueOnError) secureFlagOpts.ApplyFlags(fs) // check credentials environment variables are unset - if os.Getenv(defaultUsernameEnv) != "" { - t.Errorf("expected %s to be unset", defaultUsernameEnv) + if os.Getenv(notationUsername) != "" { + t.Errorf("expected %s to be unset", notationUsername) } - if os.Getenv(defaultPasswordEnv) != "" { - t.Errorf("expected %s to be unset", defaultPasswordEnv) + if os.Getenv(notationPassword) != "" { + t.Errorf("expected %s to be unset", notationPassword) } } From 7df4044d819dda01f1a3f6296784a326a1914268 Mon Sep 17 00:00:00 2001 From: Junjie Gao Date: Wed, 19 Jul 2023 21:02:37 +0800 Subject: [PATCH 3/6] fix: update code Signed-off-by: Junjie Gao --- cmd/notation/main.go | 8 ++++++++ cmd/notation/main_test.go | 39 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+) create mode 100644 cmd/notation/main_test.go diff --git a/cmd/notation/main.go b/cmd/notation/main.go index 3ea4219e2..d593d57c1 100644 --- a/cmd/notation/main.go +++ b/cmd/notation/main.go @@ -26,7 +26,14 @@ func main() { Use: "notation", Short: "Notation - a tool to sign and verify artifacts", SilenceUsage: true, + PersistentPreRun: func(cmd *cobra.Command, args []string) { + // unset environment registry credentials after read the value + // to avoid leaking credentials + // os.Unsetenv(defaultUsernameEnv) + // os.Unsetenv(defaultPasswordEnv) + }, } + cmd.AddCommand( signCommand(nil), verifyCommand(nil), @@ -40,6 +47,7 @@ func main() { versionCommand(), inspectCommand(nil), ) + if err := cmd.Execute(); err != nil { os.Exit(1) } diff --git a/cmd/notation/main_test.go b/cmd/notation/main_test.go new file mode 100644 index 000000000..6c7722aa8 --- /dev/null +++ b/cmd/notation/main_test.go @@ -0,0 +1,39 @@ +// Copyright The Notary Project Authors. +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package main + +import ( + "os" + "testing" +) + +func Test_UnsetEnvCredential(t *testing.T) { + const notationUsername = "NOTATION_USERNAME" + const notationPassword = "NOTATION_PASSWORD" + // Set environment variables for testing + os.Setenv(notationUsername, "testuser") + os.Setenv(notationPassword, "testpassword") + os.Args = []string{"notation", "version"} + + main() + + // check credentials environment variables are unset + if os.Getenv(notationUsername) != "" { + t.Errorf("expected %s to be unset", notationUsername) + } + + if os.Getenv(notationPassword) != "" { + t.Errorf("expected %s to be unset", notationPassword) + } +} From 649db3f9f609e17b295c4013b9c7b510fb11450a Mon Sep 17 00:00:00 2001 From: Junjie Gao Date: Wed, 19 Jul 2023 21:05:37 +0800 Subject: [PATCH 4/6] fix: unset registry credential before running each command Signed-off-by: Junjie Gao --- cmd/notation/common.go | 4 ---- cmd/notation/common_test.go | 23 ----------------------- cmd/notation/main.go | 6 +++--- 3 files changed, 3 insertions(+), 30 deletions(-) diff --git a/cmd/notation/common.go b/cmd/notation/common.go index 5037de0cb..809384ff0 100644 --- a/cmd/notation/common.go +++ b/cmd/notation/common.go @@ -68,10 +68,6 @@ func (opts *SecureFlagOpts) ApplyFlags(fs *pflag.FlagSet) { setFlagInsecureRegistry(fs, &opts.InsecureRegistry) opts.Username = os.Getenv(defaultUsernameEnv) opts.Password = os.Getenv(defaultPasswordEnv) - - // unset to avoid leaking credentials - os.Unsetenv(defaultUsernameEnv) - os.Unsetenv(defaultPasswordEnv) } // Credential returns an auth.Credential from opts.Username and opts.Password. diff --git a/cmd/notation/common_test.go b/cmd/notation/common_test.go index ed92da4ae..3910c7812 100644 --- a/cmd/notation/common_test.go +++ b/cmd/notation/common_test.go @@ -14,11 +14,9 @@ package main import ( - "os" "reflect" "testing" - "github.com/spf13/pflag" "oras.land/oras-go/v2/registry/remote/auth" ) @@ -74,24 +72,3 @@ func TestSecureFlagOpts_Credential(t *testing.T) { }) } } - -func TestCredentialsUnset(t *testing.T) { - const notationUsername = "NOTATION_USERNAME" - const notationPassword = "NOTATION_PASSWORD" - // Set environment variables for testing - os.Setenv(notationUsername, "testuser") - os.Setenv(notationPassword, "testpassword") - - secureFlagOpts := &SecureFlagOpts{} - fs := pflag.NewFlagSet("test", pflag.ContinueOnError) - secureFlagOpts.ApplyFlags(fs) - - // check credentials environment variables are unset - if os.Getenv(notationUsername) != "" { - t.Errorf("expected %s to be unset", notationUsername) - } - - if os.Getenv(notationPassword) != "" { - t.Errorf("expected %s to be unset", notationPassword) - } -} diff --git a/cmd/notation/main.go b/cmd/notation/main.go index d593d57c1..e5c9c6cf8 100644 --- a/cmd/notation/main.go +++ b/cmd/notation/main.go @@ -27,10 +27,10 @@ func main() { Short: "Notation - a tool to sign and verify artifacts", SilenceUsage: true, PersistentPreRun: func(cmd *cobra.Command, args []string) { - // unset environment registry credentials after read the value + // unset registry credentials after read the value from environment // to avoid leaking credentials - // os.Unsetenv(defaultUsernameEnv) - // os.Unsetenv(defaultPasswordEnv) + os.Unsetenv(defaultUsernameEnv) + os.Unsetenv(defaultPasswordEnv) }, } From 050bff0a35b2efc04baba92ba21db37fd97b1a38 Mon Sep 17 00:00:00 2001 From: Junjie Gao Date: Wed, 19 Jul 2023 21:08:30 +0800 Subject: [PATCH 5/6] fix: remove empty line Signed-off-by: Junjie Gao --- cmd/notation/main.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/cmd/notation/main.go b/cmd/notation/main.go index e5c9c6cf8..feb103aa3 100644 --- a/cmd/notation/main.go +++ b/cmd/notation/main.go @@ -33,7 +33,6 @@ func main() { os.Unsetenv(defaultPasswordEnv) }, } - cmd.AddCommand( signCommand(nil), verifyCommand(nil), @@ -47,7 +46,6 @@ func main() { versionCommand(), inspectCommand(nil), ) - if err := cmd.Execute(); err != nil { os.Exit(1) } From 97aa497f30d15d3a6b09b01944adb31edb5ec9a8 Mon Sep 17 00:00:00 2001 From: Junjie Gao Date: Thu, 20 Jul 2023 08:34:59 +0800 Subject: [PATCH 6/6] fix: add E2E plugin credentials check Signed-off-by: Junjie Gao --- test/e2e/plugin/main.go | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/test/e2e/plugin/main.go b/test/e2e/plugin/main.go index 03d39670f..622cb97e5 100644 --- a/test/e2e/plugin/main.go +++ b/test/e2e/plugin/main.go @@ -15,16 +15,31 @@ package main import ( "encoding/json" + "errors" "os" + "github.com/notaryproject/notation-go/plugin/proto" "github.com/spf13/cobra" ) +const NOTATION_USERNAME = "NOTATION_USERNAME" +const NOTATION_PASSWORD = "NOTATION_PASSWORD" + func main() { cmd := &cobra.Command{ Use: "plugin for Notation E2E test", SilenceUsage: true, SilenceErrors: true, + PersistentPreRunE: func(cmd *cobra.Command, args []string) error { + // check registry credentials are eliminated + if os.Getenv(NOTATION_USERNAME) != "" || os.Getenv(NOTATION_PASSWORD) != "" { + return &proto.RequestError{ + Code: proto.ErrorCodeValidation, + Err: errors.New("registry credentials are not eliminated"), + } + } + return nil + }, } cmd.AddCommand(