-
Notifications
You must be signed in to change notification settings - Fork 113
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
[A2-835] make custom endpoints projects aware for iam v2.1 #380
Changes from all commits
1c15696
3095ddc
35f142a
44e870e
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 |
---|---|---|
|
@@ -9,7 +9,6 @@ import ( | |
"strings" | ||
|
||
"github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus/ctxlogrus" | ||
"github.com/grpc-ecosystem/grpc-gateway/runtime" | ||
"github.com/sirupsen/logrus" | ||
"google.golang.org/grpc" | ||
"google.golang.org/grpc/codes" | ||
|
@@ -42,7 +41,7 @@ type SwitchingAuthorizationHandler interface { | |
GRPCAuthorizationHandler | ||
SwitchingFilterHandler | ||
IsAuthorized(ctx context.Context, subjects []string, | ||
resourceV1, actionV1, resourceV2, actionV2 string) (AnnotatedAuthorizationResponse, error) | ||
resourceV1, actionV1, resourceV2, actionV2 string, projects []string) (AnnotatedAuthorizationResponse, error) | ||
} | ||
|
||
type SwitchingFilterHandler interface { | ||
|
@@ -75,7 +74,7 @@ type GRPCAuthorizationHandler interface { | |
} | ||
|
||
type HTTPAuthorizationHandler interface { | ||
IsAuthorized(ctx context.Context, subjects []string, resource, action string) (AuthorizationResponse, error) | ||
IsAuthorized(ctx context.Context, subjects []string, resource, action string, projects []string) (AuthorizationResponse, error) | ||
} | ||
|
||
type IntrospectionHandler interface { | ||
|
@@ -90,10 +89,12 @@ type AuthorizationHandler interface { | |
} | ||
|
||
type AuthorizationResponse interface { | ||
Ctx() context.Context | ||
GetAuthorized() bool | ||
} | ||
|
||
type AnnotatedAuthorizationResponse interface { | ||
AuthorizationResponse | ||
Err() error | ||
} | ||
|
||
|
@@ -148,8 +149,7 @@ func (a *authInterceptor) UnaryServerInterceptor() grpc.UnaryServerInterceptor { | |
subs = append(authResponse.Teams, authResponse.Subject) | ||
} | ||
|
||
projectHeaderEntries := md.Get(runtime.MetadataPrefix + "projects") | ||
projects := getProjectsFromMetadata(projectHeaderEntries) | ||
projects := auth_context.ProjectsFromMetadata(md) | ||
|
||
ctx, err = a.authz.Handle(authCtx, subs, projects, req) | ||
if err != nil { | ||
|
@@ -182,7 +182,7 @@ func getProjectsFromMetadata(projectHeaderEntries []string) []string { | |
for _, entry := range projectHeaderEntries { | ||
for _, project := range strings.Split(entry, ",") { | ||
newProject := strings.TrimSpace(project) | ||
if _, value := keys[newProject]; !value { | ||
if !keys[newProject] { | ||
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. I'm sort of surprised this worked-- I would expect that if the value of Don't we need to both check the value and the success of extracting it ( Maybe I'm missing something 🤔 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.
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. @msorens I know that it reads that way, but I was under the impression that golang didn't necessarily behave how we'd expect here. I've tried this in a playground and I keep getting the error I'd expect (invalid operation)-- would you mind linking me to an example playground? 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. https://play.golang.org/p/y1GXa4D4vdv does that do the trick? Go really isn't dynamic at all, but non-existing values in maps will return their zero value, as @msorens suggested. Here's the spec section:
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. Ugh thank you for the example-- I'd read the docs but was having trouble manually testing . . . I had my map constructed incorrectly (the value as a string, not a bool) 🙄 |
||
keys[newProject] = true | ||
projects = append(projects, newProject) | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -83,13 +83,44 @@ func (c *client) Handle(ctx context.Context, subjects []string, projectsToFilter | |
return auth_context.NewContext(ctx, subjects, projects, resource, action, middleware.AuthV2.String()), nil | ||
} | ||
|
||
type resp struct { | ||
ctx context.Context | ||
authorized bool | ||
} | ||
|
||
|
||
func (r *resp) Ctx() context.Context { | ||
return r.ctx | ||
} | ||
|
||
func (r *resp) GetAuthorized() bool { | ||
return r.authorized | ||
} | ||
|
||
func (c *client) IsAuthorized(ctx context.Context, subjects []string, resource, action string, | ||
) (middleware.AuthorizationResponse, error) { | ||
return c.client.IsAuthorized(ctx, &authz.IsAuthorizedReq{ | ||
Subjects: subjects, | ||
Resource: resource, | ||
Action: action, | ||
projectsToFilter []string) (middleware.AuthorizationResponse, error) { | ||
log := ctxlogrus.Extract(ctx) | ||
filteredResp, err := c.client.ProjectsAuthorized(ctx, &authz.ProjectsAuthorizedReq{ | ||
Subjects: subjects, | ||
Resource: resource, | ||
Action: action, | ||
ProjectsFilter: projectsToFilter, | ||
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. This is looking similar to the 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. Is it? I'll outline how the authorizer is supposed to work in another comment, give me a minute 😉 |
||
}) | ||
if err != nil { | ||
if status.Convert(err).Code() == codes.FailedPrecondition { | ||
return nil, err | ||
} | ||
log.WithError(err).Error("error authorizing request") | ||
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. I'm curious about the choice to log here-- can we not guarantee that the error itself will surface in the logs? If that's the case, I imagine it would be helpful to have the subj/resource/etc in the log message as well as the error. 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. 😄 a) I don't think I've spent much time deliberating the choice of log here. Good enough? 😉 |
||
return nil, status.Errorf(codes.PermissionDenied, | ||
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. Should this message/error also include projects? 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. As this is the gatekeeper for both v2 and v2.1, and it does not know which for any given call, we cannot appropriately include projects. |
||
"error authorizing action %q on resource %q for subjects %q: %s", | ||
action, resource, subjects, err.Error()) | ||
} | ||
projects := filteredResp.Projects | ||
|
||
return &resp{ | ||
ctx: auth_context.NewContext(ctx, subjects, projects, resource, action, middleware.AuthV2.String()), | ||
authorized: len(projects) != 0, | ||
}, nil | ||
} | ||
|
||
func (c *client) FilterAuthorizedPairs(ctx context.Context, subjects []string, inputPairs []*pairs.Pair, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -58,6 +58,7 @@ import ( | |
|
||
// anything else | ||
"github.com/chef/automate/components/automate-gateway/gateway/middleware" | ||
"github.com/chef/automate/lib/grpc/auth_context" | ||
"github.com/chef/automate/lib/grpc/service_authn" | ||
) | ||
|
||
|
@@ -372,7 +373,8 @@ func (s *Server) ProfileCreateHandler(w http.ResponseWriter, r *http.Request) { | |
) | ||
resourceV1 := fmt.Sprintf("compliance:profiles:storage:%s", owner) | ||
resourceV2 := fmt.Sprintf("compliance:profiles:%s", owner) | ||
if err := s.authRequest(r, resourceV1, actionV1, resourceV2, actionV2); err != nil { | ||
ctx, err := s.authRequest(r, resourceV1, actionV1, resourceV2, actionV2) | ||
if err != nil { | ||
http.Error(w, err.Error(), http.StatusForbidden) | ||
return | ||
} | ||
|
@@ -460,7 +462,8 @@ func (s *Server) ProfileTarHandler(w http.ResponseWriter, r *http.Request) { | |
resourceV2 = "compliance:profiles:market" | ||
} | ||
|
||
if err := s.authRequest(r, resourceV1, actionV1, resourceV2, actionV2); err != nil { | ||
ctx, err := s.authRequest(r, resourceV1, actionV1, resourceV2, actionV2) | ||
if err != nil { | ||
http.Error(w, err.Error(), http.StatusForbidden) | ||
return | ||
} | ||
|
@@ -506,15 +509,15 @@ func (s *Server) ReportExportHandler(w http.ResponseWriter, r *http.Request) { | |
actionV2 = "compliance:reports:export" | ||
) | ||
|
||
if err := s.authRequest(r, resource, actionV1, resource, actionV2); err != nil { | ||
ctx, err := s.authRequest(r, resource, actionV1, resource, actionV2) | ||
if err != nil { | ||
http.Error(w, err.Error(), http.StatusForbidden) | ||
return | ||
} | ||
|
||
decoder := json.NewDecoder(r.Body) | ||
var query reporting.Query | ||
err := decoder.Decode(&query) | ||
if err != nil { | ||
if err := decoder.Decode(&query); err != nil { | ||
http.Error(w, err.Error(), http.StatusBadRequest) | ||
return | ||
} | ||
|
@@ -554,15 +557,15 @@ func (s *Server) configMgmtNodeExportHandler(w http.ResponseWriter, r *http.Requ | |
resourceV2 = "infra:nodes" | ||
) | ||
|
||
if err := s.authRequest(r, resourceV1, actionV1, resourceV2, actionV2); err != nil { | ||
ctx, err := s.authRequest(r, resourceV1, actionV1, resourceV2, actionV2) | ||
if err != nil { | ||
http.Error(w, err.Error(), http.StatusForbidden) | ||
return | ||
} | ||
|
||
decoder := json.NewDecoder(r.Body) | ||
var nodeExportRequest cfgmgmt_request.NodeExport | ||
err := decoder.Decode(&nodeExportRequest) | ||
if err != nil { | ||
if err := decoder.Decode(&nodeExportRequest); err != nil { | ||
http.Error(w, err.Error(), http.StatusBadRequest) | ||
return | ||
} | ||
|
@@ -606,7 +609,7 @@ func init() { | |
|
||
func (s *Server) authRequest(r *http.Request, | ||
resourceV1, actionV1, resourceV2, actionV2 string, | ||
) error { | ||
) (context.Context, error) { | ||
subjects := []string{} | ||
// Create a context with the request headers metadata. Normally grpc-gateway | ||
// does this, but since this is being used in a custom handler we've got do | ||
|
@@ -638,34 +641,29 @@ func (s *Server) authRequest(r *http.Request, | |
if len(subjects) < 1 { | ||
authnClient, err := s.clientsFactory.AuthenticationClient() | ||
if err != nil { | ||
return errors.Wrap(err, "authn-service unavailable") | ||
return nil, errors.Wrap(err, "authn-service unavailable") | ||
} | ||
|
||
authnResp, err := authnClient.Authenticate(ctx, &authn.AuthenticateRequest{}) | ||
if err != nil { | ||
return errors.Wrap(err, "authn-service error") | ||
return nil, errors.Wrap(err, "authn-service error") | ||
} | ||
|
||
subjects = append(authnResp.Teams, authnResp.Subject) | ||
} | ||
|
||
if len(subjects) < 1 { | ||
return errors.New("no policy subject detected in headers or verified certificates") | ||
return nil, errors.New("no policy subject detected in headers or verified certificates") | ||
} | ||
|
||
authzResp, err := s.authorizer.IsAuthorized(ctx, subjects, resourceV1, actionV1, resourceV2, actionV2) | ||
projects := auth_context.ProjectsFromMetadata(md) | ||
|
||
authzResp, err := s.authorizer.IsAuthorized(ctx, subjects, resourceV1, actionV1, resourceV2, actionV2, projects) | ||
if err != nil { | ||
return errors.Wrap(err, "authz-service error") | ||
return nil, errors.Wrap(err, "authz-service error") | ||
} | ||
|
||
// returns nil if authorization succeeded | ||
return authzResp.Err() | ||
} | ||
|
||
func logError(err error, resource, action string) { | ||
log.WithFields(log.Fields{ | ||
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. Did we move away from 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. Nope, just realised that the function hadn't been used 🤷♂ |
||
"error": err, | ||
"resource": resource, | ||
"action": action, | ||
}).Error("error authorizing request") | ||
// err is nil if authorization succeeded | ||
// Note: if we need all the auth info, use auth_context.NewOutgoingContext | ||
return auth_context.NewOutgoingProjectsContext(authzResp.Ctx()), authzResp.Err() | ||
} |
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.
I suppose this is the "last param" but oof we're passing around a lot of args at this point!
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.
yeah -- the root cause is that this is used for authorizing requests in the custom endpoints' helper function -- and there's no way they can retrieve the v1 or v2 mappings (no grpc/proto files involved). So, it'll have to be presented up front.