[A2-835] make custom endpoints projects aware for iam v2.1 #380

srenatus · 2019-05-17T12:57:25Z

🔩 Description

When we've introduced projects getting passed along from authz to the domain services, we've done that for the general case (read: GRPC). This PR extends that treatment to the special case, our hand-written HTTP endpoints defined in automate-gateway.

Caveats

⚠️ This is in the code path of data ingestion, so let's keep an eye on performance after merging this.

⚠️ Tests pass in CI, but I have only done some minimal smoke tests so far: ensuring that when IAM v2.1 is selected, the projects are passed along to the domain-service that is sitting behind one of the special endpoints (exports of compliance reports). I was able to verify that the domain-service gets them alright. However, there's probably dozens of edge cases, especially with audit-cookbook and inspec involved, and I think we need superteam-help here for review.

✔️ ~~There's some TODO in the code, and a small refactoring I'd like to wrap into this. I'll tackle that next week.~~

👍 Definition of Done

🚀 The custom endpoints are projects-aware (if the domain-services (downstream) care about the projects).

👟 Demo Script / Repro Steps

deployinate
rebuild components/automate-gateway
chef-automate iam upgrade-to-v2 --beta2.1
pick a custom endpoint where the domain-service method is projects aware
send a request with projects

For testing,

I've changed this:

