Security API #4267

davidgraeff · 2017-09-14T07:00:47Z

A first API proposal for a security package. #4169 (POC: Encrypted credentials ) and #4105 (TLS Service) are targeted. The idea is to first agree on this API and to provide implementations for both parts in follow up PRs.

Secure Vault API

It is recommended to use char[] instead of String for secrets (https://stackoverflow.com/a/8889285)
A vault service, one per requesting bundle. See http://blog.vogella.com/2017/02/13/control-osgi-ds-component-instances/. Proposed API:

/**
 * The {@link SecretsVaultService} can be used to store and retrieve secrets.
 *
 * Secrets can only be retrieved by the bundle that has stored them and by default
 * the backend is required to store secrets encrypted only.
 *
 * Because of the way how the String class is handled internally, you are strongly
 * encouraged to use char or byte arrays or the ByteBuffer class
 * for keeping encrypted secrets in memory.
 *
 * The encryption algorithm, the storage location and format depend on the implementation.
 */
public interface SecretsVaultService {
   @Nullable ByteBuffer retrieve(String key) throws IOException, GeneralSecurityException;
   void set(String key, @Nullable ByteBuffer secret) throws IOException, GeneralSecurityException;
}

A management interface to retrieve all keys, for GUIs like PaperUI. Including a way to change the master password.

/**
 * The {@link SecretsVaultManagement} allows to retrieve all keys for a given
 * bundle or all bundles. The actual secrets cannot be retrieved via this
 * management interface and are only accessible by the {@link SecretsVaultService}
 * for the bundle that stored the secrets.
 */
public interface SecretsVaultService {
   Enumeration<SimpleEntry<String, String>> getAllKeys();
   Enumeration<String> getAllKeys(String bundlename);

    /**
     * Set/Delete a secret, identified by the given key, bundlename and old secret value.
     *
     * @param bundlename The fully qualified bundle name, e.g. org.eclipse.smarthome.binding.abc
     * @param key An identifier
     * @param oldSecret The old secret value. It need to match the currently stored value.
     * @param secret A secret as a ByteBuffer. If the ByteBuffer is empty, an empty secret will be stored.
     *            If null is given, the secret will be erased.
     * @return Return true if the modification was successful
     * @throws IOException If the underlying file can't be opened or is not writable.
     * @throws GeneralSecurityException If the encryption algorithm fails, this exception is thrown
     */
     boolean set(String bundlename, String key, ByteBuffer oldSecret, @Nullable ByteBuffer secret)
            throws IOException, GeneralSecurityException;
    /**
     * All bundle secrets are stored in a protected format. An implementation could be a java {@link KeyStore} for
     * example. A master password is used for encryption. This method allows to change the master password by
     * re-encrypting all bundle secrets with the new password.
     *
     * The implementation need to guarantee that the master password change happens in a transactional way and any
     * changes will be rolled back if an error happens.
     *
     * @param oldSecret The old secret value. It need to match the current master password
     * @param secret
     * @return Returns false if the new master password could not be applied.
     */
     boolean setMasterSecret(ByteBuffer oldSecret, ByteBuffer secret);
}

Annotation interface for fields in a configuration holder object. Secret annotated fields will automatically be retrieved from {@link SecretsVaultService} and are instantly available for consumption. Usage example:

class YourConfig {
   public String username;
   public @Secret("pwd") Bytebuffer password;
   public @Secret Bytebuffer bitcoinWalletKey;
};

Configuration holder objects can be used in services like this:

void activate(@Nullable Map<String, Object> configMap) {
   YourConfig config = new Configuration(configMap).as(YourConfig.class);
}

Network secure sockets

A central service interface to provide SSL/DTLS configuration for server-like services within ESH and derived products. Server like services are for example:

The MQTT embedded broker
The :8080 web-page (The REST endpoint, the UIs)

public interface SecureSocketServers {
    /**
     * To retrieve a working secure socket implementation , you would call this
     * method and get an asynchronous callback as soon as the context is ready. It can take a while (some seconds)
     * to resolve a new SSLContext if there is none so far.
     *
     * Describe what features your secure socket implementation
     * should fulfil (mandatory) and what features are nice to have (optional).
     *
     * For example if you need a DTLS, you would set `mandatory` to
     * {SecureSocketCap.DTLS}.
     */
    CompletableFuture<SSLContext> requestSecureSocket(Set<SecureSocketCap> mandatory,
       Set<SecureSocketCap> optional, @Nullable String contextTag);
}

davidgraeff · 2017-09-14T07:02:55Z

@triller-telekom Can you have a look at this API proposal?

triller-telekom

Thank you for your proposal, please see my comments inline.

@paulianttila Do you miss any functionality in these interfaces for your configuration service?

triller-telekom · 2017-09-14T08:50:21Z

...smarthome.io.security/src/main/java/org/eclipse/smarthome/io/security/api/Cryptographic.java

+     * remember that it will change, if the user changes the master password
+     * and old cipher will not be able to be decrypted anymore.
+     */
+    byte[] masterPasswordHash();


I think in addition to obtain the current set masterpassword hash we also need a possibility to change the masterpassword and if this is done we have to inform all parts of the code that uses it, i.e. we need another listener that can be registered to be notified once the password is changed. Because if the password is changed the implementation has to re-encrypt all secrets with the new password, i.e. a notification should include both passwords.

I wanted to avoid such a listener. Everybody can register for this listener and an evil binding just logs the new plain text password and has full access.

Instead I used a different pattern, see the security vault issue in the section Use-case.

True, we definitely do not want to have a listener where everyone can obtain the password from :) But then we would need a "migration service". If you change the password I would not want to restart the system and wait for a re-encryption of all secrets. This should happen on the fly. If you offer your proposed boolean migrateMasterPassword(String oldPassword, String newPassword) method it should re-encrypt all secrets immediately and lock the configurations until this migration has finished and then store newly added data encrypted with the new password.

