Skip to content
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

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion components/automate-gateway/gateway/datacollector.go
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,8 @@ func (s *Server) dataCollectorHandler(w http.ResponseWriter, r *http.Request) {
actionV2 = "infra:ingest:create"
)

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
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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)

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!

Copy link
Contributor Author

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.

}

type SwitchingFilterHandler interface {
Expand Down Expand Up @@ -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 {
Expand All @@ -90,10 +89,12 @@ type AuthorizationHandler interface {
}

type AuthorizationResponse interface {
Ctx() context.Context
GetAuthorized() bool
}

type AnnotatedAuthorizationResponse interface {
AuthorizationResponse
Err() error
}

Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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] {
Copy link

@blakestier blakestier May 23, 2019

Choose a reason for hiding this comment

The 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 keys[newproject] was nil, this would fail or that we'd get "invalid operation".

Don't we need to both check the value and the success of extracting it (if val, ok := keys[newProject]; ok {)?

Maybe I'm missing something 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

keys[newProject], if not present, will return the zero-value for the type, in this case false. So this statement is still asserting "if there is no newProject entry in keys then..."

Copy link

@blakestier blakestier May 23, 2019

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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:

For a of map type M:

  • x's type must be assignable to the key type of M
  • if the map contains an entry with key x, a[x] is the map element with key x and the type of a[x] is the element type of M
  • if the map is nil or does not contain such an entry, a[x] is the zero value for the element type of M

Choose a reason for hiding this comment

The 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)
}
Expand Down
13 changes: 12 additions & 1 deletion components/automate-gateway/gateway/middleware/authv1/authv1.go
Original file line number Diff line number Diff line change
Expand Up @@ -78,13 +78,24 @@ func (c *client) Handle(ctx context.Context, subjects []string, _ []string,
return auth_context.NewContext(ctx, subjects, projects, resource, action, middleware.AuthV1.String()), nil
}

type resp struct {
ctx context.Context
*authz.IsAuthorizedResp
}

func (r *resp) Ctx() context.Context {
return r.ctx
}

func (c *client) IsAuthorized(ctx context.Context, subjects []string, resource, action string,
_ []string, // projects aren't used
) (middleware.AuthorizationResponse, error) {
return c.client.IsAuthorized(ctx, &authz.IsAuthorizedReq{
r, err := c.client.IsAuthorized(ctx, &authz.IsAuthorizedReq{
Subjects: subjects,
Resource: resource,
Action: action,
})
return &resp{IsAuthorizedResp: r, ctx: ctx}, err
}

func (c *client) FilterAuthorizedPairs(ctx context.Context, subjects []string, inputPairs []*pairs.Pair,
Expand Down
41 changes: 36 additions & 5 deletions components/automate-gateway/gateway/middleware/authv2/authv2.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking similar to the Handle function. When do we hit Handle vs IsAuthorized?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it? Handle is to be used in a GRPC interceptor, and depends on
a) the grpc method invoked, and
b) the policy info being mapped in our proto files
i.e. this half of the method, neither of which exist for custom handlers.

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")

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.
b) ANY GRPC response that has a status !=OK will be logged by the gateway. So, we should end up having most of the stuff in the logs because of the status message below.

Good enough? 😉

