Rest API: Redact configuration password parameters

[ThomDietrich]

Passwords provided through configuration should be redacted from the REST API responses. This is otherwise a huge security risk.

Example: The new iCloud binding: https://github.com/openhab/openhab2-addons/pull/2672
The critical iCloud account password is configured as a "password" context: See here.
Via REST API the password is then provided in clear text:

https://openhab-device:8080/rest/things/icloud%3Aaccount%3Amyaccount

{
   "statusInfo":{
      "status":"ONLINE",
      "statusDetail":"NONE"
   },
   "editable":false,
   "label":"iCloud Account",
   "configuration":{
      "password":"ClearTextPass",
      "googleAPIKey":"5",
      "refreshTimeInMinutes":5,
      "appleId":"abc@xyz.com"
   },
   "properties":{
   },
   "UID":"icloud:account:myaccount",
   "thingTypeUID":"icloud:account",
   "channels":[
      ...
   ]
}

[kaikreuzer]

What's your suggestion on how this should be best addressed?
Sending ****** instead? Leaving out the parameter completely? Limit it to TLS connections?
What about remote systems that actually might NEED the information?

[ThomDietrich]

Happy to discuss. My opinion:
Yes, *** or ***** or "(redacted)" would either be fine with me.
No, I'd say leaving out the information can be confusing. The user should be informed about the existence of the parameter.

No, TLS or not doesn't make a difference. It's not about access to the information for authorized vs. non-authorized channels. My expectation is that a password (or any other kind of sensible data) provided to the system for internal operation should never be exposed. As an end user who enters his Apple ID password through a redacting password field in Paper UI I'd be shocked to see my password exposed in clear text, even if I've taken precautions to, e.g. limit access.

External systems: Can you come up with a use case? No external system should be able to access my Apple ID password, correct? If data is supposed to be accessible by other systems it should be made clear to the end user by e.g. not wrongly marking it as sensitive data, i.e. password context, correct?

If there are indeed use cases where an add-on needs to provide sensitive data through the REST API (vs. using sensitive data to interact with external services), an additional parameter attribute might be in order?

[rodneyrehm]

Yes, *** or ***** or "(redacted)" would either be fine with me.

Any UI that renders input fields by prefilling values from the API will have a problem. The user will only see the password-dots and not realize that the actual value of that field is not the previously entered value, but ***, which will then lead to authentication failure upon saving the value. Also whatever validations exist in the UI might show unexpected messages.

I'd go with the empty-string (because it's typesafe) or null (because the property is listed, but with unknown value) if the property has to be listed in the response.

No, I'd say leaving out the information can be confusing. The user should be informed about the existence of the parameter.

On the contrary! I'd vote for completely dropping the password property. In our (Qivicon) setup any REST response that contains the sensititive strings "password", "credentials", will prevent the entire response from showing up in our error tracking. Pretty annoying if that were still to happen without any actual passwords being scrubbed…

What about remote systems that actually might NEED the information?

What's the use case? As a user I don't expect ESH to expose my configuration to other systems unless I've somehow explicitly granted access. In Qivicon we don't grant access to REST endpoints or events that expose sensitive data to anything but our own configuration UI.

---

Since we're not only talking about passwords but secrets in general, we may want to talk about introducing a new property "confidential" on the ConfigParameter that would allow opting into that parameter/parameter-value never showing up in REST responses (and events!)

[ThomDietrich]

Hey Rodney,
sounds good to me. The important aspect I'm pushing for is that passwords/secrets should not be exposed in clear text via the public http REST API. In the example of the iCloud Binding's password field, this is a real secrecy violation. How that happens (asterisks, null, whatever) depends on how other systems interact with the API and you make a good argument for not going with random asterisks. The empty string would be fine with me but how could it then be differentiated? null seems like the better choice.

I'd vote for completely dropping the password property

Not sure if we are talking about the same thing. In the example I've given above I would still prefer to see the fields "password" and "googleAPIKey" but without the sensitive value to the right. If you expect propagation issues out of that, redacting the field completely is of course an option. The null value would be a good middle path solution.