Can we lock configadmin? A restart sounds more failsafe proof. It's a bad comparison, I know, but when I change the password of my router, I need to restart it. If I change a Windows login password, I need to logout and login again. I think people are used to the restart pattern if something central like the master password is changed. But open to suggestions.

Another thought: What if you supply a new password but do not restart immediately but leave the system running for days/weeks... Which password will be used for new secrets that will be entered?

Also where do you store the new password, that you provided via rest api, in memory? But you do not have that memory if you restart the system...

If a service has the access rights of an OSGi console, it can end and restart services. But that's not the point.

We either have observers for a password change with old and new password and we agreed that's a bad idea, or we restart the framework. There's no other option that I can see here.

My idea was a environment variable with the current password (/ new password) and if it is a change then there's a variable with the old password as well. We check the current password first and then the old password if existing to decrypt. If the old matches we re encrypt everything.

Reencrypt. Github mobile doesn't allow edits. Sigh.

Sorry I still don't get what you mean. :(

My idea was a environment variable with the current password (/ new password)

Is that now one or two variables?

and if it is a change

I am assuming "currentPW" and "newPW" are two variables.

then there's a variable with the old password as well

So you copy the content of "currentPW" to "oldPW" and the content of "newPW" to "currentPW"?

We check the current password first and then the old password

Why do you even introduce a third variable? Two are enough, aren't they? One for the old and one for the new password.

Two variables only.

It's current and old. Old is optional and should it be set for a change but it's not relevant if it still exists on later starts because we always first try current. And if that works we ignore old. But again: if there's another better approach I'm open to that. I only want the API here. And even the API can be extended, just not changed later on.

triller-telekom · 2017-09-14T08:51:59Z

...ome.io.security/src/main/java/org/eclipse/smarthome/io/security/api/SecretsVaultService.java

+import java.nio.ByteBuffer;
+import java.security.GeneralSecurityException;
+
+import org.eclipse.jdt.annotation.NonNull;


We just agreed yesterday on how to use null annotations, please follow the agreed rules

triller-telekom · 2017-09-14T08:55:03Z

...rthome.io.security/src/main/java/org/eclipse/smarthome/io/security/api/TlsNetworkServer.java

+
+import javax.net.ssl.SSLContext;
+
+import org.eclipse.californium.elements.Connector;


I am not sure if we want to allow a dependency to Californium in such a central bundle... @kaikreuzer WDYT?

@davidgraeff Is this method really necessary? I mean if you can obtain the Context you can create the Connector in your implementation, can't you?

Java9 hast native Support for dtls, but right now a SSLContext is not enough unfortunately. If you look at the TLS service pr, I create those two with different default algorithms. To my knowledge it is also not possible to actually create a dtlsConnector with just an sslcontext/sslengine. And the purpose is to make it easy for the API consumer to create a secure tcp or udp socket. It doesn't need to be californium but should be something.

I can thing of a custom return class where everything necessary is included to create a dtlsConnector. This way we are independent of californium.

Can you please change it to your proposed custom class?

triller-telekom · 2017-09-14T09:04:26Z

...smarthome.io.security/src/main/java/org/eclipse/smarthome/io/security/api/Cryptographic.java

+     * @return Returns the ciphered text.
+     * @throws GeneralSecurityException
+     */
+    byte[] encrypt(byte[] password, byte[] plain) throws GeneralSecurityException;


In the SecretsVaultService you are working on ByteBuffers and here you are using byte[] if the ByteBuffer is really more convenient maybe we should use it here too, or change both to use byte[] to have it consistent.

I'm using ByteBuffer if the argument can be null and byte arrays otherwise.

If there is a param defined as byte[] param one can also pass null, so I am confused about your explanation.

I can't use annotations on a byte array, at least it didn't work for me. I have no problems of using ByteBuffer all the time. It just wraps a byte array and has almost no additional runtime costs.

If it doesn't add "much" (I don't think "no") additional runtime cost we could use it. But I wouldn't make this decision dependent on whether you can add a null annotation or not. Otherwise you also have to replace all "int", "boolean", etc. parameters in the code as well because you cannot add the null annotations on primitives.

Actually I do this. For the Mqtt connection object for example. The port parameter is an Integer instead of an int so that the user can set the parameter explicitly to null if he doesn't care. For booleans it makes sense too sometimes if you need a tristate (on/off/disabled).

triller-telekom · 2017-09-14T09:09:16Z

...smarthome.io.security/src/main/java/org/eclipse/smarthome/io/security/api/Cryptographic.java

+     * @return Returns the ciphered text.
+     * @throws GeneralSecurityException
+     */
+    char[] encrypt(byte[] password, char[] plain) throws GeneralSecurityException;


I am thinking about whether it makes sense to support String plain instead of the char[] which would be more convenient to use. Also it makes sense to provide the password as a String. The same applies to the decrypt method.

