-
-
Notifications
You must be signed in to change notification settings - Fork 415
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
[REST Auth] API tokens & openhab:users console command #1735
Conversation
Examples:
Note that the tokens are only printed on the console after they've been created, and they should be copy-pasted immediately, because they won't be retrievable afterwards. |
(I included these fixes in openhab#1735 but extracted them in a stanalone PR because it's easier to review and a little more urgent.) As a result of the refactoring in openhab#1713, the operations annotated with `@RolesAllowed` containing `Role.USER` are not anymore automatically considered accessible to all users, regardless of their actual roles. 4 operations are therefore now denied to admins if they only have the `Role.ADMIN` role, as the first admininistrator is created only with that role the UI encounters unexpected access denied errors and breaks. (See openhab/openhab-webui#422). Closes openhab/openhab-webui#422. Signed-off-by: Yannick Schaus <github@schaus.net>
(I included these fixes in #1735 but extracted them in a stanalone PR because it's easier to review and a little more urgent.) As a result of the refactoring in #1713, the operations annotated with `@RolesAllowed` containing `Role.USER` are not anymore automatically considered accessible to all users, regardless of their actual roles. 4 operations are therefore now denied to admins if they only have the `Role.ADMIN` role, as the first admininistrator is created only with that role the UI encounters unexpected access denied errors and breaks. (See openhab/openhab-webui#422). Closes openhab/openhab-webui#422. Signed-off-by: Yannick Schaus <github@schaus.net>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's nice to have some commands for this. 👍
I quickly scrolled through the code and saw a few minor improvements.
...enhab.core.io.rest.auth/src/main/java/org/openhab/core/io/rest/auth/internal/AuthFilter.java
Outdated
Show resolved
Hide resolved
...enhab.core.io.rest.auth/src/main/java/org/openhab/core/io/rest/auth/internal/AuthFilter.java
Outdated
Show resolved
Hide resolved
This adds API tokens as a new credential type. Their format is: `oh.<name>.<random chars>` The "oh." prefix is used to tell them apart from a JWT access token, because they're both used as a Bearer authorization scheme, but there is no semantic value attached to any of the other parts. They are stored hashed in the user's profile, and can be listed, added or removed managed with the new `openhab:users` console command. Currently the scopes are still not checked, but ultimately they could be, for instance a scope of e.g. `user admin.items` would mean that the API token can be used to perform user operations like retrieving info or sending a command, _and_ managing the items, but nothing else - even if the user has more permissions because of their role (which will of course still be checked). Tokens are normally passed in the Authorization header with the Bearer scheme, or the X-OPENHAB-TOKEN header, like access tokens. As a special exception, API tokens can also be used with the Basic authorization scheme, **even if the allowBasicAuth** option is not enabled in the "API Security" service, because there's no additional security risk in allowing that. In that case, the token should be passed as the username and the password MUST be empty. In short, this means that all these curl commands will work: - `curl -H 'Authorization: Bearer <token>' http://localhost:8080/rest/inbox` - `curl -H 'X-OPENHAB-TOKEN: <token>' http://localhost:8080/rest/inbox` - `curl -u '<token>[:]' http://localhost:8080/rest/inbox` - `curl http://<token>@localhost:8080/rest/inbox` 2 REST API operations were adding to the AuthResource, to allow authenticated users to list their tokens or remove (revoke) one. Self-service for creating a token or changing the password is more sensitive so these should be handled with a servlet and pages devoid of any JavaScript instead of REST API calls, therefore for now they'll have to be done with the console. This also fixes regressions introduced with openhab#1713 - the operations annotated with @RolesAllowed({ Role.USER }) only were not authorized for administrators anymore. Signed-off-by: Yannick Schaus <github@schaus.net>
Reusing the password salt is bad practice, and changing the password changes the salt as well which makes all tokens invalid. Put the salt in the same field as the hash (concatenated with a separator) to avoid modifying the JSON DB schema. Signed-off-by: Yannick Schaus <github@schaus.net>
The X-OPENHAB-TOKEN header now has priority over the Authorization header to credentials, if both are set. Signed-off-by: Yannick Schaus <github@schaus.net>
FTR, I'm not too happy with passwords being provided in clear on the command line (they are stored in the command history). |
Signed-off-by: Yannick Schaus <github@schaus.net>
Signed-off-by: Yannick Schaus <github@schaus.net>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for your awesome work!
I've added a few more (minor) comments below.
...rc/main/java/org/openhab/core/io/console/internal/extension/UserConsoleCommandExtension.java
Outdated
Show resolved
Hide resolved
...rc/main/java/org/openhab/core/io/console/internal/extension/UserConsoleCommandExtension.java
Outdated
Show resolved
Hide resolved
...rc/main/java/org/openhab/core/io/console/internal/extension/UserConsoleCommandExtension.java
Outdated
Show resolved
Hide resolved
....http.auth/src/main/java/org/openhab/core/io/http/auth/internal/AbstractAuthPageServlet.java
Outdated
Show resolved
Hide resolved
....http.auth/src/main/java/org/openhab/core/io/http/auth/internal/AbstractAuthPageServlet.java
Outdated
Show resolved
Hide resolved
bundles/org.openhab.core/src/main/java/org/openhab/core/internal/auth/UserRegistryImpl.java
Outdated
Show resolved
Hide resolved
bundles/org.openhab.core/src/main/java/org/openhab/core/internal/auth/UserRegistryImpl.java
Outdated
Show resolved
Hide resolved
bundles/org.openhab.core/src/main/java/org/openhab/core/internal/auth/UserRegistryImpl.java
Outdated
Show resolved
Hide resolved
bundles/org.openhab.core/src/main/java/org/openhab/core/internal/auth/UserRegistryImpl.java
Outdated
Show resolved
Hide resolved
bundles/org.openhab.core/src/main/java/org/openhab/core/internal/auth/UserRegistryImpl.java
Outdated
Show resolved
Hide resolved
It would be nice if another maintainer can also have a look since this deals with security. |
Signed-off-by: Yannick Schaus <github@schaus.net>
Signed-off-by: Yannick Schaus <github@schaus.net>
Signed-off-by: Yannick Schaus <github@schaus.net>
Just for helping any user, would it be possible to mention in the help of the new commands what values can be used for the In the case of my Remote openHAB binding that uses the openHAB REST API, in case the remote OH3 server was setup to require authorization for normal REST API, I understand I will have to create a token using your new console command and then use this token in the binding by injecting it in the header of each REST API call. Is it correct ? |
The scope model is not clear yet, but I'll suggest something like this:
yes currently, the "user" operations are unsecured but in case they are (you can switch that on now), you'll have to provide a token to access even the most basic operations that expose data, if you don't need admin access to make changes to the configuration then a scope of |
Ok thank you, I am waiting for the merge of this PR to test with my Remote openHAB binding. |
...ttp.auth/src/main/java/org/openhab/core/io/http/auth/internal/ChangePasswordPageServlet.java
Outdated
Show resolved
Hide resolved
....io.http.auth/src/main/java/org/openhab/core/io/http/auth/internal/AuthorizePageServlet.java
Outdated
Show resolved
Hide resolved
....io.http.auth/src/main/java/org/openhab/core/io/http/auth/internal/AuthorizePageServlet.java
Outdated
Show resolved
Hide resolved
...penhab.core.io.rest.auth/src/main/java/org/openhab/core/io/rest/auth/internal/JwtHelper.java
Outdated
Show resolved
Hide resolved
bundles/org.openhab.core/src/main/java/org/openhab/core/auth/ManagedUser.java
Outdated
Show resolved
Hide resolved
bundles/org.openhab.core/src/main/java/org/openhab/core/auth/UserRegistry.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Yannick Schaus <github@schaus.net>
Many thanks for addressing all my comments @ghys. I just did some testing on a local build and it seems that duplicate sessions end up in "johndoe": {
"class": "org.openhab.core.auth.ManagedUser",
"value": {
"name": "johndoe",
"passwordHash": "SZXFDGrobzJMLiFNQFvQJyhkpQePuIfPDrFM2wncIxCnNszAPYBnpd/vU0GqTDHgMAB9erA3wfTXttaXyx4+HQ\u003d\u003d",
"passwordSalt": "/L85mMFRatxrpLgsM+9JlawfhSPbez2GGTe7y1emvfzlVZOsJlnP9RVxtDA7w+X8YX8wJbMz2EJHTfaCm+WM9w\u003d\u003d",
"roles": [
"administrator"
],
"sessions": [
{
"sessionId": "ef5822a1-91bb-47f5-82e1-c6390b9e9fb6",
"refreshToken": "da8fe178ab344076bf80438fd5f5f14f",
"createdTime": "Oct 23, 2020, 11:52:06 AM",
"lastRefreshTime": "Oct 23, 2020, 11:52:06 AM",
"clientId": "http://localhost:8080",
"redirectUri": "http://localhost:8080",
"scope": "admin",
"sessionCookie": true
},
{
"sessionId": "ef5822a1-91bb-47f5-82e1-c6390b9e9fb6",
"refreshToken": "da8fe178ab344076bf80438fd5f5f14f",
"createdTime": "Oct 23, 2020, 11:52:06 AM",
"lastRefreshTime": "Oct 23, 2020, 11:52:06 AM",
"clientId": "http://localhost:8080",
"redirectUri": "http://localhost:8080",
"scope": "admin",
"sessionCookie": true
},
{
"sessionId": "ddb26127-3942-43e8-a519-ec5b46cee9dc",
"refreshToken": "0298b62ce5f444f296684627b880ed21",
"createdTime": "Oct 23, 2020, 11:52:17 AM",
"lastRefreshTime": "Oct 23, 2020, 11:53:18 AM",
"clientId": "http://localhost:8080",
"redirectUri": "http://localhost:8080",
"scope": "admin",
"sessionCookie": true
}
],
"apiTokens": []
}
},
But using multiple users does work nice as do the commands I've tested 👍 |
Strange, I've never noticed these session duplications before, maybe it's due to a change in this PR...? |
It is reproducible when you sign out of your session using the UI. |
bundles/org.openhab.core/src/main/java/org/openhab/core/internal/auth/UserRegistryImpl.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Yannick Schaus <github@schaus.net>
Shouldn't there also be a DELETE operation for a specific session in |
Signed-off-by: Yannick Schaus <github@schaus.net>
Are you sure? I initially thought of it as the counterpart to |
I didn't notice there was an While looking at the code in |
I see that it can properly delete other sessions but it looks like the frontend has some logic to also remove its own session data in the browser regardless of the session it deletes. |
This pull request has been mentioned on openHAB Community. There might be relevant details there: https://community.openhab.org/t/openhab3-forgot-password/107069/2 |
1 similar comment
This pull request has been mentioned on openHAB Community. There might be relevant details there: https://community.openhab.org/t/openhab3-forgot-password/107069/2 |
Signed-off-by: Yannick Schaus <github@schaus.net>
Done. Not sure what you mean by:
It shouldn't be "regardless of the session it deletes", normally it only does that if you click "Sign out of this session". |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It all seems to work well now. 👍 The deleting other sessions issue doesn't seem to be related to these changes so I'll create an issue for that.
@ghys: I just tried the new mode where the user REST API is fully secured. In this mode, I tried to launch Basic UI and I was surprised to see that it works, even if it is without SSE subscriptions. |
Basic UI renders its pages using servlets, so it's not unexpected that it "works" (displays something) because the auth layer only applies to the REST API. You won't be able to send commands or get updates since those parts are handled with API calls. |
@ghys How easy would it be to adapt our |
I will try again but I believe BasicUI was displaying the current items values. |
I think we can forego the need for a token for the servlets and simply rely on the session cookie - i.e. the user would need to open a session with the main UI first and then the cookie will identify the session which could be reused by Basic UI and others. |
(I included these fixes in openhab#1735 but extracted them in a stanalone PR because it's easier to review and a little more urgent.) As a result of the refactoring in openhab#1713, the operations annotated with `@RolesAllowed` containing `Role.USER` are not anymore automatically considered accessible to all users, regardless of their actual roles. 4 operations are therefore now denied to admins if they only have the `Role.ADMIN` role, as the first admininistrator is created only with that role the UI encounters unexpected access denied errors and breaks. (See openhab/openhab-webui#422). Closes openhab/openhab-webui#422. Signed-off-by: Yannick Schaus <github@schaus.net> GitOrigin-RevId: d262b6f
This adds API tokens as a new credential type. Their format is: `oh.<name>.<random chars>` The "oh." prefix is used to tell them apart from a JWT access token, because they're both used as a Bearer authorization scheme, but there is no semantic value attached to any of the other parts. They are stored hashed in the user's profile, and can be listed, added or removed managed with the new `openhab:users` console command. Currently the scopes are still not checked, but ultimately they could be, for instance a scope of e.g. `user admin.items` would mean that the API token can be used to perform user operations like retrieving info or sending a command, _and_ managing the items, but nothing else - even if the user has more permissions because of their role (which will of course still be checked). Tokens are normally passed in the Authorization header with the Bearer scheme, or the X-OPENHAB-TOKEN header, like access tokens. As a special exception, API tokens can also be used with the Basic authorization scheme, **even if the allowBasicAuth** option is not enabled in the "API Security" service, because there's no additional security risk in allowing that. In that case, the token should be passed as the username and the password MUST be empty. In short, this means that all these curl commands will work: - `curl -H 'Authorization: Bearer <token>' http://localhost:8080/rest/inbox` - `curl -H 'X-OPENHAB-TOKEN: <token>' http://localhost:8080/rest/inbox` - `curl -u '<token>[:]' http://localhost:8080/rest/inbox` - `curl http://<token>@localhost:8080/rest/inbox` 2 REST API operations were adding to the AuthResource, to allow authenticated users to list their tokens or remove (revoke) one. Self-service for creating a token or changing the password is more sensitive so these should be handled with a servlet and pages devoid of any JavaScript instead of REST API calls, therefore for now they'll have to be done with the console. This also fixes regressions introduced with openhab#1713 - the operations annotated with @RolesAllowed({ Role.USER }) only were not authorized for administrators anymore. * Generate a unique salt for each token Reusing the password salt is bad practice, and changing the password changes the salt as well which makes all tokens invalid. Put the salt in the same field as the hash (concatenated with a separator) to avoid modifying the JSON DB schema. * Fix API token authentication, make scope available to security context The X-OPENHAB-TOKEN header now has priority over the Authorization header to credentials, if both are set. * Add self-service pages to change password & create new API token Signed-off-by: Yannick Schaus <github@schaus.net> GitOrigin-RevId: 8b52cab
This adds API tokens as a new credential type. Their format is:
oh.<name>.<random chars>
They are stored hashed in the user's profile, and can be listed, added
or removed managed with the new
openhab:users
console command.Currently the scopes are still not checked, but ultimately they could
be, for instance a scope of e.g.
user admin.items
would mean that theAPI token can be used to perform user operations like retrieving info
or sending a command, and managing the items, but nothing else -
even if the user has more permissions because of their role (which
will of course still be checked).
Tokens are normally passed in the Authorization header with the Bearer
scheme, or the X-OPENHAB-TOKEN header, like access tokens.
As a special exception, API tokens can also be used with the Basic
authorization scheme, even if the allowBasicAuth option is not
enabled in the "API Security" service, because there's no additional
security risk in allowing that. In that case, the token should be
passed as the username and the password MUST be empty.
In short, this means that all these curl commands will work:
curl -H 'Authorization: Bearer <token>' http://localhost:8080/rest/inbox
curl -H 'X-OPENHAB-TOKEN: <token>' http://localhost:8080/rest/inbox
curl -u '<token>' http://localhost:8080/rest/inbox
(will prompt for a password, just hit Return);curl -u '<token>:' http://localhost:8080/rest/inbox
(will not prompt for a password)curl http://<token>@localhost:8080/rest/inbox
2 REST API operations were adding to the AuthResource, to allow
authenticated users to list their tokens or remove (revoke) one.
Self-service for creating a token or changing the password is more
sensitive so these are handled with a servlet and pages devoid
of any JavaScript instead of REST API calls.
Closes openhab/openhab-webui#332
Signed-off-by: Yannick Schaus github@schaus.net