Very good approach and this is from my point of view the aim of core to centralize common topics.

Would you suggest a keystore that is managed by each binding itself or a central keystore that is injected into (what ever service needs it)? I would say that it would better be managed centrally, that way we can even migrate something if we need it.

Maybe we could have a CryptoKey interface (together with a CryptoKeyImpl) which allows access to private and public key (in different representations), a CryptoKeyFactory interface/service (with a CryptoKeyFactoryImpl) to generate new keys and a CryptoKeyStorage that uses a CryptoKeyEntity to store and restore keys. The keys could use a CryptoKeyUID in the form namespace:id where the namespace part is add-on specific and the id defaults to the public key SHA1 fingerprint or another identifier passed to the CryptoKeyFactory when the key is created. WDYT?

There are definitely positive and negative points to weigh out on that. On one hand, absolutely one single KeyStore, holding all the keys in a very secured place would be the best from a security best practice standpoint. That said, it may be much harder for the developers to work with as the other functions (SSL for example) require the passing of a KeyStore which contains only one key/cert pair (and maybe a chain) to work. One of the tricky bits I ran into was dealing with the creation/initalization of the KeyStore itself. My hope with this would be to handle that in the core and just return the necessary final populated KeyStore. Theres also the issue of key formats. On the Shield, i had to start by creating a self signed key pair and then replaicing it with the key pair issued by the CA on the Shield after completing the on screen PIN process. Maybe a hybrid approach would work the best? Hybrid in the sense that we can maintain one store for the platform but could pull a key pair out and return it as a KeyStore if requested.

This is what I did with the Shield as an example: https://github.com/morph166955/openhab-addons/blob/shieldtv/bundles/org.openhab.binding.shieldtv/src/main/java/org/openhab/binding/shieldtv/internal/ShieldTVPKI.java

CryptoKey could also have a .toKeyStore() method that returns a KeyStore.

And CryptoKeyFactory could return a CryptoKey from a given pair of private/public key (and probably the cert chain) . in addition to the generation of a new pair with a given algorithm and length.

Regarding the handling you could also have a look at #2905 where I implemented certificate handling for the https connection to openHAB.

I like where you're going with this. Let's keep peeling the onion.

We would need to add a global configuration option to store a user specified keystore password. Probably something to be done at initial setup. We'd need to also tell people with existing systems that they have to go specify it otherwise. If this was lost, the entire keystore would be unrecoverable. Same problem of global loss if the file was corrupted somehow. By doing a separate KeyStore for each binding/thing then worst case is loss of that key.

We also need to define conditions for when to save the keystore to a file versus temporary keys. There may be times where a user just needs a self signed cert and key but they don't need to be stored long term.

I'm also trying to keep this higher than just the Key level because it needs to encompas the entire PKI set (keys, certs - which contain but are not limited to the public key, CSRs potentially, etc). I agree with your logic, but to avoid confusion with the existing Key libraries I'd call this more of a Store than a Key.

Here's a thought. The bulk of this is going to be making sure the actual crypto bits are working/returning as expected. Storage is ultimately going to just be "the end piece". How about I start by getting the functions all drawn out and working. Once we have that, we can work on a central key store as a part 2. I really like the idea, but I want to make sure we cover the widest possible usage where developers can do basics such as key generation if that's all they require up through a fully centralized key management if they also need that.

We can mitigate the risk by not encrypting the entire storage but only encrypting the stored values (i.e. CryptoKeyEntity in my example above, I don't care about the name). This could be achieved by a EncrpytedJsonStorage that extends the JsonStorage.

But I agree, we should get the other parts sorted out first. Since this is completely different from what we already have I think a new bundle org.openhab.core.crypto would be a good choice, that also makes it optional for solutions that don't want to use it.

I would suggest to first figure out a good interface and when we are done with that, we can come to the implementation.

@morph166955 nice idea 👍

Just a few thoughts...

By doing a separate KeyStore for each binding/thing then worst case is loss of that key.

IMHO it's always a double edged sword - splitting things out into separate stores to avoid "total loss" could be seen as reducing the pain, but I kind of think the pain is still there, and if people want to avoid the pain, then having a good backup in place, and educating people is really the answer (and it won't stop losses ;) ).

