From ed11c58ebf42cbacecdc552989b67b7d4c1f4aae Mon Sep 17 00:00:00 2001 From: Nassim Eddequiouaq Date: Mon, 29 May 2017 10:21:50 +0200 Subject: [PATCH] Prevent invalid credentials: no missing server URL or username (#62) * Prevent invalid credentials: no missing server URL or username Signed-off-by: Nassim 'Nass' Eddequiouaq * Add missing username/serverURL error checks in credstore client Signed-off-by: Nassim 'Nass' Eddequiouaq * Clean up doc on invalid creds errors and client's error checks Signed-off-by: Nassim 'Nass' Eddequiouaq * Add tests for missing ServerURL/Username Signed-off-by: Nassim 'Nass' Eddequiouaq * Clean isValidCredsMessage prototype Signed-off-by: Nassim 'Nass' Eddequiouaq * Add test for missing server URL and more detailed error msg Signed-off-by: Nassim 'Nass' Eddequiouaq --- client/client.go | 38 +++++++++++++- client/client_test.go | 6 +++ credentials/credentials.go | 27 ++++++++++ credentials/credentials_test.go | 90 +++++++++++++++++++++++++++++++++ credentials/error.go | 74 +++++++++++++++++++++++++-- 5 files changed, 230 insertions(+), 5 deletions(-) diff --git a/client/client.go b/client/client.go index d081396..d1d0434 100644 --- a/client/client.go +++ b/client/client.go @@ -9,12 +9,27 @@ import ( "github.com/docker/docker-credential-helpers/credentials" ) +// isValidCredsMessage checks if 'msg' contains invalid credentials error message. +// It returns whether the logs are free of invalid credentials errors and the error if it isn't. +// error values can be errCredentialsMissingServerURL or errCredentialsMissingUsername. +func isValidCredsMessage(msg string) error { + if credentials.IsCredentialsMissingServerURLMessage(msg) { + return credentials.NewErrCredentialsMissingServerURL() + } + + if credentials.IsCredentialsMissingUsernameMessage(msg) { + return credentials.NewErrCredentialsMissingUsername() + } + + return nil +} + // Store uses an external program to save credentials. -func Store(program ProgramFunc, credentials *credentials.Credentials) error { +func Store(program ProgramFunc, creds *credentials.Credentials) error { cmd := program("store") buffer := new(bytes.Buffer) - if err := json.NewEncoder(buffer).Encode(credentials); err != nil { + if err := json.NewEncoder(buffer).Encode(creds); err != nil { return err } cmd.Input(buffer) @@ -22,6 +37,11 @@ func Store(program ProgramFunc, credentials *credentials.Credentials) error { out, err := cmd.Output() if err != nil { t := strings.TrimSpace(string(out)) + + if isValidErr := isValidCredsMessage(t); isValidErr != nil { + err = isValidErr + } + return fmt.Errorf("error storing credentials - err: %v, out: `%s`", err, t) } @@ -41,6 +61,10 @@ func Get(program ProgramFunc, serverURL string) (*credentials.Credentials, error return nil, credentials.NewErrCredentialsNotFound() } + if isValidErr := isValidCredsMessage(t); isValidErr != nil { + err = isValidErr + } + return nil, fmt.Errorf("error getting credentials - err: %v, out: `%s`", err, t) } @@ -62,6 +86,11 @@ func Erase(program ProgramFunc, serverURL string) error { out, err := cmd.Output() if err != nil { t := strings.TrimSpace(string(out)) + + if isValidErr := isValidCredsMessage(t); isValidErr != nil { + err = isValidErr + } + return fmt.Errorf("error erasing credentials - err: %v, out: `%s`", err, t) } @@ -75,6 +104,11 @@ func List(program ProgramFunc) (map[string]string, error) { out, err := cmd.Output() if err != nil { t := strings.TrimSpace(string(out)) + + if isValidErr := isValidCredsMessage(t); isValidErr != nil { + err = isValidErr + } + return nil, fmt.Errorf("error listing credentials - err: %v, out: `%s`", err, t) } diff --git a/client/client_test.go b/client/client_test.go index e5f1bf2..649c1ae 100644 --- a/client/client_test.go +++ b/client/client_test.go @@ -56,6 +56,8 @@ func (m *mockProgram) Output() ([]byte, error) { return []byte(credentials.NewErrCredentialsNotFound().Error()), errProgramExited case invalidServerAddress: return []byte("program failed"), errProgramExited + case "": + return []byte(credentials.NewErrCredentialsMissingServerURL().Error()), errProgramExited } case "store": var c credentials.Credentials @@ -158,12 +160,16 @@ func TestGet(t *testing.T) { } } + missingServerURLErr := credentials.NewErrCredentialsMissingServerURL() + invalid := []struct { serverURL string err string }{ {missingCredsAddress, credentials.NewErrCredentialsNotFound().Error()}, {invalidServerAddress, "error getting credentials - err: exited 1, out: `program failed`"}, + {"", fmt.Sprintf("error getting credentials - err: %s, out: `%s`", + missingServerURLErr.Error(), missingServerURLErr.Error())}, } for _, v := range invalid { diff --git a/credentials/credentials.go b/credentials/credentials.go index cfe0cb1..544ab3c 100644 --- a/credentials/credentials.go +++ b/credentials/credentials.go @@ -17,6 +17,22 @@ type Credentials struct { Secret string } +// isValid checks the integrity of Credentials object such that no credentials lack +// a server URL or a username. +// It returns whether the credentials are valid and the error if it isn't. +// error values can be errCredentialsMissingServerURL or errCredentialsMissingUsername +func (c *Credentials) isValid() (bool, error) { + if len(c.ServerURL) == 0 { + return false, NewErrCredentialsMissingServerURL() + } + + if len(c.Username) == 0 { + return false, NewErrCredentialsMissingUsername() + } + + return true, nil +} + // Docker credentials should be labeled as such in credentials stores that allow labelling. // That label allows to filter out non-Docker credentials too at lookup/search in macOS keychain, // Windows credentials manager and Linux libsecret. Default value is "Docker Credentials" @@ -81,6 +97,10 @@ func Store(helper Helper, reader io.Reader) error { return err } + if ok, err := creds.isValid(); !ok { + return err + } + return helper.Add(&creds) } @@ -100,6 +120,9 @@ func Get(helper Helper, reader io.Reader, writer io.Writer) error { } serverURL := strings.TrimSpace(buffer.String()) + if len(serverURL) == 0 { + return NewErrCredentialsMissingServerURL() + } username, secret, err := helper.Get(serverURL) if err != nil { @@ -107,6 +130,7 @@ func Get(helper Helper, reader io.Reader, writer io.Writer) error { } resp := Credentials{ + ServerURL: serverURL, Username: username, Secret: secret, } @@ -135,6 +159,9 @@ func Erase(helper Helper, reader io.Reader) error { } serverURL := strings.TrimSpace(buffer.String()) + if len(serverURL) == 0 { + return NewErrCredentialsMissingServerURL() + } return helper.Delete(serverURL) } diff --git a/credentials/credentials_test.go b/credentials/credentials_test.go index 6b8bbe1..e7ffa69 100644 --- a/credentials/credentials_test.go +++ b/credentials/credentials_test.go @@ -73,6 +73,46 @@ func TestStore(t *testing.T) { } } +func TestStoreMissingServerURL(t *testing.T) { + creds := &Credentials{ + ServerURL: "", + Username: "foo", + Secret: "bar", + } + + b, err := json.Marshal(creds) + if err != nil { + t.Fatal(err) + } + in := bytes.NewReader(b) + + h := newMemoryStore() + + if err := Store(h, in); IsCredentialsMissingServerURL(err) == false { + t.Fatal(err) + } +} + +func TestStoreMissingUsername(t *testing.T) { + creds := &Credentials{ + ServerURL: "https://index.docker.io/v1/", + Username: "", + Secret: "bar", + } + + b, err := json.Marshal(creds) + if err != nil { + t.Fatal(err) + } + in := bytes.NewReader(b) + + h := newMemoryStore() + + if err := Store(h, in); IsCredentialsMissingUsername(err) == false { + t.Fatal(err) + } +} + func TestGet(t *testing.T) { serverURL := "https://index.docker.io/v1/" creds := &Credentials{ @@ -115,6 +155,32 @@ func TestGet(t *testing.T) { } } +func TestGetMissingServerURL(t *testing.T) { + serverURL := "https://index.docker.io/v1/" + creds := &Credentials{ + ServerURL: serverURL, + Username: "foo", + Secret: "bar", + } + b, err := json.Marshal(creds) + if err != nil { + t.Fatal(err) + } + in := bytes.NewReader(b) + + h := newMemoryStore() + if err := Store(h, in); err != nil { + t.Fatal(err) + } + + buf := strings.NewReader("") + w := new(bytes.Buffer) + + if err := Get(h, buf, w); IsCredentialsMissingServerURL(err) == false { + t.Fatal(err) + } +} + func TestErase(t *testing.T) { serverURL := "https://index.docker.io/v1/" creds := &Credentials{ @@ -144,6 +210,30 @@ func TestErase(t *testing.T) { } } +func TestEraseMissingServerURL(t *testing.T) { + serverURL := "https://index.docker.io/v1/" + creds := &Credentials{ + ServerURL: serverURL, + Username: "foo", + Secret: "bar", + } + b, err := json.Marshal(creds) + if err != nil { + t.Fatal(err) + } + in := bytes.NewReader(b) + + h := newMemoryStore() + if err := Store(h, in); err != nil { + t.Fatal(err) + } + + buf := strings.NewReader("") + if err := Erase(h, buf); IsCredentialsMissingServerURL(err) == false { + t.Fatal(err) + } +} + func TestList(t *testing.T) { //This tests that there is proper input an output into the byte stream //Individual stores are very OS specific and have been tested in osxkeychain and secretservice respectively diff --git a/credentials/error.go b/credentials/error.go index d24bf16..588d4a8 100644 --- a/credentials/error.go +++ b/credentials/error.go @@ -1,8 +1,15 @@ package credentials -// ErrCredentialsNotFound standarizes the not found error, so every helper returns -// the same message and docker can handle it properly. -const errCredentialsNotFoundMessage = "credentials not found in native keychain" +const ( + // ErrCredentialsNotFound standardizes the not found error, so every helper returns + // the same message and docker can handle it properly. + errCredentialsNotFoundMessage = "credentials not found in native keychain" + + // ErrCredentialsMissingServerURL and ErrCredentialsMissingUsername standardize + // invalid credentials or credentials management operations + errCredentialsMissingServerURLMessage = "no credentials server URL" + errCredentialsMissingUsernameMessage = "no credentials username" +) // errCredentialsNotFound represents an error // raised when credentials are not in the store. @@ -35,3 +42,64 @@ func IsErrCredentialsNotFound(err error) bool { func IsErrCredentialsNotFoundMessage(err string) bool { return err == errCredentialsNotFoundMessage } + + +// errCredentialsMissingServerURL represents an error raised +// when the credentials object has no server URL or when no +// server URL is provided to a credentials operation requiring +// one. +type errCredentialsMissingServerURL struct{} + +func (errCredentialsMissingServerURL) Error() string { + return errCredentialsMissingServerURLMessage +} + +// errCredentialsMissingUsername represents an error raised +// when the credentials object has no username or when no +// username is provided to a credentials operation requiring +// one. +type errCredentialsMissingUsername struct{} + +func (errCredentialsMissingUsername) Error() string { + return errCredentialsMissingUsernameMessage +} + + +// NewErrCredentialsMissingServerURL creates a new error for +// errCredentialsMissingServerURL. +func NewErrCredentialsMissingServerURL() error { + return errCredentialsMissingServerURL{} +} + +// NewErrCredentialsMissingUsername creates a new error for +// errCredentialsMissingUsername. +func NewErrCredentialsMissingUsername() error { + return errCredentialsMissingUsername{} +} + + +// IsCredentialsMissingServerURL returns true if the error +// was an errCredentialsMissingServerURL. +func IsCredentialsMissingServerURL(err error) bool { + _, ok := err.(errCredentialsMissingServerURL) + return ok +} + +// IsCredentialsMissingServerURLMessage checks for an +// errCredentialsMissingServerURL in the error message. +func IsCredentialsMissingServerURLMessage(err string) bool { + return err == errCredentialsMissingServerURLMessage +} + +// IsCredentialsMissingUsername returns true if the error +// was an errCredentialsMissingUsername. +func IsCredentialsMissingUsername(err error) bool { + _, ok := err.(errCredentialsMissingUsername) + return ok +} + +// IsCredentialsMissingUsernameMessage checks for an +// errCredentialsMissingUsername in the error message. +func IsCredentialsMissingUsernameMessage(err string) bool { + return err == errCredentialsMissingUsernameMessage +}