-
Notifications
You must be signed in to change notification settings - Fork 170
ROX-32452: implement token generation service for the OCP plugin #18545
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
base: master-yann/plug-common-tokenbased-source
Are you sure you want to change the base?
ROX-32452: implement token generation service for the OCP plugin #18545
Conversation
|
Images are ready for the commit at a432044. To use with deploy scripts, first |
934ec64 to
0a84d21
Compare
9256d36 to
5752516
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master-yann/plug-common-tokenbased-source #18545 +/- ##
=============================================================================
+ Coverage 49.12% 49.14% +0.02%
=============================================================================
Files 2649 2652 +3
Lines 198837 198981 +144
=============================================================================
+ Hits 97669 97795 +126
- Misses 93732 93750 +18
Partials 7436 7436
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Any particular reason to put this service under central/auth? Other services are under central.
| const ( | ||
| id = "https://stackrox.io/jwt-sources#ocp-rox-tokens" | ||
| //#nosec G101 -- This constant is only there as a source type name, not as a credential. | ||
| ocpToken = "ocp-plugin-token" |
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 would probably avoid mentioning the ocp plugin too explicitly here. Sure that is the end use case, but from Central's point of view, it is serving an internal token service. What Sensor then does with these tokens is out of scope for Central. For example, what if we expand the feature to plain k8s plugins? It would be awkward to have ocp everywhere just because it was the first end consumer of the api.
| "github.com/stackrox/rox/pkg/grpc" | ||
| ) | ||
|
|
||
| // Service provides the interface to the microservice that serves tokens for the OCP plugin. |
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.
Microservice?
| const ( | ||
| permissionSetNameFormat = "Generated permission set for %s" | ||
| accessScopeNameFormat = "Generated access scope for %s" | ||
| roleNameFormat = "Generated role for PermissionSet %s and AccessScope %s" |
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.
| roleNameFormat = "Generated role for PermissionSet %s and AccessScope %s" | |
| roleNameFormat = "Generated role for permission set %s and access scope %s" |
Or change the formatting in the other strings to be consistent about the case.
| return nil, errors.Wrap(err, "getting expiration time") | ||
| } | ||
| claimName := fmt.Sprintf( | ||
| "Generated claims for role %s expiring at %s", |
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.
Why is this string not extracted like the others?
| return response, nil | ||
| } | ||
|
|
||
| // region helpers |
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.
| // region helpers |
Not helpful imo.
| ) (time.Time, error) { | ||
| duration, err := protocompat.DurationFromProto(req.GetValidFor()) | ||
| if err != nil { | ||
| return time.Time{}, errors.Wrap(err, "converting requested token validity duration") |
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.
Shouldn't this one be errox.InvalidArgs as well?
| ctx context.Context, | ||
| req *central.GenerateTokenForPermissionsAndScopeRequest, | ||
| ) (string, error) { | ||
| // TODO: Consider pruning the generated permission sets after some idle time. |
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.
We should certainly prune the dynamic resources, especially the access scopes and roles. Would the permission sets, access scopes and roles be visible in the UI similar to user created resources?
| return nil, err | ||
| } | ||
| response := ¢ral.GenerateTokenForPermissionsAndScopeResponse{ | ||
| Token: tokenInfo.Token, |
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.
From @parametalol - comment lines 58 to 73 on central/auth/ocpplugin/service/service_impl.go in the first commit (reference at time of writing: 3755769#diff-892fd289d36f704f4f0565732fe8df62e7726ca57d0f1afe2c367aca1b2cadfcR73 )
- The expiry could be assed via the
WithExpiryoption issuedAtshould be taken from thetokenInfo
I'm not even sure thatRoles,IssuedAtandExpiresAtare needed in the response
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.
The call to s.issuer.Issue now uses the WithExpiry option to set the expiration in the issued token.
The response message is now reduced to its minimal form: a single string containing the issued token.
| permissionSetID, err := rm.createPermissionSet(ctx, req) | ||
| if err != nil { | ||
| return "", errors.Wrap(err, "creating permission set for role") | ||
| } | ||
| accessScopeID, err := rm.createAccessScope(ctx, req) | ||
| if err != nil { | ||
| return "", errors.Wrap(err, "creating access scope for role") | ||
| } |
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.
From @parametalol comment lines 96 and 97 on central/auth/ocpplugin/service/service_impl.go in the first commit (reference at time of writing: 3755769#diff-892fd289d36f704f4f0565732fe8df62e7726ca57d0f1afe2c367aca1b2cadfcR97)
If we need only IDs, maybe we could optimize the getters now and in the future: (change suggestion not ported over).
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.
The API was updated to return the identifier string for access scopes and permission sets (role name for roles).
The functions were renamed to reflect the fact that they are not simple getters on objects, but rather change the underlying database.
| } | ||
|
|
||
| var ( | ||
| generatedObjectTraits = &storage.Traits{Origin: storage.Traits_IMPERATIVE} |
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.
From @parametalol comment line 105 on central/auth/ocpplugin/service/service_impl.go in the first commit (reference at time of writing: 3755769#diff-892fd289d36f704f4f0565732fe8df62e7726ca57d0f1afe2c367aca1b2cadfcR105)
Would it make sense to add Traits_DYNAMIC for visibility or other needs?
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.
That is a good idea. Not implemented in the current PR.
It would be a candidate for a follow-up PR, as it might touch multiple locations in the code, most likely
stackrox/pkg/declarativeconfig/context.go
Lines 38 to 47 in 387c138
| // CanModifyResource returns whether context holder is allowed to modify resource. | |
| func CanModifyResource(ctx context.Context, resource ResourceWithTraits) bool { | |
| if ctx.Value(originCheckerKey{}) == allowOnlyDeclarativeOperations { | |
| return IsDeclarativeOrigin(resource) | |
| } | |
| if ctx.Value(originCheckerKey{}) == allowModifyDeclarativeOrImperative { | |
| return IsDeclarativeOrigin(resource) || resource.GetTraits().GetOrigin() == storage.Traits_IMPERATIVE | |
| } | |
| return resource.GetTraits().GetOrigin() == storage.Traits_IMPERATIVE | |
| } |
It would likely require to add primitives like
IsImperativeOrigin and IsDynamicOrigin and use them in traits validation conditions.
0a84d21 to
d16f7a5
Compare
5752516 to
a432044
Compare
Description
PR stack:
User-facing documentation
Testing and quality
Automated testing
How I validated my change
change me!