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

Always require a ClientInfo with a AuthenticatedUser (no user present should fail authz) #1673

Open
mpenick opened this issue Mar 2, 2022 · 6 comments

Comments

@mpenick
Copy link
Contributor

mpenick commented Mar 2, 2022

We should remove (or severely deprecate) overloads of Persistence.newConnection() that don't require a ClientInfo with a valid authenticated user. Maybe this isn't possible while supporting the C* backends in persistence?

Also, when there's a authentication service configured an AuthenticatedUser (token) should not be optional i.e. in StargateQueryHandler:

    if (getAuthorizationService().isPresent() &&
        (customPayload == null || !customPayload.containsKey("stargate.auth.subject.token"))) {
      // Error: A token should be required if there's an authentication service
    }
@tatu-at-datastax
Copy link
Collaborator

Looks like there are just 2 overloads of newConnection() one with no arguments, the other with ClientInfo. So it'd be former to deprecate?

With StargateV2 plan we could probably:

  1. Add "@deprecate" on no-args variant for Stargate 1.0.x
  2. Remove method completely from Stargate 2.0.x

and consider supporting any use cases otherwise unsupported by figuring out how to create variant ClientInfos with placeholder content necessary.

@olim7t
Copy link
Collaborator

olim7t commented Mar 2, 2022

Copying my comments from #1671 for visibility:

There are legitimate cases where we don't have a user token: background "admin" queries that are not directly in response to an external user action.
In v1 this is anywhere we use DataStoreFactory.createInternal(). The v2 equivalent is when we create a gRPC client with the bridge's "admin token" (source).

If we close the Persistence.newConnection() loophole, we'd need to handle it further downstream. For example creating a "superuser" AuthenticatedUser, that is special-cased by the authorization service.

@mpenick
Copy link
Contributor Author

mpenick commented Mar 2, 2022

Could we create a dummy token for that use case that has to be explicitly set? versus not having a token being a superuser.

public static AuthenticatedUser SUPERUSER = /* Some generator */;

// ...

void doSomething() {
   // ...
   connection.clientInfo().ifPresent(c -> c.setAuthenticatedUser(SUPERUSER));
}

@tatu-at-datastax
Copy link
Collaborator

tatu-at-datastax commented Mar 2, 2022

Perhaps we could require Authentication Provider to implement accessor for providing such super-user AuthenticatedUser instance? That way they would recognize that special type.
In fact that could take some descriptive metadata so we could have multiple such "service users" which could be good for logging/audit purposes too.

Sounds like this could/should be v2 feature since that's where it would be needed.

I agree that if feasible something like this -- and in general, not implicitly assuming "no user means super user"! -- would be a better solution than what we have currently.

@ivansenic
Copy link
Contributor

Wait, but you are not considering the HTTP API usage paths, that do authn/authz before doing any queries to the data store and then use the newConnection() to avoid any other authorizations in the StargateQueryHandler.. Think this is something that @olim7t also mentioned above.

@tatu-at-datastax
Copy link
Collaborator

@ivansenic In Stargate v2 these calls/checks are no longer made by front-end APIs like REST but backend (persistence / bridge). Front-end will pass ClientInfo of the external request. Unauthenticated calls are only for background schema access.

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

No branches or pull requests

4 participants