Skip to content
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

🐛 Bug Report: Unable to use a different name for the Authorization token #23015

Closed
2 tasks done
alexef opened this issue Feb 16, 2024 · 29 comments · Fixed by #24878
Closed
2 tasks done

🐛 Bug Report: Unable to use a different name for the Authorization token #23015

alexef opened this issue Feb 16, 2024 · 29 comments · Fixed by #24878
Labels
bug Something isn't working

Comments

@alexef
Copy link
Contributor

alexef commented Feb 16, 2024

📜 Description

We use an external authorization layer on our ingresses, and configured oauthProxy as the auth provider.
While some plugins properly implement the FetchMiddlewares.injectIdentityAuth method, which allows us to use a custom header name, there are multiple places in code where we assume the name of the header is 'Authorization' (examples: https://github.com/search?q=repo%3Abackstage%2Fbackstage+%22Authorization%3A+%60Bearer%22+language%3ATypeScript+path%3A&type=code) - techdocs, catalog being the most striking.

LE: this is by design. Fronted plugins must use fetchAPI while backend plugins must pass an Authorization header. The bug scope is reduced to only fronted plugins.

👍 Expected behavior

All frontend API requests should use the configurable fetchAPI so that the header name can be configured and not overlap with other authentication layers.

👎 Actual Behavior with Screenshots

Authorization header is (sometimes) hardcoded

👟 Reproduction steps

Try to change the name of the Authorization header.

📃 Provide the context for the Bug.

No response

🖥️ Your Environment

No response

👀 Have you spent some time to check if this bug has been raised before?

  • I checked and didn't find similar issue

🏢 Have you read the Code of Conduct?

Are you willing to submit PR?

Yes I am willing to submit a PR!

@aramissennyeydd
Copy link
Contributor

aramissennyeydd commented Feb 17, 2024

Hey 👋 ! This is a bit of a two-headed problem.

  1. Some clients haven't been migrated to the new fetchApi in the frontend or pulled code from older implementations. These are pretty easy to find as they use the browser's fetch and call identityApi.getCredentials. Based on the number of plugins that are still doing this, we may need to update the docs for this as well.
  2. Some clients are intended to be consumed by both the frontend and the backend. These are a bit more difficult to transfer if they aren't already, the CatalogClient is a great example of this. It needs to support service-to-service requests as well as frontend authenticated requests. In the frontend, it uses the fetchApi, https://github.com/backstage/backstage/blob/patch/v1.22.0/plugins/catalog/src/plugin.ts#L63-L71 and in the backend defaults to using cross-fetch. In the backend, the pattern of calling it looks something like this, https://github.com/backstage/backstage/blob/patch/v1.22.0/plugins/catalog-backend-module-backstage-openapi/src/InternalOpenApiDocumentationProvider.ts#L123.

The project has settled on using Authorization for backend requests, what we call service-to-service auth. For frontend requests, we should definitely be consistent in using fetchApiRef.

There's also a bit of work going on in #22603 that will affect the pattern of calling these clients from the backend.

@freben
Copy link
Member

freben commented Feb 17, 2024

Total side note @aramissennyeydd - that was actually meant to use node-fetch :) Not that it makes a huge difference in practice.

@aramissennyeydd
Copy link
Contributor

@freben Ooo, that would make sense. Should I update the OpenAPI template for that? I was basing off of the existing CatalogClient.

@freben
Copy link
Member

freben commented Feb 17, 2024

For backend, yeah.

The deal for the catalog has been

  • in the frontend, inject fetchApi (whatever it happens to use under the hood, which likely is the native browser fetch)
  • in the backend, inject node-fetch and supply tokens to each individual call.

Now with the new auth work we're doing, we'll see if that backend picture shifts or not :)

@aramissennyeydd
Copy link
Contributor

Wait, wouldn't this fit into the last section of the ADR?

Isomorphic packages should have a dependency on the cross-fetch package for mocking and type definitions.

Just spent like 15 minutes going down the rabbit hole of node-fetch as fetch not working 😅 .

@freben
Copy link
Member

freben commented Feb 17, 2024

As which particular fetch? ;)

yeah I see the catalog client actually uses global.fetch as its input type but uses cross-fetch as the default actual fallback implementation

@alexef
Copy link
Contributor Author

alexef commented Feb 18, 2024

There's also a bit of work going on in #2260 that will affect the pattern of calling these clients from the backend.

@aramissennyeydd I believe #2260 is the wrong link here?

If I understand correctly, frontend should be consistent in using the fetchApi (which allows the authorization header to be named differently) while backend (service-to-service) must use Authorization. We self host backstage so if I'm not mistaken backend traffic doesn't leave the pod - there are scheduled tasks that run and talk to the API but I assume they do it on localhost:7007.

To me this sounds like making frontend consistent would solve the proxy authentication requirement (that we leave the external Authorization header intact).

@aramissennyeydd
Copy link
Contributor

@alexef Yup, should be #22603, must have accidentally deleted the last digit. And yes, you're correct, the frontend should use fetchApi and the backend should continue to use Authorization. Backend traffic shouldn't leave the pod, but some users may have split backends with traffic going between pods.

We'd love your help in making that frontend consistency happen!

@alexef
Copy link
Contributor Author

alexef commented Feb 18, 2024

ok, so here is the battle plan. frontend plugins to adjust to use the fetchApi:

  • plugins/todo (gone)
  • plugins/sonarqube (gone)
  • plugins/rollbar (gone)
  • plugins/kafka (gone)
  • plugins/jenkins (gone)
  • plugins/fossa (gone)
  • plugins/azure-sites (gone)
  • plugins/search
  • plugins/kubernetes-react
  • plugins/catalog-import
  • plugins/linguist
  • plugins/devtools
  • plugins/azure-devops

frontend plugins to adjust passing auth with configurable header name but without fetchApi:

  • plugins/techdocs (uses event-source-polyfill and hardcoded Authorization header name)
  • plugins/scaffolder (same)

need more clarity:

  • plugins/permission-common - common library, but it hardcodes the header name
  • packages/catalog-client - should we just not pass options.token when the library is used from client code? or indicate somehow that only backend code may bring its own token?

can I bundle multiple plugin updates in one PR or should I do one per plugin?

@freben
Copy link
Member

freben commented Feb 18, 2024

The BEP explicitly calls out in its non-goals section that it does not aim to make room for the ability to change out the Authorization header as the carrier.

One big reason for leaving that out of that particular workstream is to not scope creep. But also, we realize that Authorization is uniquely positioned in the sense that it doesn't get logged or otherwise handled in such a way that it might be compromised. All parties in all call chains know the semantics of how to handle it. Therefore it seemed like something that if it were to be made a priority, it warranted being a separate thing to look into.

In the current work based on that BEP, the old contrib article for securing your backend goes away entirely, and is replaced by the platform "forcibly" locating, validating and unpacking the incoming credentials so that when the plugin is reached, the validity and chain of trust for the caller is already established. And the platform code will only, as part of this initial work at least, look at the Authorization header.

We did speak a bit about this subject when we started out, which is why it's mentioned at all in the non-goals. There were some ideas that were floated, including using multiple Authorization headers so as not to interfere with any other ones added by the original caller. The proxy backend could even be smart about it and strip out the Backstage specific one if it sees several, before sending the request along.

To get unblocked, if you only want to deal with the frontend calls to the backend, you could replace the fetchApi and then on the server side have a small middleware at the very top that looks for the custom header and injects it again as an Authorization header so the rest of the stack gets happy.

I will say though, moving forward with ensuring that the fetchApi is universally used is a massively valuable thing either way. And if/when there's official support for alternative headers, the groundwork has been laid.

@awanlin
Copy link
Collaborator

awanlin commented Feb 18, 2024

I've got a PR for updating the Azure DevOps, DevTools, and Linguist plugins 👍

#23046 (this has been on my todo list for a bit now and I wanted to make these changes myself to better understand)

@aramissennyeydd
Copy link
Contributor

aramissennyeydd commented Feb 18, 2024