diff --git a/components/compliance-service/api/reporting/server/server.go b/components/compliance-service/api/reporting/server/server.go
index d2846aa1..197fedf6 100644
--- a/components/compliance-service/api/reporting/server/server.go
+++ b/components/compliance-service/api/reporting/server/server.go
@@ -5,6 +5,7 @@ import (
        "encoding/json"
        "fmt"
        "io"
+       "os"
        sorter "sort"
        "strings"

@@ -344,6 +345,7 @@ func filterByProjects(ctx context.Context, filters map[string][]string) (map[str
        if err != nil {
                return nil, err
        }
+       fmt.Fprintf(os.Stdout, "projects: %v\n\n", projectsFilter)
        if auth_context.AllProjectsRequested(projectsFilter) {
                return filters, nil
        }

rebuilt components/compliance-service, and
sent a request (where $TOK is an admin token)

$ curl -kH "api-token: $TOK" https://127.0.0.1/api/v0/compliance/reporting/export -d '{"type":"json","filters":[{"type":"start_time","values":["2019-05-07T00:00:00Z"]},{"type":"end_time","values":["2019-05-17T23:59:59Z"]}]}'  -H "projects: foo,bar" -XGET

observed the logs:

[2019-05-20T08:10:21+00:00] authz-service.default(O): time="2019-05-20T08:10:21Z" level=warning msg="finished unary call with code FailedPrecondition" error="rpc error: code = FailedPrecondition desc = authz-service set to v2" grpc.code=FailedPrecondition grpc.method=IsAuthorized grpc.service=chef.automate.domain.authz.Authorization grpc.start_time="2019-05-20T08:10:21Z" grpc.time_ms=0.029 span.kind=server system=grpc
[2019-05-20T08:10:21+00:00] authz-service.default(O): time="2019-05-20T08:10:21Z" level=info msg="Projects Authorized Query" action="compliance:reports:export" projects="[foo bar]" resource="compliance:reporting:reports" result="[foo bar]" subject="[token:55e8020c-3100-4863-be85-65f9d6c53d7c]"
[2019-05-20T08:10:21+00:00] compliance-service.default(O): projects: [foo bar]

Note:

the FailedPrecondition denotes the moment where the "authorizer" package in automate-gateway realises that sending a v1 authz query is wrong (authz-service will return FailedPreconditon), and switches itself to using v2.

the v2 query, executed from the custom endpoint helper method changed in this PR, looks correct:

Projects Authorized Query" action="compliance:reports:export" projects="[foo bar]" resource="compliance:reporting:reports" result="[foo bar]" subject="[token:55e8020c-3100-4863-be85-65f9d6c53d7c]"

the domain-service can retrieve the projects:

[2019-05-20T08:10:21+00:00] compliance-service.default(O): projects: [foo bar]

⛓️ Related Resources

A2-835

✅ Checklist

Necessary tests added/updated?
Necessary docs added/updated?
Code actually executed?
Vetting performed (unit tests, lint, etc.)?

tylercloke

Looking like Handle and IsAuthorized are getting pretty redundant and I'm wondering if we still need both? I'd love a few function comments for both of those two explaining when they are used and how they are different 😅

tylercloke · 2019-05-20T21:23:57Z

components/automate-gateway/gateway/middleware/authv2/authv2.go

+		Subjects:       subjects,
+		Resource:       resource,
+		Action:         action,
+		ProjectsFilter: projectsToFilter,


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

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 😉

srenatus · 2019-05-21T07:31:19Z

ℹ️ That's the authorizer machinery in a nutshell:

It wraps an authorizer for v1 and one for v2; and exposes roughly the same methods that these do:

Handle is for use in the GRPC interceptor: it reads the method from the context, and retrieves the policy mappings (v1 or v2 respectively, in the separate authv{1,2} packages); and uses that to constrict an authz query to the respective service, interpreting the result accordingly. v2 will take care of injecting projects into the context
IsAuthorized is a (bolted-on) method for using the authorizer construct in our custom handlers: it needs to be passed the resource/action directly.
Since they also need switching, the authz introspection calls are similar wrapped and exposed.

The authorizer package wrapping the v1 and v2 authorizers takes care of switching between v1 and v2: it has the version in its state, and if the authorizer for v1 fails with FailedPrecondition, switches to v2, and vice-versa. This is how we've achieved that the gateway config doesn't have to be changed for IAM v2 upgrade, but authz-service is the single source of truth.
To expose versioned error messages, the authorizer interface needs both v1 and v2 info; whereas the versioned authv{1,2} packages only take one. This is because there's no way for the IsAuthorized methods to retrieve the info themselves.
The relevant types are defined here

srenatus · 2019-05-21T08:04:03Z

Everything would be nice and tidy if there was only GRPC (and if we rearranged how introspection works, maybe?). 😉

Also: please note that I'm not particularly attached to how this currently works (I entirely blame my lack of imagination and not my tools) -- if anyone comes up with some better means to reach the same end, I'm all ears.

blakestier · 2019-05-23T21:33:26Z

components/automate-gateway/gateway/middleware/auth_interceptor.go

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


I suppose this is the "last param" but oof we're passing around a lot of args at this point!

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.

blakestier · 2019-05-23T21:43:45Z

components/automate-gateway/gateway/middleware/auth_interceptor.go

@@ -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] {


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 🤔

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

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

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

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

blakestier · 2019-05-23T21:55:25Z

components/automate-gateway/gateway/middleware/authv2/authv2.go

+			return nil, err
+		}
+		log.WithError(err).Error("error authorizing request")
+		return nil, status.Errorf(codes.PermissionDenied,


Should this message/error also include projects?

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.

blakestier · 2019-05-23T21:56:24Z

components/automate-gateway/gateway/middleware/authv2/authv2.go

+		if status.Convert(err).Code() == codes.FailedPrecondition {
+			return nil, err
+		}
+		log.WithError(err).Error("error authorizing request")


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.

😄 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? 😉

blakestier · 2019-05-23T22:42:40Z

components/automate-gateway/gateway/services.go

@@ -662,10 +665,21 @@ func (s *Server) authRequest(r *http.Request,
 	return authzResp.Err()
 }

-func logError(err error, resource, action string) {
-	log.WithFields(log.Fields{


Did we move away from WithFields?

Nope, just realised that the function hadn't been used 🤷‍♂

blakestier · 2019-05-23T22:43:36Z

components/automate-gateway/gateway/services.go

-	}).Error("error authorizing request")
+// XXX needs headers, doesn't it?
+func getProjectsFromMetadata(projectHeaderEntries []string) []string {
+	if projectHeaderEntries == nil {


👍 but also 😢

Sorry that one should have been deleted -- I've done the refactor here, after all. 😄

Signed-off-by: Stephan Renatus <[email protected]>

…text method Signed-off-by: Stephan Renatus <[email protected]>

There's no need to translate incoming MD to outgoing MD. Outgoing metadata is only required if another grpc CLIENT call uses that context, and the key-val pairs are supposed to be sent along. Also fixes the comment. Signed-off-by: Stephan Renatus <[email protected]>

vjeffrey

tested with audit cookbook and inspec -> automate direct. all working as expected! 😃

srenatus added WIP automate-auth automate-gateway iamv2 This issue or pull request applies to iamv2 work for Automate labels May 17, 2019

srenatus self-assigned this May 17, 2019

srenatus force-pushed the sr/a2-835/make-custom-endpoints-projects-aware-for-iam-v2.1 branch 2 times, most recently from 2d08d13 to a2a442f Compare May 20, 2019 08:15

srenatus removed the WIP label May 20, 2019

tylercloke approved these changes May 20, 2019

View reviewed changes

msorens approved these changes May 20, 2019

View reviewed changes

blakestier reviewed May 23, 2019

View reviewed changes

srenatus added 4 commits May 24, 2019 08:46

automate-gateway/authorizer: rough in projects

1c15696

Signed-off-by: Stephan Renatus <[email protected]>

automate-gateway: finish wiring up the authorizer refactor

3095ddc

Signed-off-by: Stephan Renatus <[email protected]>

automate-gateway: refactor projects-from-metadata logic into auth_con…

35f142a

…text method Signed-off-by: Stephan Renatus <[email protected]>

srenatus force-pushed the sr/a2-835/make-custom-endpoints-projects-aware-for-iam-v2.1 branch from a2a442f to 44e870e Compare May 24, 2019 07:06

vjeffrey approved these changes May 28, 2019

View reviewed changes

srenatus merged commit 21e2d9c into master May 28, 2019

chef-ci deleted the sr/a2-835/make-custom-endpoints-projects-aware-for-iam-v2.1 branch May 28, 2019 14:10

susanev added the auth-team anything that needs to be on the auth team board label Jul 20, 2019

ekmixon mentioned this pull request Nov 16, 2022

[Snyk] Fix for 1 vulnerabilities ekmixon/automate#84

Open

ekmixon mentioned this pull request Nov 29, 2022

[Snyk] Fix for 1 vulnerabilities ekmixon/automate#92

Open

ekmixon mentioned this pull request Dec 1, 2023

[Snyk] Fix for 1 vulnerabilities ekmixon/automate#145

Open

ekmixon mentioned this pull request Dec 20, 2023

[Snyk] Fix for 29 vulnerabilities ekmixon/automate#149

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[A2-835] make custom endpoints projects aware for iam v2.1 #380

[A2-835] make custom endpoints projects aware for iam v2.1 #380

srenatus commented May 17, 2019 •

edited

Loading

tylercloke left a comment •

edited

Loading

tylercloke May 20, 2019

srenatus May 21, 2019

srenatus commented May 21, 2019 •

edited

Loading

srenatus commented May 21, 2019

blakestier May 23, 2019

srenatus May 24, 2019

blakestier May 23, 2019 •

edited

Loading

msorens May 23, 2019

blakestier May 23, 2019 •

edited

Loading

srenatus May 24, 2019

blakestier May 24, 2019

blakestier May 23, 2019

msorens May 23, 2019

blakestier May 23, 2019

srenatus May 24, 2019

blakestier May 23, 2019

srenatus May 24, 2019

blakestier May 23, 2019

srenatus May 24, 2019

vjeffrey left a comment

[A2-835] make custom endpoints projects aware for iam v2.1 #380

[A2-835] make custom endpoints projects aware for iam v2.1 #380

Conversation

srenatus commented May 17, 2019 • edited Loading

🔩 Description

Caveats

👍 Definition of Done

👟 Demo Script / Repro Steps

⛓️ Related Resources

✅ Checklist

tylercloke left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srenatus commented May 21, 2019 • edited Loading

srenatus commented May 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blakestier May 23, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blakestier May 23, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vjeffrey left a comment

Choose a reason for hiding this comment

srenatus commented May 17, 2019 •

edited

Loading

tylercloke left a comment •

edited

Loading

srenatus commented May 21, 2019 •

edited

Loading

blakestier May 23, 2019 •

edited

Loading

blakestier May 23, 2019 •

edited

Loading