Skip to content

Navigation Menu

Sign in
Sign up

Report a malformed --account file instead of panicking #1687

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
arpitjain099 wants to merge 1 commit into smallstep:master
base: master
Choose a base branch
Loading
from arpitjain099:fix/oauth-account-file-assertions
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
114 changes: 90 additions & 24 deletions command/oauth/cmd.go
View file Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -348,6 +348,88 @@ func (o *options) Validate() error {
return nil
}

// accountCredentials holds the OAuth endpoints and client credentials read from
// an --account file.
type accountCredentials struct {
authzEp string
tokenEp string
clientID string
clientSecret string
issuer string
do2lo bool
}

// accountString reads a string value out of a decoded account file, naming the
// key when it is absent or of the wrong type. The values come straight from the
// file, so a type assertion here would panic on any partial or non-Google JSON.
func accountString(m map[string]interface{}, key, filename string) (string, error) {
v, ok := m[key].(string)
if !ok {
return "", errors.Errorf("error reading %s: missing or invalid %q", filename, key)
}
return v, nil
}

// readAccountCredentials parses an --account file. It supports the "installed"
// shape written by the Google console and Google service accounts.
func readAccountCredentials(filename string) (*accountCredentials, error) {
b, err := os.ReadFile(filename)
if err != nil {
return nil, errors.Wrapf(err, "error reading account from %s", filename)
}

account := make(map[string]interface{})
if err := json.Unmarshal(b, &account); err != nil {
return nil, errors.Wrapf(err, "error reading %s: unsupported format", filename)
}

creds := &accountCredentials{}
if _, ok := account["installed"]; ok {
details, ok := account["installed"].(map[string]interface{})
if !ok {
return nil, errors.Errorf("error reading %s: %q must be an object", filename, "installed")
}
for _, f := range []struct {
dst *string
key string
}{
{&creds.authzEp, "auth_uri"},
{&creds.tokenEp, "token_uri"},
{&creds.clientID, "client_id"},
{&creds.clientSecret, "client_secret"},
} {
if *f.dst, err = accountString(details, f.key, filename); err != nil {
return nil, err
}
}
return creds, nil
}

if accountType, ok := account["type"]; ok && accountType == "service_account" {
for _, f := range []struct {
dst *string
key string
}{
{&creds.authzEp, "auth_uri"},
{&creds.tokenEp, "token_uri"},
{&creds.clientID, "private_key_id"},
{&creds.clientSecret, "private_key"},
{&creds.issuer, "client_email"},
} {
if *f.dst, err = accountString(account, f.key, filename); err != nil {
return nil, err
}
}
creds.do2lo = true
return creds, nil
}

// The original code wrapped a nil err here, and errors.Wrapf(nil, ...) is
// nil, so an unsupported file reported nothing and the flow continued with
// empty endpoints.
return nil, errors.Errorf("error reading %s: unsupported account type", filename)
}