I would agree convenient-wise but please read the linked article in the security vault issue. The problem is: strings are allocated on the VM heap, and their content cannot be overwritten or cleared (immutable). The passwords would remain in memory until the next Garbage collection.
We shouldn't risk that for a security API. The user can still use strings and call toCharArray if he doesn't care.

The password argument can be char or byte. The problem I see with chars is that they are 16bit wide. What if your password is {1,2,3,4,5} (read as bytes). How do you express this in chars? I think only bytes allow us the full range of possible password combinations.

Is password the best term? Would secret, secretKey, encyptionKey or passphrase be better? It depends on algorithm what kind of parameters is need for encryption (e.g. IV), so is "password" enough?

You mean that if you use char[] we would need to add a padding, because byte is 8bit and char is 16bit, so the bytes have to be a multiple of 2?

Yes, that's a perfect suggestion. So we would have: add(key,secret)/delete(key)/get(key)/modify(key,secret).
Add throws if it would overwrite or secret is null.
Modify throws if key is not existing.

And we have getAllKeys() but without the actual decrypted secrets for the rest interface.

Get()/modify() should only work for the bundle that stored the secret. So the rest interface etc will not be able to access the secrets at all.

Configadmin should not decrypt therefore, but the configuration class, that can be used in the correct bundle context then.

So the rest interface etc will not be able to access the secrets at all.

Well, there could be (maybe later) dedicated REST interface also for secret vault where user could manage the secrets. But yes, for security reason, existing credentials should not ever exposed as decrypted to the REST interface. Only add and modify could be possible including secret, and then remove and getAllKeys. That REST interface should only be supported via TLS connections and hopefully someday restricted by the user roles as well.

That sounds like a plan. So the Secret Vault service keeps all secrets to himself and handles the encryption/decryption of configuration values transparently for all other services.

The implementation of this Secret vault service should be configurable, i.e. algorithms and keys sizes should be changeable with secure default (as far as possible with the US import/export restrictions).

For the REST interface I think at the moment we do not have any other choice than what @paulianttila suggests since we do not have roles to restrict it. I still have an aching stomach with the modification through the REST interface because potentially everyone could change the secrets then. And changing a secret in this context is almost the same as reading the current one in plaintext since for example as an attacker if I replace the secret key of an ssl connection service, I am able to read the traffic. Any good ideas on restricting the modification even more?

The only idea I have now is to maybe restrict the modify command on the REST interface to only be called from external hosts, i.e. the user in his browser and not a malicious bundle executing the call. WDYT?

Also we need a way for users using text files to configure their ESH instance to somehow store a given plain text token in the secrets vault service and refer to that in the config file.

Modify API could look like this:
void modify(key, old_secret, new_secret)
So the rest interface would ask for the old secret as well. That way it cannot be changed that easily by malicious code on top of the rest interface.

Sounds good.

paulianttila · 2017-09-14T11:32:21Z

...smarthome.io.security/src/main/java/org/eclipse/smarthome/io/security/api/Cryptographic.java

+     * @return Returns the plain text
+     * @throws GeneralSecurityException
+     */
+    char[] decrypt(byte[] password, char[] cipher) throws GeneralSecurityException;


Cipher? Does cipher means here encrypted password? Cipher term is normally used as a term for used algorithm.

He probably means cipherText. But I agree, this name choice might be confusing.

encryptedData would be an option

encryptedDate implies that it is a date from a calendar, so I would rather use encrytedData

paulianttila · 2017-09-14T17:47:33Z

...ome.io.security/src/main/java/org/eclipse/smarthome/io/security/api/SecretsVaultService.java

+     * Set or delete a secret, identified by the given key.
+     *
+     * @param key An identifier
+     * @param secret A secret as a ByteBuffer. Can be null to delete the secret.


Addition: If the vault previously contained a value for the key, the old value is replaced.

paulianttila · 2017-09-14T17:49:40Z

...ome.io.security/src/main/java/org/eclipse/smarthome/io/security/api/SecretsVaultService.java

