[automate-3198] reduce validation #3312

blakestier · 2020-04-07T22:38:21Z

🔩 Description: What code changed, and why?

When creating a project, we use the same validation as other objects at the server level: character count must be 64 or less.

However, we auto-generate several roles and policies using that id and additional characters. The resulting policy id can then be over the expected limit, making it impossible to Get the policy bc of our validation on id length in Get requests.

With this change, project_ids must be 48 chars or less (allowing the other id creation without trouble).

Validation on policy Get requests (and, while I was in there, all other resource requests that didn't create the id) no longer validate on length. Instead, validation confirms the required field is present and has more than whitespace characters.

It was a toss up whether to put the validation for this in the server (where we do our other proto validations, and where the problem would be caught early) or in the db (where we probably should have our more fine-grained validation code). The answer is probably both, but this change keeps it in the server, with a few moves to storage-level validation that had parity with this kind of validation.

👟 How to Build and Test the Change

start on a studio without this version of authz

make a project with an id longer than 50 chars
curl -kH "api-token: $TOK" -d '{"name":"p123456789p123456789p123456789p123456789p123456789p123456789", "id":"p123456789p123456789p123456789p123456789p12342222"}' https://a2-dev.test/apis/iam/v2/projects

attempt to fetch the associated policy (this should fail)
curl -kH "api-token: $TOK" https://a2-dev.test/apis/iam/v2/policies/p123456789p123456789p123456789p123456789p123456789p123456789-project-editors

rebuild components/authz-service

attempt to fetch the same policy (should now succeed)
curl -kH "api-token: $TOK" https://a2-dev.test/apis/iam/v2/policies/p123456789p123456789p123456789p123456789p123456789p123456789-project-editors

attempt to create a project with an id longer than 48 chars (should now fail)
curl -kH "api-token: $TOK" -d '{"name":"p123456789p123456789p123456789p123456789p123456789p123456789", "id":"p123456789p123456789p123456789p123456789p12342222"}' https://a2-dev.test/apis/iam/v2/projects {"error":"invalid CreateProjectReq.Id: value does not match regex pattern \"^[a-z0-9-_]{1,48}$\"","code":3,"message":"invalid CreateProjectReq.Id: value does not match regex pattern \"^[a-z0-9-_]{1,48}$\"","details":[]}

Also, any other requests (GetRule, GetPolicy, etc) now fail with InvalidArg if the required ids are blank or absent.

✅ Checklist

I have read the CONTRIBUTING document.
Tests added/updated?
Docs added/updated?
All commits have been signed-off for the Developer Certification of Origin.

📷 Screenshots, if applicable

api/interservice/authz/v2/project.proto

blakestier · 2020-04-10T17:51:21Z

components/automate-cli/pkg/diagnostics/integration/helpers.go

@@ -13,7 +13,7 @@ import (
 // TimestampName returns a name with a timestamp
 func TimestampName() string {
 	ts := time.Now()
-	return "integration-diagnostic-" + ts.Format("20060102150405")
+	return "diagnostic-" + ts.Format("20060102150405")


didn't fit the new char limit

blakestier · 2020-04-10T17:51:31Z

inspec/a2-api-integration/controls/iam/iam_apis.rb

@@ -948,7 +948,7 @@
      end

      after(:all) do
-        resp = automate_api_request("/apis/iam/v2/projects/#{CUSTOM_RULE[:project_id]}/rules/#{CUSTOM_RULE[:id]}", http_method: 'DELETE')
+        resp = automate_api_request("/apis/iam/v2/projects/#{CUSTOM_RULE_1[:project_id]}/rules/#{CUSTOM_RULE_1[:id]}", http_method: 'DELETE')


driveby fix

bcmdarroch · 2020-04-10T19:51:31Z

api/interservice/authz/v2/policy_test.go

@@ -134,31 +134,11 @@ func TestValidationCreatePolicy(t *testing.T) {
 			Statements: []*v2.Statement{&validStatement},
 		},
 		// projects
-		"whitespace projects list": &v2.CreatePolicyReq{


these are all pushed into server tests?

Several that were about validation are removed as they are now the equivalent of a "not found"-- instead of duplicating more invalid things that would result in "not found", I removed them.

bcmdarroch

Looks good to me!

I did find something that we should probably fix in another card:

Fetching a rule with a missing rule ID gets the nice validation error you added.

[42][default:/src:0]# curl -kH "api-token: $TOK" https://a2-dev.test/apis/iam/v2/projects/neato/rules/ | jq
{
  "error": "a rule id is required and must contain at least one non-whitespace character",
  "code": 3,
  "message": "a rule id is required and must contain at least one non-whitespace character",
  "details": []
}

But fetching a project with a missing ID gets this error that's a bit of a red herring:

[47][default:/src:0]# curl -kH "api-token: $TOK" https://a2-dev.test/apis/iam/v2/projects/ | jq
{
  "error": "error authorizing action \"iam:projects:get\" on resource \"iam:projects:\" filtered by projects [] for members [\"token:admin-token-1586543035\"]: rpc error: code = InvalidArgument desc = invalid ProjectsAuthorizedReq.Resource: value does not match regex pattern \"^[a-z][^:*]*(?::[^:*]+)*$\"",
  "code": 7,
  "message": "error authorizing action \"iam:projects:get\" on resource \"iam:projects:\" filtered by projects [] for members [\"token:admin-token-1586543035\"]: rpc error: code = InvalidArgument desc = invalid ProjectsAuthorizedReq.Resource: value does not match regex pattern \"^[a-z][^:*]*(?::[^:*]+)*$\"",
  "details": []
}

bcmdarroch · 2020-04-10T20:02:25Z

components/authz-service/server/v2/project_test.go

@@ -189,8 +189,8 @@ func TestGetProject(t *testing.T) {
 			grpctest.AssertCode(t, codes.InvalidArgument, err)
 			assert.Nil(t, resp)
 		}},
-		{"if the project id is invalid, returns 'invalid argument'", func(t *testing.T) {
-			resp, err := cl.GetProject(ctx, &api.GetProjectReq{Id: "no spaces allowed"})


so these no space allowed would return not found right? Could we add tests that verify that behavior?

since we're not validating at this level, an invalid name is the same as a not found. there are other tests for not found names, but they seemed functionally equivalent. What do think?

bcmdarroch · 2020-04-10T20:06:07Z

components/authz-service/server/v2/validation.go

+	return err
+}
+
+func confirmRequiredField(field, fieldName, resourceName string) error {


nice DRY refactor 🎉

components/authz-service/storage/v2/rule.go

msorens

Nice work! Some notes below but everything looks sound so 👍 .

api/interservice/authz/v2/project.proto

components/authz-service/server/v2/policy.go

msorens · 2020-04-13T14:54:30Z

components/authz-service/server/v2/policy.go

+	if err != nil {
+		return nil, status.Errorf(codes.InvalidArgument, err.Error())
+	}
+	err = validateProjects(req.Projects)


You have two calls to this new function, but they return very different messages from each other. Was that intentional...?

Yes-- once it's called within the primary server call itself (where the code is processed/returned), the other time in a helper, which just bubbles the error. That said, I can try to make that less confusing!

msorens · 2020-04-13T14:58:26Z

components/authz-service/server/v2/policy_test.go

@@ -881,8 +881,8 @@ func TestGetPolicy(t *testing.T) {
 			require.Nil(t, pol)
 			grpctest.AssertCode(t, codes.InvalidArgument, err)
 		}},
-		{"fails with InvalidArgument when ID isn't valid", func(t *testing.T) {
-			req := api_v2.GetPolicyReq{Id: "no spaces"}
+		{"fails with InvalidArgument when ID is whitespace", func(t *testing.T) {


I think this change to "is whitespace" is valuable; consider propagating to the other touched tests in this file that still say "is not valid".

components/authz-service/server/v2/policy_test.go

msorens · 2020-04-13T15:08:11Z

components/authz-service/server/v2/project.go

 	p, err := storage.NewProject(req.Id, req.Name, storage.Custom, storage.NoRules)
 	if err != nil {
-		return nil, status.Errorf(codes.InvalidArgument,
+		return nil, status.Errorf(codes.Internal,


msorens · 2020-04-13T15:11:39Z

components/authz-service/server/v2/rules_test.go

@@ -74,9 +74,9 @@ func TestCreateRule(t *testing.T) {
 			grpctest.AssertCode(t, codes.InvalidArgument, err)
 			assert.Nil(t, resp)
 		}},
-		{"if the passed rule project id is invalid, returns 'invalid argument'", func(t *testing.T) {
+		{"if the passed rule project id is empty spaces, returns 'invalid argument'", func(t *testing.T) {


Consistency with previous 2 files suggest "is whitespace":

Suggested change

{"if the passed rule project id is empty spaces, returns 'invalid argument'", func(t *testing.T) {

{"if the passed rule project id is whitespace, returns 'invalid argument'", func(t *testing.T) {

components/authz-service/server/v2/rules_test.go

msorens · 2020-04-13T15:22:05Z

components/authz-service/storage/v2/role.go

-	if len(actions) == 0 {
-		return errors.New("a role must contain at least one action")


I see this check is covered by the proto file so OK to remove it here. (Just wondering if we are happy with that error message vs. this one, thinking about our move away from proto-validation. 🤔 ).

msorens · 2020-04-13T15:22:17Z

components/authz-service/storage/v2/role.go

-	if len(actions) == 0 {
-		return errors.New("a role must contain at least one action")
-	}
-	err := ValidateProjects(projects)


Is this check covered elsewhere?

Yep-- back in the server

Oh the unassigned! Now it is :D

Signed-off-by: Blake Johnson <[email protected]>

Co-Authored-By: M Sorens <[email protected]>

Signed-off-by: Blake Johnson <[email protected]>

Signed-off-by: Brenna Hewer-Darroch <[email protected]>

srenatus

Nothing substantial, just some nitpicks... 😉

It could be a fun exercise to try to validate IDs from a custom middleware -- not PGV -- by peeking at the protobuf message. (Not trival, I think.)

components/authz-service/server/v2/policy.go

srenatus · 2020-04-15T11:30:31Z

components/authz-service/server/v2/project.go

@@ -151,10 +160,15 @@ func (s *ProjectState) CreateProject(ctx context.Context,

 func (s *ProjectState) UpdateProject(ctx context.Context,
 	req *api.UpdateProjectReq) (*api.UpdateProjectResp, error) {
+
+	err := confirmRequiredIDandName(req.Id, req.Name, "project")


[nit] It looks weird, but the canonical casing here would be this:

Suggested change

err := confirmRequiredIDandName(req.Id, req.Name, "project")

err := confirmRequiredIDAndName(req.Id, req.Name, "project")

...but I'm down to turning a blind eye 🙈

this does seem like an opportunity conceding that the "right" way is not always the most readable, huh

Yeah probably. Also naming is hard 😉

components/authz-service/server/v2/validation.go

Co-Authored-By: Stephan Renatus <[email protected]>

msorens · 2020-04-15T15:14:05Z

FYI: Created issue #3371 to mirror this strategy change to teams and authn services.

Signed-off-by: Blake Johnson <[email protected]>

blakestier force-pushed the automate-3198/reduce-validation branch 5 times, most recently from 4250081 to fee7483 Compare April 10, 2020 16:29

blakestier marked this pull request as ready for review April 10, 2020 17:32

blakestier requested a review from a team as a code owner April 10, 2020 17:32

blakestier changed the title ~~Automate 3198/reduce validation~~ [automate-3198] reduce validation Apr 10, 2020

blakestier commented Apr 10, 2020

View reviewed changes

api/interservice/authz/v2/project.proto Show resolved Hide resolved

blakestier commented Apr 10, 2020

View reviewed changes

blakestier self-assigned this Apr 10, 2020

blakestier added auth-team anything that needs to be on the auth team board automate-auth labels Apr 10, 2020

bcmdarroch reviewed Apr 10, 2020

View reviewed changes

bcmdarroch approved these changes Apr 10, 2020

View reviewed changes

msorens approved these changes Apr 13, 2020

View reviewed changes

Blake Johnson and others added 13 commits April 13, 2020 16:42

Reduce proj id length rec, remove validation on fetch, etc

f550975

Signed-off-by: Blake Johnson <[email protected]>

Remove validation of project id

fb68a92

Move validation to server for policies/roles

c026187

Signed-off-by: Blake Johnson <[email protected]>

Validate for Projects

88e59d7

Signed-off-by: Blake Johnson <[email protected]>

Port/fix validation tests

923fd47

Signed-off-by: Blake Johnson <[email protected]>

Shorten vars in diagnostics

5f083c8

Signed-off-by: Blake Johnson <[email protected]>

Rename functions

9422a68

Signed-off-by: Blake Johnson <[email protected]>

Shorten id

94c86d6

Signed-off-by: Blake Johnson <[email protected]>

Make single check

abce3fa

Signed-off-by: Blake Johnson <[email protected]>

Update components/authz-service/server/v2/rules_test.go

2d4c1dd

Co-Authored-By: M Sorens <[email protected]>

Update components/authz-service/server/v2/policy_test.go

8b85de6

Co-Authored-By: M Sorens <[email protected]>

Move condition check to server

85a8d7d

Signed-off-by: Blake Johnson <[email protected]>

remove magic strings and consolidate validation

9d762ee

Signed-off-by: Blake Johnson <[email protected]>

blakestier force-pushed the automate-3198/reduce-validation branch from 4a8cc20 to b324067 Compare April 13, 2020 23:42

blakestier mentioned this pull request Apr 14, 2020

Missing proj ID on Get results in unauthorized #3358

Closed

blakestier force-pushed the automate-3198/reduce-validation branch 4 times, most recently from 7bc89f0 to 8d31cf9 Compare April 14, 2020 22:38

Fix tests

9511906

Signed-off-by: Blake Johnson <[email protected]>

blakestier force-pushed the automate-3198/reduce-validation branch from 8d31cf9 to 9511906 Compare April 14, 2020 23:03

bcmdarroch force-pushed the automate-3198/reduce-validation branch from c5d745f to 50060ef Compare April 14, 2020 23:46

fix tests pt 2

e4dd2b9

Signed-off-by: Brenna Hewer-Darroch <[email protected]>

bcmdarroch force-pushed the automate-3198/reduce-validation branch from 50060ef to e4dd2b9 Compare April 14, 2020 23:46

srenatus approved these changes Apr 15, 2020

View reviewed changes

Blake Johnson and others added 2 commits April 15, 2020 08:01

Update components/authz-service/server/v2/policy.go

2cc64fb

Co-Authored-By: Stephan Renatus <[email protected]>

Update components/authz-service/server/v2/validation.go

e25833b

Co-Authored-By: Stephan Renatus <[email protected]>

msorens mentioned this pull request Apr 15, 2020

Mirror latest validation strategy from authz to teams, authn, and users #3371

Closed

Consolidate err return duh

e1dd64c

Signed-off-by: Blake Johnson <[email protected]>

blakestier force-pushed the automate-3198/reduce-validation branch 2 times, most recently from 751ac0f to cb80e5b Compare April 15, 2020 19:30

Another long id

bb21886

Signed-off-by: Blake Johnson <[email protected]>

blakestier force-pushed the automate-3198/reduce-validation branch from cb80e5b to bb21886 Compare April 15, 2020 21:14

blakestier merged commit 27ee701 into master Apr 15, 2020

blakestier deleted the automate-3198/reduce-validation branch April 15, 2020 22:05

bcmdarroch mentioned this pull request Apr 24, 2020

[automate-3498] add validate lib #3502

Merged

4 tasks

ekmixon mentioned this pull request Dec 20, 2023

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

Open

This was referenced Mar 23, 2024

[Snyk] Fix for 2 vulnerabilities ekmixon/automate#163

Open

[Snyk] Fix for 2 vulnerabilities ekmixon/automate#164

Open

This was referenced May 13, 2024

[Snyk] Fix for 2 vulnerabilities ekmixon/automate#175

Open

[Snyk] Fix for 2 vulnerabilities ekmixon/automate#178

Open

ekmixon mentioned this pull request May 22, 2024

[Snyk] Fix for 3 vulnerabilities ekmixon/automate#181

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[automate-3198] reduce validation #3312

[automate-3198] reduce validation #3312

blakestier commented Apr 7, 2020 •

edited

Loading

blakestier Apr 10, 2020

blakestier Apr 10, 2020

bcmdarroch Apr 10, 2020 •

edited

Loading

blakestier Apr 13, 2020

bcmdarroch left a comment

bcmdarroch Apr 10, 2020

blakestier Apr 13, 2020

bcmdarroch Apr 10, 2020

msorens left a comment

msorens Apr 13, 2020

blakestier Apr 13, 2020

msorens Apr 13, 2020

msorens Apr 13, 2020

msorens Apr 13, 2020

msorens Apr 13, 2020

blakestier Apr 13, 2020

msorens Apr 13, 2020

blakestier Apr 13, 2020

blakestier Apr 13, 2020

srenatus left a comment

srenatus Apr 15, 2020

blakestier Apr 15, 2020

srenatus Apr 15, 2020

msorens commented Apr 15, 2020

	{"if the passed rule project id is empty spaces, returns 'invalid argument'", func(t *testing.T) {
	{"if the passed rule project id is whitespace, returns 'invalid argument'", func(t *testing.T) {

		if len(actions) == 0 {
		return errors.New("a role must contain at least one action")

	err := confirmRequiredIDandName(req.Id, req.Name, "project")
	err := confirmRequiredIDAndName(req.Id, req.Name, "project")

[automate-3198] reduce validation #3312

[automate-3198] reduce validation #3312

Conversation

blakestier commented Apr 7, 2020 • edited Loading

🔩 Description: What code changed, and why?

👟 How to Build and Test the Change

✅ Checklist

📷 Screenshots, if applicable

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bcmdarroch Apr 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bcmdarroch left a 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

msorens left a 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

Choose a reason for hiding this comment

srenatus left a 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

msorens commented Apr 15, 2020

blakestier commented Apr 7, 2020 •

edited

Loading

bcmdarroch Apr 10, 2020 •

edited

Loading