I didn't quite follow your "error tracking" example. You are also talking about Qivicon-internal communication, we might need to differ as I am talking about the publicly available (and hence critical) http interface.

introducing a new property "confidential" on the ConfigParameter

👍

[kaikreuzer]

The empty string would be fine with me.

What we are losing over "******" is some information about whether it is at all set and its length. As a user, I actually like to see a redacted string rather than nothing at all in the input field.

introducing a new property "confidential" on the ConfigParameter

Doesn't that more or less duplicate what we already have with the "password" context? The idea of this context is that the field holds sensitive information that should not be displayed. What do we win by an additional "confidential" information? And how do we explain developers the difference?

[maggu2810]

I am still not sure if we should modify our REST interface so special information cannot be read out anymore.

What if I want to read that password using the REST interface, because I have a special automation stuff running that use the REST API?

Why should we start to replace the password context?

• you can have a switch item that allows to turn of the alarm protection of a house
• you can have an item that state represents some secure information
• you can have other configurations that should be protected

Isn't it more a topic that needs to be solved by e.g. a general access restriction to the data (things, properties, ...)?

• One step would be to use a secure channel for the REST interface itself (e.g. TLS).
• Another step would be a general authentication requirement for the REST interface

Both easily doable by e.g. NGINX.

Another part would be to implement a mechanism to get a link between an authenticated instance (e.g. user) and system information (e.g. the password context).

But completely stop reading of password context by the REST interface is IMHO not correct, but perhaps a use case for some products.

[ThomDietrich]

@kaikreuzer

rather than nothing at all in the input field.

Confused. The way how the REST API provides data and how a user interface presents that data is unrelated, no?

some information about whether it is at all set and its length

If it is required, isn't it set per design? Why would we inform about the length? The public REST API should not even convey that information because it's confidential data and hence of no business for the outside.

[ThomDietrich]

@maggu2810 it seems like we are talking about different use cases. Let me once again clarify the goal of this issue:

What if I want to read that password using the REST interface, because I have a special automation stuff running that use the REST API?

No. As the user of the iCloud Binding the system MUST NOT under any circumstances provide my iCloud password to any interface outside of it's local Binding scope. This would be a serious secrecy violation...

[kaikreuzer]

The way how the REST API provides data and how a user interface presents that data is unrelated, no?

Well, so far the UI shows exactly what the REST API returns, so this isn't unrelated, is it?

If it is required, isn't it set per design?

Who says that a field with context=password has to be required?

Why would we inform about the length?

Because this is a common way to do it. If I look at my Amazon account details, I see:

(and yes, this is the real length of my password, but I also use 2-factor auth ;-))

perhaps a use case for some products

@maggu2810 That was also my concern above, but I honestly cannot come up with any real-world use case for @ThomDietrich and @rodneyrehm...
In general, I do not see any issue with defining that context=password fields are only meant to be set through the REST API, but not read.

[maggu2810]

Let's think about a product that is using the Eclipse SmartHome framework to manage certain tasks.
The product limits the REST interface to the localhost (127.0.0.1).
So, the REST interface is used only internally on that machine as a special IPC.
Other programs on that product share the whole information using the Eclipse SmartHome, they write information to ESH using the REST API and they read information from ESH using the REST API.
Why shouldn't it possible to share a password for a service using a thing instance?

So, I am happy if we could limit information to only that ones that are allowed to access that information.
But why dropping passwords from the REST interface and keeping the password information still readable on the system.
If the system is accessible it is the same potential risk as if you leave the REST interface open.

To be clear: For me it is a valid use case to limit the readability of a password over the REST interface.
But IMHO we should not limit passwords over REST and keeping other information accessible over REST that needs to be protected, too.

[maggu2810]

Another question:
The context is set by the binding developer.

• Should the developer be responsible which information / configuration is readable?
• Should the user that fills in a configuration value be able to mark this value as "not readable anymore"?
• Is password the only use case (WRT to context confidential instead of password -- perhaps context should not be chosen as the readable flag)?

