ROX-10304: Remove NoValidRole error sentinel by mtodor · Pull Request #1409 · stackrox/stackrox

mtodor · 2022-04-25T13:27:51Z

Description

Removed GenericNoValidRole error. We have added new Error sentinel in pkg/auth/role.

Note: This PR is based on mtodor/ROX-10303-replace-new-error-wrappers branch. After PR #1389 is merged, we will change the base branch.

Checklist

Investigated and inspected CI test results
~~[ ] Unit test and regression tests added~~ - nothing is added
~~[ ] Evaluated and added CHANGELOG entry if required~~ - not relevant
~~[ ] Determined and documented upgrade steps~~ - nothing relevant for upgrade steps
~~[ ] Documented user facing changes (create PR based on stackrox/openshift-docs and merge into rhacs-docs)~~ - no user-facing changes

Testing Performed

1. Compare in UI - No change - no change

2. REST request with token without Role - no change

{"error":"access for this user is not authorized: no valid role, please contact your system administrator","code":16,"message":"access for this user is not authorized: no valid role, please contact your system administrator","details":[]}

3. Request with not Admin role: - no change

❯ ./bin/darwin/roxctl --insecure-skip-tls-verify --insecure --endpoint https://localhost:8000 --token-file ./token.txt  central backup
ERROR:  Invalid credentials. Please add/fix your credentials

❯ ./bin/darwin/roxctl --insecure-skip-tls-verify --insecure --endpoint https://localhost:8000 --token-file ./token.txt  central debug log
ERROR:	could not get log level from central: rpc error: code = Unauthenticated desc = access for this user is not authorized: no valid role, please contact your system administrator

Additionally, default checks are also done:

make image works
make style works so that the linting of all go files is correct

…ntViolation, ErrNoCredentials, ErrNoValidRole, ErrNotAuthorized, ErrNoAuthzConfigured

ghost · 2022-04-25T13:44:44Z

Tag for build #486192 is 3.69.x-556-g91bf9b4558.

💻 For deploying this image using the dev scripts, run the following first:

export MAIN_IMAGE_TAG='3.69.x-556-g91bf9b4558'

🕹️ A roxctl binary can be downloaded from the CircleCI artifacts.

janisz · 2022-04-26T08:40:07Z

