n4bb12 / verdaccio-github-oauth-ui

📦🔐 GitHub OAuth plugin for Verdaccio
https://verdaccio.org
MIT License
73 stars 45 forks source link

Regression: 431 error when trying to login on 3.1.0 #139

Closed hybridherbst closed 2 years ago

hybridherbst commented 2 years ago

Bug Report

Seems there's a regression from 3.0.0 to 3.1.0: when trying to log in, a 431 error is raised. A google search seems to point towards the authorization headers being too large (too much stuff requested in the header), potentially related to #136.

The issue doesn't repro on 3.0.0. It seems its caused by having access to many orgs and repositories.

Versions

Version
verdaccio-github-oauth-ui (Plugin) 3.1.0

Expected behavior

Can login independent of the number of repos and orgs a user has access to.

Observed behaviour

431 error (Auth Headers Too Large) when trying to log in

Steps to reproduce

  1. Update to 3.1.0
  2. Try to log in with an account that has access to many (>50? >100?) repos and/or orgs
  3. Note the 431 error
  4. Downgrade to 3.0.0
  5. Try to log in
  6. Note it works
n4bb12 commented 2 years ago

Thank you for reporting. I'm able to reproduce the problem.

Some first thoughts:

We probably can't change the format of the token (e.g. compress it), because it is not parsed by the plugin but by Verdaccio itself.

One way to keep the token small could be to go through the package rules in the Verdaccio config and only add groups to the login token that are actually used in the permission config. The trade-off would be that users would need to re-authenticate whenever the permission gets changed to using a group that was not previously used. To allow for both we could make this an opt-in or opt-out behavior.

What do you think of this? Do you have any other suggestions?

hybridherbst commented 2 years ago

It seems that currently, the same JWT token is sent twice, once for UI and once for NPM. This could be a first step to reduce the size. Alternatively, I think in Verdaccio once can turn JWT off entirely (legacy:true, e.g. when no token expiration is desired). Does this plugin still work when JWT is turned off?

EDIT: quick hack in AuthCore.ts

const user: User = {
      name: username,
      groups: ["$all", "@all", "$authenticated", "@authenticated"],
      real_groups: [username, ...groups.filter(x => x.startsWith(this.requiredGroup))],
    }
n4bb12 commented 2 years ago
const user: User = {
      name: username,
      groups: ["$all", "@all", "$authenticated", "@authenticated"],
      real_groups: [username, ...groups.filter(x => x.startsWith(this.requiredGroup))],
    }

real_groups needs to contain all groups the user is part of. That's how the field is conceptually defined.

As a bare minimum for access to function properly, it needs to contain all groups used in the Verdaccio packages config. Otherwise users won't have access to things they should have access to. If you only include the org, access through teams or repos or (as you suggested) other orgs, won't work.

I won't consider using the legacy authentication as a solution to this problem.

I'd like to go back to the suggestion I made and think about how the plugin should behave and be configurable. I'd appreciate having that conversation over "code quick fixes". Once we have a working concept, implementing it is going to be the least problem.

hybridherbst commented 2 years ago

I fully agree, this was mostly a quick idea to test something. Regarding configuration, "just" using the groups that are referenced in the package config isn't ideal either (it doesn't scale to more complex setups, and it requires re-authentication whenever the server configuration changes).

Generally I think JWT tokens will require a LOT of re-authentication of users if any kind of access rights change in their current form, which is definitely a problem at scale, and I started a discussion here: https://github.com/verdaccio/verdaccio/discussions/2698

n4bb12 commented 2 years ago

Regarding the duplication, the plugin only includes the list of GH orgs, teams, and repos once in the real_groups array here. Verdaccio copies this into the groups array, for example here and here. I don't think the plugin can avoid or prevent this.

hybridherbst commented 2 years ago

With duplication I mostly meant that here the same token (if the settings of API and web match in the config) is copied twice into the callback URL: https://github.com/n4bb12/verdaccio-github-oauth-ui/blob/ea7555814e8f0aa7f64776825c4d30b819af0115/src/server/plugin/AuthCore.ts#L40

n4bb12 commented 2 years ago

With duplication I mostly meant that here the same token (if the settings of API and web match in the config) is copied twice into the callback URL:

https://github.com/n4bb12/verdaccio-github-oauth-ui/blob/ea7555814e8f0aa7f64776825c4d30b819af0115/src/server/plugin/AuthCore.ts#L40

Good find.

The reason they are both needed is that they are not necessarily the same.

The first is used by the UI. This is always a JWT. The second is an npm token (displayed in the "Registry Info" on the UI) that depends on the Verdaccio security config and in the past also the Verdaccio version.

Both tokens are either signed or encrypted with a server-side secret. So neither can the client create them nor can it decide which one is going to be needed by npm.

In case the npm token is not set, the UI could fall back to showing the UI JWT, allowing us to use a shorter callback URL. We might be able to do this as a secondary optimization, but it's not going to be enough to avoid 431 errors.

n4bb12 commented 2 years ago

https://github.com/n4bb12/verdaccio-github-oauth-ui/releases/tag/3.1.2

hybridherbst commented 2 years ago

You might want to add to the docs that this means wildcards can't be used for package access anymore if I understand the code in the commit right?

n4bb12 commented 2 years ago

If you mean this, yes it still works.