openhab / openhab-core

Core framework of openHAB
https://www.openhab.org/
Eclipse Public License 2.0
911 stars 420 forks source link

Add Role Based Access Control [RBAC] #3305

Open hmerk opened 1 year ago

hmerk commented 1 year ago

See community discussion and proposal https://community.openhab.org/t/rbac-model-in-openhab-and-potential-security-vulnerability-found/136419

hmerk commented 1 year ago

@openhab/core-maintainers Would you please add this to the project list for openHAB 4.0. This would be a very good addition.

hmerk commented 1 year ago

Could add it myself ;-)

splatch commented 1 year ago

@hmerk while not being a core contributor I might be able to work on this in my spare time based on earlier work I did with Open Smart House.

I outlined some of concepts in forums https://community.openhab.org/t/rbac-model-in-openhab-and-potential-security-vulnerability-found/136419/4. They are based on initial implementation of eclipse/smarthome#6034 which covered first step of design made under eclipse/smarthome#579. Long story short - we got authentication API which was adjusted/stripped for openHAB 3.0, we never reached step 2 which would involve wider support for user roles and permissions.

hmerk commented 1 year ago

@splatch To be honest, I don't care who is doing this. Just asked in the community, as it sounded like ready, just needs some code cleaning. So why not join forces with OP ?

splatch commented 1 year ago

@hmerk I could argue on some of statements made there, ie.:

The openHAB community has been visited 93 days and 257 topics and 1 800 posts have been viewed with a total read time of 22 hours. The activity in this community demonstrates the difficulty of incorporating the RBAC model into openHAB and the complexity of the platform.

;-)

hmerk commented 1 year ago

@splatch Sorry, i might have been a bit unclear. I have asked the OP if he could provide a PR for this, to have it implement in oh4. https://community.openhab.org/t/rbac-model-in-openhab-and-potential-security-vulnerability-found/136419/12 He answered that the code needs some cleanup and he is going to provide a PR in de next weeks. https://community.openhab.org/t/rbac-model-in-openhab-and-potential-security-vulnerability-found/136419/13

So under the bottom line, I only wanted to prevent anyone from double work...

splatch commented 1 year ago

Going over code made by @gennartn I found several troubles. There are multiple commits, including merges, which make moving it into current code base troublesome as we would need to make a "back merge" changes which are started on 3.2.x state of auth packages and UI. Both core and ui moved forward since then. I attempted to rebase/squash commits but its not going straight as there are conflicting changes between commits. Latest version of changes (main branch of Nicolas fork) contains unresolved conflict markers which means it can't be compiled and tested.

Aside of above there are some more troubles I see:

  1. There is implicit assumption of JWT as a primary authentication mechanism which means that AudioServlet, ChartServlet and IconServlet are not secured. They do not work with JWT. I think some browsers have troubles with sending headers to SSE endpoints (we need to confirm that with @ghys or webui maintainers).
  2. Provided code duplicates JWT and basic credentials validation logic between two places, AuthFilter and newly introduced VerifyToken interface. This is not only duplicated code but also duplicate functionality and duplicate execution. The AuthFilter is fired by underlying frameworks for all resources, including ItemResource where VerifyToken is being used, making its execution redundant just to obtain principal which is indirectly available in jaxrs.SecurityContext or openhab.Authentication interfaces. The VerifyToken is till some degree duplicate of AuthenticationManager limited to just one credential and input kind (http headers).
  3. The additional securing of command line is nice addon, but administrator verification there again reaches UserRegistry which backs JWT checks but does not follow principles introduced in eclipse/smarthome#579 (through calling AuthenticationManager with UsernamePasswordCredentials), question if it should? Generally speaking statement in thesis says that its better to have that than given the default password set to access shell.
  4. While related work points to extension of role handling mechanism it does not conclude how roles are mapped to items and probably other elements (things?, sitemaps?)
  5. The SSE endpoint is still not secured, effectively allowing any user who has access to system to listen for changes in all items, even these which he has no access through implemented REST checks.

Given the community inquiries about support for external oauth authentication, support for LDAP going forward with proposed way will not solve any of troubles we ended with after Eclipse SmartHome migration and will add more places to work with with eventual introduction of external authentication support for openHAB. Even if we do not wish to follow the whishlist then there is an issue of stronger authentication which is based ie. on client certificate - this will not be possible as well, because whole code right now does rely on user/password auth and does not leave any space for extension.

My proposal of workflow is following:

  1. Make UserRegistry implement AuthenticationProvider which supports UsernamePasswordCredential.
  2. Introduce JWTCredential for cases which rely on Authorization header with Bearer scheme.
  3. Extract JWT validation from AuthFilter logic into LocalJwtAuthenticationProvider which will rely on openHAB own RSA key.
  4. Introduce JWTAuthenticationProvider which would work with standard openid connect providers (google, okta, keycloak etc.), this will probably require an extra html form with related servlet to redirect user to external provider in order to obtain authentication and small UI adjustment to accept external token.
  5. Extract authorization logic into AuthorizationManager which would become a common place for access checks, so access control logic is not distributed across REST, rules (hopefully covered in future) and other layers. Introduce consistent set of Principal types which should be used by AuthenticationProvider implementations to map Group, Role and Permission entries.

Above steps will permit to switch authentication mechanism based on installed features and runtime configuration (possible to control using service configuration in OH webui) and also introduce a authorization logic which could be unified independently of user authentication source.

hmerk commented 1 year ago

@splatch As you offered to help with this issue, did you make any progress ?

splatch commented 1 year ago

I have not touched openHAB side due to awaiting of publication of trademark policies which happened just few days ago.