Skip to content

SOLR-12666: Add authn & authz plugins that supports multiple authentication schemes, such as Bearer and Basic#355

Merged
thelabdude merged 12 commits intoapache:mainfrom
thelabdude:SOLR-12666
Nov 1, 2021
Merged

SOLR-12666: Add authn & authz plugins that supports multiple authentication schemes, such as Bearer and Basic#355
thelabdude merged 12 commits intoapache:mainfrom
thelabdude:SOLR-12666

Conversation

@thelabdude
Copy link
Contributor

@thelabdude thelabdude commented Oct 19, 2021

see: https://issues.apache.org/jira/browse/SOLR-12666

Unit tests still in progress but tested locally with Basic and JWT (Keycloak's OIDC). Overall, the basic concept works using the plugin framework as-is and doesn't require a major overhaul.

Also, need to implement the edit method for MultiAuthRuleBasedAuthorizationPlugin (a basic impl is already in place in this PR for MultiAuthPlugin).

Still need to work this plugin into the security UI so that users can interact with the BasicAuthPlugin if it's one of the schemes in the multi- list.

@thelabdude thelabdude marked this pull request as draft October 19, 2021 20:39
for (CommandOperation c : commands) {
String scheme = (String) c.getDataMap().get(PROPERTY_SCHEME);
if (scheme == null) {
throw new SolrException(ErrorCode.BAD_REQUEST, "All edit commands must include a 'scheme' attribute!");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the edit() command can now take a scheme property to say which sub plugin to edit? Do you plan to implement this in AdminUI, so we can still edit BasicAuth in UI? Will it have other implications elsewhere? I guess users just need to stik a scheme: foo property in their existing edit commands to target the correct plugin?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The edit impl there was a quick hack to see what was possible. I don't love all the adding / removing of the scheme from the maps being manipulated, so maybe I just add an extra layer of wrapping, e.g.

{
  "set-property": {
    "bearer": {
      "blockUnknown":true
    }
  }
}

Although not sure that ^^ is any better than:

{"set-property": {"blockUnknown":true, "scheme":"bearer"}}

Ultimately I think it comes down to us just documenting what this plugin expects when editing.

And yes, definitely want to incorporate support for this plugin into the Admin UI screen.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think scheme meeds to be indicated on a "higher" level. Take the {"set-user": {"tom":"TomIsCool", "harry":"HarrysSecret"}} command for instance - it would create a user "scheme" with password "bearer" :-)

If CoreContainer was aware of the multi-plugin situation we could indicate scheme in the url path, i.e.

curl http://localhost:8983/solr/admin/authentication/basic -d  '{"set-property": {"blockUnknown":false}}'

If our plugin had the power to add URL sub-paths below itself, then this could be doable...

Or we could add it as a top-level k/v, and require that you cannot mix commands to multiple plugins in the same call:

{"scheme":"basic", "set-property":{...}}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went with a wrapping single key object to identify the scheme, e.g.

{
  "set-user": {
    "basic": {"tom":"TomIsCool", "harry":"HarrysSecret"}
  }
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think 'set-user' is a BasicAuth only command, so feels a bit strange this. Woudl probably be easier to write generic command-wrapping logic if scheme was upper level

{
  "schemes": {
    "basic": {
      "set-user": {"tom":"TomIsCool", "harry":"HarrysSecret"},
      "set-property": {"foo": "bar"}
    },
    "bearer": {
      "set-property": {"blockUnknown": "false"}
    }
  }
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will probably be easier to judge once someone tries to integrate this into some script or tool that needs to configure multiple auth programatically.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have multiple plugins so you need to identify which the command applies to. Of course basic only supports set-user but what if someone writes a digest plugin and it also supports set-user, now my approach isn't all that "strange". I think having the outer schemes wrapper is cumbersome for single command requests. At this point, not sure it matters but I don't want to keep going around and around on silly JSON nuances as that requires code and doc changes each time.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, we can even mark the plugin as experimental in the first few releases until more integrations validate it.

@janhoy
Copy link
Contributor

janhoy commented Oct 21, 2021

A concern with enabling BasicAuth is that it is less secure than OIDC which has a expiry of tokens, while a password is long-lived. To mitigate this added surface area, I wonder how easy it would be to "lock down" what a BasicAuth user can do in the system, such as limiting the role's permissions to block any request except /admin/system/info and /admin/collection?cmd=CLUSTERSTATUS. I think this is doable in authorization..

@thelabdude
Copy link
Contributor Author

This is mostly ready to go, minus the ref guide updates, will get those done soon.

@thelabdude thelabdude marked this pull request as ready for review October 26, 2021 15:11
@thelabdude
Copy link
Contributor Author

A concern with enabling BasicAuth is that it is less secure than OIDC which has a expiry of tokens, while a password is long-lived. To mitigate this added surface area, I wonder how easy it would be to "lock down" what a BasicAuth user can do in the system, such as limiting the role's permissions to block any request except /admin/system/info and /admin/collection?cmd=CLUSTERSTATUS. I think this is doable in authorization..

Not sure why this is a concern? Locking down endpoints to a specific role (or roles) is why we have the authorization framework. The introduction of the MultiAuthPlugin really has no bearing on how people choose to configure their permissions.

@janhoy
Copy link
Contributor

janhoy commented Oct 27, 2021

Not sure why this is a concern? Locking down endpoints to a specific role (or roles) is why we have the authorization framework. The introduction of the MultiAuthPlugin really has no bearing on how people choose to configure their permissions.

Yes, probably you have a small set of BasicAuth users for tools, and if those tools only need R/O access to some endpoints then the security is good. But if you want all of bin/solr features to use BasicAuth, then you need to give that user several WRITE/ADMIN permissions, and a PW leak becomes a real convern.

Copy link
Contributor

@janhoy janhoy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I won't hold this up with bike shedding JSON :)

Consider marking the Auth+Authz plugins as lucene.experimental and note the same in ref-guide. That way we have a way to adjust in 9.x.

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

Successfully merging this pull request may close these issues.

2 participants