-
Notifications
You must be signed in to change notification settings - Fork 24
feat(authz): defer to request auth as decision/entitlements entity #2789
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: main
Are you sure you want to change the base?
Conversation
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Standard Benchmark Metrics Skipped or FailedBulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
|
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Standard Benchmark Metrics Skipped or FailedBulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
|
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Standard Benchmark Metrics Skipped or FailedBulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
|
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Standard Benchmark Metrics Skipped or FailedBulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
|
// *EntityIdentifier_EntityChain | ||
// *EntityIdentifier_RegisteredResourceValueFqn | ||
// *EntityIdentifier_Token | ||
// *EntityIdentifier_UseRequestToken |
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.
just a nit but will it be confusing for folks? If EntityIdentifier_UseRequestToken
is false, it kind of implies "the request token is not being used", rather than, "the request token is not being used AS the entity itself". maybe that's just me
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.
Yeah that's a valid point @cassandrabailey293. Originally it was EntityIdentifier_RequestToken
, but I requested a verb to prefix it since its a boolean unlike the other options.
@jakedoublev @c-r33d we could consider something like _PassthroughRequestToken
which might be a little more descriptive and less ambiguous per Cass' feedback.
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'm happy to change the name to whatever makes most sense. I think some other options are:
FromRequestToken
DeriveFromRequestToken
FromAuthHeader
PassthroughRequestAuthToken
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 like the _From
prefix because it's clear you're deriving the entity from the request token. but the fact that this arg itself is a boolean makes it feel a little weird, since _From
kind of implies that the object type is whatever follows the _From
...
_PassthroughRequestToken
or _PassthroughRequestToken
is more explicit. It doesn't "sound as nice" but is more clear IMO. maybe something like _IsRequestToken
?
naming things is hard 😅
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'm good with either of those, but just to throw one more candidate in: _PassthroughFromRequestToken
?
* 🟩 **Good**, because better API UX/DX for use cases where a PEP auth == user auth | ||
* 🟩 **Good**, because keeps token handling logic in SDKs centralized | ||
* 🟩 **Good**, because simplified Obligations decision check flow | ||
* 🟩 **Good**, because explicit flag on request avoids footguns in super-privileged PEPs |
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 PEP / client code using the SDK would be responsible for properly setting this, is that correct? since it's explicit, does that mean the client code will explicitly pass in NULL or FALSE when you don't want to use the token as the entity and use an entity representation instead?
what would be the implication if I as a developer accidentally (or intentionally) set this flag to TRUE within a PEP where the NPE client is super-privileged?
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 PEP / client code using the SDK would be responsible for properly setting this, is that correct?
Yes, this would be part of the GetDecision(GetDecisionRequest)
request property.
Since it's explicit, does that mean the client code will explicitly pass in NULL or FALSE when you don't want to use the token as the entity and use an entity representation instead?
There is a check in the proto that the value MUST be true (expression: "this == true"
). So setting to NULL or FALSE would result in a clientside error if interceptors are used and a serverside error when the request is sent.
what would be the implication if I as a developer accidentally (or intentionally) set this flag to TRUE within a PEP where the NPE client is super-privileged?
The developer would get a get a successful decision response and the audit trail would show that the NPE was granted access (not the entity who initiated the request with the PEP). The same accidental / malicious concerns could be raised about a PEP sending a GetDecisionRequest
with the token
field populated with their PEP's token.
Additionally, we should no longer need to be concerned with the accidental super-privileged PEP with the work @jakedoublev did to add environmental support to subject mappings.
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.
There is a check in the proto that the value MUST be true (expression: "this == true"). So setting to NULL or FALSE would result in a clientside error if interceptors are used and a serverside error when the request is sent.
The other piece here @cassandrabailey293 @jrschumacher is that this EntityIdentifier
itself is a oneof
, so the PEP could never set both this passthrough token flag to true and an entity chain, JWT, or registered resource FQN within a valid request. If this new option is true, it must be the only identified entity in the request, and vice versa if one of the other types is provided.
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.
super helpful explanations, thank you both! it quells my concerns around there being an exploit. almost goes without saying but this should be reflected in the documentation (both where we talk about authorization and entities, as well as in the openapi generated docs).
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.
Note, talking to @jakedoublev my recollection of where that logic got put is wrong its part of Auth service.
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.
Note, talking to @jakedoublev my recollection of where that logic got put is wrong its part of Auth service.
Exactly. Environment/Subject entities are defined in an entity chain by ERS, and when Auth Service is decisioning, it will skip environment entities in the chain.
Proposed Changes
Checklist
Testing Instructions