Centralized Crypto Library

[morph166955]

I'll preface this with, im volunteering to write this myself as it probably has a decent work effort involved and I just did a bunch of this recently for the ShieldTV binding. I am however looking for input on how best to structure it.

Over the past few weeks of working on the ShieldTV binding I've encountered a few decent learning curves. The biggest was working with crypto functions (specifically PKI, not transport). Several bindings need to create, store, and interact with devices using specific keys and certificates. Unfortunately, we don't have a centralized set of functions to help the developers/maintainers easily perform these functions. This came up during a separate discussion I was having with @kaikreuzer and he suggested opening an issue here for a larger discussion.

I propose a new addition to the core (looking for suggestions as to where it's most appropriate). Forgive the bad pseudocode but I would envision it working something like this:

CryptoStore myCrypto = new CryptoStore(); //add a variety of different options to accept keys, filenames, etc at creation

myCrypto.generateKey(Int keyLength, String algorithm); // Generate a private key of requested length and with stated algorithm
myCrypto.generateSelfSignedCert(String name); //use the private key and the certificate name/data to create a self signed certificate
myCrypto.loadFromFile(String fileName);
myCrypto.writeToFile(String fileName);
myCrypto.getKeyStore() //Return a KeyStore that's properly initialized and populated.
myCrypto.setKey(String myKey); //Store a base64 encoded private key
myCrypto.setCert(String myCert); //Store a base64 encoded certificate

Keys and certs would be held as base64 encoded strings when not directly in the KeyStore format. There's probably a dozen functions I'm not listing, as well as variants of those above which need to be mapped out (e.g. to return the private key as a Key instead of a String if that's what's needed, get and set functions for each variable, some sort of consolidated exception catch/throw to a single CryptoException, etc.). By doing it this way you also can have multiple sets of keys being used without adding confusion.

Assuming this gets accepted/merged, we would then need to petition the developers to make the necessary changes to remove their crypto bits and adopt this as a standard. Documentation will be obviously necessary. I'd volunteer to adapt the ShieldTV first so it can be used as an example to others.

Thoughts?

[clinique]

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

[J-N-K]

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?

[morph166955]

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.

[morph166955]

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

[J-N-K]

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.

[morph166955]

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.

[morph166955]

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.

[J-N-K]

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.

[cdjackson]

@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.

[morph166955]

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).

[cdjackson]

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.

[J-N-K]

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.

[morph166955]

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).

[cdjackson]

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.

[morph166955]

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.