Skip to content
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.

Sign up for GitHub

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

Return role info for each role on pathRoleList #3532

Merged
merged 4 commits into from
Nov 3, 2017
Merged

Return role info for each role on pathRoleList #3532

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
585d626
Return role info for each role on pathRoleList
calvn Nov 3, 2017
13a8fd7
Change roles -> key_info, only return key_type
calvn Nov 3, 2017
6e9e88c
Do not initialize result map in parseRole, refactor ListResponseWithInfo
calvn Nov 3, 2017
388b0f0
Add role list test
calvn Nov 3, 2017
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
171 changes: 108 additions & 63 deletions builtin/logical/ssh/path_roles.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -488,13 +488,113 @@ func (b *backend) getRole(s logical.Storage, n string) (*sshRole, error) {
return &result, nil
}

// parseRole converts a sshRole object into its map[string]interface representation,
// with appropriate values for each keyType. If the keyType is invalid, it will retun
// an error.
func (b *backend) parseRole(role *sshRole) (map[string]interface{}, error) {
result := map[string]interface{}{}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this intended to ensure that this method doesn't return a nil? If not, the implicit make here can be avoided.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea since we are assigning whole result map in the switch statement, we don't have to initialize it right away.


switch role.KeyType {
case KeyTypeOTP:
result = map[string]interface{}{
"default_user": role.DefaultUser,
"cidr_list": role.CIDRList,
"exclude_cidr_list": role.ExcludeCIDRList,
"key_type": role.KeyType,
"port": role.Port,
"allowed_users": role.AllowedUsers,
}
case KeyTypeCA:
ttl, err := parseutil.ParseDurationSecond(role.TTL)
if err != nil {
return nil, err
}
maxTTL, err := parseutil.ParseDurationSecond(role.MaxTTL)
if err != nil {
return nil, err
}

result = map[string]interface{}{
"allowed_users": role.AllowedUsers,
"allowed_domains": role.AllowedDomains,
"default_user": role.DefaultUser,
"ttl": int64(ttl.Seconds()),
"max_ttl": int64(maxTTL.Seconds()),
"allowed_critical_options": role.AllowedCriticalOptions,
"allowed_extensions": role.AllowedExtensions,
"allow_user_certificates": role.AllowUserCertificates,
"allow_host_certificates": role.AllowHostCertificates,
"allow_bare_domains": role.AllowBareDomains,
"allow_subdomains": role.AllowSubdomains,
"allow_user_key_ids": role.AllowUserKeyIDs,
"key_id_format": role.KeyIDFormat,
"key_type": role.KeyType,
"default_critical_options": role.DefaultCriticalOptions,
"default_extensions": role.DefaultExtensions,
}
case KeyTypeDynamic:
result = map[string]interface{}{
"key": role.KeyName,
"admin_user": role.AdminUser,
"default_user": role.DefaultUser,
"cidr_list": role.CIDRList,
"exclude_cidr_list": role.ExcludeCIDRList,
"port": role.Port,
"key_type": role.KeyType,
"key_bits": role.KeyBits,
"allowed_users": role.AllowedUsers,
"key_option_specs": role.KeyOptionSpecs,
// Returning install script will make the output look messy.
// But this is one way for clients to see the script that is
// being used to install the key. If there is some problem,
// the script can be modified and configured by clients.
"install_script": role.InstallScript,
}
default:
return nil, fmt.Errorf("invalid key type: %v", role.KeyType)
}

return result, nil
}

func (b *backend) pathRoleList(req *logical.Request, d *framework.FieldData) (*logical.Response, error) {
entries, err := req.Storage.List("roles/")
if err != nil {
return nil, err
}

return logical.ListResponse(entries), nil
keyInfo := map[string]interface{}{}
for _, entry := range entries {
role, err := b.getRole(req.Storage, entry)
if err != nil {
// On error, log warning and continue
if b.Logger().IsWarn() {
b.Logger().Warn("ssh: error getting role info", "role", entry, "error", err)
}
continue
}
if role == nil {
// On empty role, log warning and continue
if b.Logger().IsWarn() {
b.Logger().Warn("ssh: no role info found", "role", entry)
}
continue
}

roleInfo, err := b.parseRole(role)
if err != nil {
if b.Logger().IsWarn() {
b.Logger().Warn("ssh: error parsing role info", "role", entry, "error", err)
}
continue
}

keyInfo[entry] = map[string]interface{}{
"key_type": roleInfo["key_type"],
}
}

return logical.ListResponseWithInfo(entries, keyInfo), nil
}

