Skip to content

Commit

Permalink
add malformed ssh key unit test (#142)
Browse files Browse the repository at this point in the history 
* add malformed ssh key unit test and integration test

* fix unit test name

* fix go check

* remove integration test and keep unit test

* remove reflect deepEqual

* refactor function removeExpiredKeys

* rename

* rename

* go check

* go check

* fix

* Apply suggestions from code review

Co-authored-by: Liam Hopkins <[email protected]>

* Apply suggestions from code review

Co-authored-by: Liam Hopkins <[email protected]>

* refactor

* fix failed test and rename test

* fix

* address comments

* fix

Co-authored-by: Liam Hopkins <[email protected]>
  • Loading branch information
@gaohannk @hopkiw
gaohannk and hopkiw authored Dec 14, 2021
1 parent b0c8cbd commit fbdff75
Show file tree
Hide file tree
Showing 2 changed files with 90 additions and 74 deletions.
104 changes: 51 additions & 53 deletions google_guest_agent/non_windows_accounts.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ func (a *accountsMgr) diff() bool {

// If any on-disk keys have expired.
for _, keys := range sshKeys {
if len(keys) != len(removeExpiredKeys(keys)) {
if len(keys) != len(getUserKeys(keys)) {
return true
}
}
Expand Down Expand Up @@ -118,22 +118,7 @@ func (a *accountsMgr) set() error {
mdkeys = append(mdkeys, newMetadata.Project.Attributes.SSHKeys...)
}

mdKeyMap := make(map[string][]string)
for _, key := range removeExpiredKeys(mdkeys) {
idx := strings.Index(key, ":")
if idx == -1 {
logger.Debugf("invalid ssh key entry: %q", key)
continue
}
user := key[:idx]
if user == "" {
logger.Debugf("invalid ssh key entry: %q", key)
continue
}
userKeys := mdKeyMap[user]
userKeys = append(userKeys, key[idx+1:])
mdKeyMap[user] = userKeys
}
mdKeyMap := getUserKeys(mdkeys)

logger.Debugf("read google users file")
gUsers, err := readGoogleUsersFile()
Expand Down Expand Up @@ -197,6 +182,55 @@ func (a *accountsMgr) set() error {
return nil
}

// getUserKeys returns the keys which are not expired and non-expiring key.
// valid formats are:
// user:ssh-rsa [KEY_VALUE] [USERNAME]
// user:ssh-rsa [KEY_VALUE]
// user:ssh-rsa [KEY_VALUE] google-ssh {"userName":"[USERNAME]","expireOn":"[EXPIRE_TIME]"}
func getUserKeys(mdkeys []string) map[string][]string {
mdKeyMap := make(map[string][]string)
for i := 0; i < len(mdkeys); i++ {
key := strings.Trim(mdkeys[i], " ")
if key == "" {
logger.Debugf("invalid ssh key entry: %q", key)
continue
}
idx := strings.Index(key, ":")
if idx == -1 {
logger.Debugf("invalid ssh key entry: %q", key)
continue
}
user := key[:idx]
if user == "" {
logger.Debugf("invalid ssh key entry: %q", key)
continue
}
fields := strings.SplitN(key, " ", 4)
if len(fields) == 3 && fields[2] == "google-ssh" {
logger.Debugf("invalid ssh key entry: %q", key)
// expiring key without expiration format.
continue
}
if len(fields) > 3 {
lkey := linuxKey{}
if err := json.Unmarshal([]byte(fields[3]), &lkey); err != nil {
// invalid expiration format.
logger.Debugf("invalid ssh key entry: %q", key)
continue
}
if lkey.expired() {
logger.Debugf("expired ssh key entry: %q", key)
continue
}
}
// key which is not expired or non-expiring key, add it.
userKeys := mdKeyMap[user]
userKeys = append(userKeys, key[idx+1:])
mdKeyMap[user] = userKeys
}
return mdKeyMap
}

// passwdEntry is a user.User with omitted passwd fields restored.
type passwdEntry struct {
Username string
Expand Down Expand Up @@ -311,42 +345,6 @@ func (k linuxKey) expired() bool {
return t.Before(time.Now())
}

// removeExpiredKeys returns the provided list of keys with expired keys removed.
// valid formats are:
// ssh-rsa [KEY_VALUE] [USERNAME]
// ssh-rsa [KEY_VALUE]
// ssh-rsa [KEY_VALUE] google-ssh {"userName":"[USERNAME]","expireOn":"[EXPIRE_TIME]"}
//
// see: https://cloud.google.com/compute/docs/instances/adding-removing-ssh-keys#sshkeyformat
func removeExpiredKeys(keys []string) []string {
var res []string
for i := 0; i < len(keys); i++ {
key := strings.Trim(keys[i], " ")
if key == "" {
continue
}
fields := strings.SplitN(key, " ", 4)
if len(fields) < 3 || fields[2] != "google-ssh" {
// non-expiring key, add it.
res = append(res, key)
continue
}
if len(fields) < 4 {
// expiring key without expiration format.
continue
}
lkey := linuxKey{}
if err := json.Unmarshal([]byte(fields[3]), &lkey); err != nil {
// invalid expiration format.
continue
}
if !lkey.expired() {
res = append(res, key)
}
}
return res
}

// Replaces {user} or {group} in command string. Supports legacy python-era
// user command overrides.
func createUserGroupCmd(cmd, user, group string) *exec.Cmd {
Expand Down
60 changes: 39 additions & 21 deletions google_guest_agent/windows_accounts_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -216,32 +216,50 @@ func TestCompareAccounts(t *testing.T) {
}
}

func TestRemoveExpiredKeys(t *testing.T) {
func TestGetUserKeys(t *testing.T) {
var tests = []struct {
key string
valid bool
key string
expectedValid int
}{
{`user:ssh-rsa [KEY] google-ssh {"userName":"[email protected]", "expireOn":"2028-11-08T19:30:47+0000"}`, true},
{`user:ssh-rsa [KEY] google-ssh {"userName":"[email protected]", "expireOn":"2028-11-08T19:30:47+0700"}`, true},
{`user:ssh-rsa [KEY] google-ssh {"userName":"[email protected]", "expireOn":"2028-11-08T19:30:47+0700", "futureField": "UNUSED_FIELDS_IGNORED"}`, true},
{`user:ssh-rsa [KEY] google-ssh {"userName":"[email protected]", "expireOn":"2018-11-08T19:30:46+0000"}`, false},
{`user:ssh-rsa [KEY] google-ssh {"userName":"[email protected]", "expireOn":"2018-11-08T19:30:46+0700"}`, false},
{`user:ssh-rsa [KEY] google-ssh {"userName":"[email protected]", "expireOn":"INVALID_TIMESTAMP"}`, false},
{`user:ssh-rsa [KEY] google-ssh`, false},
{`user:ssh-rsa [KEY] user`, true},
{`user:ssh-rsa [KEY]`, true},
{},
{`user:ssh-rsa [KEY] google-ssh {"userName":"[email protected]", "expireOn":"2028-11-08T19:30:47+0000"}`,
1,
},
{`user:ssh-rsa [KEY] google-ssh {"userName":"[email protected]", "expireOn":"2028-11-08T19:30:47+0700"}`,
1,
},
{`user:ssh-rsa [KEY] google-ssh {"userName":"[email protected]", "expireOn":"2028-11-08T19:30:47+0700", "futureField": "UNUSED_FIELDS_IGNORED"}`,
1,
},
{`user:ssh-rsa [KEY] google-ssh {"userName":"[email protected]", "expireOn":"2018-11-08T19:30:46+0000"}`,
0,
},
{`user:ssh-rsa [KEY] google-ssh {"userName":"[email protected]", "expireOn":"2018-11-08T19:30:46+0700"}`,
0,
},
{`user:ssh-rsa [KEY] google-ssh {"userName":"[email protected]", "expireOn":"INVALID_TIMESTAMP"}`,
0,
},
{`user:ssh-rsa [KEY] google-ssh`,
0,
},
{`user:ssh-rsa [KEY] user`,
1,
},
{`user:ssh-rsa [KEY]`,
1,
},
{`malformed-ssh-keys [KEY] google-ssh`,
0,
},
{`:malformed-ssh-keys [KEY] google-ssh`,
0,
},
}

for _, tt := range tests {
ret := removeExpiredKeys([]string{tt.key})
if tt.valid {
if len(ret) == 0 || ret[0] != tt.key {
t.Errorf("valid key was removed: %q", tt.key)
}
}
if !tt.valid && len(ret) == 1 {
t.Errorf("invalid key was kept: %q", tt.key)
ret := getUserKeys([]string{tt.key})
if userKeys, _ := ret["user"]; len(userKeys) != tt.expectedValid {
t.Errorf("expected %d valid keys from getUserKeys, but %d", tt.expectedValid, len(userKeys))
}
}
}

0 comments on commit fbdff75

Please sign in to comment.