return nil, status.Errorf(codes.PermissionDenied,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this message/error also include projects?

Copy link
Contributor

Choose a reason for hiding this comment

The 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,
Expand Down
46 changes: 22 additions & 24 deletions components/automate-gateway/gateway/services.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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{

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did we move away from WithFields?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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()
}
17 changes: 9 additions & 8 deletions components/automate-gateway/pkg/authorizer/authorizer.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,20 +39,20 @@ func (a *state) Handle(ctx context.Context,
}

func (a *state) IsAuthorized(ctx context.Context, subjects []string,
resourceV1, actionV1, resourceV2, actionV2 string,
resourceV1, actionV1, resourceV2, actionV2 string, projects []string,
) (middleware.AnnotatedAuthorizationResponse, error) {
var (
resp middleware.AuthorizationResponse
err error
)
switch a.next {
case a.v1:
resp, err = a.v1.IsAuthorized(ctx, subjects, resourceV1, actionV1)
resp, err = a.v1.IsAuthorized(ctx, subjects, resourceV1, actionV1, nil) // projects are not used here
if err == nil {
return annotate(resp, subjects, resourceV1, actionV1), nil
}
case a.v2:
resp, err = a.v2.IsAuthorized(ctx, subjects, resourceV2, actionV2)
resp, err = a.v2.IsAuthorized(ctx, subjects, resourceV2, actionV2, projects)
if err == nil {
return annotate(resp, subjects, resourceV2, actionV2), nil
}
Expand All @@ -61,7 +61,7 @@ func (a *state) IsAuthorized(ctx context.Context, subjects []string,
switch st.Code() {
case codes.FailedPrecondition:
if a.fromStatus(st) {
return a.IsAuthorized(ctx, subjects, resourceV1, actionV1, resourceV2, actionV2)
return a.IsAuthorized(ctx, subjects, resourceV1, actionV1, resourceV2, actionV2, projects)
}
fallthrough
default: // any other error status
Expand Down Expand Up @@ -158,20 +158,21 @@ func (a *state) FilterAuthorizedProjects(ctx context.Context, subjects []string,
}

type annotated struct {
r middleware.AuthorizationResponse
middleware.AuthorizationResponse
err error
}

func (r *annotated) Err() error {
if r.r.GetAuthorized() {
if r.GetAuthorized() {
return nil
}
return r.err
}

func annotate(resp middleware.AuthorizationResponse, subjects []string, resource, action string) middleware.AnnotatedAuthorizationResponse {
return &annotated{r: resp, err: fmt.Errorf("subject %q is not authorized to %q resource %q",
subjects, action, resource)}
return &annotated{AuthorizationResponse: resp,
err: fmt.Errorf("subject %q is not authorized to %q resource %q",
subjects, action, resource)}
}

func (a *state) fromStatus(st *status.Status) bool {
Expand Down
17 changes: 13 additions & 4 deletions components/automate-gateway/pkg/authorizer/authorizer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,9 @@ func TestAuthorizerHandle(t *testing.T) {

func TestAuthorizerIsAuthorized(t *testing.T) {
ctx := context.Background()
args := func() (context.Context, []string, string, string, string, string) {
return ctx, []string{"user:local:admin"}, "resource:v1", "v1action", "svc:type:v2resource", "svc:type:v2action"
args := func() (context.Context, []string, string, string, string, string, []string) {
return ctx, []string{"user:local:admin"}, "resource:v1", "v1action", "svc:type:v2resource", "svc:type:v2action",
[]string{"foo-project", "bar-project"}
}

t.Run("if first succeeds, doesn't call second", func(t *testing.T) {
Expand Down Expand Up @@ -127,7 +128,7 @@ func (*success) Handle(ctx context.Context, _ []string, _ []string, _ interface{
return ctx, nil
}

func (s *success) IsAuthorized(context.Context, []string, string, string) (middleware.AuthorizationResponse, error) {
func (s *success) IsAuthorized(context.Context, []string, string, string, []string) (middleware.AuthorizationResponse, error) {
return authzSuccess, nil
}

Expand All @@ -143,7 +144,7 @@ func (f *failure) Handle(ctx context.Context, _ []string, _ []string, _ interfac
return ctx, f.Err()
}

func (f *failure) IsAuthorized(context.Context, []string, string, string) (middleware.AuthorizationResponse, error) {
func (f *failure) IsAuthorized(context.Context, []string, string, string, []string) (middleware.AuthorizationResponse, error) {
return nil, f.Err()
}

Expand All @@ -165,6 +166,14 @@ func (*authorized) GetAuthorized() bool {
return true
}

func (*authorized) Ctx() context.Context {
return nil
}

func (*notAuthorized) GetAuthorized() bool {
return false
}

func (*notAuthorized) Ctx() context.Context {
return nil
}
Loading