func oauthCmd(c *cli.Context) error {
opts := &options{
Provider: c.String("provider"),
Expand Down Expand Up @@ -432,32 +514,16 @@ func oauthCmd(c *cli.Context) error {
// This code supports Google service accounts. Probably maybe also support JWKs?
if c.IsSet("account") {
opts.Provider = ""
filename := c.String("account")
b, err := os.ReadFile(filename)
creds, err := readAccountCredentials(c.String("account"))
if err != nil {
return errors.Wrapf(err, "error reading account from %s", filename)
}
account := make(map[string]interface{})
if err = json.Unmarshal(b, &account); err != nil {
return errors.Wrapf(err, "error reading %s: unsupported format", filename)
}

if _, ok := account["installed"]; ok {
details := account["installed"].(map[string]interface{})
authzEp = details["auth_uri"].(string)
tokenEp = details["token_uri"].(string)
clientID = details["client_id"].(string)
clientSecret = details["client_secret"].(string)
} else if accountType, ok := account["type"]; ok && accountType == "service_account" {
authzEp = account["auth_uri"].(string)
tokenEp = account["token_uri"].(string)
clientID = account["private_key_id"].(string)
clientSecret = account["private_key"].(string)
issuer = account["client_email"].(string)
do2lo = true
} else {
return errors.Wrapf(err, "error reading %s: unsupported account type", filename)
return err
}
authzEp = creds.authzEp
tokenEp = creds.tokenEp
clientID = creds.clientID
clientSecret = creds.clientSecret
issuer = creds.issuer
do2lo = creds.do2lo
}

scope := "openid email"
Expand Down
125 changes: 125 additions & 0 deletions command/oauth/cmd_test.go
View file Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,125 @@
package oauth

import (
"os"
"path/filepath"
"strings"
"testing"
)

// The --account file is read and unmarshalled into map[string]interface{}, and
// every value used to be pulled out with a bare type assertion. Each case below
// panicked, except the unsupported-type one, which returned a nil error because
// errors.Wrapf(nil, ...) is nil.
func TestReadAccountCredentialsErrors(t *testing.T) {
dir := t.TempDir()

for _, tt := range []struct {
name string
content string
wantErr string
}{
{
name: "installed is not an object",
content: `{"installed":"notamap"}`,
wantErr: `"installed" must be an object`,
},
{
name: "installed is missing its keys",
content: `{"installed":{}}`,
wantErr: `missing or invalid "auth_uri"`,
},
{
name: "installed has a non-string value",
content: `{"installed":{"auth_uri":1}}`,
wantErr: `missing or invalid "auth_uri"`,
},
{
name: "service account is missing its keys",
content: `{"type":"service_account"}`,
wantErr: `missing or invalid "auth_uri"`,
},
{
name: "unsupported account type",
content: `{"other":1}`,
wantErr: "unsupported account type",
},
{
name: "not json",
content: `not json at all`,
wantErr: "unsupported format",
},
} {
t.Run(tt.name, func(t *testing.T) {
path := filepath.Join(dir, strings.ReplaceAll(tt.name, " ", "_")+".json")
if err := os.WriteFile(path, []byte(tt.content), 0o600); err != nil {
t.Fatal(err)
}

creds, err := readAccountCredentials(path)
if err == nil {
t.Fatalf("expected an error, got credentials %+v", creds)
}
if !strings.Contains(err.Error(), tt.wantErr) {
t.Fatalf("expected error containing %q, got %v", tt.wantErr, err)
}
})
}
}

func TestReadAccountCredentialsMissingFile(t *testing.T) {
if _, err := readAccountCredentials(filepath.Join(t.TempDir(), "nope.json")); err == nil {
t.Fatal("expected an error for a missing file")
}
}

func TestReadAccountCredentialsInstalled(t *testing.T) {
path := filepath.Join(t.TempDir(), "installed.json")
content := `{"installed":{"auth_uri":"https://accounts.example.com/auth",` +
`"token_uri":"https://oauth2.example.com/token",` +
`"client_id":"cid","client_secret":"secret"}}`
if err := os.WriteFile(path, []byte(content), 0o600); err != nil {
t.Fatal(err)
}

creds, err := readAccountCredentials(path)
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if creds.authzEp != "https://accounts.example.com/auth" {
t.Errorf("unexpected authzEp %q", creds.authzEp)
}
if creds.tokenEp != "https://oauth2.example.com/token" {
t.Errorf("unexpected tokenEp %q", creds.tokenEp)
}
if creds.clientID != "cid" || creds.clientSecret != "secret" {
t.Errorf("unexpected client credentials %q / %q", creds.clientID, creds.clientSecret)
}
if creds.do2lo {
t.Error("do2lo should be false for an installed account")
}
}

func TestReadAccountCredentialsServiceAccount(t *testing.T) {
path := filepath.Join(t.TempDir(), "sa.json")
content := `{"type":"service_account","auth_uri":"https://accounts.example.com/auth",` +
`"token_uri":"https://oauth2.example.com/token","private_key_id":"kid",` +
`"private_key":"pk","client_email":"svc@example.com"}`
if err := os.WriteFile(path, []byte(content), 0o600); err != nil {
t.Fatal(err)
}

creds, err := readAccountCredentials(path)
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if creds.clientID != "kid" || creds.clientSecret != "pk" {
t.Errorf("unexpected client credentials %q / %q", creds.clientID, creds.clientSecret)
}
if creds.issuer != "svc@example.com" {
t.Errorf("unexpected issuer %q", creds.issuer)
}
if !creds.do2lo {
t.Error("do2lo should be true for a service account")
}
}

AltStyle によって変換されたページ (->オリジナル) /