func (b *backend) pathRoleRead(req *logical.Request, d *framework.FieldData) (*logical.Response, error) {
Expand All @@ -506,69 +606,14 @@ func (b *backend) pathRoleRead(req *logical.Request, d *framework.FieldData) (*l
return nil, nil
}

// Return information should be based on the key type of the role
if role.KeyType == KeyTypeOTP {
return &logical.Response{
Data: map[string]interface{}{
"default_user": role.DefaultUser,
"cidr_list": role.CIDRList,
"exclude_cidr_list": role.ExcludeCIDRList,
"key_type": role.KeyType,
"port": role.Port,
"allowed_users": role.AllowedUsers,
},
}, nil
} else if role.KeyType == KeyTypeCA {
ttl, err := parseutil.ParseDurationSecond(role.TTL)
if err != nil {
return nil, err
}
maxTTL, err := parseutil.ParseDurationSecond(role.MaxTTL)
if err != nil {
return nil, err
}

return &logical.Response{
Data: map[string]interface{}{
"allowed_users": role.AllowedUsers,
"allowed_domains": role.AllowedDomains,
"default_user": role.DefaultUser,
"ttl": int64(ttl.Seconds()),
"max_ttl": int64(maxTTL.Seconds()),
"allowed_critical_options": role.AllowedCriticalOptions,
"allowed_extensions": role.AllowedExtensions,
"allow_user_certificates": role.AllowUserCertificates,
"allow_host_certificates": role.AllowHostCertificates,
"allow_bare_domains": role.AllowBareDomains,
"allow_subdomains": role.AllowSubdomains,
"allow_user_key_ids": role.AllowUserKeyIDs,
"key_id_format": role.KeyIDFormat,
"key_type": role.KeyType,
"default_critical_options": role.DefaultCriticalOptions,
"default_extensions": role.DefaultExtensions,
},
}, nil
} else {
return &logical.Response{
Data: map[string]interface{}{
"key": role.KeyName,
"admin_user": role.AdminUser,
"default_user": role.DefaultUser,
"cidr_list": role.CIDRList,
"exclude_cidr_list": role.ExcludeCIDRList,
"port": role.Port,
"key_type": role.KeyType,
"key_bits": role.KeyBits,
"allowed_users": role.AllowedUsers,
"key_option_specs": role.KeyOptionSpecs,
// Returning install script will make the output look messy.
// But this is one way for clients to see the script that is
// being used to install the key. If there is some problem,
// the script can be modified and configured by clients.
"install_script": role.InstallScript,
},
}, nil
roleInfo, err := b.parseRole(role)
if err != nil {
return nil, err
}

return &logical.Response{
Data: roleInfo,
}, nil
}

func (b *backend) pathRoleDelete(req *logical.Request, d *framework.FieldData) (*logical.Response, error) {
Expand Down
15 changes: 15 additions & 0 deletions logical/response.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,3 +110,18 @@ func ListResponse(keys []string) *Response {
}
return resp
}

// ListResponseWithInfo is used to format a response to a list operation and
// return the keys as well as a map with corresponding key info.
func ListResponseWithInfo(keys []string, keyInfo map[string]interface{}) *Response {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ooh interesting.

resp := ListResponse(keys)

resp.Data["key_info"] = map[string]interface{}{}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should create the map separately and assign to resp.Data at the end of the response. This avoids having to keep type asserting in the loop...cleaner and faster!

Separately, I'm not sure whether, if there are no actual keys, we should return an empty map, a null value, or not add the key at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went with not returning "key_info" at all if it's empty, similar to the behavior with "keys".

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good.

for _, key := range keys {
val, ok := keyInfo[key]
if ok {
resp.Data["key_info"].(map[string]interface{})[key] = val
}
}
return resp
}