pkg/grpc/authn/interceptor.go

 		// Only service identities can have no roles assigned.
 		if len(id.Roles()) == 0 && id.Service() == nil {
-			return context.WithValue(ctx, identityErrorContextKey{}, errox.GenericNoValidRole()), nil
+			return context.WithValue(ctx, identityErrorContextKey{}, errox.NotAuthorized.CausedBy("no valid role")), nil


Suggested change

return context.WithValue(ctx, identityErrorContextKey{}, errox.NotAuthorized.CausedBy("no valid role")), nil

return context.WithValue(ctx, identityErrorContextKey{}, errox.NotAuthorized.CausedBy("only service identities can have no roles assigned")), nil

janisz · 2022-04-26T09:05:37Z

pkg/grpc/authz/user/role.go

 func (p *roleChecker) checkRole(roleNames []string) error {
 	if len(roleNames) == 0 {
-		return errox.NoValidRole
+		return errox.NotAuthorized


how about adding more context here e.g.

Suggested change

return errox.NotAuthorized

return errox.NotAuthorized.CausedBy("no roles assigned to the user"))

janisz · 2022-04-26T09:11:39Z

pkg/auth/authproviders/registry_httphandler.go

 	userRoles := userInfo.GetRoles()
 	if len(userRoles) == 0 {
-		err := errox.GenericNoValidRole()
+		err := errox.NotAuthorized.CausedBy("no valid role for the user")


How about adding explicit information about missing roles. no valid role sounds like there are some roles but not valid for this situation

Suggested change

err := errox.NotAuthorized.CausedBy("no valid role for the user")

err := errox.NotAuthorized.CausedBy(fmt.Sprintf("%s has no roles", userInfo.GetFriendlyName()))

…ole-error

rukletsov

I believe 401 is the right choice of error here, see https://issues.redhat.com/browse/ROX-8092 including comments. cc @rhybrillou

I'd suggest to introduce a new sentinel error in pkg/grpc/auth, maybe in none.go or in a new file, that will replace both NoValidRole and GenericNoValidRole(), e.g.

ErrNoValidRole := errox.NoCredentials.New("access for this user is not authorized: no valid role," +
    " please contact your system administrator")

…ole-error

…ROX-10304-remove-generic-no-valid-role-error

rukletsov · 2022-04-26T18:57:50Z

pkg/auth/role/error.go

@@ -0,0 +1,11 @@
+package role


Maybe put this file directly into pkg/auth/? This way we avoid having a auth/role package with just an error definition and the call sites look sleeker: auth.ErrNoValidRole.

md2119

The changes are fine, but is there an easy way for developers to look for sentinal values, especially if they are scattered in different pkgs?

…ROX-10304-remove-generic-no-valid-role-error

gitguardian · 2022-04-27T09:14:13Z

⚠️ GitGuardian has uncovered 2 secrets following the scan of your pull request.

Please consider investigating the findings and remediating the incidents. Failure to do so may lead to compromising the associated services or software components.

🔎 Detected hardcoded secrets in your pull request

GitGuardian id	Secret	Commit	Filename
3038029	Generic High Entropy Secret	`138264d`	qa-tests-backend/src/main/groovy/services/NotifierService.groovy	View secret
3038029	Generic High Entropy Secret	`c751bce`	qa-tests-backend/src/main/groovy/services/NotifierService.groovy	View secret

🛠 Guidelines to remediate hardcoded secrets

Understand the implications of revoking this secret by investigating where it is used in your code.
Replace and store your secrets safely. Learn here the best practices.
Revoke and rotate these secrets.
If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.

To avoid such incidents in the future consider

following these best practices for managing and storing secrets including API keys and other credentials
install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.

^{🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.

Our GitHub checks need improvements? Share your feedbacks!}

rukletsov · 2022-04-27T10:15:34Z

The changes are fine, but is there an easy way for developers to look for sentinal values, especially if they are scattered in different pkgs?

For global sentinels — for sure, but you already know that : ). For package-level sentinels, we can say that those changing or using a package shall be able to discover errors defined in that package but I think your comment is valid. Maybe we can have a gentlemen's agreement (and a linter) that all package sentinels must be defined in errors.go file in that package?

…ole-error

mtodor added 16 commits April 21, 2022 14:46

Replaced deprecated errors - ErrReferencedByAnotherObject, ErrInvaria…

bee77b1

…ntViolation, ErrNoCredentials, ErrNoValidRole, ErrNotAuthorized, ErrNoAuthzConfigured

Replaced deprecated error - ErrAlreadyExists

ce213c9

Replaced deprecated error - ErrInvalidArgs

f9af2c7

Replaced deprecated error - ErrNotFound

4b4edba

Replaced deprecated error - GenericNoValidRole

3f180f3

Replaced deprecated error function - NewErrNoCredentials

ddf4771

Replaced deprecated error function - NewErrInvariantViolation

e9aba67

Replaced deprecated error function - NewErrNotAuthorized

e69442e

Replaced deprecated error function - NewErrInvalidArgs

a0129aa

Remove NewErrNoCredentials

dc345c3

Remove NewErrInvariantViolation

a85e2d3

Remove NewErrNotAuthorized

0ab6c9f

Remove NewErrInvalidArgs

c6575d4

Exclude roxctl from wrapcheck

18d1609

Merge branch 'master' into mtodor/ROX-10303-replace-new-error-wrappers

ec30dc8

Remove NoValidRole error sentinel

daf991c

mtodor added the team/merlin label Apr 25, 2022

github-actions bot added the area/auth label Apr 25, 2022

mtodor marked this pull request as ready for review April 25, 2022 16:26

mtodor requested a review from a team as a code owner April 25, 2022 16:26

mtodor requested review from parametalol and rukletsov April 25, 2022 16:26

Fix regex and put roxctl back for linter wrapcheck

92f3cfe

janisz reviewed Apr 26, 2022

View reviewed changes

Merge branch 'master' into mtodor/ROX-10304-remove-generic-no-valid-r…

1e6a988

…ole-error

rukletsov requested changes Apr 26, 2022

View reviewed changes

Added ErrNoValidRole sentinel for authproviders

910d96f

mtodor added 2 commits April 26, 2022 16:59

Tweak comment

dcf60a2

Merge branch 'master' into mtodor/ROX-10304-remove-generic-no-valid-r…

52e50ff

…ole-error

github-actions bot added area/postgres area/sensor ci-postgres-tests labels Apr 26, 2022

Merge branch 'master' into mtodor/ROX-10303-replace-new-error-wrappers

299b8a5

mtodor changed the base branch from mtodor/ROX-10303-replace-new-error-wrappers to master April 26, 2022 15:15

Merge branch 'mtodor/ROX-10303-replace-new-error-wrappers' into mtodor/…

00591d8

…ROX-10304-remove-generic-no-valid-role-error

mtodor changed the base branch from master to mtodor/ROX-10303-replace-new-error-wrappers April 26, 2022 15:17

janisz approved these changes Apr 26, 2022

View reviewed changes

rukletsov approved these changes Apr 26, 2022

View reviewed changes

mtodor removed ci-upgrade-tests area/postgres labels Apr 26, 2022

md2119 approved these changes Apr 26, 2022

View reviewed changes

mtodor added 4 commits April 27, 2022 09:27

Merge branch 'master' into mtodor/ROX-10303-replace-new-error-wrappers

138264d

Updated error messages - review comment discussion_r859310460

9cde10b

Merge branch 'mtodor/ROX-10303-replace-new-error-wrappers' into mtodor/…

c751bce

…ROX-10304-remove-generic-no-valid-role-error

Move ErrNoValidRole to auth package

7e39204

mtodor requested a review from rukletsov April 27, 2022 09:15

rukletsov approved these changes Apr 27, 2022

View reviewed changes

md2119 approved these changes Apr 27, 2022

View reviewed changes

parametalol approved these changes Apr 28, 2022

View reviewed changes

Base automatically changed from mtodor/ROX-10303-replace-new-error-wrappers to master April 28, 2022 08:12

Merge branch 'master' into mtodor/ROX-10304-remove-generic-no-valid-r…

91bf9b4

…ole-error

mtodor merged commit 27e775a into master Apr 28, 2022

mtodor deleted the mtodor/ROX-10304-remove-generic-no-valid-role-error branch April 28, 2022 10:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ROX-10304: Remove NoValidRole error sentinel#1409

ROX-10304: Remove NoValidRole error sentinel#1409
mtodor merged 28 commits intomasterfrom
mtodor/ROX-10304-remove-generic-no-valid-role-error

mtodor commented Apr 25, 2022 •

edited

Loading

Uh oh!

ghost commented Apr 25, 2022 •

edited by ghost

Loading

Uh oh!

janisz Apr 26, 2022

Uh oh!

janisz Apr 26, 2022

Uh oh!

janisz Apr 26, 2022

Uh oh!

rukletsov left a comment •

edited

Loading

Uh oh!

rukletsov Apr 26, 2022

Uh oh!

mtodor Apr 27, 2022

Uh oh!

md2119 left a comment

Uh oh!

gitguardian bot commented Apr 27, 2022 •

edited

Loading

Uh oh!

rukletsov commented Apr 27, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

	return context.WithValue(ctx, identityErrorContextKey{}, errox.NotAuthorized.CausedBy("no valid role")), nil
	return context.WithValue(ctx, identityErrorContextKey{}, errox.NotAuthorized.CausedBy("only service identities can have no roles assigned")), nil

	return errox.NotAuthorized
	return errox.NotAuthorized.CausedBy("no roles assigned to the user"))

	err := errox.NotAuthorized.CausedBy("no valid role for the user")
	err := errox.NotAuthorized.CausedBy(fmt.Sprintf("%s has no roles", userInfo.GetFriendlyName()))

Conversation

mtodor commented Apr 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Testing Performed

Uh oh!

ghost commented Apr 25, 2022 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

janisz Apr 26, 2022

Choose a reason for hiding this comment

Uh oh!

janisz Apr 26, 2022

Choose a reason for hiding this comment

Uh oh!

janisz Apr 26, 2022

Choose a reason for hiding this comment

Uh oh!

rukletsov left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rukletsov Apr 26, 2022

Choose a reason for hiding this comment

Uh oh!

mtodor Apr 27, 2022

Choose a reason for hiding this comment

Uh oh!

md2119 left a comment

Choose a reason for hiding this comment

Uh oh!

gitguardian bot commented Apr 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

⚠️ GitGuardian has uncovered 2 secrets following the scan of your pull request.

Uh oh!

rukletsov commented Apr 27, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mtodor commented Apr 25, 2022 •

edited

Loading

ghost commented Apr 25, 2022 •

edited by ghost

Loading

rukletsov left a comment •

edited

Loading

gitguardian bot commented Apr 27, 2022 •

edited

Loading