Skip to content

Commit 0536d2c

Browse files
AvielSegevAvielSegev
committed
ECOPROJECT-4721 | fix: Validate JWT source_id in agent handlers
Signed-off-by: Aviel Segev <asegev@redhat.com>
1 parent fca2941 commit 0536d2c

6 files changed

Lines changed: 195 additions & 44 deletions

File tree

‎internal/api_server/agentserver/server.go‎

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -72,15 +72,9 @@ func (s *AgentServer) Run(ctx context.Context) error {
7272
metricMiddleware.Handler,
7373
middleware.RequestID,
7474
log.ConditionalLogger(s.cfg.Service.LogLevel, zap.L(), "router_agent"),
75+
auth.NewAgentAuthenticator(s.cfg.Service.Auth.AgentAuthenticationEnabled, s.store).Authenticator,
7576
)
7677

77-
zap.S().Infow("agent authentication", "enabled", s.cfg.Service.Auth.AgentAuthenticationEnabled)
78-
if s.cfg.Service.Auth.AgentAuthenticationEnabled {
79-
router.Use(
80-
auth.NewAgentAuthenticator(s.store).Authenticator,
81-
)
82-
}
83-
8478
router.Use(
8579
middleware.Recoverer,
8680
oapimiddleware.OapiRequestValidatorWithOptions(swagger, &oapiOpts),

‎internal/auth/agent_authenticator.go‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,17 @@ type AgentAuthenticator struct {
7676
store store.Store
7777
}
7878

79-
func NewAgentAuthenticator(store store.Store) *AgentAuthenticator {
79+
func NewAgentAuthenticator(enabled bool, store store.Store) Authenticator {
80+
if enabled {
81+
zap.S().Named("auth").Info("agent authentication enabled")
82+
return newProductionAgentAuthenticator(store)
83+
}
84+
85+
zap.S().Named("auth").Info("agent authentication disabled, using none authenticator")
86+
return NewNoneAgentAuthenticator()
87+
}
88+
89+
func newProductionAgentAuthenticator(store store.Store) *AgentAuthenticator {
8090
return &AgentAuthenticator{store: store}
8191
}
8292

‎internal/auth/agent_authenticator_test.go‎

Lines changed: 29 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package auth_test
22

33
import (
4+
"bytes"
45
"crypto/rand"
56
"crypto/rsa"
67
"crypto/x509"
@@ -60,7 +61,7 @@ var _ = Describe("agent authentication", Ordered, func() {
6061

6162
token := generateAgentToken("kid", "my_source", "GothamCity", privateKey)
6263

63-
agentAuthenticator := auth.NewAgentAuthenticator(s)
64+
agentAuthenticator := auth.NewAgentAuthenticator(true, s).(*auth.AgentAuthenticator)
6465

6566
agentJwt, err := agentAuthenticator.Authenticate(token)
6667
Expect(err).To(BeNil())
@@ -87,7 +88,7 @@ var _ = Describe("agent authentication", Ordered, func() {
8788

8889
token := generateAgentToken("missing-key-kid", "my_source", "GothamCity", privateKey)
8990

90-
agentAuthenticator := auth.NewAgentAuthenticator(s)
91+
agentAuthenticator := auth.NewAgentAuthenticator(true, s).(*auth.AgentAuthenticator)
9192

9293
_, err = agentAuthenticator.Authenticate(token)
9394
Expect(err).ToNot(BeNil())
@@ -112,7 +113,7 @@ var _ = Describe("agent authentication", Ordered, func() {
112113

113114
token := generateAgentToken("kid", "my_source", "GothamCity", signingKey)
114115

115-
agentAuthenticator := auth.NewAgentAuthenticator(s)
116+
agentAuthenticator := auth.NewAgentAuthenticator(true, s).(*auth.AgentAuthenticator)
116117

117118
_, err = agentAuthenticator.Authenticate(token)
118119
Expect(err).ToNot(BeNil())
@@ -140,7 +141,7 @@ var _ = Describe("agent authentication", Ordered, func() {
140141

141142
token := generateAgentToken("1234_kid", "my_source", "org_id", privateKey)
142143

143-
agentAuthenticator := auth.NewAgentAuthenticator(s)
144+
agentAuthenticator := auth.NewAgentAuthenticator(true, s)
144145
h := &handler{}
145146
ts := httptest.NewServer(agentAuthenticator.Authenticator(h))
146147
defer ts.Close()
@@ -159,6 +160,30 @@ var _ = Describe("agent authentication", Ordered, func() {
159160
})
160161
})
161162

163+
Context("none authenticator middleware", func() {
164+
It("successfully extracts sourceId from request body", func() {
165+
type handler struct{}
166+
serveHTTP := func(h *handler) http.HandlerFunc {
167+
return func(w http.ResponseWriter, r *http.Request) {
168+
w.WriteHeader(200)
169+
}
170+
}
171+
172+
noneAuthenticator := auth.NewNoneAgentAuthenticator()
173+
h := &handler{}
174+
ts := httptest.NewServer(noneAuthenticator.Authenticator(serveHTTP(h)))
175+
defer ts.Close()
176+
177+
body := `{"sourceId":"test-source-123"}`
178+
req, err := http.NewRequest(http.MethodPost, ts.URL, bytes.NewBufferString(body))
179+
Expect(err).To(BeNil())
180+
181+
resp, rerr := http.DefaultClient.Do(req)
182+
Expect(rerr).To(BeNil())
183+
Expect(resp.StatusCode).To(Equal(200))
184+
})
185+
})
186+
162187
})
163188

164189
func generateAgentToken(kid, sourceID, orgID string, signingKey *rsa.PrivateKey) string {

‎internal/auth/none_agent_authenticator.go‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
package auth
2+
3+
import (
4+
"encoding/json"
5+
"fmt"
6+
"net/http"
7+
"time"
8+
)
9+
10+
type NoneAgentAuthenticator struct{}
11+
12+
func NewNoneAgentAuthenticator() *NoneAgentAuthenticator {
13+
return &NoneAgentAuthenticator{}
14+
}
15+
16+
func (n *NoneAgentAuthenticator) Authenticator(next http.Handler) http.Handler {
17+
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
18+
var req struct {
19+
SourceID string `json:"sourceId"`
20+
}
21+
22+
if err := json.NewDecoder(r.Body).Decode(&req); err != nil {
23+
http.Error(w, fmt.Sprintf("missing source id from request body: %v", err), http.StatusBadRequest)
24+
return
25+
}
26+
27+
agentJWT := AgentJWT{
28+
ExpireAt: time.Now().Add(defaultExpirationPeriod * time.Hour),
29+
IssueAt: time.Now(),
30+
Issuer: "none",
31+
OrgID: "internal",
32+
SourceID: req.SourceID,
33+
}
34+
ctx := NewTokenContext(r.Context(), agentJWT)
35+
next.ServeHTTP(w, r.WithContext(ctx))
36+
})
37+
}

‎internal/handlers/v1alpha1/agent.go‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import (
77

88
v1alpha1 "github.com/kubev2v/migration-planner/api/v1alpha1/agent"
99
agentServer "github.com/kubev2v/migration-planner/internal/api/server/agent"
10+
"github.com/kubev2v/migration-planner/internal/auth"
1011
apiMappers "github.com/kubev2v/migration-planner/internal/handlers/v1alpha1/mappers"
1112
"github.com/kubev2v/migration-planner/internal/handlers/validator"
1213
"github.com/kubev2v/migration-planner/internal/service"
@@ -31,6 +32,13 @@ func (h *AgentHandler) UpdateSourceInventory(ctx context.Context, request agentS
3132
return agentServer.UpdateSourceInventory400JSONResponse{Message: "empty body"}, nil
3233
}
3334

35+
agentJWT := auth.MustHaveAgent(ctx)
36+
if agentJWT.SourceID != request.Id.String() {
37+
return agentServer.UpdateSourceInventory403JSONResponse{
38+
Message: fmt.Sprintf("agent is not authorized to update source %s", request.Id),
39+
}, nil
40+
}
41+
3442
data, err := json.Marshal(request.Body.Inventory)
3543
if err != nil {
3644
return agentServer.UpdateSourceInventory500JSONResponse{Message: err.Error()}, nil
@@ -66,6 +74,13 @@ func (h *AgentHandler) UpdateSourceInventory(ctx context.Context, request agentS
6674
// UpdateAgentStatus updates or creates a new agent resource
6775
// If the source has not agent than the agent is created.
6876
func (h *AgentHandler) UpdateAgentStatus(ctx context.Context, request agentServer.UpdateAgentStatusRequestObject) (agentServer.UpdateAgentStatusResponseObject, error) {
77+
agentJWT := auth.MustHaveAgent(ctx)
78+
if agentJWT.SourceID != request.Body.SourceId.String() {
79+
return agentServer.UpdateAgentStatus403JSONResponse{
80+
Message: fmt.Sprintf("agent is not authorized to update source %s", request.Body.SourceId),
81+
}, nil
82+
}
83+
6984
form := v1alpha1.AgentStatusUpdate(*request.Body)
7085

7186
v := validator.NewValidator()

‎internal/handlers/v1alpha1/agent_test.go‎

Lines changed: 102 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -47,11 +47,11 @@ var _ = Describe("agent service", Ordered, func() {
4747
Expect(tx.Error).To(BeNil())
4848
agentID := uuid.New()
4949

50-
user := auth.User{
51-
Username: "admin",
52-
Organization: "admin",
50+
agentJWT := auth.AgentJWT{
51+
OrgID: "admin",
52+
SourceID: sourceID.String(),
5353
}
54-
ctx := auth.NewTokenContext(context.TODO(), user)
54+
ctx := auth.NewTokenContext(context.TODO(), agentJWT)
5555

5656
srv := handlers.NewAgentHandler(service.NewAgentService(s))
5757
resp, err := srv.UpdateAgentStatus(ctx, server.UpdateAgentStatusRequestObject{
@@ -94,11 +94,11 @@ var _ = Describe("agent service", Ordered, func() {
9494
Expect(tx.Error).To(BeNil())
9595
agentID := uuid.New()
9696

97-
user := auth.User{
98-
Username: "admin",
99-
Organization: "admin",
97+
agentJWT := auth.AgentJWT{
98+
OrgID: "admin",
99+
SourceID: sourceID.String(),
100100
}
101-
ctx := auth.NewTokenContext(context.TODO(), user)
101+
ctx := auth.NewTokenContext(context.TODO(), agentJWT)
102102

103103
srv := handlers.NewAgentHandler(service.NewAgentService(s))
104104
resp, err := srv.UpdateAgentStatus(ctx, server.UpdateAgentStatusRequestObject{
@@ -128,11 +128,11 @@ var _ = Describe("agent service", Ordered, func() {
128128
tx = gormdb.Exec(fmt.Sprintf(insertAgentStm, agentID, "not-connected", "status-info-1", "cred_url-1", sourceID))
129129
Expect(tx.Error).To(BeNil())
130130

131-
user := auth.User{
132-
Username: "admin",
133-
Organization: "admin",
131+
agentJWT := auth.AgentJWT{
132+
OrgID: "admin",
133+
SourceID: sourceID.String(),
134134
}
135-
ctx := auth.NewTokenContext(context.TODO(), user)
135+
ctx := auth.NewTokenContext(context.TODO(), agentJWT)
136136

137137
srv := handlers.NewAgentHandler(service.NewAgentService(s))
138138
resp, err := srv.UpdateAgentStatus(ctx, server.UpdateAgentStatusRequestObject{
@@ -174,11 +174,11 @@ var _ = Describe("agent service", Ordered, func() {
174174
tx := gormdb.Exec(fmt.Sprintf(insertSourceWithUsernameStm, sourceID, "admin", "admin"))
175175
Expect(tx.Error).To(BeNil())
176176

177-
user := auth.User{
178-
Username: "batman",
179-
Organization: "wayne_enterprises",
177+
agentJWT := auth.AgentJWT{
178+
OrgID: "wayne_enterprises",
179+
SourceID: sourceID,
180180
}
181-
ctx := auth.NewTokenContext(context.TODO(), user)
181+
ctx := auth.NewTokenContext(context.TODO(), agentJWT)
182182

183183
srv := handlers.NewAgentHandler(service.NewAgentService(s))
184184
resp, err := srv.UpdateAgentStatus(ctx, server.UpdateAgentStatusRequestObject{
@@ -194,6 +194,41 @@ var _ = Describe("agent service", Ordered, func() {
194194
Expect(reflect.TypeOf(resp).String()).To(Equal(reflect.TypeOf(server.UpdateAgentStatus400JSONResponse{}).String()))
195195
})
196196

197+
It("rejects update when JWT source_id does not match target source", func() {
198+
sourceID := uuid.New()
199+
differentSourceID := uuid.New()
200+
tx := gormdb.Exec(fmt.Sprintf(insertSourceWithUsernameStm, sourceID, "admin", "admin"))
201+
Expect(tx.Error).To(BeNil())
202+
agentID := uuid.New()
203+
204+
// JWT has a different source_id than the one being updated
205+
agentJWT := auth.AgentJWT{
206+
OrgID: "admin",
207+
SourceID: differentSourceID.String(),
208+
}
209+
ctx := auth.NewTokenContext(context.TODO(), agentJWT)
210+
211+
srv := handlers.NewAgentHandler(service.NewAgentService(s))
212+
resp, err := srv.UpdateAgentStatus(ctx, server.UpdateAgentStatusRequestObject{
213+
Id: agentID,
214+
Body: &apiAgent.UpdateAgentStatusJSONRequestBody{
215+
Status: string(v1alpha1.AgentStatusWaitingForCredentials),
216+
StatusInfo: "waiting-for-credentials",
217+
CredentialUrl: "http://agent.com",
218+
Version: "version-1",
219+
SourceId: sourceID,
220+
},
221+
})
222+
Expect(err).To(BeNil())
223+
Expect(reflect.TypeOf(resp).String()).To(Equal(reflect.TypeOf(server.UpdateAgentStatus403JSONResponse{}).String()))
224+
225+
// Verify no agent was created
226+
count := -1
227+
tx = gormdb.Raw("SELECT COUNT(*) FROM agents;").Scan(&count)
228+
Expect(tx.Error).To(BeNil())
229+
Expect(count).To(Equal(0))
230+
})
231+
197232
AfterEach(func() {
198233
gormdb.Exec("DELETE FROM agents;")
199234
gormdb.Exec("DELETE FROM sources;")
@@ -209,11 +244,11 @@ var _ = Describe("agent service", Ordered, func() {
209244
tx = gormdb.Exec(fmt.Sprintf(insertAgentStm, agentID, "not-connected", "status-info-1", "cred_url-1", sourceID))
210245
Expect(tx.Error).To(BeNil())
211246

212-
user := auth.User{
213-
Username: "admin",
214-
Organization: "admin",
247+
agentJWT := auth.AgentJWT{
248+
OrgID: "admin",
249+
SourceID: sourceID.String(),
215250
}
216-
ctx := auth.NewTokenContext(context.TODO(), user)
251+
ctx := auth.NewTokenContext(context.TODO(), agentJWT)
217252

218253
srv := handlers.NewAgentHandler(service.NewAgentService(s))
219254
resp, err := srv.UpdateSourceInventory(ctx, server.UpdateSourceInventoryRequestObject{
@@ -251,11 +286,11 @@ var _ = Describe("agent service", Ordered, func() {
251286
tx = gormdb.Exec(fmt.Sprintf(insertAgentStm, secondAgentID, "not-connected", "status-info-1", "cred_url-1", sourceID))
252287
Expect(tx.Error).To(BeNil())
253288

254-
user := auth.User{
255-
Username: "admin",
256-
Organization: "admin",
289+
agentJWT := auth.AgentJWT{
290+
OrgID: "admin",
291+
SourceID: sourceID.String(),
257292
}
258-
ctx := auth.NewTokenContext(context.TODO(), user)
293+
ctx := auth.NewTokenContext(context.TODO(), agentJWT)
259294

260295
// first agent request
261296
srv := handlers.NewAgentHandler(service.NewAgentService(s))
@@ -308,11 +343,11 @@ var _ = Describe("agent service", Ordered, func() {
308343
tx = gormdb.Exec(fmt.Sprintf(insertAgentStm, uuid.New(), "not-connected", "status-info-1", "cred_url-1", secondSourceID))
309344
Expect(tx.Error).To(BeNil())
310345

311-
user := auth.User{
312-
Username: "admin",
313-
Organization: "admin",
346+
agentJWT := auth.AgentJWT{
347+
OrgID: "admin",
348+
SourceID: firstSourceID.String(),
314349
}
315-
ctx := auth.NewTokenContext(context.TODO(), user)
350+
ctx := auth.NewTokenContext(context.TODO(), agentJWT)
316351

317352
srv := handlers.NewAgentHandler(service.NewAgentService(s))
318353
resp, err := srv.UpdateSourceInventory(ctx, server.UpdateSourceInventoryRequestObject{
@@ -334,11 +369,11 @@ var _ = Describe("agent service", Ordered, func() {
334369
tx = gormdb.Exec(fmt.Sprintf(insertAgentStm, firstAgentID, "not-connected", "status-info-1", "cred_url-1", firstSourceID))
335370
Expect(tx.Error).To(BeNil())
336371

337-
user := auth.User{
338-
Username: "admin",
339-
Organization: "admin",
372+
agentJWT := auth.AgentJWT{
373+
OrgID: "admin",
374+
SourceID: firstSourceID.String(),
340375
}
341-
ctx := auth.NewTokenContext(context.TODO(), user)
376+
ctx := auth.NewTokenContext(context.TODO(), agentJWT)
342377

343378
srv := handlers.NewAgentHandler(service.NewAgentService(s))
344379
resp, err := srv.UpdateSourceInventory(ctx, server.UpdateSourceInventoryRequestObject{
@@ -367,6 +402,41 @@ var _ = Describe("agent service", Ordered, func() {
367402

368403
})
369404

405+
It("rejects inventory update when JWT source_id does not match target source", func() {
406+
sourceID := uuid.New()
407+
differentSourceID := uuid.New()
408+
agentID := uuid.New()
409+
tx := gormdb.Exec(fmt.Sprintf(insertSourceWithUsernameStm, sourceID, "admin", "admin"))
410+
Expect(tx.Error).To(BeNil())
411+
tx = gormdb.Exec(fmt.Sprintf(insertAgentStm, agentID, "not-connected", "status-info-1", "cred_url-1", sourceID))
412+
Expect(tx.Error).To(BeNil())
413+
414+
// JWT has a different source_id than the one being updated
415+
agentJWT := auth.AgentJWT{
416+
OrgID: "admin",
417+
SourceID: differentSourceID.String(),
418+
}
419+
ctx := auth.NewTokenContext(context.TODO(), agentJWT)
420+
421+
srv := handlers.NewAgentHandler(service.NewAgentService(s))
422+
resp, err := srv.UpdateSourceInventory(ctx, server.UpdateSourceInventoryRequestObject{
423+
Id: sourceID,
424+
Body: &apiAgent.SourceStatusUpdate{
425+
AgentId: agentID,
426+
Inventory: v1alpha1.Inventory{
427+
VcenterId: "vcenter",
428+
},
429+
},
430+
})
431+
Expect(err).To(BeNil())
432+
Expect(reflect.TypeOf(resp).String()).To(Equal(reflect.TypeOf(server.UpdateSourceInventory403JSONResponse{}).String()))
433+
434+
// Verify inventory was not updated
435+
source, err := s.Source().Get(ctx, sourceID)
436+
Expect(err).To(BeNil())
437+
Expect(source.Inventory).To(BeNil())
438+
})
439+
370440
AfterEach(func() {
371441
gormdb.Exec("DELETE FROM agents;")
372442
gormdb.Exec("DELETE FROM sources;")

0 commit comments

Comments
 (0)
⚡