CyberCoveLLC/threshold
1
0 Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Support multiple IDs in grants (#3263)

Browse Source 
This allows specifying multiple IDs in grants via an `ids` field, which
will eventually become the normal way to specify IDs. Internally, each
grant with multiple IDs turns into multiple ACL grants each with a
single ID; to make this distinction clearer and allow for better
separation of concerns an AclGrant type has been introduced, so really
multiple IDs in a Grant turn into multiple AclGrants.

The nice thing about doing it this way is that it becomes purely a
parsing concern; no actual validation logic has changed.

For backwards compatibility with grants already stored in roles, we do
not error out if `id` is specified. However, eventually we will simply
not support grants with `id` coming via the API; they will fail API
validation, such that new grants will require the use of the updated
`ids` field.

A follow-on PR will plumb this through the service handlers/API/CLI, but
I decided to do it in separate chunks to make reviewing easier, given
that they are easily separable into the internal and external aspects of
the change.

* Add CLI/API checks around using ID in grants (#3264)

* Plumb through some contexts to remove a lot of deprecated errors (#3268)
This commit is contained in:
Jeff Mitchell committed 2023-06-14 21:55:24 -04:00
1 parent 8acc73dc6e
commit feb3aea38f
25 files changed
+1043 -381
Download Patch File Download Diff File Expand all files Collapse all files

No files matched your search

internal/daemon/controller/auth/auth.go
+1
View File
@@ -656,6 +656,7 @@ func (v verifier) performAuthCheck(ctx context.Context) (
permsOpts = append(permsOpts, perms.WithAccountId(*userData.Account.Id))
}
parsed, err := perms.Parse(
ctx,
pair.ScopeId,
pair.Grant,
permsOpts...)
internal/daemon/controller/handlers/roles/role_service.go
+28 -10
View File
@@ -22,6 +22,7 @@ import (
"github.com/hashicorp/boundary/internal/types/resource"
"github.com/hashicorp/boundary/internal/types/scope"
pb "github.com/hashicorp/boundary/sdk/pbs/controller/api/resources/roles"
"github.com/hashicorp/boundary/version"
"github.com/hashicorp/go-secure-stdlib/strutil"
"google.golang.org/grpc/codes"
"google.golang.org/protobuf/types/known/wrapperspb"
@@ -401,7 +402,7 @@ func (s Service) RemoveRolePrincipals(ctx context.Context, req *pbs.RemoveRolePr
func (s Service) AddRoleGrants(ctx context.Context, req *pbs.AddRoleGrantsRequest) (*pbs.AddRoleGrantsResponse, error) {
const op = "roles.(Service).AddRoleGrants"
if err := validateAddRoleGrantsRequest(req); err != nil {
if err := validateAddRoleGrantsRequest(ctx, req); err != nil {
return nil, err
}
authResults := s.authResult(ctx, req.GetId(), action.AddGrants)
@@ -439,7 +440,7 @@ func (s Service) AddRoleGrants(ctx context.Context, req *pbs.AddRoleGrantsReques
func (s Service) SetRoleGrants(ctx context.Context, req *pbs.SetRoleGrantsRequest) (*pbs.SetRoleGrantsResponse, error) {
const op = "roles.(Service).SetRoleGrants"
if err := validateSetRoleGrantsRequest(req); err != nil {
if err := validateSetRoleGrantsRequest(ctx, req); err != nil {
return nil, err
}
authResults := s.authResult(ctx, req.GetId(), action.SetGrants)
@@ -477,7 +478,7 @@ func (s Service) SetRoleGrants(ctx context.Context, req *pbs.SetRoleGrantsReques
func (s Service) RemoveRoleGrants(ctx context.Context, req *pbs.RemoveRoleGrantsRequest) (*pbs.RemoveRoleGrantsResponse, error) {
const op = "roles.(Service).RemoveRoleGrants"
if err := validateRemoveRoleGrantsRequest(req); err != nil {
if err := validateRemoveRoleGrantsRequest(ctx, req); err != nil {
return nil, err
}
authResults := s.authResult(ctx, req.GetId(), action.RemoveGrants)
@@ -853,7 +854,7 @@ func toProto(ctx context.Context, in *iam.Role, principals []*iam.PrincipalRole,
}
if outputFields.Has(globals.GrantsField) {
for _, g := range grants {
parsed, err := perms.Parse(in.GetGrantScopeId(), g.GetRawGrant())
parsed, err := perms.Parse(ctx, in.GetGrantScopeId(), g.GetRawGrant())
if err != nil {
// This should never happen as we validate on the way in, but let's
// return what we can since we are still returning the raw grant
@@ -869,6 +870,7 @@ func toProto(ctx context.Context, in *iam.Role, principals []*iam.PrincipalRole,
Canonical: g.GetCanonicalGrant(),
Json: &pb.GrantJson{
Id: parsed.Id(),
Ids: parsed.Ids(),
Type: parsed.Type().String(),
Actions: actions,
},
@@ -1039,7 +1041,7 @@ func validateRemoveRolePrincipalsRequest(req *pbs.RemoveRolePrincipalsRequest) e
return nil
}
func validateAddRoleGrantsRequest(req *pbs.AddRoleGrantsRequest) error {
func validateAddRoleGrantsRequest(ctx context.Context, req *pbs.AddRoleGrantsRequest) error {
badFields := map[string]string{}
if !handlers.ValidId(handlers.Id(req.GetId()), globals.RolePrefix) {
badFields["id"] = "Incorrectly formatted identifier."
@@ -1055,7 +1057,7 @@ func validateAddRoleGrantsRequest(req *pbs.AddRoleGrantsRequest) error {
badFields["grant_strings"] = "Grant strings must not be empty."
break
}
grant, err := perms.Parse("p_anything", v)
grant, err := perms.Parse(ctx, "p_anything", v)
if err != nil {
badFields["grant_strings"] = fmt.Sprintf("Improperly formatted grant %q.", v)
break
@@ -1066,6 +1068,14 @@ func validateAddRoleGrantsRequest(req *pbs.AddRoleGrantsRequest) error {
badFields["grant_strings"] = fmt.Sprintf("Action %q has been deprecated and is not allowed to be set in grants. Use %q instead.", actStr, depAct.String())
}
}
switch {
case grant.Id() == "":
// Nothing
case version.SupportsFeature(version.Binary, version.SupportIdInGrants):
// This will warn on the CLI
default:
badFields["grant_strings"] = fmt.Sprintf("Grant %q uses the %q field which is no longer supported. Please use %q instead.", v, "id", "ids")
}
}
if len(badFields) > 0 {
return handlers.InvalidArgumentErrorf("Errors in provided fields.", badFields)
@@ -1073,7 +1083,7 @@ func validateAddRoleGrantsRequest(req *pbs.AddRoleGrantsRequest) error {
return nil
}
func validateSetRoleGrantsRequest(req *pbs.SetRoleGrantsRequest) error {
func validateSetRoleGrantsRequest(ctx context.Context, req *pbs.SetRoleGrantsRequest) error {
badFields := map[string]string{}
if !handlers.ValidId(handlers.Id(req.GetId()), globals.RolePrefix) {
badFields["id"] = "Incorrectly formatted identifier."
@@ -1086,7 +1096,7 @@ func validateSetRoleGrantsRequest(req *pbs.SetRoleGrantsRequest) error {
badFields["grant_strings"] = "Grant strings must not be empty."
break
}
grant, err := perms.Parse("p_anything", v)
grant, err := perms.Parse(ctx, "p_anything", v)
if err != nil {
badFields["grant_strings"] = fmt.Sprintf("Improperly formatted grant %q.", v)
break
@@ -1097,6 +1107,14 @@ func validateSetRoleGrantsRequest(req *pbs.SetRoleGrantsRequest) error {
badFields["grant_strings"] = fmt.Sprintf("Action %q has been deprecated and is not allowed to be set in grants. Use %q instead.", actStr, depAct.String())
}
}
switch {
case grant.Id() == "":
// Nothing
case version.SupportsFeature(version.Binary, version.SupportIdInGrants):
// This will warn on the CLI
default:
badFields["grant_strings"] = fmt.Sprintf("Grant %q uses the %q field which is no longer supported. Please use %q instead.", v, "id", "ids")
}
}
if len(badFields) > 0 {
return handlers.InvalidArgumentErrorf("Errors in provided fields.", badFields)
@@ -1104,7 +1122,7 @@ func validateSetRoleGrantsRequest(req *pbs.SetRoleGrantsRequest) error {
return nil
}
func validateRemoveRoleGrantsRequest(req *pbs.RemoveRoleGrantsRequest) error {
func validateRemoveRoleGrantsRequest(ctx context.Context, req *pbs.RemoveRoleGrantsRequest) error {
badFields := map[string]string{}
if !handlers.ValidId(handlers.Id(req.GetId()), globals.RolePrefix) {
badFields["id"] = "Incorrectly formatted identifier."
@@ -1120,7 +1138,7 @@ func validateRemoveRoleGrantsRequest(req *pbs.RemoveRoleGrantsRequest) error {
badFields["grant_strings"] = "Grant strings must not be empty."
break
}
if _, err := perms.Parse("p_anything", v); err != nil {
if _, err := perms.Parse(ctx, "p_anything", v); err != nil {
badFields["grant_strings"] = fmt.Sprintf("Improperly formatted grant %q.", v)
break
}
internal/daemon/controller/handlers/roles/role_service_test.go
+50 -29
View File
@@ -24,6 +24,7 @@ import (
"github.com/hashicorp/boundary/internal/types/scope"
pb "github.com/hashicorp/boundary/sdk/pbs/controller/api/resources/roles"
"github.com/hashicorp/boundary/sdk/pbs/controller/api/resources/scopes"
"github.com/hashicorp/boundary/version"
"github.com/kr/pretty"
"google.golang.org/genproto/protobuf/field_mask"
"google.golang.org/grpc/codes"
@@ -571,7 +572,7 @@ func TestCreate(t *testing.T) {
func TestUpdate(t *testing.T) {
grantString := "id=*;type=*;actions=*"
g, err := perms.Parse("global", grantString)
g, err := perms.Parse(context.Background(), "global", grantString)
require.NoError(t, err)
_, actions := g.Actions()
grant := &pb.Grant{
@@ -579,6 +580,7 @@ func TestUpdate(t *testing.T) {
Canonical: g.CanonicalString(),
Json: &pb.GrantJson{
Id: g.Id(),
Ids: g.Ids(),
Type: g.Type().String(),
Actions: actions,
},
@@ -1632,7 +1634,7 @@ func checkEqualGrants(t *testing.T, expected []string, got *pb.Role) {
require.Equal(len(expected), len(got.GrantStrings))
require.Equal(len(expected), len(got.Grants))
for i, v := range expected {
parsed, err := perms.Parse("o_abc123", v)
parsed, err := perms.Parse(context.Background(), "o_abc123", v)
require.NoError(err)
assert.Equal(expected[i], got.GrantStrings[i])
assert.Equal(expected[i], got.Grants[i].GetRaw())
@@ -1640,6 +1642,7 @@ func checkEqualGrants(t *testing.T, expected []string, got *pb.Role) {
j := got.Grants[i].GetJson()
require.NotNil(j)
assert.Equal(parsed.Id(), j.GetId())
assert.Equal(parsed.Ids(), j.GetIds())
assert.Equal(parsed.Type().String(), j.GetType())
_, acts := parsed.Actions()
assert.Equal(acts, j.GetActions())
@@ -1666,34 +1669,43 @@ func TestAddGrants(t *testing.T) {
}{
{
name: "Add grant on empty role",
add: []string{"id=*;type=*;actions=delete"},
result: []string{"id=*;type=*;actions=delete"},
add: []string{"ids=*;type=*;actions=delete"},
result: []string{"ids=*;type=*;actions=delete"},
},
{
name: "Add grant on role with grant",
existing: []string{"id=u_foo;actions=read"},
add: []string{"id=*;type=*;actions=delete"},
result: []string{"id=u_foo;actions=read", "id=*;type=*;actions=delete"},
add: []string{"ids=*;type=*;actions=delete"},
result: []string{"id=u_foo;actions=read", "ids=*;type=*;actions=delete"},
},
{
name: "Add duplicate grant on role with grant",
existing: []string{"id=u_fooaA1;actions=read"},
add: []string{"id=*;type=*;actions=delete", "id=*;type=*;actions=delete"},
result: []string{"id=u_fooaA1;actions=read", "id=*;type=*;actions=delete"},
add: []string{"ids=*;type=*;actions=delete", "ids=*;type=*;actions=delete"},
result: []string{"id=u_fooaA1;actions=read", "ids=*;type=*;actions=delete"},
},
{
name: "Add grant matching existing grant",
existing: []string{"id=u_foo;actions=read", "id=*;type=*;actions=delete"},
add: []string{"id=*;type=*;actions=delete"},
existing: []string{"ids=u_foo;actions=read", "ids=*;type=*;actions=delete"},
add: []string{"ids=*;type=*;actions=delete"},
wantErr: true,
},
{
name: "Check deprecation",
name: "Check add-host-sets deprecation",
existing: []string{"id=u_foo;actions=read", "id=*;type=*;actions=delete"},
add: []string{"id=*;type=target;actions=add-host-sets"},
add: []string{"ids=*;type=target;actions=add-host-sets"},
wantErr: true,
wantErrContains: "Use \\\"add-host-sources\\\" instead",
},
{
name: "Check id field deprecation",
existing: []string{"id=u_fooaA1;actions=read"},
add: []string{"id=*;type=*;actions=delete"},
result: []string{"id=u_fooaA1;actions=read", "id=*;type=*;actions=delete"},
wantErr: func() bool {
return !version.SupportsFeature(version.Binary, version.SupportIdInGrants)
}(),
},
}
for _, tc := range addCases {
@@ -1739,7 +1751,7 @@ func TestAddGrants(t *testing.T) {
name: "Bad Version",
req: &pbs.AddRoleGrantsRequest{
Id: role.GetPublicId(),
GrantStrings: []string{"id=*;type=*;actions=create"},
GrantStrings: []string{"ids=*;type=*;actions=create"},
Version: role.GetVersion() + 2,
},
err: handlers.ApiErrorWithCode(codes.Internal),
@@ -1748,7 +1760,7 @@ func TestAddGrants(t *testing.T) {
name: "Bad Role Id",
req: &pbs.AddRoleGrantsRequest{
Id: "bad id",
GrantStrings: []string{"id=*;type=*;actions=create"},
GrantStrings: []string{"ids=*;type=*;actions=create"},
Version: role.GetVersion(),
},
err: handlers.ApiErrorWithCode(codes.InvalidArgument),
@@ -1757,7 +1769,7 @@ func TestAddGrants(t *testing.T) {
name: "Unparseable Grant",
req: &pbs.AddRoleGrantsRequest{
Id: role.GetPublicId(),
GrantStrings: []string{"id=*;type=*;actions=create", "unparseable"},
GrantStrings: []string{"ids=*;type=*;actions=create", "unparseable"},
Version: role.GetVersion(),
},
err: handlers.ApiErrorWithCode(codes.InvalidArgument),
@@ -1766,7 +1778,7 @@ func TestAddGrants(t *testing.T) {
name: "Empty Grant",
req: &pbs.AddRoleGrantsRequest{
Id: role.GetPublicId(),
GrantStrings: []string{"id=*;type=*;actions=create", ""},
GrantStrings: []string{"ids=*;type=*;actions=create", ""},
Version: role.GetVersion(),
},
err: handlers.ApiErrorWithCode(codes.InvalidArgument),
@@ -1805,26 +1817,26 @@ func TestSetGrants(t *testing.T) {
}{
{
name: "Set grant on empty role",
set: []string{"id=*;type=*;actions=delete"},
result: []string{"id=*;type=*;actions=delete"},
set: []string{"ids=*;type=*;actions=delete"},
result: []string{"ids=*;type=*;actions=delete"},
},
{
name: "Set grant on role with grant",
existing: []string{"id=u_foo;actions=read"},
set: []string{"id=*;type=*;actions=delete"},
result: []string{"id=*;type=*;actions=delete"},
set: []string{"ids=*;type=*;actions=delete"},
result: []string{"ids=*;type=*;actions=delete"},
},
{
name: "Set grant matching existing grant",
existing: []string{"id=u_foo;actions=read", "id=*;type=*;actions=delete"},
set: []string{"id=*;type=*;actions=delete"},
result: []string{"id=*;type=*;actions=delete"},
set: []string{"ids=*;type=*;actions=delete"},
result: []string{"ids=*;type=*;actions=delete"},
},
{
name: "Set duplicate grant matching existing grant",
existing: []string{"id=u_foo;actions=read", "id=*;type=*;actions=delete"},
set: []string{"id=*;type=*;actions=delete", "id=*;type=*;actions=delete"},
result: []string{"id=*;type=*;actions=delete"},
set: []string{"ids=*;type=*;actions=delete", "ids=*;type=*;actions=delete"},
result: []string{"ids=*;type=*;actions=delete"},
},
{
name: "Set empty on role",
@@ -1833,12 +1845,21 @@ func TestSetGrants(t *testing.T) {
result: nil,
},
{
name: "Check deprecation",
name: "Check add-host-sets deprecation",
existing: []string{"id=u_foo;actions=read", "id=*;type=*;actions=delete"},
set: []string{"id=*;type=target;actions=add-host-sets"},
set: []string{"ids=*;type=target;actions=add-host-sets"},
wantErr: true,
wantErrContains: "Use \\\"add-host-sources\\\" instead",
},
{
name: "Check id field deprecation",
existing: []string{"id=u_fooaA1;actions=read"},
set: []string{"id=*;type=*;actions=delete"},
result: []string{"id=*;type=*;actions=delete"},
wantErr: func() bool {
return !version.SupportsFeature(version.Binary, version.SupportIdInGrants)
}(),
},
}
for _, tc := range setCases {
@@ -1885,7 +1906,7 @@ func TestSetGrants(t *testing.T) {
name: "Bad Version",
req: &pbs.SetRoleGrantsRequest{
Id: role.GetPublicId(),
GrantStrings: []string{"id=*;type=*;actions=create"},
GrantStrings: []string{"ids=*;type=*;actions=create"},
Version: role.GetVersion() + 2,
},
err: handlers.ApiErrorWithCode(codes.Internal),
@@ -1894,7 +1915,7 @@ func TestSetGrants(t *testing.T) {
name: "Bad Role Id",
req: &pbs.SetRoleGrantsRequest{
Id: "bad id",
GrantStrings: []string{"id=*;type=*;actions=create"},
GrantStrings: []string{"ids=*;type=*;actions=create"},
Version: role.GetVersion(),
},
err: handlers.ApiErrorWithCode(codes.InvalidArgument),
@@ -1903,7 +1924,7 @@ func TestSetGrants(t *testing.T) {
name: "Unparsable grant",
req: &pbs.SetRoleGrantsRequest{
Id: role.GetPublicId(),
GrantStrings: []string{"id=*;type=*;actions=create", "unparseable"},
GrantStrings: []string{"ids=*;type=*;actions=create", "unparseable"},
Version: role.GetVersion(),
},
err: handlers.ApiErrorWithCode(codes.InvalidArgument),