+    ByteBuffer getSecret(@NonNull String key) throws IOException, GeneralSecurityException;
+
+    /**
+     * Set or delete a secret, identified by the given key.


Should delete have own method rather than use null as special behaviour?

For me both is fine, but maybe an extra method would be cleaner since this method does not need to accept null.

Less is better sometimes :D if there's delete I expect add. Set/get is easy and simple

davidgraeff · 2017-09-19T16:37:17Z

@paulianttila @triller-telekom: I have adapted the API proposal

triller-telekom

Thank you for incorporating the changes from our discussion so far. Please see my comments inline.

triller-telekom · 2017-09-20T08:22:42Z

...ome.io.security/src/main/java/org/eclipse/smarthome/io/security/api/SecretsVaultService.java

+ * The {@link SecretsVaultService} can be used to store and retrieve secrets.
+ *
+ * Secrets can only be retrieved by the bundle that stored them and by default
+ * will be encrypted and and stored on disk immediately.


Typo "and and"

triller-telekom · 2017-09-20T08:26:28Z

...smarthome.io.security/src/main/java/org/eclipse/smarthome/io/security/api/Cryptographic.java

+ *
+ * @author David Graeff - Initial contribution
+ */
+public interface Cryptographic {


As @paulianttila suggested this interface can be removed, because the SecretsVaultService is doing the encryption internally, right?

Should we provide this anyway? We need something like this to implement the secret vault service and could export it for other use cases

You mean for the secret vault service you need some methods that actually do the encryption and decryption of the configuration values? I am not sure if we should define an interface for that because this strongly depends on the implementation how this is done. As we might also need to provide an IV if the underlying cryptographic function needs it. So my suggestion is that we leave this entirely to the implementation of the secret vault service. @paulianttila WDYT?

triller-telekom · 2017-09-20T08:34:51Z

...ome.io.security/src/main/java/org/eclipse/smarthome/io/security/api/SecretsVaultService.java

+     * @throws IOException If the underlying file can't be opened or is not writable.
+     * @throws GeneralSecurityException If the encryption algorithm fails, this exception is thrown
+     */
+    void add(String key, ByteBuffer secret) throws IOException, GeneralSecurityException;


Should this method not throw an exception if the given key already exists and not silently overwrite it? Because as far as I understand the current API a modification has to go through SecretsVaultManagement.modify()?

Yes. It should return a Boolean or throw.

triller-telekom · 2017-09-20T08:44:38Z

...ome.io.security/src/main/java/org/eclipse/smarthome/io/security/api/SecretsVaultService.java

+     * @throws GeneralSecurityException If the decryption algorithm fails, this exception is thrown
+     */
+    @Nullable
+    ByteBuffer retrieve(String key) throws IOException, GeneralSecurityException;


I still do not understand how this retrieving of a secret works. As a user of this new security API if I want to retrieve a secret for my bundle what do I have to do?

List mySecrets = SecretsVaultManagement.getAllKeys("myBundle")

for(String secret : mySecrets) { SecretsVaultService.retrieve(secret) }

If this is correct for retrieve, how does add work? Do I use SecretsVaultManagement.modify also to add new keys? If so we should rename it to something like addOrModify to make it more transparent.

As a consumer of the api, you know your keys. You probably would not iterate over all your stored secrets.

Add only adds and should either return with a Boolean or throw if the key is already existing.

If the consumer needs to modify, it would delete and add again.

The rest API can't add with the current proposal, I forgot about that. We probably rename modify to "set" and allow to add a secret as well with an empty old secret if there's nothing saved so far under the current key.

If the consumer needs to modify, it would delete and add again.

Why do we offer SecretsVaultManagement.modify (or set after the change below) then?

The rest API can't add with the current proposal, I forgot about that. We probably rename modify to "set" and allow to add a secret as well with an empty old secret if there's nothing saved so far under the current key.

So you suggest to rename SecretsVaultManagement.modify to SecretsVaultManagement.set and allow for adding new secrets and overwriting existing ones (with matching oldSecret)? That's what I meant, so fine with me.

To answer your question: The service interface is to be used within a bundle for that bundles secrets. The old secret is not necessary to modify a secret here.

The management interface can be used by any bundle but might be security restricted internally and the bundle name needs to be supplied as well as old secret to change a secret.

I understand your separation but how do we ensure that the implementation of the service interface is not called by someone else? I mean this implementation has a delete and add method that are public, right? But maybe this is a stupid question because we would have to expose these methods from the implementation somehow first before others can use it?

I thought about the OSGi service factory. If a bundle requests the secret vault service, an specific instance for that bundle is created. I haven't tested this with code yet.

Could you please give it a try and give some details on how that could work? If it doesn't I would clearly vote for an additional argument to the delete method which is the oldValue and only delete it if it matches.

triller-telekom · 2017-09-20T08:49:39Z

...ome.io.security/src/main/java/org/eclipse/smarthome/io/security/api/SecretsVaultService.java

+     *
+     * @param key An identifier
+     */
+    void delete(String key);


Since with a delete and add we could simulate a modify, should we also require the oldSecret as a parameter if one wants to delete its key?

See other reply. This interface is used within a bundle and accesses only that bundles secrets. The old secret does not need to be provided here. That was my plan at least, I don't know if this is realizable.

triller-telekom · 2017-09-20T08:50:34Z

...rthome.io.security/src/main/java/org/eclipse/smarthome/io/security/api/TlsNetworkServer.java

+
+import javax.net.ssl.SSLContext;
+
+import org.eclipse.californium.elements.Connector;


Can you please change it to your proposed custom class?

davidgraeff · 2017-09-25T04:56:31Z

After some research I came to the conclusion that it is actually not that easy to move away from the californium elements connector. The facts are:

SSLContext and DTLSConnector are working similar in hiding the actual secrets away from the API consumer
Both are build with a KeyStore that holds all private/public keys, certificates and PSKs.

To be able to create a DTLSConnector just with the API we provide here, we would need access to the KeyStore. We cannot expose KeyStore though:

It could be manipulated and even stored again.
We would need to expose secrets like the KeyStore password and private key passwords as well.

Just `SSLContext createSSLContext` can be enough

It is possible to create a DTLS connection in Java9 with just a SSLEngine. See http://hg.openjdk.java.net/jdk9/jdk9/jdk/file/6721ff11d592/test/javax/net/ssl/DTLS/DTLSOverDatagram.java for an example. I have adapted the API proposal to reflect this.

Because I still need californium for the LwM2M binding, it still needs to be possible to use californium with the API we provide here. I created an issue for californium: eclipse-californium/californium#421.

To sum up

So we either keep the californium elements connector for now (maybe by declaring the API as non-stable) or switch to Java9 soon for just the SSLContext createSSLContext(...) API to be usable for DTLS connections.

paulianttila · 2017-09-25T05:54:34Z

...ome.io.security/src/main/java/org/eclipse/smarthome/io/security/api/SecretsVaultService.java

+/**
+ * The {@link SecretsVaultService} can be used to store and retrieve secrets.
+ *
+ * Secrets can only be retrieved by the bundle that stored them and by default


Secrets can only be retrieved by the bundle that stored them

How this is handled? Should secret storing and modifying be possible e.g. from karaf console (not bundle which will use the secrets)?

This is possible via the management interface of this proposal, that is also used by the rest interface. Retrieving is not possible. Modifying requires to supply the old secret.

triller-telekom · 2017-09-25T09:18:39Z

bundles/io/org.eclipse.smarthome.io.security/META-INF/MANIFEST.MF

+Bundle-Vendor: Eclipse.org/SmartHome
+Bundle-RequiredExecutionEnvironment: JavaSE-1.8
+Export-Package: org.eclipse.smarthome.io.security.api
+Import-Package: org.eclipse.californium.elements,


I think this is a left over and should be removed too, right?

triller-telekom · 2017-09-25T09:20:13Z

...ome.io.security/src/main/java/org/eclipse/smarthome/io/security/api/SecretsVaultService.java

+     * @throws GeneralSecurityException If the decryption algorithm fails, this exception is thrown
+     */
+    @Nullable
+    ByteBuffer retrieve(String key) throws IOException, GeneralSecurityException;


Could you please give it a try and give some details on how that could work? If it doesn't I would clearly vote for an additional argument to the delete method which is the oldValue and only delete it if it matches.

triller-telekom · 2017-09-25T09:30:45Z

So we either keep the californium elements connector for now (maybe by declaring the API as non-stable)

I saw that you have removed the californium dependency, so did you decide for the option below already?

or switch to Java9 soon for just the SSLContext createSSLContext(...) API to be usable for DTLS connections.

This is clearly not an option, a switch in the major revision of java is a huge step for ESH and cannot be done within days/weeks.

To be able to create a DTLSConnector just with the API we provide here, we would need access to the KeyStore

Instead of providing our "holy keystore" to them, how about handing over a "dummy keystore" with just the keys that are necessary? Unfortunately in this case we need to offer a method to extract the public and private key from our "holy keystore" and put it into the dummy one which isn't nice either. WDYT?

davidgraeff · 2017-09-25T10:36:34Z

Exactly, we expose the private key. And the API would allow to enumerate all private keys. The entire purpose of this API would be defeated.

davidgraeff · 2017-09-25T10:37:53Z

I removed the part, because we should first agree on this part of the API. When I implement lwm2m we can discuss, what additional API (or temporary API) is necessary to support DTLS as well.

davidgraeff · 2017-09-25T10:41:49Z

Could you please give it a try and give some details on how that could work? If it doesn't I would clearly vote for an additional argument to the delete method which is the oldValue and only delete it if it matches.

I'm not sure if I understand. What prevents me from doing this: delete(key, retrieve(key))?

triller-telekom · 2017-09-25T11:35:28Z

I removed the part, because we should first agree on this part of the API. When I implement lwm2m we can discuss, what additional API (or temporary API) is necessary to support DTLS as well.

Great, let's discuss this outside of this PR.

I'm not sure if I understand. What prevents me from doing this: delete(key, retrieve(key))?

You are right, but this brings us back to your proposed OSGI service factory and if it is able to successfully hide this service from other bundles than the one which is allowed to use it. any news on that one?

davidgraeff · 2017-09-26T04:47:51Z

@triller-telekom According to this page (http://blog.vogella.com/2017/02/13/control-osgi-ds-component-instances/) it is possible for a factory to create an instance per bundle. I thought that @paulianttila will adapt his solution. If I would need to implement the API, I would just add it to the SSLService package and would not touch anything in core (I would not adapt the Configuration class like @paulianttila did).

triller-telekom · 2017-09-26T12:24:32Z

@triller-telekom According to this page (http://blog.vogella.com/2017/02/13/control-osgi-ds-component-instances/) it is possible for a factory to create an instance per bundle.

Sounds exactly like what we need!

I thought that @paulianttila will adapt his solution.

Can you give me some context what exactly you mean by that?

If I would need to implement the API, I would just add it to the SSLService package and would not touch anything in core (I would not adapt the Configuration class like @paulianttila did).

I think the security vault is a core service and I would appreciate if @paulianttila adopts his configuration PR #4169 so the current Configuration service can use the new security vault service.

maggu2810 · 2017-09-26T14:35:50Z

I assume as long as we have to support OSGi 4.2 we need to use features of DS 1.1 only.

sjsf · 2017-09-26T14:49:40Z

At least service factories should be available in DS 1.1 already (see OSGi R4.2 CMPN 112.4.6 "Service Element").

maggu2810 · 2017-09-26T16:01:57Z

See this read https://dev.eclipse.org/mhonarc/lists/smarthome-dev/msg00056.html

R4.2 and Java 7 were the requirements of the initial contribution. Any change to this will need a discussion, where every committer has veto rights.

@davidgraeff I am fine with Release 6 but I assume it depends on the currently existing products and I am pretty sure that a vote will be declined. 😉

davidgraeff · 2017-09-27T05:06:57Z

New versions usually come with improved developer comfort or in this case necessary functionality. It usually makes sense to not fall behind too much. In this case we would update, according to the semantic versioning, to a compatible minor version and stay within Release 4 (4.3 Released 2011, updated 2012).

ESH is an open-source showcase for OSGi. Java 9 tries to offer a competing module solution. From a marketing point of view, it would make sense to update to an even more recent version.

Why does ESH not has a yearly dependency version update vote or something similar? Everybody that is using ESH and interested in its development would subscribe to this poll and is able to comment on why a newer version might not fit his or other use-cases and point out release blockers. If people are only consuming and not contributing (participating in those votes), it is not ESHs fault, if those solutions do not work with newer ESH versions.

At the moment I don't see how ESH can stay up-to-date with libraries and dependencies, if everybody is afraid of touching what is already there with no clear update path.

davidgraeff · 2017-09-27T05:24:27Z

To sum my last post up:

I have no idea how a security vault API can be realized with OSGi 4.2 only.
I have no idea how a SSLService API with DTLS support should look like without either Java9 or a fixed dependency on californium.

triller-telekom · 2017-09-27T08:01:55Z

I have no idea how a security vault API can be realized with OSGi 4.2 only.

What about @SJKA's comment about service factories, those are available, do we need anything else?

At least service factories should be available in DS 1.1 already (see OSGi R4.2 CMPN 112.4.6 "Service Element").

davidgraeff · 2017-09-27T09:21:14Z

@triller-telekom If you know how to do this, sure. The only material that I found on the web for bundle instances uses DS 1.2 at least.

But someone need to explain to me why we use Java 8 (March 18, 2014) but not OSGi 4.3 (Released 2011). All OSGi implementations that potential ESH/Java 8 users use, will with a high chance be released after Java 8 and therefore at least support OSGi Release 5.

triller-telekom · 2017-09-27T09:35:56Z

In OSGi R4.2 CMPN 112.4.6 "Service Element" I read:

Service Element
The service element is optional. It describes the service information to be
used when a component configuration is to be registered as a service.
A service element has the following attribute:
• servicefactory – Controls whether the service uses the ServiceFactory
concept of the OSGi Framework. The default value is false. If
servicefactory is set to true, a different component configuration is
created, activated and its component instance returned as the service
object for each distinct bundle that requests the service. Each of these
component configurations has the same component properties. Otherwise,
the same component instance from the single component configuration
is returned as the service object for all bundles that request the
service.
The servicefactory attribute must not be true if the component is a factory
component or an immediate component. This is because SCR is not free to
create component configurations as necessary to support servicefactory. A
component description is ill-formed if it specifies that the component is a
factory component or an immediate component and servicefactory is set to
true.

If @SJKA is right that DS 1.1 is OSGI 4.2 then this should work. I also found this on the openHAB page: http://docs.openhab.org/developers/prerequisites/osgids.html

maggu2810 · 2017-09-27T09:40:31Z

If @SJKA is right that DS 1.1 is OSGI 4.2

Have a look at the top page header of the OSGi R4.2 document in the capter DS 😉

triller-telekom · 2017-09-27T10:06:04Z

Have a look at the top page header of the OSGi R4.2 document in the capter DS 😉

Such a large document and I was just jumping into the mentioned paragraph. But that's good news that DS 1.1 supports service factories. @davidgraeff

davidgraeff · 2017-09-27T10:09:03Z

So the vault service would just declare itself as service factory, which leads to a new instance for every bundle requesting it. That's nice. So we can continue on agreeing on the API :)

triller-telekom · 2017-09-27T11:22:19Z

I think this service factory discussion was the last open point where we disagreed, right? I just scrolled through our comments but didn't find any other issues. So if we implement the vault service as a factory and have the management service as a general service to configure all vaults, I am fine with this API now.

triller-telekom · 2017-09-29T08:53:31Z

@davidgraeff Are you also fine with the current content (interfaces) of this PR? If so, please let us know and we will merge it soon so you can start implementing the basic implementation that uses whatever best algorithms the default JDK offers.

triller-telekom · 2017-11-01T14:21:37Z

@davidgraeff: ping :)

htreu

just some last minute comments on comments ;-)

htreu · 2017-10-04T10:15:51Z

....io.security/src/main/java/org/eclipse/smarthome/io/security/api/SecretsVaultManagement.java

+     * @param oldSecret The old secret value
+     * @param secret A secret as a ByteBuffer. If the ByteBuffer is empty, an empty secret will be stored.
+     * @return Return true if the modification was successful
+     * @throws IOException If the underlying file can't be opened or is not writable.


Since this is an interface, should we already talk about files here? The implementation is up to decide on how the secret vault will be stored. I would expect a more generic exception in case the operation failed. Maybe we should provide our own exception type here?

IOException perfectly expresses the type of errors that can occur, if you think about it. The only layer (apart from "no memory") that could cause problems for this function is the storage layer. I agree that we should not talk about files here, but IOException also works for Memory-only, Special hardware or DB storage.

htreu · 2017-10-04T13:13:59Z

...ome.io.security/src/main/java/org/eclipse/smarthome/io/security/api/SecretsVaultService.java

+ * The {@link SecretsVaultService} can be used to store and retrieve secrets.
+ *
+ * Secrets can only be retrieved by the bundle that stored them and by default
+ * will be encrypted and stored on disk immediately.


Again, storing the key/secret tuples in a file is up to the implementation. Even encryption seems to be in the responsibility of the implementation. This should be expressed more broad here I guess.

I wanted to express requirements for the implementation in this API as well, I guess. The implementation / storage layer SHOULD encrypt immediately. How the storage is implemented is not that important of course.

davidgraeff · 2017-11-14T12:48:53Z

...rthome.io.security/src/main/java/org/eclipse/smarthome/io/security/api/TlsNetworkServer.java

+     *             signed certificate fails (because the requested algorithms are not supported for instance) or if the
+     *             existing certificate is expired and service is configured to not automatically create a new one.
+     */
+    SSLContext createSSLContext(String context, String securityProvider) throws GeneralSecurityException, IOException;


In retrospective I would say this API is not very OSGi like. The context parameter should be removed. A service factory should be used. The requester should be able to request a special SSLContext by providing a context string via a service parameter.

davidgraeff · 2018-03-21T07:23:14Z

I have pushed a new API proposal. I have only slightly changed the SecureVault API and incorporated given comments. The creation of a secure socket implementation instance (aka SSLContext instance) API has changed a lot though. The requirements are:

async API, because it may take a while for the backend service (which may live in another thread) to provide a working SSLContext or other instance. It might even be necessary to perform network calls to create the secure socket context.
secure socket implementation neutral. It should be possible to request a non SSLContext based secure socket implementation, for instance a Scandium DtlsConnector to have DTLS support before Java 9.

* Annotation interface for fields in a configuration holder object. * Async request/response pattern with implementation neutral API to allow a backend to return a Scandium DtlsConnector as well as a standard SSLContext. Signed-off-by: David Graeff <david.graeff@web.de>

triller-telekom

Thank you for picking up this PR again!

I have left some (minor) comments in the code.

triller-telekom · 2018-03-22T14:17:09Z

...thome.io.security/src/main/java/org/eclipse/smarthome/io/security/api/annotation/Secret.java

+ * configuration and the services `activate(@Nullable Map<String, Object> configMap)` method
+ * is called. The `Secret` annotated fields will be null/not-set in that map.
+ * If you follow the configuration holder object pattern, you will do something like:
+ * `YourConfig config = new Configuration(configMap).as(YourConfig.class);`.


I really like your idea of "marking" a configuration parameter as "security sensitive".

However, we do have a mechanism for that already: You can mark a config parameter with the context "password". Maybe we should exploit this instead of introducing this other mechanism with an annotation?

Why I have put my comment here is because your idea is to do the fetching+decryption of these sensitive config parameters within the static call to new Configuration(configMap).as or after #5292 ConfigUtil.as. This fetching and decryption will be done by an OSGi service, right? How do you access it from a static context?

Yes we should avoid handling one case in different ways here. Maybe we should discuss #5300 first. As a result we might get a core annotation for the "password context" and this Secure annotation class is not necessary anymore.

I can imagine that the potential ConfigUtil.as method moves again to the core bundle into a class constructor that also takes a bundleContext. That way we would be able to resolve the secureVault service. Hypothetical code example:

@Modified public void modified(BundleContext bundleContext, Map<String, Object> config) { YourConfig configuration = new ConfigParser(bundleContext,config,YourConfig.class); }

Shall we leave this Secret annotation out of this PR and create a new issue/PR for that after we have discussed #5300?

I think it is better to discuss it afterwards and finalize this security API. Apart from my uncertainty on how one should use the SecureSocketCapabilities (https://github.com/eclipse/smarthome/pull/4267/files#r176436119) and the minor javadoc remarks in the SecureSocketRequest class: https://github.com/eclipse/smarthome/pull/4267/files#r176450243 and https://github.com/eclipse/smarthome/pull/4267/files#r176450487 I am fine with the API.

triller-telekom · 2018-03-22T14:18:59Z

...o.security/src/main/java/org/eclipse/smarthome/io/security/api/SecureSocketCapabilities.java

+    TLS,
+    /**
+     * Capability: Request a DTLS capable secure socket implementation.
+     * On Java 8 this is usually a Scandium DtlsConnector. On Java 9 it is a SSLContext.


If we have to make the distinction between SSLCONTEXT and SCANDIUM_CONNECTOR, why do you duplicate the information by also offering DTLS here?

I'd like to be the first user of this API in another binding and I plan to support Java9 SSLContexts as well as a DtlsConnector. Preferably it is a SSLContext of course. So I would only require the capability "DTLS" and don't care which implementation is actually returned.

Other bindings might do this different and definitely want a DtlsConnector. They would request the SCANDIUM_CONNECTOR+DTLS capability.

Which of the possible options below is a user of the API required to ask for?

DTLS AND SSLContext OR DTLS AND SCANDIUM_CONNECTOR

SSLContext OR SCANDIUM_CONNECTOR

just DTLS and depending on the java runtime version you dynamically get a SSLContext OR SCANDIUM_CONNECTOR

It is unfortunately that complicated because Java 8 installations need to be considered. The API is more generic therefore. The implementation of this API that ESH provides would return a SSLContext only, to keep dependencies low. On Java 8, if you request TLS, you get something back, if you request DTLS, no provider can be returned. You can optionally require SSLContext to make sure, it is always a SSLContext. Because:

Another bundle can provide more options e.g. Scandium Connectors additionally. It is more or less undefined which secure socket provider will be choosen, if you just request DTLS.

I have adjusted the comments in code to explain the flags a little more.

triller-telekom · 2018-03-22T14:22:52Z

bundles/io/org.eclipse.smarthome.io.security/about.html

@@ -0,0 +1,28 @@
+<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN"


Please replace this file with a NOTICE file, because with the change to EPL-2 we are no longer using about.html files, see https://github.com/eclipse/smarthome/blob/master/bundles/core/org.eclipse.smarthome.core/NOTICE for an example.

triller-telekom · 2018-03-22T14:54:33Z

...ome.io.security/src/main/java/org/eclipse/smarthome/io/security/api/SecureSocketRequest.java

+    Set<SecureSocketCapability> optionalCapabilities();
+
+    /**
+     * Returns a potentially new secure socket implementation instance that fulfills the requested mandatory


If you say that this method should return something, why is it void? :)

Since you use this SecureSocketRequest class as a callback object, maybe you should make this clear that this methods sets the socket implementation on this callback object but does not return it.

triller-telekom · 2018-03-22T14:55:05Z

...ome.io.security/src/main/java/org/eclipse/smarthome/io/security/api/SecureSocketRequest.java

+     * @param secureSocketContext May be null, if at least one capability could not be matched. May be null in a
+     *            subsequent call if the user removed an implementation deliberately.
+     */
+    void secureSocketImplementationResponse(@Nullable SSLContext secureSocketContext);


…etProvider. Add unregister method Signed-off-by: David Graeff <david.graeff@web.de>

triller-telekom

Thank you for your changes. The provided API looks good to me now.

wborn · 2018-08-16T16:13:47Z

bundles/io/org.eclipse.smarthome.io.security/pom.xml

+  <properties>
+    <bundle.symbolicName>org.eclipse.smarthome.io.security</bundle.symbolicName>
+    <bundle.namespace>org.eclipse.smarthome.io.security</bundle.namespace>
+  </properties>


Please remove these properties. See #6060.

openhab-bot · 2018-10-23T15:31:58Z

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/roadmap-to-happiness-what-is-missing-in-the-core-framework/54522/1

kaikreuzer · 2019-01-31T14:50:05Z

Closing it here - once required by some feature and you want to continue working on it, please port this code over to https://github.com/openhab/openhab-core. Thanks!

triller-telekom reviewed Sep 14, 2017

View reviewed changes

paulianttila reviewed Sep 14, 2017

View reviewed changes

davidgraeff force-pushed the securityapi branch from b013676 to a39e3f7 Compare September 19, 2017 16:25

triller-telekom reviewed Sep 20, 2017

View reviewed changes

davidgraeff force-pushed the securityapi branch from 355aea1 to d7d84fe Compare September 25, 2017 05:03

paulianttila reviewed Sep 25, 2017

View reviewed changes

triller-telekom reviewed Sep 25, 2017

View reviewed changes

davidgraeff force-pushed the securityapi branch from d7d84fe to 59eaff3 Compare September 25, 2017 18:24

htreu reviewed Nov 6, 2017

View reviewed changes

davidgraeff commented Nov 14, 2017

View reviewed changes

triller-telekom mentioned this pull request Dec 7, 2017

Rest API: Redact configuration password parameters #4583

Open

davidgraeff force-pushed the securityapi branch 2 times, most recently from 6376997 to d92d82f Compare March 21, 2018 07:09

Security API

40f946f

* Annotation interface for fields in a configuration holder object. * Async request/response pattern with implementation neutral API to allow a backend to return a Scandium DtlsConnector as well as a standard SSLContext. Signed-off-by: David Graeff <david.graeff@web.de>

davidgraeff force-pushed the securityapi branch from d92d82f to 40f946f Compare March 21, 2018 10:10

triller-telekom reviewed Mar 22, 2018

View reviewed changes

Improved documentation. Rename SecureSocketServers->RequestSecureSock…

fb9b055

…etProvider. Add unregister method Signed-off-by: David Graeff <david.graeff@web.de>

davidgraeff force-pushed the securityapi branch from ef6cdb6 to fb9b055 Compare March 28, 2018 11:17

triller-telekom approved these changes Mar 29, 2018

View reviewed changes

wborn reviewed Aug 16, 2018

View reviewed changes

kaikreuzer closed this Jan 31, 2019


		import javax.net.ssl.SSLContext;

		import org.eclipse.californium.elements.Connector;

		@@ -0,0 +1,28 @@
		<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN"

Security API #4267

Security API #4267

Conversation

davidgraeff commented Sep 14, 2017 • edited Loading

Secure Vault API

Network secure sockets

davidgraeff commented Sep 14, 2017

triller-telekom left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidgraeff Sep 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidgraeff commented Sep 19, 2017

triller-telekom left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidgraeff commented Sep 25, 2017 • edited Loading

Just SSLContext createSSLContext can be enough

To sum up

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

triller-telekom commented Sep 25, 2017

davidgraeff commented Sep 25, 2017

davidgraeff commented Sep 25, 2017 • edited Loading

davidgraeff commented Sep 25, 2017

davidgraeff commented Sep 14, 2017 •

edited

Loading

davidgraeff Sep 14, 2017 •

edited

Loading

davidgraeff commented Sep 25, 2017 •

edited

Loading

Just `SSLContext createSSLContext` can be enough

davidgraeff commented Sep 25, 2017 •

edited

Loading

maggu2810 commented Sep 26, 2017 •

edited

Loading