One thing I would suggest, and maybe it's already in hand, is to have a zero trust policy between bindings - ie don't allow bindingA from reading the keys from bindingB. This just avoids someone writing a binding to read out all your keys. I'm not sure of other implications here but if this compartmentalisation is possible, it would be a good thing IMHO.

I'll go ask some of my colleagues who have dealt with building centralized key stores before how they did it and what speed bumps they run into. The general rule with crypto is to never recreate the wheel because it's really easy to screw up complex things (or create security holes) without realizing it. There may be a very simple way to do this with existing libraries that we just need to implement.

I'll start to write this as org.openhab.core.crypto.CryptoStore and get the basic functionality going. We can review and adjust from there.

I do like the zero trust idea, but as devils advocate do we really need it given that the platform is entirely open source and requires reviews before code is merged in. Also since we don't allow outside connections (e.g. statistics, logging for developers, etc), minus to APIs as a function of the binding, even if a binding did read the keys from another they aren't ever leaving the OH environment. Worst case would be a denial of service if another binding nuked your keys, which hopefully would be caught during code review. The exception here would be the marketplace as those don't have the same review (but do have the requirements of not sending data out).

Worst case would be a denial of service if another binding nuked your keys, which hopefully would be caught during code review.

I think the worst case is that a rogue binding downloads the keys and sends them somewhere else. Then all your keys are on the net for "anyone" to use. Given that many of the keys are used to access cloud systems, once the keys are out there, then users systems are compromised regardless of how well OH might be locked down.

the platform is entirely open source and requires reviews before code is merged in

That's only for "official" bindings - there are plenty of marketplace bindings out there. We could say "bad luck" to anyone that uses a marketplace binding and gets hacked in this way and looses all their keys, but I'm not really sure that's acceptable given that the marketplace is an official way to distribute bindings.

Maybe not the best idea, but if we pass the instance that requests access to the keystore, call .getClass on that and use the BSN of the bundle that added the class as namespace it‘s very hard to get access to another bundles keystore.

