-
Notifications
You must be signed in to change notification settings - Fork 834
feat(runs): populate RunDetails.executed_by from forwarded auth headers #7547
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.
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
base: main
Are you sure you want to change the base?
Changes from 1 commit
8a4c1c5
32b10ea
5236f86
0a8dbb5
9598171
4f9befc
125d7a1
e7dfbf1
1d901bd
fdc0ea0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| -- Add created_by to actions: the OIDC subject of the identity that created the run. | ||
| -- Captured from the auth headers the load balancer forwards (it enforces auth), | ||
| -- and used to populate ActionMetadata.executed_by on read. | ||
| ALTER TABLE actions ADD COLUMN IF NOT EXISTS created_by VARCHAR(255); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,72 @@ | ||
| package service | ||
|
|
||
| import ( | ||
| "encoding/base64" | ||
| "encoding/json" | ||
| "net/http" | ||
| "strings" | ||
|
|
||
| "github.com/flyteorg/flyte/v2/gen/go/flyteidl2/common" | ||
| ) | ||
|
|
||
| const ( | ||
| // albIdentityHeader is set by ALB authenticate-oidc (browser/cookie path) and | ||
| // carries the OIDC subject (`sub`) directly. | ||
| albIdentityHeader = "X-Amzn-Oidc-Identity" | ||
| // authorizationHeader carries the Bearer token on the JWT-validation path | ||
| // (SDK/CLI). The load balancer validates it and forwards it unchanged. | ||
| authorizationHeader = "Authorization" | ||
| bearerPrefix = "Bearer " | ||
| ) | ||
|
|
||
| // subjectFromHeaders extracts the authenticated subject (OIDC `sub`) from the auth | ||
| // headers the load balancer forwards. Auth is enforced upstream (e.g. ALB OIDC / | ||
| // JWT validation), so the claims are trusted and only decoded here — not | ||
| // re-verified. Returns "" when no authenticated identity is present. | ||
| func subjectFromHeaders(h http.Header) string { | ||
| // authenticate-oidc (browser/cookie) path: subject is forwarded directly. | ||
| if sub := strings.TrimSpace(h.Get(albIdentityHeader)); sub != "" { | ||
| return sub | ||
| } | ||
| // JWT (SDK/CLI) path: read the `sub` claim from the forwarded Bearer token. | ||
| return subjectFromBearer(h.Get(authorizationHeader)) | ||
| } | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in |
||
|
|
||
| // subjectFromBearer returns the `sub` claim of a Bearer JWT without verifying its | ||
| // signature (the load balancer already validated it). Returns "" on any malformed input. | ||
| func subjectFromBearer(authz string) string { | ||
| if len(authz) <= len(bearerPrefix) || !strings.EqualFold(authz[:len(bearerPrefix)], bearerPrefix) { | ||
| return "" | ||
| } | ||
| parts := strings.Split(strings.TrimSpace(authz[len(bearerPrefix):]), ".") | ||
| if len(parts) != 3 { | ||
| return "" | ||
| } | ||
| payload, err := base64.RawURLEncoding.DecodeString(parts[1]) | ||
| if err != nil { | ||
| return "" | ||
| } | ||
| var claims struct { | ||
| Sub string `json:"sub"` | ||
| } | ||
| if err := json.Unmarshal(payload, &claims); err != nil { | ||
| return "" | ||
| } | ||
| return claims.Sub | ||
| } | ||
|
|
||
| // subjectOnlyIdentity builds a minimal EnrichedIdentity carrying just the subject. | ||
| // Mirrors the cloud transformer fallback; the standalone runs service has no | ||
| // identity service to enrich the subject into full user details (email, name, groups). | ||
| func subjectOnlyIdentity(subject string) *common.EnrichedIdentity { | ||
| if subject == "" { | ||
| return nil | ||
| } | ||
| return &common.EnrichedIdentity{ | ||
| Principal: &common.EnrichedIdentity_User{ | ||
| User: &common.User{ | ||
| Id: &common.UserIdentifier{Subject: subject}, | ||
| }, | ||
| }, | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,67 @@ | ||
| package service | ||
|
|
||
| import ( | ||
| "encoding/base64" | ||
| "net/http" | ||
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/assert" | ||
| ) | ||
|
|
||
| // bearerToken builds a syntactically valid (unsigned) JWT carrying the given sub claim. | ||
| func bearerToken(payloadJSON string) string { | ||
| enc := func(s string) string { return base64.RawURLEncoding.EncodeToString([]byte(s)) } | ||
| return "Bearer " + enc(`{"alg":"RS256"}`) + "." + enc(payloadJSON) + ".sig" | ||
| } | ||
|
|
||
| func TestSubjectFromHeaders(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| headers map[string]string | ||
| want string | ||
| }{ | ||
| { | ||
| name: "amzn oidc identity header (cookie path)", | ||
| headers: map[string]string{albIdentityHeader: "okta|user-123"}, | ||
| want: "okta|user-123", | ||
| }, | ||
| { | ||
| name: "amzn oidc identity header is trimmed", | ||
| headers: map[string]string{albIdentityHeader: " user-456 "}, | ||
| want: "user-456", | ||
| }, | ||
| { | ||
| name: "bearer token sub claim (jwt path)", | ||
| headers: map[string]string{authorizationHeader: bearerToken(`{"sub":"sdk-user-789","email":"a@b.com"}`)}, | ||
| want: "sdk-user-789", | ||
| }, | ||
| { | ||
| name: "amzn header takes precedence over bearer", | ||
| headers: map[string]string{ | ||
| albIdentityHeader: "cookie-user", | ||
| authorizationHeader: bearerToken(`{"sub":"bearer-user"}`), | ||
| }, | ||
| want: "cookie-user", | ||
| }, | ||
| {name: "no auth headers", headers: map[string]string{}, want: ""}, | ||
| {name: "non-bearer authorization", headers: map[string]string{authorizationHeader: "Basic abc"}, want: ""}, | ||
| {name: "malformed bearer (two segments)", headers: map[string]string{authorizationHeader: "Bearer a.b"}, want: ""}, | ||
| {name: "bearer without sub", headers: map[string]string{authorizationHeader: bearerToken(`{"email":"a@b.com"}`)}, want: ""}, | ||
| } | ||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| h := http.Header{} | ||
| for k, v := range tt.headers { | ||
| h.Set(k, v) | ||
| } | ||
| assert.Equal(t, tt.want, subjectFromHeaders(h)) | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func TestSubjectOnlyIdentity(t *testing.T) { | ||
| assert.Nil(t, subjectOnlyIdentity("")) | ||
|
|
||
| id := subjectOnlyIdentity("user-123") | ||
| assert.Equal(t, "user-123", id.GetUser().GetId().GetSubject()) | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -325,8 +325,12 @@ func (s *RunService) CreateRun( | |
| } | ||
| } | ||
|
|
||
| // Capture who created the run from the auth headers the load balancer forwards | ||
| // (it enforces auth upstream). Empty when there is no authenticated identity. | ||
| createdBy := subjectFromHeaders(req.Header()) | ||
|
|
||
| // Persist task spec and create run model | ||
| run, err := s.persistRunModel(ctx, runId, taskID, taskSpec, inputPrefix, runOutputBase, runSpec, request.GetSource(), triggerName, triggerTaskName, triggerRevision, triggerType) | ||
| run, err := s.persistRunModel(ctx, runId, taskID, taskSpec, inputPrefix, runOutputBase, runSpec, request.GetSource(), triggerName, triggerTaskName, triggerRevision, triggerType, createdBy) | ||
| if err != nil { | ||
| logger.Errorf(ctx, "Failed to create run: %v", err) | ||
| return nil, connect.NewError(connect.CodeInternal, err) | ||
|
|
@@ -372,6 +376,7 @@ func (s *RunService) persistRunModel( | |
| triggerName, triggerTaskName string, | ||
| triggerRevision int64, | ||
| triggerType string, | ||
| createdBy string, | ||
| ) (*models.Run, error) { | ||
| // Store task spec and compute digest | ||
| info := &workflow.RunInfo{InputsUri: inputPrefix + "/inputs.pb"} | ||
|
|
@@ -443,6 +448,7 @@ func (s *RunService) persistRunModel( | |
| RunSpec: runSpecBytes, | ||
| Attempts: 1, | ||
| RunSource: source.String(), | ||
| CreatedBy: nullStr(createdBy), | ||
| TriggerTaskName: nullStr(triggerTaskName), | ||
| TriggerName: nullStr(triggerName), | ||
| TriggerRevision: sql.NullInt64{Int64: triggerRevision, Valid: triggerRevision != 0}, | ||
|
|
@@ -1549,6 +1555,10 @@ func actionMetadataFromModel(action *models.Action) *workflow.ActionMetadata { | |
| } | ||
| } | ||
|
|
||
| if action.CreatedBy.Valid { | ||
| metadata.ExecutedBy = subjectOnlyIdentity(action.CreatedBy.String) | ||
| } | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed: added |
||
|
|
||
| if action.TriggerName.Valid { | ||
| metadata.TriggerName = action.TriggerName.String | ||
| metadata.TriggerId = &common.TriggerIdentifier{ | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed: the column is now
TEXT(the migration comment notes the OIDCsublength is IdP-dependent and can exceed 255).