-
-
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
Allow basic authentication to authorize API access #1713
Conversation
Closes openhab#1699. Note, this opens a minor security issue that allows an attacker to brute force passwords by making calls to the API - contrary to the authorization page, the credentials parsing for the REST API is stateless & doesn't have a lock mechanism to lock user accounts after too many failed login attempts. Signed-off-by: Yannick Schaus <github@schaus.net>
I like how you do not go into lengthy discussions, but rather produce code ;-) |
Sure, this one was more to prove a point than anything else, really... :) |
@@ -63,6 +71,25 @@ public void filter(ContainerRequestContext requestContext) throws IOException { | |||
Authentication auth = jwtHelper.verifyAndParseJwtAccessToken(authParts[1]); | |||
requestContext.setSecurityContext(new JwtSecurityContext(auth)); | |||
return; | |||
} else if ("Basic".equals(authParts[0])) { |
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.
A small suggestion here - it might be worth to use equalsIgnoreCase
as some tools can send auth type lowercase.
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.
@ghys This suggestion does not yet seem to be addressed.
I was thinking, maybe this could be made optional with a configurable service "REST API Authentication > Allow Basic Authentication", and disabled by default. Most people wouldn't need it, unless they want to perform admin operations from external tools without doing the OAuth2 dance. Another option could be "Protect User Operations", also disabled by default. Currently these operations aren't protected at all: getting items, sending commands, the reason being that would probably break most end-user UIs. But if we enable it and make it advanced, the UI developers can enable it to help developing support. I have to check, but I believe the main UI can work with this option enabled, so you can't lock yourself out. |
How about a "host" whitelist? I.e. I suspect most people who would need to
run scripts against the REST api, would be from localhost, or other
"things" in the local area network that could be allowed to bypass the
authentication.
/Chris :-)
man. 12. okt. 2020 kl. 12:01 skrev Yannick Schaus <notifications@github.com
…:
I was thinking, maybe this could be made optional with a configurable
service "REST API Authentication > Allow Basic Authentication", and
disabled by default. Most people wouldn't need it, unless they want to
perform admin operations from external tools without doing the OAuth2 dance.
Another option could be "Protect User Operations", also disabled by
default. Currently these operations aren't protected at all: getting items,
sending commands, the reason being that would probably break most end-user
UIs. But if we enable it and make it advanced, the UI developers can enable
it to help developing support. I have to check, but I believe the main UI
can work with this option enabled, so you can't lock yourself out.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#1713 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAWRT23X2ZT2G6MQXUX25NDSKLHXPANCNFSM4SLVFZWA>
.
|
I'd rather see the rules (e.g. admin, user) implemented consistently so they work independent of auth type (Oauth, BasicAuth). |
I think an "Authentication" (or "Security"?) System Services entry will be helpful in any case.
Yes, it would be at least a first step towards a fully secured setup. 👍 Adding whitelisting etc. can then also be future options for this new configurable service. Would you want to add such a configurable service (with a switch for basic auth) as part of this PR? |
I'd still like Basic Auth at be least be enabled by default, if not made the default method for authentication. I know it's annoying that I stress this so much because it's already implemented but I firmly believe OAuth will only create problems for the users of openhab whilst providing hardly any benefits. |
This pull request has been mentioned on openHAB Community. There might be relevant details there: https://community.openhab.org/t/oh3-with-nginx-reverse-proxy-and-authentication/106528/3 |
Absolutely. |
Introduce a new AnonymousUserSecurityContext for unauthenticated users that should be allowed the "user" role according to service configuration. Signed-off-by: Yannick Schaus <github@schaus.net>
@kaikreuzer I added the options. I also contemplated adding the support for API tokens in this PR (they would prevent external tools from having to perform the OAuth2 flow to get a token, while not requiring to enable Basic auth) and Karaf commands as mentioned here: openhab/openhab-webui#332 (comment) Btw, not particularly happy to mention this, but this would bring us on par with what another popular home automation platform has implemented and somehow didn't get nearly as much backlash as we see here... |
We can go into a lengthy discussion what Home Assistant does better (or worse) than Openhab but this will not bring any new factual arguments to the table. Imho the goal should not be to implement Home Assistant in Java but rather to develop well thought and consistent concepts and to build a better smart home system. And while one could say it doesn't matter if you generate a basic auth header or a header with a token it really does. |
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.
Awesome, thanks! Just some minor comments.
@@ -63,6 +71,25 @@ public void filter(ContainerRequestContext requestContext) throws IOException { | |||
Authentication auth = jwtHelper.verifyAndParseJwtAccessToken(authParts[1]); | |||
requestContext.setSecurityContext(new JwtSecurityContext(auth)); | |||
return; | |||
} else if ("Basic".equals(authParts[0])) { |
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.
@ghys This suggestion does not yet seem to be addressed.
bundles/org.openhab.core.io.rest.auth/src/main/resources/OH-INF/config/config.xml
Outdated
Show resolved
Hide resolved
|
||
<config-description uri="system:restauth"> | ||
<parameter name="allowBasicAuth" type="boolean" required="false"> | ||
<label>Allow Basic Authentication</label> |
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.
Maybe add default=false here, just to make it clear what is used when not being set?
<description>Allow the use of Basic authentication to access protected API resources.</description> | ||
</parameter> | ||
<parameter name="noUnauthenticatedUserRole" type="boolean" required="false"> | ||
<advanced>true</advanced> |
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.
Maybe add default=true here, just to make it clear what is used when not being set?
Object value = properties.get(CONFIG_ALLOW_BASIC_AUTH); | ||
allowBasicAuth = value != null && "true".equals(value.toString()); | ||
value = properties.get(CONFIG_NO_UNAUTHENTICATED_USER_ROLE); | ||
noUnauthenticatedUserRole = value != null && "true".equals(value.toString()); |
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.
Argh, I am really getting a knot in my brain here (German proverb) by this double negation. How about changing this parameter to something like "enableAuthForUserResources"?
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 was more to the tune of "prevent unauthenticated requests from getting an implicit user role in the security context", so I tried to rename it to "noImplicitUserRole" / "No implicit user role for unauthenticated requests", that's perhaps clearer. Still a double negation, so if you still disagree I'll change it to "enableAuthForUserResources" :)
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.
And if we change it to "ImplicitUserRole" and have default=true, knowing that on the long run our wish is to get the setting to OFF? It would be in line with the basic auth setting, i.e. enabling something means your system becomes less secure.
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.
Good point, I insisted on having false as the default but that's not really a requirement.
Fully agree, that should be a separate PR.
I think @ghys main point here was that there are also very clever guys at HA, who have put a lot of thought into shaping their Auth API and it isn't by accident that they have chosen something different than Basic Auth.
That's deprecated and should not be used. See https://www.ietf.org/rfc/rfc3986.txt sections 3.2.1 and 7.5.
IP cams are the best example how to NOT do any secure software. They are hacked together in no time and I have hardly seen more buggy IoT devices out there. They are the backbone of Mirai and other botnets for a reason...
This opinion is pretty much against any security best practice of the last decade. Again, Basic Auth has some major flaws that a token based authentication prevents:
But never mind - this PR introduces Basic Auth for the ones that really want it despite its shortcomings, so everyone should be served. |
I was not aware that it should not be used, yet it still is widely supported.
I would have really like a constructive discussion like this in the original issue, but I'll comment anyway.
I still don't see any difference to Basic Auth! OAuth has become the standard because it allows to delegate access and allows separation of access granting and api infrastructure - a separation we don't have in the case of openhab. |
Signed-off-by: Yannick Schaus <github@schaus.net>
…F/config/config.xml Signed-off-by: Yannick Schaus <github@schaus.net> Co-authored-by: Kai Kreuzer <kai@openhab.org>
Which is something that one day we might want to do, for instance there's a Synology SSO server (https://global.download.synology.com/download/Document/Software/DeveloperGuide/Package/SSOServer/All/enu/Synology_SSO_API_Guide.pdf) which is also OAuth2; openHAB runs on Synology so it's not that far fetched that we might offer delegating to it for the identity & credentials management.
Agreed, my remark was not merely whataboutism or emulation at all costs (though there's no shame in borrowing good ideas), but simply pointing out the fact that they've implemented it like this for 2 years, so we might profit from this hindsight and AFAIK there were no major complaints. |
That would imho make the configuration pretty complex - I do not see much value in this.
Service accounts should use individual tokens (api keys), which are created for them and which can anytime be revoked without requiring the user to change his password. |
Signed-off-by: Yannick Schaus <github@schaus.net>
Signed-off-by: Yannick Schaus <github@schaus.net>
bundles/org.openhab.core.io.rest.auth/src/main/resources/OH-INF/config/config.xml
Outdated
Show resolved
Hide resolved
…F/config/config.xml
This pull request has been mentioned on openHAB Community. There might be relevant details there: https://community.openhab.org/t/openhab-3-0-milestone-1-discussion/106330/105 |
Why should Basic AUTHENTICATION ( identifying WHO you are) As part of my daily job I administer RADIUS AAA servers and utilize REST API clients. |
@ghys needs to claim a bounty for this. |
What I was trying to say is, formerly the only place where you could submit your password is in the form on the /auth page, where there are mechanisms to lock out for an increasing number of seconds after a unsuccessful attempt, so trying to guess the password with e.g. a dictionary attack becomes very unpractical very fast. |
From what I understood, the OpenHAB UIs only access OH through the REST API. The REST Tokens are used for authorization, not authentication. Some systems use a complex shared secret for authentication between trusted systems but still require authorization tokens for each use of the API. Others use username / password ( the lockout should be a server function anyway, independent of the client) and then require an authorization token. You are the architect here, though. |
Yes, the tokens are generated by already authenticated users (which might have used a much more secure way of doing so, like multi-factor) to authorize the bearer of this token to access the API on their behalf, eventually for a very limited scope. |
Just my 2c, I agree with @ghys and @spacemanspiff2007 on the pros/cons of tokens and basic auth, but I think tokens fit better for service access. All over the web you see mounting use of SSO+2FA to authenticate users, but as Yannick says, services get a token to access the API on behalf of the user. Both are transmitted in the clear (unless you are running https, which I am via nginx but still have local access on http) but with basic auth you have a username and password, both of which users (myself included tisk tisk) are very likely to reuse. Add together that and the lack of throttling on auth attempts via basic auth, inside your average home network. Seems like a not ideal setup given the scope of access some installations have to hardware in people's homes. I'm no security expert, but I can't see the firewall on a $50 router being rock solid, never mind the cheap modem/router you get from most ISPs. I'd rather provide a token that is in the URL or a header than user/pass for basic auth. For ease of use, tokens could default to full admin access but be restricted from there if desired. I'm not suggesting the removal of what this PR adds, I just agree that it doesn't seem like the best way to provide access+security. |
Caused by openhab#1713 Signed-off-by: Wouter Born <github@maindrain.net>
Caused by #1713 Signed-off-by: Wouter Born <github@maindrain.net>
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>
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>
(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>
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>
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 #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>
* Allow basic authentication to authorize API access Closes openhab#1699. Note, this opens a minor security issue that allows an attacker to brute force passwords by making calls to the API - contrary to the authorization page, the credentials parsing for the REST API is stateless & doesn't have a lock mechanism to lock user accounts after too many failed login attempts. Signed-off-by: Yannick Schaus <github@schaus.net> GitOrigin-RevId: e26c49b
Caused by openhab#1713 Signed-off-by: Wouter Born <github@maindrain.net> GitOrigin-RevId: b2c045d
(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
Closes #1699.
Note, this opens a minor security issue that allows an attacker
to brute force passwords by making calls to the API - contrary to
the authorization page, the credentials parsing for the REST API
is stateless & doesn't have a lock mechanism to lock user accounts
after too many failed login attempts.
Signed-off-by: Yannick Schaus github@schaus.net