[rodneyrehm]

You are also talking about Qivicon-internal communication, we might need to differ as I am talking about the publicly available (and hence critical) http interface.

I'm not talking about solution internal communication. It's a matter of how a solution exposes ESH to the "public". @maggu2810 pretty much explained how one could deal with this topic by limiting endpoints exposing sensitive data to certain users & UIs.

What we are losing over "******" is some information about whether it is at all set and its length.

We're probably not going to get around touching our UIs anyway. If the API provides an empty value for a required field it would not validate. If the API supplies ****… it would simply send that value back to the API upon saving (given there is no additional validation that excludes the character *).

However, I'd prefer to avoid anything that smells like magic strings…

Doesn't that more or less duplicate what we already have with the "password" context?

It's similar, but not that similar. A parameter's context is intended to inform the UI of the semantics of that parameter.

As far as I understand ESH itself completely ignores the context (beyond passing it through from config to UI).

What do we win by an additional "confidential" information? And how do we explain developers the difference?

I don't think it's obvious to a developer that any secret value should be considered a "password" (I know I wouldn't), whereas the confidential flag is immediately understandable. As far as I remember parameter contexts were only defined for TEXT. With a new confidential flag you could cover everything. (no, I don't have a use-case at hand)

As the user of the iCloud Binding the system MUST NOT under any circumstances provide my iCloud password to any interface outside of it's local scope.

Why doesn't the binding encrypt sensitive information that "MUST NOT under any circumstance … [be provided] outside of [its] local scope"? No, not trolling, just wondering if hiding data from the REST API is enough for you, as any other bundle would still be able to read the data from Java APIs, no?

[ThomDietrich]

@kaikreuzer I was curious and checked: Amazon always shows 8 asterisks, even though my password is 21 characters in length 😄 I don't remember the exact resource but there was an article once (by Jeff Atwood I believe) talking about the importance of secrecy wrt user data. Your Amazon account settings do not allow to view your password, nor does it hint any properties of it, not even its length.

@maggu2810 I believe @rodneyrehm made an excellent proposal by introducing the "confidential" property (I did also propose that in my first response here). You expressed your believe that not every password information should automatically be redacted from the REST API - I agree. However there are properties which should indeed be "confidential", meaning the given data is user-confidential and may not be automatically provided to external systems and only be used by intended subsystems. While the first will be partly covered by the REST API change, the other part can only be fulfilled by a moral contract.

The context is set by the binding developer.

That would be my understanding. The developer has to decide about the nature of given data and label it accordingly.

@maggu2810 I believe most of your concerns are actually respected if we were to add a confidential property next to the existing password context. Password merely means that the data is sensitive and should e.g. not be shown in clear text on UI but can still be accessed by other systems (that are morally allowed to). A confidential field is however sensitive data that must not be accessible by other systems out of secrecy reasons and would hence e.g. be redacted from the REST API. As a developer of another component or solution you need to respect that because even with good intentions in mind, you are not to access my iCloud password (sorry for bringing up that example all the time. It's catchy. I don't even have an apple account 🙈 )

@rodneyrehm

Why doesn't the binding encrypt sensitive information

Not sure if that is feasible. It's a configuration property and will be used by the binding to interact with another system. "Never build your own crypto" comes to mind 😅 I'm not sure if we can expect every binding developer to develop some magical obfuscation around a property that will be secure and still usable... This will for sure create chaos and new (security) issues. Could still be an option on top for things like the iCloud password. I'm unsure if that would nullify the need for conditional redaction from the REST API.

[maggu2810]

IMHO:
If we consider that some configuration contains some "private" data, it is not about the REST API only. We need to change the internal handling of configuration data.
Currently every addon (binding, storage, ...) can access the respective registries. All data could be read and sent to some server. Let's assume you install a "bad" extension using the market place (or some other provider), it could upload all your system stuff to a backend server.
If we would like to protect configuration data, the discussion should not be about making the REST API more secure but the whole infrastructure.
Let's assume you are a "bad" storage extension. Should you be able to read the configuration in plan text? As you only need to store and restore data you don't need to know the plain text.
I assume the whole configuration of thing (handlers) needs only to be known by the binding itself.
Wouldn't it make sense to think about a method so only the binding (that provides the handler of the thing) knows the plain text about configuration?
So, the thing configuration (read / write) is delegated from the framework to the respective binding without knowing the values.
I know it is not as easy, as you can configure things without the binding being started....
But I don't like to talk about "harden the REST API" and all other interfaces / extension still can access the data.