Fair enough, and I do agree that a zero trust model would definitely be preferred. I'll see what I can come up with as an authentication mechanism. What about a user configurable, per thing encryption key that could be set as part of the thing configuration? That way the user can specify the password directly and it's totally separate from anything in the code. We could also follow some of the crypto RFCs and do double encryption (encrypt each private key with it's own AES key and then bulk encrypt the entire keystore with a master key).

What about a user configurable, per thing encryption key that could be set as part of the thing configuration?

To me that seems a bit OTT and might be a pain to users if it's required for every thing... To me the binding level seems a good middle ground since this is a code "block" and within this, the code will have access to all things configuration anyway so I think there's little to be gained by extending the security down to a per-thing level.

We could make it as trivial as a forced prepend of the binding name on the key alias similar to what @J-N-K said on the core side. For example, if the shield binding stores a keypair "livingroom", we store it as shield.livingroom. Then if another binding tried to call it would only get what is available behind it's prepend.

So next question, do we have a need to also implement a centralized TrustStore? This is probably going to be less used comparatively to KeyStore.

It would of course be nice to have something like that. We currently can only chose between "standard trust" and "trust all", which is not the best choice. But we should first focus on the CryptoStore here. The TrustStore can be added in another PR.

Regarding the "zero trust": I was thinking of something like (completely untested!):

    public CryptoStore getStore(String id) {
        StackWalker walker = StackWalker.getInstance(Option.RETAIN_CLASS_REFERENCE);
        Class<?> callerClass = walker.walk(s -> s.map(StackFrame::getDeclaringClass)
            .filter(c -> !c.getName().startsWith("org.openhab.core.crypto")).findFirst().orElse(null);
        if (callerClass == null) {
            logger.error("Illegal call to get crypto store. Could not determine namespace.");
            return;
        }
        Bundle callerBundle = FrameworkUtil.getBundle(callerClass);
        if (callerBundle == null) {
            logger.error("Illegal call to get crypto store. Could not determine namespace.");
            return;
        }
        String namespace = callerBundle.getSymbolicName();
        if (namespace == null) {
            logger.error("Illegal call to get crypto store. Could not determine namespace.");
            return;
        }

        logger.debug("Accessing CryptoStore for '{}:{}'.", namespace, id);

        <do other stuff>
    }

Agreed on TrustStore. I'll hold that for future.

I'll see what I can do with the zero trust as I get there. I like the idea of forcing a check.

On a side, related note. Could I convince one of you to spend a few minutes to review the shieldtv PR? I'd like to use that binding as a test mechansim for this but I want to avoid having to maintain several different versions of that binding code. My hope is to get it reviewed and merged so that I can just create a branch specific to this for testing. Thanks!

There's nothing in it yet, but I'll be using this branch for this effort: https://github.com/morph166955/openhab-core/tree/CryptoStore

I've setup a basic bundle and started to pull some of the PKI functions from shieldtv into this library. One thing to note, im pulling org.bouncycastle in at an older version right now because I know it works. Once we get the code working we probably need to upgrade the code base and then adjust because a few of the functions have been depreciated.

Just a comment, but also suggestion. I like the idea and on backend side there are tools which are reffered as vaults which serve similar purpose as you began to draw above.

WRT security perspective and zero trust approach my suggestion is to consider passwords to be also element which require extra care. While openHAB have some security for oauth tokens, passwords required by binding configurations are stored in plaintext. Treating them similar to private keys is needed as it is yet another access credential.
Given existence of TPM modules (available even for Raspberry's) it would be possible to employ hardware security in longer term.

I've made some decent progress on this, I'll probably push it up this weekend once I do a few more things. I've decided to change my approach after all of the security discussions and working through the implementation. I'm likely not going to hold the data as B64 but as an encrypted String and Certificate. By doing this, I can keep the private keys encrypted which is better obviously. I considered a few different approaches and this seems to be the easiest to implement and use. As we push to a centralized storage, it will also force a layer of protection that's almost two factor as the binding will hold the decryption key for its encrypted private keys.

EDIT: I did some further work on the encrypted private key. Code has been uploaded to the branch linked above. Basically it would work like this:

Key cryptoStoreKey = CryptoStore.generateEncryptionKey();
CryptoStore myCryptoStore = new CryptoStore();
myCryptoStore.setPrivKey(String myPrivKey,cryptoStoreKey);

The cryptoStoreKey is a long AES key that is generated to be used on the CryptoStore. While the private key is stored in memory, it's encrypted using that AES key rather than being in the clear. That key is retained as a private String inside of the binding (which makes it very hard to get to). Now I realize that it's marginally useless for the one-off in-binding storage, but when we move to centralized storage will play a huge part. Basically, we store all of the cryptoStoreKeys inside some sort of encrypted container. The binding then could request it's cryptoStoreKey from the central storage, and then request it's CryptoStore(s) from central storage. To note, until central storage is implemented, there is still the concept of the keystorePassword which is used to write the KeyStore to disk (and retrieve). Which brings me to the idea below...

As a side question, I think an interesting idea would be the concept of "unlocking openhab" at time of start. When openhab starts, user is prompted to enter the password that decrypts the cryptoStoreKey for each binding to use. This would delay the things from coming online, but it would provide a significant security boost. Thoughts?

Created PR above as this is getting close to being ready. I'd appreciate an initial review/comments. Thanks!

As a side question, I think an interesting idea would be the concept of "unlocking openhab" at time of start. When openhab starts, user is prompted to enter the password that decrypts the cryptoStoreKey for each binding to use. This would delay the things from coming online, but it would provide a significant security boost. Thoughts?

I like it as an option, but not as the only choice and not as the default choice. Many people run OH in hard to access locales (e.g. a remote cabin with only cell data service). Requiring human interaction to start up could be a significant problem for these sorts of users.

Lots of people also have concerns about OH coming back online reasonably after a power outage and this would interfere with that as well.

Fully agree with that. Maybe some sort of selectable level?

Low to high security:

1. Use some sort of unique ID of the install
2. Use some kind of text file which stores the key
3. Unlock via gui/cli
4. TPM or other secure token

@morph166955 Small suggestion, since you are deep in topic - you might look into pcks11 as well as softtpm which allows to simulate security chip. The pcks11 itself is a standard interfaces for cryptographic devices which can deliver public keys and certificates while retaining safely private keys - reason why I point these up is basic - these are used in to assure that either device is trusted or keys are safe as secure credentials never leave tpm2 chip itself (unless you have portable tpm module).
As far I remember there are ways to grant process access to subset of tpm2 contents through specific "handles". There is even a unit test proving tpm2-pcks11 library working with java: https://github.com/tpm2-software/tpm2-pkcs11/blob/1.8.0/test/integration/PKCS11JavaTests.java

Best,
Łukasz

Fully agreed TPM or even a USB token would be great and I'll definitely look into it for the higher security modes.
The problem is, there's no assurance of either existing in a deployment for a variety of reasons, and we can't compell users into using or purchasing them either. I'll add it to the list as option 4.

Maybe some sort of selectable level?

That seems reasonable.

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

https://community.openhab.org/t/openhab-4-0-wishlist/142388/304

In an attempt to test this, I pulled the current copy into the androidtv binding's PKI section. Bugs were found, bugs were fixed, and I've pushed the bug fixes back into the PR. From the limited testing I've done, this seems to work nicely. It definitely needs more testing before we call it baked though. Examples of use from the AndroidTV binding:

private CryptoStore cryptoStore = new CryptoStore();
private byte[] encryptionKey = cryptoStore.generateEncryptionKey();

    cryptoStore.setKeystoreFileName(config.keystoreFileName);
    cryptoStore.setAlias("nvidia");
    try {
        File keystoreFile = new File(config.keystoreFileName);

        if (!keystoreFile.exists()) {
            cryptoStore.generateNewKeyPair(this.encryptionKey);
            cryptoStore.saveKeyStore(config.keystorePassword, this.encryptionKey);
        } else {
            cryptoStore.loadFromKeyStore(config.keystorePassword, this.encryptionKey);
        }

        logger.trace("Initializing SSL Context");
        KeyManagerFactory kmf = KeyManagerFactory.getInstance(KeyManagerFactory.getDefaultAlgorithm());
        kmf.init(cryptoStore.getKeyStore(config.keystorePassword, this.encryptionKey),
                config.keystorePassword.toCharArray());

        TrustManager[] trustManagers = defineNoOpTrustManager();

        sslContext = SSLContext.getInstance("TLS");
        sslContext.init(kmf.getKeyManagers(), trustManagers, null);

        sslSocketFactory = sslContext.getSocketFactory();

        asyncInitializeTask = scheduler.submit(this::connect);

    } catch (NoSuchAlgorithmException | IOException e) {
        logger.debug("Error initializing keystore", e);
    } catch (UnrecoverableKeyException e) {
        logger.debug("Key unrecoverable with supplied password", e);
    } catch (GeneralSecurityException e) {
        logger.debug("General security exception", e);
    } catch (Exception e) {
        logger.debug("General exception", e);
    }

public void setKeys(String privKey, String cert) {
    try {
        cryptoStore.setKeys(privKey, this.encryptionKey, cert);
        cryptoStore.saveKeyStore(config.keystorePassword, this.encryptionKey);
    } catch (GeneralSecurityException e) {
        logger.debug("General security exception", e);
    } catch (IOException e) {
        logger.debug("IO Exception", e);
    } catch (Exception e) {
        logger.debug("General Exception", e);

    }
}

For anyone following this, I have submitted the PR for review. As stated in the PR, I am NOT expecting this to get into 4.0.0. There isn't any reasonable time for the developers to adjust bindings to use this. I do plan to adjust AndroidTV to use this once merged in. This is PR 1 of 2. The second PR will implement the central storage concept.

@morph166955 Have you worked with TPM modules before? I have created a proof of concept to protect the keys for KNX secure. openhab/openhab-addons#15326
Hard to find a suitable java library for TPM, it uses TSS.MSR and also pulls in bouncycastle as dependency.

Is anyone still interested in this? The PR has been open and ready for review for some time now.

@pacive @morph166955 Maybe we can discuss the general approach here and not in PR #3302. Let me summarize my understanding of the discussion:

1. We want to include a solid crypto library which can be reused in addons (and core).
2. On top of the crypto library, we want to have helper functions making it even easier to use in the addons.
3. The CryptoStore in PR Implementation of CryptoStore #3302 is the first piece. The code is already in use in androidtv addon.

For 1) bouncycastle seems to be a valid choice.
bouncycastle (depending on functionality needed) adds ~6,5MB to each addon using it (and ~3000 class files in the jar).
Currently bouncycastle is used by the following addons already:

The question is if we could harmonize the versions as there might be additional constraints when bc is added as transitive dependency of another library.

For 2) I am not exactly sure about the requirements we have from the bindings.
My guess what would be needed: handling of certificates (and that is where we might add a helper class on top of bc), encryption/decryption algorithms (could be provided by bouncycastle or jdk), signatures/mac (could be provided by bouncycastle or jdk), protecting secrets in memory (alternative to using string objects for passwords), protecting secrets on disk (may also be supported by a TPM).

When speaking about protecting data in memory - this might not improve security at all.
The question is how far you can get on top of a typical non-trusted Raspberry PI system (no secure boot / attestation by TPM possible), inside a JDK, with the given TPM libraries....

What is your opinion on this?

I would also want to know more about what requirements different bindings have. @morph166955 can you give some details on how it's used in the Android TV/shield TV bindings? Is it just that the TV provides a certificate that needs bo be trusted by OH, or the other way around, that OH needs to provide a certificate to the TV that it can put in a trust store? Is it then only used for TLS establishment, or do you need to sign/verify application level data as well?

One thing to consider as well is that there's a lot of different types of credentials used by bindings, that are mostly just stored in plaintext in the thing configuration. Just because it's called a private/secret key and is used in cryptography doesn't make it inherently more valuable than a regular password and needs to have a higher level of protection. The protection level should be determined by the value of whatever the credential is protecting (an API-token for a cloud service would therefore need stronger protection than a RSA private key for communicating with a single local device). A Cryptostore that can handle any type of credentials (passwords, API-tokens as well as crypto keys) would be the ideal solution IMHO.

@pacive for KNX binding is is easy:

• For IP secure, we need to protect the tunnel password / the backbone key - this is basically a password string. The secret needs to be supplied to Calimero API below whenever the KNX bridge is initialized.
• For data secure we would need to protect the content of a so called KNX keyring file. It is encrypted and we need to supply the password during initialization of the KNX bridge to the Calimero library. The library will keep the plaintext data in memory.
  -> so basically just string passwords.

As there is currently not way of reading a password from user through the UI / apps during OH start, I looked at TPM. Using a TPM would at least make sure that passwords can only be recovered with exactly this TPM. Though, anyone with access to the machine can either recover the password using the TPM or steal it from memory.
But it would protect against exploiting passwords from unprotected backups, copied configs, or accidentally posted passwords.

Are those passwords supplied by the user in the Thing configuration or are they generated by the binding? Since this is the case for most bindings at least I can see several things that will need to be changed in order to enable (more) secure storage of sensitive credentials.

1. There needs to be a way for binding authors to set a flag on sensitive configuration parameters
2. These parameters will then need to be handled and stored differently by the framework (i.e., using the CryptoStore instead of jsondb), but still be available to the bindings. They will also need to be exportable so they can easily be migrated to a different system.
3. The REST API should preferably not return them in responses, this will need to be handled correctly by the UI as well so they can be explicitly overwritten by the user but not accidentally deleted because the input field is empty when the user change some other parameter.
4. There might be more things affected that I'm currently unaware about (possibly for people using textual configuration of Things)

Alternatively, we can choose to only offer a cryptographic interface which offers encrypt/decrypt sign/verify operations where the binding developers don't have to think about managing keys. They might need to set the key once if they get it from an external source, but after that it should never leave the CryptoStore, all crypto operations should go through a standard interface.

But all depends on what the needs of different developers are. (These are just my thoughts everyone is welcome to disagree)

@pacive One could use the karaf console to enter passwords. For TPM draft I have used a binding specific console command to generate password representations. With TPM it is easy, as this gets you a public key representation which can be put in the desired password fields as clear text. No special handling necessary.
Decryption is triggered from the binding if such a password is detected.

One could use the karaf console to enter passwords.

I don't think this would be a good idea from a usability perspective. Many users want to deal with SSH and console commands as little as possible, and requiring use of the console to configure any binding that uses a password would be a huge step backwards after all the work that has been done to make as much as possible configurable via the UI.

But just to get some grips of what sensitive data would need to be protected I thought of a generic list of different types of credentials and the impact if someone would get access to them

Of course things might look different if users have their local devices exposed to the internet, then the impact might be even higher than for cloud services (user may have more privileges than for most cloud services, adversary can get access to other devices on the network etc.)

Please fill in if you feel I've missed something (@holgerfriedrich how would you categorize the KNX credentials?) or if you think something should have higher or lower impact!

Hey so I'm going to chime in here. I have a lot of IT security/crypto experience (20+ years), am CISSP certified, and am currently working on my masters in Cybersecurity.

I came across the Cryptostore work as I recently resumed my work on zwave S2, and found some situations where the bouncycastle classes weren't loading properly.

I was then asked to have a look at the PR for this work. I wanted to take a step back from the code and review the general thought process/design/goals around it, so I came here. While I think there are some cool security ideas here, some just aren't practical or necessary. And there's some scope creep happening.

When it comes to security, it's important to think of specific problems to be solved and use those as requirements. Start very small and add from there.

I can see a lot of code has been written already, and given that, I'm going to warn you that you're not going to like what I have to say..

But I think the scope needs to be reduced dramatically. Specially:

1. Remove encryption of keys in memory from scope. It serves no security benefit (the keys are going to be in the clear somewhere in memory, a heap dump will reveal them). It just adds unnecessary complexity.

2. Remove "crypto helper" functions from scope. I've seen this in the IT world a few times and it just doesn't work. You end up wrapping the entire Java crypto API with a non-standard API. There are an endless number of use cases, especially with the advent of quantum resistant cryptography. Start without and go from there. If there is really a need for one or two helper functions, add it later if there is an actual consistent need.

3. Don't store "Keys and certs as base64 encoded strings when not directly in the KeyStore format". Store everything in the keystore itself. Do not return strings for keys, this just creates a ton of extra work. Instead, return the actual key objects that java provides (do not wrap them, this will backfire). Better yet, simplify the API to just return the KeyStore object to the caller so the raw java API can be used as normal. Cryptostore would provide a "load" and "save" methods for the keystore with some basic access controls.

I know this is undoing a lot of work that was already done. Which will be really frustrating. But this should start small. Very small. As a central way to load a java keystore for an addon with some very basic permission protection. One keystore per addon, with automated naming based on package name to avoid keystore collisions.

Start there, make it work, integrate it, see how it's used. Add features from there only if they are truly justified and worthwhile. I hope this helps - even if it may not feel like it