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

Jump to bottom

Fix panic due to metadata being nil #4719

Merged
merged 10 commits into from
Jun 11, 2018
Merged

Fix panic due to metadata being nil #4719

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
6b51207
Fix panic due to metadata being nil
vishalnayak Jun 7, 2018
d223e03
added a nil check
vishalnayak Jun 7, 2018
1b8c1d5
Added a test
vishalnayak Jun 7, 2018
4b82b1c
Merge branch 'master' into approle-metadata-fix
vishalnayak Jun 7, 2018
070e796
ensure metadata is never nil
vishalnayak Jun 11, 2018
e9cbcc8
Merge branch 'master' into approle-metadata-fix
vishalnayak Jun 11, 2018
5ee44f2
Remove unnecessary allocation
vishalnayak Jun 11, 2018
49dcf09
Merge branch 'master' into approle-metadata-fix
vishalnayak Jun 11, 2018
401dab1
revert back to early initialization
vishalnayak Jun 11, 2018
f6c3a10
Merge branch 'master' into approle-metadata-fix
vishalnayak Jun 11, 2018
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
6 changes: 4 additions & 2 deletions builtin/credential/approle/path_login.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ func (b *backend) pathLoginUpdate(ctx context.Context, req *logical.Request, dat
return logical.ErrorResponse("invalid role ID"), nil
}

var metadata map[string]string
metadata := make(map[string]string)
Copy link
Member

Choose a reason for hiding this comment

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

In the secret id case, we unilaterally assign to metadata, which means we should still add a nil map check before writing in the role name in case that ever is (or might become with future revisions) nil.

Copy link
Member Author

Choose a reason for hiding this comment

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

Secret IDs constructor is initializing the map. So, it is unlikely that metadata is nil. But still, I've added a nil check before assigning the role_name.

Copy link
Member

Choose a reason for hiding this comment

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

Now that we have the nil check below, allocating the map isn't strictly necessary but I'm good either way :)

if role.BindSecretID {
secretID := strings.TrimSpace(data.Get("secret_id").(string))
if secretID == "" {
Expand Down Expand Up @@ -262,7 +262,9 @@ func (b *backend) pathLoginUpdate(ctx context.Context, req *logical.Request, dat
}

// Always include the role name, for later filtering
metadata["role_name"] = role.name
if metadata != nil {
Copy link
Member

Choose a reason for hiding this comment

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

This makes the comment not actually be correct. If metadata is nil, you don't actually end up with a map with role_name. So either this is best effort, or if it's nil you need to create a map with role_name in it.

metadata["role_name"] = role.name
}

auth := &logical.Auth{
NumUses: role.TokenNumUses,
Expand Down
52 changes: 52 additions & 0 deletions builtin/credential/approle/path_login_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,58 @@ import (
"github.com/hashicorp/vault/logical"
)

func TestAppRole_BoundCIDRLogin(t *testing.T) {
var resp *logical.Response
var err error
b, s := createBackendWithStorage(t)

// Create a role with secret ID binding disabled and only bound cidr list
// enabled
resp, err = b.HandleRequest(context.Background(), &logical.Request{
Path: "role/testrole",
Operation: logical.CreateOperation,
Data: map[string]interface{}{
"bind_secret_id": false,
"bound_cidr_list": []string{"127.0.0.1/8"},
},
Storage: s,
})
if err != nil || (resp != nil && resp.IsError()) {
t.Fatalf("err:%v resp:%#v", err, resp)
}

// Read the role ID
resp, err = b.HandleRequest(context.Background(), &logical.Request{
Path: "role/testrole/role-id",
Operation: logical.ReadOperation,
Storage: s,
})
if err != nil || (resp != nil && resp.IsError()) {
t.Fatalf("err:%v resp:%#v", err, resp)
}

roleID := resp.Data["role_id"]

// Fill in the connection information and login with just the role ID
resp, err = b.HandleRequest(context.Background(), &logical.Request{
Path: "login",
Operation: logical.UpdateOperation,
Data: map[string]interface{}{
"role_id": roleID,
},
Storage: s,
Connection: &logical.Connection{RemoteAddr: "127.0.0.1"},
})
if err != nil || (resp != nil && resp.IsError()) {
t.Fatalf("err:%v resp:%#v", err, resp)
}

// Login should pass
if resp.Auth == nil {
t.Fatalf("expected login to succeed")
}
}

func TestAppRole_RoleLogin(t *testing.T) {
var resp *logical.Response
var err error
Expand Down