[triller-telekom]

@maggu2810 has mentioned two points here:

One is the storing of secrets in "an encrypted way" which we are discussing in PR #4267 and I think this is really needed.

The other point IMHO touches the discussion of about handler vs device configuration from issue #3484.

Regarding the additional "confidential" tag, I would not make it more complicated than it already is. I agree with @kaikreuzer that we already have the "password" context and that we should treat this as sensitive data without introducing the additional "confidential" tag. The argument that developers are aware that certain data is sensitive by tagging it "confidential" is IMHO not a good one, because you can also argue that a developer knows that a password is sensitive data... Even worse, if we have both terms, a developer might get confused why a password should not be tagged as "confidential" in some cases.

[kaikreuzer]

Amazon always shows 8 asterisks

Ok, you've got me there 😇

my password is 21 characters in length

You're a mad lunatic 😝

So where do we stand with the overall discussion?

Imho we should leave encryption of parameters and an overall security&permission concept out of scope here - while we certainly all agree that those make sense, they are imho far too complex for the concern here.

Wrt a "confidential" property: I would also prefer to leave that out of the picture, unless we really have a concrete situation, where we need this specific differentiation. As @triller-telekom stated, the current semantic of the password context is exactly that: It is an information that needs to be treated confidential and as such the UI must not show it. If we consider the REST API as a kind of "machine UI", I would be fine with interpreting it that the REST API shouldn't expose it. Note that the password context is already used for other confidential information such as API keys, OAuth tokens, etc. We could add a word in the documentation that this is what this context is meant for.

In order to tell whether a value is set or not, the REST API could return nothing (not set) or an empty string (set) for the property. This would probably be the least invasive solution without using magic strings.

@maggu2810 Would such a solution be acceptible for you? I could also imagine that for the (rather rare) use case with some local system integration through REST, we make this feature configurable, so that it can be turned off and all password fields filled with this setting.

@ThomDietrich To be honest, I think the biggest issue actually is that we at all NEED the iCloud password. Apple should really rather provide a way to have application specific passwords that can be revoked at any time or to use OAuth2 flows and tokens... This would dramatically reduce the criticality of your issue 😎

[ThomDietrich]

You're a mad lunatic 😝

Haha I guess you could say that
(I use automatic unique LastPass passwords)

Regarding the iCloud password: I totally agree! I've asked the developer to reach out in Apple forums. Maybe there is a solution he's not yet aware of. Didn't hear back from him.

I believe we all agree that it doesn't stop with the iCloud Binding and a more general solution is needed.
Furthermore I feel we should handle the most eminent issue now and look into more complex aspects later. It would be a disaster if anyone were to spot and publicly expose openHAB 2.2 for it's talkative REST API. Your proposed solution would solve that issue and it would not be conflicting with further steps (I guess). Let's see what @maggu2810 has to add.

[maggu2810]

I still think filter the password on the REST interface only is a workaround and not a solution. And I don't like such security workarounds that does not solve the whole problem but only filter some information on one place but keep a secret readable on all other places / interfaces.
So, installing a binding from the market place can still access all password contexts (so, every secret information is marked by the context) etc.

I see the need for a solution but don't think filtering in the REST interface only is the correct solution.

This is my personal meaning about this topic. It does not need to be the correct one.

If we need a really fast workaround and the others agree that the framework should contain it then we should to it.

And hey @ThomDietrich WRT to your comment in the other issue, see e.g. #3082. The consensus has been that we wait for profiles (IIRC) and I patched my ESH build for half a year. Nothing to worry about, if the final solution is the better one.