Skip to content

Commit e49ee6d

Browse files
sguiheuxsguiheux
authored
fix(api): check rbac permission before deleting hatchery and region (#6428)
1 parent 4edfb0f commit e49ee6d

File tree

10 files changed

+237
-16
lines changed

10 files changed

+237
-16
lines changed

‎engine/api/rbac/dao_rbac.go

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import (
55
"time"
66

77
"github.com/go-gorp/gorp"
8+
"github.com/lib/pq"
89
"github.com/rockbears/log"
910

1011
"github.com/ovh/cds/engine/api/database/gorpmapping"
@@ -22,6 +23,11 @@ func LoadRBACByID(ctx context.Context, db gorp.SqlExecutor, id string, opts ...L
2223
return get(ctx, db, gorpmapping.NewQuery(query).Args(id), opts...)
2324
}
2425

26+
func LoadRBACByIDs(ctx context.Context, db gorp.SqlExecutor, IDs sdk.StringSlice, opts ...LoadOptionFunc) ([]sdk.RBAC, error) {
27+
query := `SELECT * FROM rbac WHERE id = ANY ($1)`
28+
return getAll(ctx, db, gorpmapping.NewQuery(query).Args(pq.StringArray(IDs)), opts...)
29+
}
30+
2531
// Insert a RBAC permission in database
2632
func Insert(ctx context.Context, db gorpmapper.SqlExecutorWithTx, rb *sdk.RBAC) error {
2733
if err := sdk.IsValidRBAC(rb); err != nil {
@@ -94,6 +100,32 @@ func Delete(_ context.Context, db gorpmapper.SqlExecutorWithTx, rb sdk.RBAC) err
94100
return nil
95101
}
96102

103+
func getAll(ctx context.Context, db gorp.SqlExecutor, q gorpmapping.Query, opts ...LoadOptionFunc) ([]sdk.RBAC, error) {
104+
var rs []sdk.RBAC
105+
var rbacsDB []rbac
106+
if err := gorpmapping.GetAll(ctx, db, q, &rbacsDB); err != nil {
107+
return nil, err
108+
}
109+
110+
for _, rbac := range rbacsDB {
111+
isValid, err := gorpmapping.CheckSignature(rbac, rbac.Signature)
112+
if err != nil {
113+
return nil, sdk.WrapError(err, "error when checking signature for rbac %s", rbac.ID)
114+
}
115+
if !isValid {
116+
log.Error(ctx, "rbac.get> rbac %s (%s) data corrupted", rbac.Name, rbac.ID)
117+
continue
118+
}
119+
for _, f := range opts {
120+
if err := f(ctx, db, &rbac); err != nil {
121+
return nil, err
122+
}
123+
}
124+
rs = append(rs, rbac.RBAC)
125+
}
126+
return rs, nil
127+
}
128+
97129
func get(ctx context.Context, db gorp.SqlExecutor, q gorpmapping.Query, opts ...LoadOptionFunc) (*sdk.RBAC, error) {
98130
var r sdk.RBAC
99131
var rbacDB rbac

‎engine/api/rbac/dao_rbac_hatchery.go

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,10 @@ package rbac
33
import (
44
"context"
55

6+
"github.com/go-gorp/gorp"
7+
"github.com/ovh/cds/sdk"
8+
"github.com/rockbears/log"
9+
610
"github.com/ovh/cds/engine/api/database/gorpmapping"
711
"github.com/ovh/cds/engine/gorpmapper"
812
)
@@ -13,3 +17,54 @@ func insertRBACHatchery(ctx context.Context, db gorpmapper.SqlExecutorWithTx, rb
1317
}
1418
return nil
1519
}
20+
21+
func getAllRBACHatchery(ctx context.Context, db gorp.SqlExecutor, q gorpmapping.Query) ([]rbacHatchery, error) {
22+
var rbacHatcheries []rbacHatchery
23+
if err := gorpmapping.GetAll(ctx, db, q, &rbacHatcheries); err != nil {
24+
return nil, err
25+
}
26+
27+
hatcheriesFiltered := make([]rbacHatchery, 0, len(rbacHatcheries))
28+
for _, rbacHatch := range rbacHatcheries {
29+
isValid, err := gorpmapping.CheckSignature(rbacHatch, rbacHatch.Signature)
30+
if err != nil {
31+
return nil, sdk.WrapError(err, "error when checking signature for rbac_hatchery %d", rbacHatch.ID)
32+
}
33+
if !isValid {
34+
log.Error(ctx, "rbac.getAllRBACHatchery> rbac_hatchery %d data corrupted", rbacHatch.ID)
35+
continue
36+
}
37+
hatcheriesFiltered = append(hatcheriesFiltered, rbacHatch)
38+
}
39+
return hatcheriesFiltered, nil
40+
}
41+
42+
func LoadRBACByHatcheryID(ctx context.Context, db gorp.SqlExecutor, hatcheryID string) ([]sdk.RBAC, error) {
43+
query := gorpmapping.NewQuery(`SELECT * FROM rbac_hatchery WHERE hatchery_id = $1`).Args(hatcheryID)
44+
rbHatcheries, err := getAllRBACHatchery(ctx, db, query)
45+
if err != nil {
46+
return nil, err
47+
}
48+
rbacIDs := make(sdk.StringSlice, 0)
49+
for _, rh := range rbHatcheries {
50+
rbacIDs = append(rbacIDs, rh.RbacID)
51+
}
52+
rbacIDs.Unique()
53+
54+
return LoadRBACByIDs(ctx, db, rbacIDs, LoadOptions.All)
55+
56+
}
57+
58+
func loadRBacIDsByHatcheryRegionID(ctx context.Context, db gorp.SqlExecutor, regionID string) ([]string, error) {
59+
query := gorpmapping.NewQuery(`SELECT * FROM rbac_hatchery WHERE region_id = $1`).Args(regionID)
60+
rbHatcheries, err := getAllRBACHatchery(ctx, db, query)
61+
if err != nil {
62+
return nil, err
63+
}
64+
rbacIDs := make(sdk.StringSlice, 0)
65+
for _, rh := range rbHatcheries {
66+
rbacIDs = append(rbacIDs, rh.RbacID)
67+
}
68+
rbacIDs.Unique()
69+
return rbacIDs, nil
70+
}

‎engine/api/rbac/dao_rbac_region.go

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,6 @@ func getAllRBACRegions(ctx context.Context, db gorp.SqlExecutor, q gorpmapping.Q
5757

5858
func LoadRegionIDsByRoleAndUserID(ctx context.Context, db gorp.SqlExecutor, role string, userID string) ([]sdk.RBACRegion, error) {
5959
// Get rbac_region_groups
60-
6160
rbacRegionGroups, err := loadRBACRegionGroupsByUserID(ctx, db, userID)
6261
if err != nil {
6362
return nil, err
@@ -105,3 +104,24 @@ func loadRBACRegionOnAllUsers(ctx context.Context, db gorp.SqlExecutor, role str
105104
q := gorpmapping.NewQuery("SELECT * from rbac_region WHERE role = $1 AND all_users = true").Args(role)
106105
return getAllRBACRegions(ctx, db, q)
107106
}
107+
108+
func LoadRBACByRegionID(ctx context.Context, db gorp.SqlExecutor, regionID string) ([]sdk.RBAC, error) {
109+
query := gorpmapping.NewQuery(`SELECT * FROM rbac_region WHERE region_id = $1`).Args(regionID)
110+
rbRegions, err := getAllRBACRegions(ctx, db, query)
111+
if err != nil {
112+
return nil, err
113+
}
114+
rbacIDs := make(sdk.StringSlice, 0)
115+
for _, rg := range rbRegions {
116+
rbacIDs = append(rbacIDs, rg.RbacID)
117+
}
118+
119+
ids, err := loadRBacIDsByHatcheryRegionID(ctx, db, regionID)
120+
if err != nil {
121+
return nil, err
122+
}
123+
rbacIDs = append(rbacIDs, ids...)
124+
rbacIDs.Unique()
125+
return LoadRBACByIDs(ctx, db, rbacIDs, LoadOptions.All)
126+
127+
}

‎engine/api/rbac/loader.go

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,12 +20,14 @@ var LoadOptions = struct {
2020
LoadRBACProject LoadOptionFunc
2121
LoadRBACHatchery LoadOptionFunc
2222
LoadRBACRegion LoadOptionFunc
23+
All LoadOptionFunc
2324
}{
2425
Default: loadDefault,
2526
LoadRBACGlobal: loadRBACGlobal,
2627
LoadRBACProject: loadRBACProject,
2728
LoadRBACHatchery: loadRBACHatchery,
2829
LoadRBACRegion: loadRBACRegion,
30+
All: loadAll,
2931
}
3032

3133
func loadDefault(ctx context.Context, db gorp.SqlExecutor, rbac *rbac) error {
@@ -38,6 +40,22 @@ func loadDefault(ctx context.Context, db gorp.SqlExecutor, rbac *rbac) error {
3840
return nil
3941
}
4042

43+
func loadAll(ctx context.Context, db gorp.SqlExecutor, rbac *rbac) error {
44+
if err := loadRBACGlobal(ctx, db, rbac); err != nil {
45+
return err
46+
}
47+
if err := loadRBACProject(ctx, db, rbac); err != nil {
48+
return err
49+
}
50+
if err := loadRBACRegion(ctx, db, rbac); err != nil {
51+
return err
52+
}
53+
if err := loadRBACHatchery(ctx, db, rbac); err != nil {
54+
return err
55+
}
56+
return nil
57+
}
58+
4159
func loadRBACProject(ctx context.Context, db gorp.SqlExecutor, rbac *rbac) error {
4260
query := "SELECT * FROM rbac_project WHERE rbac_id = $1"
4361
var rbacProjects []rbacProject

‎engine/api/v2_hatchery.go

Lines changed: 29 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import (
1010
"github.com/ovh/cds/engine/api/authentication"
1111
hatch_auth "github.com/ovh/cds/engine/api/authentication/hatchery"
1212
"github.com/ovh/cds/engine/api/hatchery"
13+
"github.com/ovh/cds/engine/api/rbac"
1314
"github.com/ovh/cds/engine/service"
1415
"github.com/ovh/cds/sdk"
1516
)
@@ -150,7 +151,12 @@ func (api *API) deleteHatcheryHandler() ([]service.RbacChecker, service.Handler)
150151
vars := mux.Vars(req)
151152
hatcheryIdentifier := vars["hatcheryIdentifier"]
152153

153-
reg, err := api.getHatcheryByIdentifier(ctx, hatcheryIdentifier)
154+
hatch, err := api.getHatcheryByIdentifier(ctx, hatcheryIdentifier)
155+
if err != nil {
156+
return err
157+
}
158+
159+
hatcheryPermissions, err := rbac.LoadRBACByHatcheryID(ctx, api.mustDB(), hatch.ID)
154160
if err != nil {
155161
return err
156162
}
@@ -161,7 +167,28 @@ func (api *API) deleteHatcheryHandler() ([]service.RbacChecker, service.Handler)
161167
}
162168
defer tx.Rollback() // nolint
163169

164-
if err := hatchery.Delete(tx, reg.ID); err != nil {
170+
// Remove all permissions on this hatchery
171+
for _, rbacPerm := range hatcheryPermissions {
172+
rbacHatcheries := make([]sdk.RBACHatchery, 0)
173+
for _, h := range rbacPerm.Hatcheries {
174+
if h.HatcheryID != hatch.ID {
175+
rbacHatcheries = append(rbacHatcheries, h)
176+
}
177+
}
178+
rbacPerm.Hatcheries = rbacHatcheries
179+
180+
if rbacPerm.IsEmpty() {
181+
if err := rbac.Delete(ctx, tx, rbacPerm); err != nil {
182+
return err
183+
}
184+
} else {
185+
if err := rbac.Update(ctx, tx, &rbacPerm); err != nil {
186+
return err
187+
}
188+
}
189+
}
190+
191+
if err := hatchery.Delete(tx, hatch.ID); err != nil {
165192
return err
166193
}
167194
return sdk.WithStack(tx.Commit())

‎engine/api/v2_hatchery_test.go

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import (
44
"context"
55
"encoding/json"
66
"fmt"
7+
"github.com/ovh/cds/engine/api/region"
78
"net/http/httptest"
89
"testing"
910
"time"
@@ -67,6 +68,7 @@ func Test_crudHatchery(t *testing.T) {
6768
api, db, _ := newTestAPI(t)
6869

6970
db.Exec("DELETE FROM hatchery")
71+
db.Exec("DELETE FROM rbac")
7072

7173
u, pass := assets.InsertLambdaUser(t, db)
7274

@@ -123,6 +125,23 @@ global:
123125
require.NoError(t, json.Unmarshal(wSignin.Body.Bytes(), &authSigninResponse))
124126
require.NotEmpty(t, authSigninResponse.Token)
125127

128+
reg := sdk.Region{Name: sdk.RandomString(10)}
129+
require.NoError(t, region.Insert(context.TODO(), db, &reg))
130+
131+
// Add RBAC
132+
r1 := fmt.Sprintf(`name: perm-hatchery-only
133+
hatcheries:
134+
- role: %s
135+
hatchery: %s
136+
region: %s
137+
`, sdk.HatcheryRoleSpawn, hatcheryGet.Name, reg.Name)
138+
var rbac1 sdk.RBAC
139+
require.NoError(t, yaml.Unmarshal([]byte(r1), &rbac1))
140+
rLoader := NewRBACLoader(db)
141+
rLoader.FillRBACWithIDs(context.TODO(), &rbac1)
142+
143+
require.NoError(t, rbac.Insert(context.TODO(), db, &rbac1))
144+
126145
// Then Delete hatchery
127146
uriDelete := api.Router.GetRouteV2("DELETE", api.deleteHatcheryHandler, map[string]string{"hatcheryIdentifier": h.Name})
128147
test.NotEmpty(t, uriDelete)
@@ -142,4 +161,9 @@ global:
142161
var hs []sdk.Hatchery
143162
require.NoError(t, json.Unmarshal(wList.Body.Bytes(), &hs))
144163
require.Len(t, hs, 0)
164+
165+
// Check rbac deletion
166+
_, err := rbac.LoadRBACByName(context.TODO(), db, rbac1.Name, rbac.LoadOptions.All)
167+
require.Error(t, err)
168+
require.True(t, sdk.ErrorIs(err, sdk.ErrNotFound))
145169
}

‎engine/api/v2_rbac.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -267,7 +267,7 @@ func (rl *RBACLoader) fillRBACRegionWithNames(ctx context.Context, rbacRegion *s
267267
rbacRegion.RBACGroupsName = make([]string, 0, len(rbacRegion.RBACGroupsIDs))
268268
for _, groupID := range rbacRegion.RBACGroupsIDs {
269269
groupName := rl.groupCache[groupID]
270-
if groupID == 0 {
270+
if groupName == "" {
271271
groupDB, err := group.LoadByID(ctx, rl.db, groupID)
272272
if err != nil {
273273
return err

‎engine/api/v2_region.go

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -108,12 +108,45 @@ func (api *API) deleteRegionHandler() ([]service.RbacChecker, service.Handler) {
108108
return err
109109
}
110110

111+
rbacRegions, err := rbac.LoadRBACByRegionID(ctx, api.mustDB(), reg.ID)
112+
if err != nil {
113+
return err
114+
}
115+
111116
tx, err := api.mustDB().Begin()
112117
if err != nil {
113118
return sdk.WithStack(err)
114119
}
115120
defer tx.Rollback() // nolint
116121

122+
for _, rbacPerm := range rbacRegions {
123+
rbacPermRegions := make([]sdk.RBACRegion, 0)
124+
for _, r := range rbacPerm.Regions {
125+
if r.RegionID != reg.ID {
126+
rbacPermRegions = append(rbacPermRegions, r)
127+
}
128+
}
129+
rbacPerm.Regions = rbacPermRegions
130+
131+
rbacPermHatcheries := make([]sdk.RBACHatchery, 0)
132+
for _, h := range rbacPerm.Hatcheries {
133+
if h.RegionID != reg.ID {
134+
rbacPermHatcheries = append(rbacPermHatcheries, h)
135+
}
136+
}
137+
rbacPerm.Hatcheries = rbacPermHatcheries
138+
139+
if rbacPerm.IsEmpty() {
140+
if err := rbac.Delete(ctx, tx, rbacPerm); err != nil {
141+
return err
142+
}
143+
} else {
144+
if err := rbac.Update(ctx, tx, &rbacPerm); err != nil {
145+
return err
146+
}
147+
}
148+
}
149+
117150
if err := region.Delete(tx, reg.ID); err != nil {
118151
return err
119152
}

0 commit comments

Comments
 (0)