@freben Thoughts on a new eventSourceApi that uses EventSourcePolyfill under the hood for plugins/catalog and plugins/scaffolder (to be used for other SSE's as well)?

On a bit of a separate note, might it be worth moving some of that backend complexity to an abstracted backend implementation of fetchApi? Rugvip added some thoughts in the BEP around that, https://github.com/backstage/backstage/blob/master/beps/0003-auth-architecture-evolution/README.md?plain=1#L319-L324, but I'd be curious to see if this is something we can revisit especially as we start adding more to the service-to-service request layer. This would also hopefully save us entirely from passing the token into the client for just backend requests and we could move towards the pluginId://{path} format consistently that's existed for a while but stayed generally unused. Might be follow on work after the BEP/implementation lands.

@alexef For the CatalogClient, yes, we shouldn't be passing token when this is used in the frontend. I'm not sure if this a restriction we can enforce, but it should generally not be happening. For the PermissionClient, I think this should be updated to follow the pattern in CatalogClient (allowing an override for the fetch implementation it uses instead of just using cross-fetch).

For PRs, I'd recommend splitting it into 2 pieces, the PermissionClient updates and the frontend fetchApi updates. @awanlin is a speed demon and already hopped on his plugins 😁 .

@freben
Copy link
Member

freben commented Feb 20, 2024

@freben Thoughts on a new eventSourceApi that uses EventSourcePolyfill under the hood for plugins/catalog and plugins/scaffolder (to be used for other SSE's as well)?

Hm maybe. The usages are pretty few though. Would it be worth making it something even higher-level in its naming, so that it could support other tech backing it, or does that just make things unnecessarily complex? Probably.

Copy link
Contributor

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale label Apr 20, 2024
@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Apr 27, 2024
@aramissennyeydd
Copy link
Contributor

Gonna reopen this as the requested clients haven't been updated AFAIK

@alexef
Copy link
Contributor Author

alexef commented May 17, 2024

@freben

then on the server side have a small middleware at the very top that looks for the custom header and injects it again as an Authorization header so the rest of the stack gets happy.

how do I add a middleware to my backend, I couldn't find an example?

@freben
Copy link
Member

freben commented May 17, 2024

@alexef You register a custom rootHttpRouter service, and pass in a configure callback to it that adds what you want.

Something along the lines of (not tested ...)

backend.add(rootHttpRouterServiceFactory({
  configure({ app, applyDefaults }) {
    app.use(myMiddleware);
    applyDefaults();
  }
});

@alexef
Copy link
Contributor Author

alexef commented May 17, 2024

tested, works!

@Rugvip
Copy link
Member

Rugvip commented May 17, 2024

I wonder if another approach here could be to switch to a fetch-based event source? Afaik there's nothing particularly special about the EventSource API, and it's just a client that knows how to slowly parse a chunked HTTP response.

Quick search found https://github.com/Azure/fetch-event-source, which could be a good candidate. It allows for a custom fetch implementation to be passed too.

@alexef
Copy link
Contributor Author

alexef commented May 17, 2024

I can try an alternative implementation using microsoft/fetch-event-source. thanks for the pointer!

@alexef
Copy link
Contributor Author

alexef commented May 23, 2024

I can see that some of the plugins in the list were moved unmodified to https://github.com/search?q=repo%3Abackstage%2Fcommunity-plugins%20Authorization&type=code

will follow up with the remainder

@alexef
Copy link
Contributor Author

alexef commented May 27, 2024

#24861 and #24878 ready

@alexef
Copy link
Contributor Author

alexef commented May 28, 2024

Not fully completed (missing events and a finall checkup that no hardcoded indentityApi token remains)

@ggkoning
Copy link

ggkoning commented Jun 3, 2024

@alexef How can I change the name of the backstage authorization header?

@alexef
Copy link
Contributor Author

alexef commented Jun 3, 2024

@ggkoning first of all, in packages/backend/src/index.ts, you need:

...
const backend = createBackend();
 backend.add(rootHttpRouterServiceFactory({
   configure: ({ app, applyDefaults }) => {
     app.use((req, _, next) => {
       if (req.headers['backstage-authorization']) {
         req.headers.authorization = Array.isArray(req.headers['backstage-authorization']) ? req.headers['backstage-authorization'][0] : req.headers['backstage-authorization'];
         delete req.headers['backstage-authorization'];
       }
       next();
     });
     applyDefaults();
   },
 }));

(to trick the backend into consuming authorization from the custom header)

then in: packages/app/src/apis.tsx:

...
 createApiFactory({
     api: fetchApiRef,
     deps: {
       configApi: configApiRef,
       identityApi: identityApiRef,
       discoveryApi: discoveryApiRef,
     },
     factory: ({ configApi, identityApi, discoveryApi }) => {
       return createFetchApi({
         middleware: [
           FetchMiddlewares.resolvePluginProtocol({
             discoveryApi,
           }),
           FetchMiddlewares.injectIdentityAuth({
             identityApi,
             config: configApi,
             header: {
               name: 'Backstage-Authorization',
               value: (backstageToken) => `Bearer ${backstageToken}`,
             }
           }),
         ],
       });
     },
   }),

(to trick clients to send the custom header instead of default)

@ggkoning
Copy link

ggkoning commented Jun 4, 2024

@alexef Thanks it works. You said that it is not fully completed yet what can I do to help?

@alexef
Copy link
Contributor Author

alexef commented Jun 4, 2024

@ggkoning thanks for offering. you can try to spot other frontend->backend clients that don't use the fetchApi or that hardcode the "Authorization" header. (backend->backend is fine)

the events PR is merged so it will be part of 1.28.

also, similar changes to those in the PRs linked to this ticket need to be done to the plugins that moved out in: https://github.com/backstage/community-plugins

@ggkoning
Copy link

ggkoning commented Jun 17, 2024

@alexef I want to make these changes to this plugin https://github.com/backstage/community-plugins/tree/main/workspaces/jenkins/plugins/jenkins But how can I locally add and test that this plugin will work in our backstage deployment?

@ggkoning
Copy link

@alexef I don't know how to test it but do you think these changes I made are good? backstage/community-plugins#548

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants