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

Enabling Eclipse as an authentication provider alongside GitHub makes usernames potentially non-unique and potentially breaks access control #2529

Closed
ghost opened this issue Aug 17, 2020 · 3 comments

Comments

@ghost
Copy link

ghost commented Aug 17, 2020

Before enabling authentication with Eclipse as a provider (see #2330), the only OAuth provider that used arbitrary usernames was Github (Bosch providers use UIDs in some form or another).

The user table has been designed with no user name uniqueness in mind originally, although it actually had unique usernames, since none of the OAuth providers supported would "step on each other's toes" in that regard.

The lack of uniqueness restriction on one end, and the fact that there can be two identical usernames belonging to two different users on the other has the potential to break access control entirely throughout the application.

There are two alternative solutions to this, and an immediate workaround:

  • Solution 1: enforce usernames unique. This would require less changes in the code base or DDL, but prevent sign up of users whose username matches another user. It would also leverage the authentication provider we persist when any user signs up, to restrict access to only that username + authentication provider association.
  • Solution 2: re-write every bit of authorization code based on username only, since a repository search by username would no longer be sufficient to identify the right entity. Any former authorization call that only resolves the user by username would now check for their authentication provider as well. This is much more work potentially, but would produce safer code in the end.
  • Immediate workaround: disable authentication through Eclipse for now, as it's the newest provider and still has few to no users in dev (none in prod since it hasn't been deployed there).
@ghost ghost added bug security labels Aug 17, 2020
ghost pushed a commit to bosch-io/vorto that referenced this issue Aug 17, 2020
Temporarily disables eclipse as authentication provider due to discoveries in eclipse-vorto#2529

Signed-off-by: Menahem Julien Raccah Lisei <[email protected]>
@ghost ghost mentioned this issue Aug 17, 2020
@JulianFeinauer
Copy link
Contributor

@mena-bosch shouldnt it be possible to distinguish between an id which is prefix + username like "eclipse:j.feinauer@..." or "github:j.feinauer@..." and the name itself which is just the id without the prefix (would be "j.feinauer@..." in both cases but doesnt matter there.
Shouldnt this be doable without too much change in the application?

@ghost
Copy link
Author

ghost commented Aug 21, 2020

Hi @JulianFeinauer ,

first of all, many late thanks for your contribution implementing the #2330 task!

Unfortunately at the time where I noted it down, I couldn't perform any pre-analysis, which is why we're here now.
On the merry side of things, disabling your implementation only took commenting out a couple of Spring annotations, so it will be trivial to re-enable it once we've solved this problem.

The solution you propose seems like a middle ground between having unique user IDs (my solution 1) and letting users with the same username coexist in Vorto (expected outcome of solution 2).

As a side-note, we already persist the authentication provider for each user's first-time sign up, so we do have this data.
Rather than concatenating the provider and user name, we could just re-design the DDL to use a composite ID of both fields.

Either this or keeping the two separate wouldn't come without consequences though, as we have quite a few foreign key references to the user's current ID as it was designed long ago (which would be just a 64 bit integer), so we'd have to work through that.

The cons of this idea consist both in:

  • non-trivial data migration (consider existing users, but also the fact that on paper, anyone can run their own Vorto repository so the migration would have to be automatized and documented - we've been through this sort of thing a couple of times for Vorto and it's never easy!).
  • if we kept the "new" composite user ID and actual user ID separate, an application-wide differentiation between internal usage for authorization vs presentational values (e.g. "Welcome, {{presentational user ID}}"). This in turn could become pretty cumbersome e.g. for REST calls involving a username, etc.

TL;DR

  • "Shouldnt this be doable without too much change in the application?" --> I have a feeling that none of the solutions so far will involve a small change set.
  • We will need to discuss pros and cons among the team and agree on the least cumbersome solution.
  • Happy to keep you in the loop of course.
  • We could also set up a call and discuss at our leisure when time allows.

Happy weekend and hopefully talk soon!

@kolotu
Copy link
Contributor

kolotu commented Sep 1, 2020

The original implementation of SuiteAuth used the "sub" field of the token as user ID to create/identify the Vorto user - this has now been changed to use the "ext/orig_id/sub" field, which contains a CIAM Bosch ID (S-***), so when you login, you will retrieve your Bosch ID user. This behaviour (retrieving a user from a potentially different authentication provider, was considered a bug in the context of Github/Eclipse, but is the desired behaviour for Bosch ID/ SuiteAuth. This needs to be considered, when addressing the Github/Eclipse issue.

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants