Implement the access restrictions feature of external access service tokens #24702

freben · 2024-05-08T22:16:14Z

As part of BEP-0007.

The config format is a little different from the internal names of things, for convenience. Open to suggestions here.

I put it as a visible type on the service principal, even though I really originally intended for it to be hidden on the internal credential type instead. This would let us iterate more easily on it in secrecy. But reaching that field becomes cumbersome since the types involved are only in backend-app-api, but need to be consumed from plugin-permission-node. In the end, I decided to hope that these interfaces are stable enough, that there isn't significant risk in setting this in stone in backend-plugin-api. Again, feedback is welcome.

Another potential middle ground could be to make the scope type something semi opaque like Record<string, string[]> and have different subsystems use keys in that as they see fit.

Regard this as being up for early review, as I'm away for a little bit now.

backstage-goalie · 2024-05-08T22:16:47Z

Changed Packages

Package Name	Package Path	Changeset Bump	Current Version
@backstage/backend-app-api	packages/backend-app-api	patch	`v0.7.6-next.0`
@backstage/backend-plugin-api	packages/backend-plugin-api	patch	`v0.6.19-next.0`
@backstage/backend-test-utils	packages/backend-test-utils	patch	`v0.3.9-next.0`
@backstage/plugin-permission-node	plugins/permission-node	patch	`v0.7.30-next.0`

Rugvip

Sweet 😎 🚀

Rugvip · 2024-05-15T12:17:12Z

packages/backend-app-api/config.d.ts

+             * applies for the framework level; individual plugins may have
+             * their own access control mechanisms on top of this.
+             */
+            scope?: {


I'm thinking restrictions here might be a better name? "scope" (in OAuth) is generally used for additions and extending the scope of what a token is authorized for. This is kind of the opposite, where we restrict access instead.

Naming is hard! "restrictions" sounds a bit like the inverse of "scope" as in being used for subtractions from an existing set. Maybe "allow" or "rules"?

Oh and an addon - maybe the wording "where we restrict access" isn't how I think of it at least, this field is how we GRANT access - but the caveat is, the default value is "all" so to speak. If the default value weren't "all", then "scope" might be less confusing right? As a matter of fact, maybe that's a discussion to have; what SHOULD the default policy be? Or do we need to talk about making the field required?

I think allowing all as a default policy is pretty sensible tbh, but I agree it gets a bit tricky where we start by restricting but then expand. Sill feeling that it makes sense with the example from below with either restrictions or accessRestrictions:

externalAccess: - type: static options: { ... } accessRestrictions: - plugin: scaffolder - plugin: catalog permissionAttributes: { action: read }

Rugvip · 2024-05-15T12:19:44Z

packages/backend-app-api/config.d.ts

+               * the permissions system at all, are not affected by this
+               * setting.
+               */
+              permissions?: string[];


Just to have the discussion: do you think it would make sense to configure permissions per plugin instead? One particularly good reason for that I think is that if you're not also restricting to certain plugins the token scope will still be very wide. If you just restrict to catalog.entity.read or w/e it is, then you'd still be allowed to call any other endpoint on any plugin that doesn't have permission integrations.

It's just that it's so redundant. You have to make the mapping from plugin to permission (and can accidentally list permissions under a plugin where they don't belong). It felt like an unnecessary complication.

You're right though that there's a grey area where permissions aren't enabled at all. I wish that the permissions were gated upfront like the auth is today, such that we can make framework code that rejects requests if tje token has permissions restrictions but the plugin DID NOT have a permissions limitation.

But let's play with the idea of what the config would look like. Would we use plugins as the top level perhaps?

externalAccess: - type: static options: { ... } plugins: - scaffolder # just a string - catalog: permissions: [A, B] # similar to frontend but awkward indent

or

externalAccess: - type: static options: { ... } allow: - plugin: scaffolder # make plugin mandatory - plugin: catalog permissionAttributes: { action: read }

and then people just have to make the right mapping.

I think the last one kinda looks clear and concise enough. Maybe like you say, name it accessRestrictions or restrictions instead of allow, that's another side discussion. Drawback is, you can't give a token blanket permission "you can perform reads anywhere", but as long as there aren't permission checks everywhere, i guess that won't be a possibility anyway

Yep that last one is what I had in mind

Rugvip · 2024-05-15T12:27:16Z

packages/backend-app-api/src/services/implementations/auth/DefaultAuthService.ts

+      return createCredentialsWithServicePrincipal(
+        externalResult.subject,
+        undefined,
+        externalResult.scope,


The scope doesn't get forwarded to plugin request tokens. For example, if you've issued a token that allows a user to call the TechDocs APIs, you will indirectly get access to read from the catalog, or at least enumerate entities. I don't think that's necessarily wrong, but something to document clearly and think a bit more about.

One option could be to forward the external request token, but that kinda goes against what we're trying to do here. We could also forward the scope in the plugin request tokens. That'll keep things more locked down, but it does mean that you'll need to anticipate all of the indirect permissions that you need to allow for a token based on what calls the target plugin is making.

Yep I noted the same thing. I don't think we want to deal with transitive permissions though. It makes things much harder to reason about. We have to trust plugins to do the right thing anyway, with requests that come in. The same problem kind of applies to user tokens as well, right? There's a security perspective here for sure but I'd prefer to leave it out of scope here.

Yep I think it's the way to go, just gotta make sure it's all clearly communicated too

packages/backend-app-api/config.d.ts

plugins/permission-node/src/ServerPermissionClient.ts

Rugvip

Alright, digging into details a bit more

Rugvip · 2024-05-17T12:19:19Z

packages/backend-app-api/config.d.ts

+             * individual plugins may have their own access control mechanisms
+             * on top of this.
+             */
+            accessRestrictions?: Array<{


Hmm, wonder if we should avoid adding this to legacy auth? Feels a bit cleaner to add it only once we've introduced new methods for HMAC/asymmetric auth?

Sure, we could. Internally it's basically all the same,but we can strip this out

Hm thinking about this a bit more, why though? It's not like it's in the old auth.keys section. I actually wanted the access restrictions to be outside of the type, in practice, such that you can put it on any external access method no matter what. I'd have wanted to declare this essentially as

externalAccess?: Array< | ExternalAccessItem<'legacy', LegacyOptions> | ExternalAccessItem<'static', StaticOptions> >,

or the corresponding thing in json-schema, and it just being part of the actual item structure whether you want it or not. But the format isn't particularly amenable to it.

Only reason is really to drive adoption of new formats once available. No super strong opinion here though, if you think it's better for symmetry I think we can keep it too.

Rugvip · 2024-05-17T12:25:30Z

packages/backend-app-api/src/services/implementations/auth/DefaultAuthService.ts

+          r => r.pluginId === this.pluginId,
+        )
+      ) {
+        throw new NotAllowedError('Access restricted');


Maybe a bit more info here tbh? Thinking about the case where someone provides a token to someone else and they then get this error when making a call. Something like "the provided token is restricted to the '...' plugin(s)". Feels like that handling of user input is relatively safe

Sure! Maybe I was overly cautious here. After all, these are tokens that you may hand out to random systems on the internet so if they leak, this lets attackers iterate a bit more easily. But I can add that info

packages/backend-app-api/src/services/implementations/auth/external/helpers.ts

Rugvip · 2024-05-17T12:38:10Z

packages/backend-plugin-api/api-report.md

@@ -136,6 +137,15 @@ export type BackstageNonePrincipal = {
  type: 'none';
 };

+// @public
+export type BackstagePrincipalAccessRestrictions = Array<{
+  pluginId: string;


Hmm, not sure this should be here. At this point we've already validated the plugin right? I think it would even make sense to unpack the permission restrictions to apply to this plugin, i.e. this shouldn't be an array

Hm interesting. Yeah maybe that will always be the case in practice. It sure should be true today. Good observation, I could scale this back then

Especially if we aren't going to send this transitively to upstreams

Yep, and if we find that to be a huge misstep we should be able to add it back in as restrictions for other plugins in a separate field, or something of that sort.

Rugvip · 2024-05-19T11:32:38Z

plugins/permission-node/src/ServerPermissionClient.ts

  }) {
    this.#auth = options.auth;
    this.#permissionClient = options.permissionClient;
    this.#permissionEnabled = options.permissionEnabled;
+    this.#pluginId = options.pluginId;


This isn't needed anymore right?

Correct! I thought I addressed that. Fixed.

Rugvip · 2024-05-19T11:35:25Z

plugins/permission-node/src/ServerPermissionClient.ts

  }

  async authorizeConditional(
    queries: QueryPermissionRequest[],
    options?: PermissionsServiceRequestOptions,
  ): Promise<PolicyDecision[]> {
+    const maybeResponse = this.#decideBasedOnPrincipalAccessRestrictions(


Feeling it might be a bit cleaner to move this down to replace the queries.map(_ => ({ result: AuthorizeResult.ALLOW })) logic? Having that be the service principal path where we decide based on the access restrictions. Feels a bit strange that we're checking for the service principal twice right now.

The reason I made it a standalone method that's done at the top, is that I wanted to retain the old fallback functionality perfectly (which ALSO in a way has logic for this token type). Thus, when no restrictions exist, it'll just keep running the old code path. Would you prefer I update that? Just kinda have to unwind shouldPermissionsBeApplied a little then.

Hm, or maybe not. I think I see what you mean. I'll move over that code tomorrow and see how it feels.

ok @Rugvip tried for a bit of a refactor - but the way that things are intertwined, am not sure it got all that much better 🤔

Rugvip · 2024-05-19T11:38:22Z

packages/backend-app-api/src/services/implementations/auth/external/types.ts

+  verifyToken(token: string): Promise<
+    | {
+        subject: string;
+        accessRestrictions?: AccessRestriptionsMap;


Here we also already know the plugin ID right? as in this could be a BackstagePrincipalAccessRestrictions

Actually no. The ExternalTokenHandler is made in createRootContext, so it's the DefaultAuthService that's responsible for picking out the right entry in the map for now. We can refactor that later though.

Thinking it would be a bit cleaner to have the token handlers be per plugin though, it's what I was on about over here: #24321 (review)

… BEP-0007 Signed-off-by: Fredrik Adelöw <freben@gmail.com>

Signed-off-by: Fredrik Adelöw <freben@gmail.com>

Rugvip

Najs, 🚢 🇮🇹

github-actions · 2024-05-23T13:50:21Z

Thank you for contributing to Backstage! The changes in this pull request will be part of the 1.28.0 release, scheduled for Tue, 18 Jun 2024.

github-actions · 2024-05-23T13:50:59Z

Uffizzi Cluster pr-24702 was deleted.

freben added auth area:permission Related to the Permission Project Area labels May 8, 2024

freben requested a review from Rugvip May 8, 2024 22:16

freben requested review from a team as code owners May 8, 2024 22:16

freben requested a review from camilaibs May 8, 2024 22:16

github-actions bot removed the auth label May 8, 2024

freben force-pushed the freben/service-token-scopes branch from 1ac8dac to 82b4ea8 Compare May 10, 2024 07:58

Rugvip reviewed May 15, 2024

View reviewed changes

vinzscam requested changes May 16, 2024

View reviewed changes

packages/backend-app-api/config.d.ts Outdated Show resolved Hide resolved

plugins/permission-node/src/ServerPermissionClient.ts Outdated Show resolved Hide resolved

freben force-pushed the freben/service-token-scopes branch 2 times, most recently from 0c7f2ff to fe1905a Compare May 17, 2024 11:41

freben changed the title ~~Implement the scope feature of external access service tokens~~ Implement the access restrictions feature of external access service tokens May 17, 2024

freben force-pushed the freben/service-token-scopes branch 2 times, most recently from b49481c to 39842d5 Compare May 17, 2024 11:53

Rugvip requested changes May 17, 2024

View reviewed changes

freben force-pushed the freben/service-token-scopes branch from 39842d5 to 060f7ea Compare May 17, 2024 20:02

Rugvip requested changes May 19, 2024

View reviewed changes

freben force-pushed the freben/service-token-scopes branch from 1827970 to 6587e84 Compare May 20, 2024 10:13

vinzscam approved these changes May 20, 2024

View reviewed changes

freben force-pushed the freben/service-token-scopes branch 2 times, most recently from c8b85dc to 547a529 Compare May 20, 2024 14:10

freben added 4 commits May 22, 2024 16:14

Implement the scope feature of external access service tokens, as per…

9e63318

… BEP-0007 Signed-off-by: Fredrik Adelöw <freben@gmail.com>

skip the plugin id for permissions client

b155d85

Signed-off-by: Fredrik Adelöw <freben@gmail.com>

one token manager per plugin

17be3e6

Signed-off-by: Fredrik Adelöw <freben@gmail.com>

refactor the client a bit

0639b07

Signed-off-by: Fredrik Adelöw <freben@gmail.com>

freben force-pushed the freben/service-token-scopes branch from 547a529 to 0639b07 Compare May 22, 2024 14:15

freben mentioned this pull request May 22, 2024

update the jwks external auth to use singular nouns #24874

Merged

Rugvip approved these changes May 23, 2024

View reviewed changes

freben merged commit 0568ac6 into master May 23, 2024
28 checks passed

freben deleted the freben/service-token-scopes branch May 23, 2024 13:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement the access restrictions feature of external access service tokens #24702

Implement the access restrictions feature of external access service tokens #24702

freben commented May 8, 2024 •

edited

backstage-goalie bot commented May 8, 2024 •

edited

Rugvip left a comment

Rugvip May 15, 2024

freben May 16, 2024

freben May 16, 2024 •

edited

Rugvip May 16, 2024

Rugvip May 15, 2024

freben May 16, 2024 •

edited

Rugvip May 16, 2024

Rugvip May 15, 2024

freben May 16, 2024

Rugvip May 16, 2024

Rugvip left a comment

Rugvip May 17, 2024

freben May 17, 2024

freben May 17, 2024 •

edited

Rugvip May 17, 2024

Rugvip May 17, 2024

freben May 17, 2024

Rugvip May 17, 2024

freben May 17, 2024

freben May 17, 2024

Rugvip May 17, 2024

Rugvip May 19, 2024

freben May 19, 2024

Rugvip May 19, 2024

freben May 19, 2024 •

edited

freben May 20, 2024

Rugvip May 19, 2024

freben May 19, 2024

Rugvip May 19, 2024

Rugvip left a comment

github-actions bot commented May 23, 2024

github-actions bot commented May 23, 2024

Implement the access restrictions feature of external access service tokens #24702

Implement the access restrictions feature of external access service tokens #24702

Conversation

freben commented May 8, 2024 • edited

backstage-goalie bot commented May 8, 2024 • edited

Changed Packages

Rugvip left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

freben May 16, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

freben May 16, 2024 • edited

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

Rugvip left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

freben May 17, 2024 • edited

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

freben May 19, 2024 • edited

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

Rugvip left a comment

Choose a reason for hiding this comment

github-actions bot commented May 23, 2024

github-actions bot commented May 23, 2024

freben commented May 8, 2024 •

edited

backstage-goalie bot commented May 8, 2024 •

edited

freben May 16, 2024 •

edited

freben May 16, 2024 •

edited

freben May 17, 2024 •

edited

freben May 19, 2024 •

edited