Implementation of CryptoStore #3302
Conversation
Signed-off-by: Ben Rosenblum <rosenblumb@gmail.com>
Signed-off-by: Ben Rosenblum <rosenblumb@gmail.com>
Signed-off-by: Ben Rosenblum <rosenblumb@gmail.com>
Signed-off-by: Ben Rosenblum <rosenblumb@gmail.com>
Signed-off-by: Ben Rosenblum <rosenblumb@gmail.com>
J-N-K
added
enhancement
|
This pull request has been mentioned on openHAB Community. There might be relevant details there: https://community.openhab.org/t/openhab-4-0-wishlist/142388/304 |
Signed-off-by: Ben Rosenblum <rosenblumb@gmail.com>
Signed-off-by: Ben Rosenblum <rosenblumb@gmail.com>
|
pom.xml conflict should be fixed. This was the addon/binding change. |
Signed-off-by: Ben Rosenblum <rosenblumb@gmail.com>
|
I'm down to just a few changes at this point. Code is very stable on the androidtv binding. I need to add a few extra functions to handle CA certs. |
|
Just so this doesn't go stale, this is pending additional testing once the androidtv binding is reviewed and merged. |
Signed-off-by: Ben Rosenblum <rosenblumb@gmail.com>
|
@openhab/core-maintainers This is ready for review. I'm NOT expecting this to make it in for 4.0.0. I highly doubt any binding developer will be able to adapt their binding in the 6 days between now and the code freeze. I see this as a 4.1.0 feature. Also note, this is part 1 of 2. Part 2 will be the centralized crypto storage as discussed in #3289. In theory, CryptoStore replaces and enhances the need for a KeyStore in the bindings (or anywhere else that's relevant). CryptoStore implements several features such as key generation, encryption/decryption of strings, and self signed certificate creation. Code is all based off of the AndroidTV binding implementation which seems to work very well. Once this is locked down I'll work on part 2 with the goal of having both available for 4.1.0. As a note, I considered adding in some things like TrustManagers to this, but I think that may be better separated out into a separate library. If the consensus is to add it here, I can easily add the functions. |
morph166955
changed the title
Signed-off-by: Ben Rosenblum <rosenblumb@gmail.com>
Signed-off-by: Ben Rosenblum <rosenblumb@gmail.com>
Signed-off-by: Ben Rosenblum <rosenblumb@gmail.com>
|
The builds are failing due to the automation integration tests, not due to any specific issue with this PR. This builds cleanly against 4.1.0 on my local system when just compiling the bundle. |
|
@openhab/core-maintainers Respectfully requesting a review on this. Code is very stable on AndroidTV binding. I really want to start working on the second part of this to try and get both into 4.1. Thank you! |
Signed-off-by: Ben Rosenblum <rosenblumb@gmail.com>
Signed-off-by: Ben Rosenblum <rosenblumb@gmail.com>
Signed-off-by: Ben Rosenblum <rosenblumb@gmail.com>
Signed-off-by: Ben Rosenblum <rosenblumb@gmail.com>
Signed-off-by: Ben Rosenblum <rosenblumb@gmail.com>
|
This pull request has been mentioned on openHAB Community. There might be relevant details there: |
|
@morph166955 I think it would be helpful to integrate bcprov-jdk18on as part of core. |
|
Fully agreed and this PR should do that already. My ultimate goal is to really pull as much crypto as we can into a (very) reviewed and hardened implementation to make sure we don't have security leaks/holes in bindings. bcpkix-jdk18on, bcprov-jdk18on, and bcutil-jdk18on are all added to bom/compile/pom.xml as part of this PR. In respect to TPM usage, my is to do a second PR which will create a central storage location for crypto (certs, keys, passwords, etc). TPM would definitely be a good addition to that once it works. There was some discussion on this in #3289 but it definitely needs further thought. |
There was a problem hiding this comment.
I made a quick review, but didn't have time to look at everything in detail unfortunately. Apart from the specific comments, JavaDoc needs to be added which will make further reviewing easier. Now I had to try to deduce the functionality and intended use by analyzing the code, which makes it take more time.
But keep up the good work, I think this would be a valuable feature!
...b.core.crypto.cryptostore/src/main/java/org/openhab/core/crypto/cryptostore/CryptoStore.java
Show resolved
Hide resolved
| } | ||
|
|
||
| public void setCert(Certificate cert) throws CertificateEncodingException { | ||
| this.cert = new String(Base64.getEncoder().encode(cert.getEncoded())); |
There was a problem hiding this comment.
Why is it stored as a String instead of as a Certificate object?
There was a problem hiding this comment.
I went back and forth on this several times. It was just easier to store and manipulate as a Base64 encoded String in the end. If you look through some older versions of the AndroidTV PR I had it in as a Certificate and ended up changing it.
There was a problem hiding this comment.
What kinds of manipulations would be needed? If a certificate is modified after being signed, the signature (and therefore the entire certificate) becomes invalid.
There was a problem hiding this comment.
I'll have to go back and look to see what the issue was. I believe part of it was simply the way exceptions were handled (where Certificate throws several that we would have to catch and deal with, String did not which made the code much cleaner). Another part of it was that it's very likely that the bindings are dealing with them in base64 (ATV was for example being sent a certificate as part of something else that I had to deal with) so flipping back and forth was becoming complicated.
There was a problem hiding this comment.
Was mainly a thought because the getter for the certificate returns a Certificate object, so there are more conversions when storing as a string that it would be the other way around. Providing a string as a setter should definitely be supported, but it would probably be better to get an exception when trying to set an invalid string-encoded cert than getting it when retrieving the cert.
There was a problem hiding this comment.
I went back and looked at the old commits and why I changed this. This is absolutely because of exception handling. I was trying to avoid a try/catch inside the class (so that the binding could handle the try/catch and logs were more accurate). By storing as a string I avoided that (and could simply put it in a throws line for the end user to catch).
| throws GeneralSecurityException, IOException, Exception { | ||
| Key key = convertByteToKey(keyString); | ||
| KeyStore keystore = KeyStore.getInstance("JKS"); | ||
| FileInputStream keystoreInputStream = new FileInputStream(this.keystoreFileName); |
There was a problem hiding this comment.
Is the filename an absolute path? How do the binding developers make sure that it works properly on all supported operating systems?
There was a problem hiding this comment.
Yes, this is absolute path. When implemented in ATV I used "String folderName = OpenHAB.getUserDataFolder() + "/androidtv";" to create a path supported accross implementations. I suppose looking at it now that Windows could have an issue with the / versus . I'll check into that. Either way, it's absolute so the binding can decide.
There was a problem hiding this comment.
I think it would be better to use OpenHAB.getUserDataFolder() + "/keystore/" + this.filename or something like it to make it easier to use.
There was a problem hiding this comment.
So the question I have is, is this something we want to force onto the developers. What if there is a use case to interact with something outside of the user data folder for example?
| } | ||
|
|
||
| public void loadFromKeyStore(String keystorePassword, byte[] keyString) | ||
| throws GeneralSecurityException, IOException, Exception { |
There was a problem hiding this comment.
Does any of the methods called throw Exception? Otherwise it's best to be as specific as possible regarding the possible exceptions thrown.
There was a problem hiding this comment.
It does actually (and unfortunately). I'd have to go back through my notes on which. I agree, I hate putting Exception but in this case it actually just throws that.
There was a problem hiding this comment.
maybe for convenience we should catch it and throw a more specific exception to avoid that the users neet to catch Exception (which may cause SAT warnings about raw exception types?).
There was a problem hiding this comment.
I'll see if I can narrow down which specific line throws Exception. Not a bad idea.
| } | ||
| } | ||
|
|
||
| public KeyStore getKeyStore(String keystorePassword, byte[] keyString) |
There was a problem hiding this comment.
Is this for creating a new keystore? Would be better to name it accordingly
There was a problem hiding this comment.
Yes, this generates a new keystore. What would you suggest to adjust the name too? getKeyStore seemed clean.
There was a problem hiding this comment.
getSomething is usually used to get a field, createKeyStore would better reflect the purpose.
There was a problem hiding this comment.
That's fair. In my original thinking, since we were never actually storing a KeyStore, a get seemed like a good facade to the binding developer where we were returning it. I can change this easily enough.
|
|
||
| public void saveKeyStore(String keystoreFileName, String keystorePassword, byte[] keyString) | ||
| throws GeneralSecurityException, IOException, Exception { | ||
| FileOutputStream keystoreStream = new FileOutputStream(keystoreFileName); |
There was a problem hiding this comment.
If two bindings accidentally choose the same filename, they could overwrite each other's keystore.
There was a problem hiding this comment.
Unfortunately I'm not sure how to resolve that in this structure. In theory, the binding review process should catch accidental use of another binding name (or something generic like my.keystore). The ultimate goal is to create the next step of this which is effectively a KeyStore warehouse for OH which would handle all keys across all bindings securely and remove this issue.
There was a problem hiding this comment.
Would a binding need more than one keystore? Otherwise it might be best to just use the binding id? (Together with my other suggestion above)
There was a problem hiding this comment.
Absolutely. Even at the individual thing level yes. For example, ATV uses a mechanism that creates a MITM for purposes of debugs. I have to maintain a keystore for each side of that just for that connection. Also, ATV works with two different protocol stacks in the case of ShieldTV, each uses it's own KeyStore.
There was a problem hiding this comment.
Then this would probably have to be solved with an instruction on how to name the file (e.g bindingId + identifier) that's in the javadoc so it can be (more) easily checked when doing reviews
There was a problem hiding this comment.
Building on the discussion of "would a binding need more than one keystore"... technically they wouldn't.
You can store any number of keys in a keystore file. My suggestion would be that each binding have a single keystore file. Expose the keystore object itself for bindings which have complex/multi-key requirements and let them manage key naming themselves.
I'm also thinking of my work on zwave S2 security where every device has its own set of keys. Think it's much better to store those in a single keystore vs a hundred keystore files just for one binding.
This would allow for the use of binding id as keystore name (as suggested above) and resolve the filename collision issue
There was a problem hiding this comment.
I went down the road of a single keystore and there were several issues. First, working across things made it complex. Second, the CA path was different for each which added complexity (because they were self signed certs by the device). Ultimately, the work involved with making a single keystore wasn't worth it when maintaining multiple keystores was easy and kept a natural divide across the keys.
| Signature signer = Signature.getInstance("SHA256withRSA", "BC"); | ||
| signer.initSign(kp.getPrivate()); | ||
| signer.update("openhab".getBytes(StandardCharsets.UTF_8)); | ||
| signer.sign(); |
There was a problem hiding this comment.
EDIT: Let me get back to you on that. I think that's how I was originally signing the certs. It can probably come out.
There was a problem hiding this comment.
From what I understand of the JavaDoc, this only creates a signature for the string "openhab", and the returned signature gets discarded anyway.
There was a problem hiding this comment.
I think lines 308-310 is what signs the certificate.
There was a problem hiding this comment.
Yes, looking back this looks like an artifact I missed from a previous revision. I can remove it.
| keystore.store(keystoreStream, keystorePassword.toCharArray()); | ||
| } | ||
|
|
||
| private X509Certificate generateSelfSignedCertificate(KeyPair keyPair, String distName) |
There was a problem hiding this comment.
Why is distName passed as an argument instead of using the instance variable?
There was a problem hiding this comment.
I suppose I could pull that directly. It's only called once internally and when it's called it uses this.distName.
| this.privKey = encrypt(privKey, key); | ||
| } | ||
|
|
||
| public String getPrivKey(byte[] keyString) throws Exception { |
There was a problem hiding this comment.
Wouldn't it be better to have methods for sign/verify and encrypt/decrypt in this class instead of exposing the private key? Or what other use case would a binding developer have to use the key?
There was a problem hiding this comment.
I added those into this in the event someone had a use case where they needed to access their private keys. You're absolutely right, it should be held in the class IMHO. I don't use these functions in ATV. I was just trying to be complete by providing access should someone have a use case. I can pull them out if they aren't needed.
| public String encrypt(String data, Key key) throws Exception { | ||
| return encrypt(data, key, this.cipher); | ||
| } | ||
|
|
||
| public String encrypt(String data, Key key, String cipher) throws Exception { | ||
| byte[] dataInBytes = data.getBytes(); | ||
| Cipher encryptionCipher = this.encryptionCipher; | ||
| if (encryptionCipher != null) { | ||
| encryptionCipher.init(Cipher.ENCRYPT_MODE, key); | ||
| byte[] encryptedBytes = encryptionCipher.doFinal(dataInBytes); | ||
| return Base64.getEncoder().encodeToString(encryptedBytes); | ||
| } else { | ||
| return ""; | ||
| } | ||
| } | ||
|
|
||
| public String decrypt(String encryptedData, Key key) throws Exception { | ||
| return decrypt(encryptedData, key, this.cipher); | ||
| } | ||
|
|
||
| public String decrypt(String encryptedData, Key key, String cipher) throws Exception { | ||
| byte[] dataInBytes = Base64.getDecoder().decode(encryptedData); | ||
| Cipher decryptionCipher = Cipher.getInstance(cipher); | ||
| Cipher encryptionCipher = this.encryptionCipher; | ||
| if (encryptionCipher != null) { | ||
| GCMParameterSpec spec = new GCMParameterSpec(keySize, encryptionCipher.getIV()); | ||
| decryptionCipher.init(Cipher.DECRYPT_MODE, key, spec); | ||
| byte[] decryptedBytes = decryptionCipher.doFinal(dataInBytes); | ||
| return new String(decryptedBytes); | ||
| } else { | ||
| return ""; | ||
| } | ||
| } |
There was a problem hiding this comment.
I think it's a bit confusing to have a CryptoStore for managing public/private keys and certificates, but at the same time it has encrypt/decrypt functions using symmetric encryption. I don't really see the need for encrypting the keys in-memory (what threat is it supposed to protect against?), it just adds complexity for the binding developers.
There was a problem hiding this comment.
The ultimate goal was to provide a set of functions that are uniform to handle keys rather than have different implementations in the bindings. Different implementations could lead to security risks. Having a central location to handle the functions helps to prevent (notice I didn't say prevents definitively) bad implementations.
The purpose of encrypting the keys is for when we go to the next step of this. It was the though process that each binding would maintain a symmetric key for itself so that no other binding could access keys it wasn't supposed to. In theory, when OH starts and the warehouse is unlocked, that symmetric key would be released to the bindings in some secure manner (still working this part out). Then, when the binding wanted to access it's public/private keys it would ask the warehouse for them, but also have to provide a key to decrypt them properly on the way out. Just keeps everyone honest.
There was a problem hiding this comment.
Yes but no matter how many keys you use to encrypt other keys, one must be stored somewhere in plaintext in memory (unless you want to ask the user to input it every time the private key is needed) so if someone is in a position to retrieve the private key, they can just as easily get the symmetric key and then decrypt it.
I think your basic idea of a central warehouse is a good one, but as you say, cryptography is very hard to get right. I would be happy to continue discussing how it can be implemented in a good way.
There was a problem hiding this comment.
Agreed, and it's a step along the path. Long term goal is to utilize things like TPMs to store keys. I believe it will be a tiered mechanism that the user will ultimately decide. For example, we could (for basic users) have them store a password in a file that's read on start. For more advanced, we could query the user for that password on start to unlock the system. For expert users, maybe we go with TPMs to store everything. This is something I'm planning on dealing with in the second part of this. This simply establishes the foundation for it.
There was a problem hiding this comment.
But what is the actual threat here? Who is the attacker that would benefit from retrieving the key? What would they gain? The ability to control your TV? And if they can access memory on your OH server, they could probably do much more damage than that without using the key. Don't get me wrong, having a central credential storage would be a great addition to OH, but the cost of added complexity needs to be weighed against the actual risk.
There was a problem hiding this comment.
After reading a bit more on this, the KeyStore already is an implementation to store the keys securely (both in memory and on disk), so instead of implementing your own security solution, just retrieve the private key and certificate(s) from the KeyStore when needed and don't store it in your class' variables.
There was a problem hiding this comment.
I agree with pacive that encrypting keys in memory adds unnecessary complexity. It's a nice goal, but it's a chicken and egg situation. At some point, something has to be in cleartext which will unlock everything else.
The key isolation goal a very good one, but is very hard to achieve with shared code running on the same filesystem
There was a problem hiding this comment.
The concern here was a "rogue binding" more than anything else. For example, a marketplace binding. We didn't want to risk that binding being able to get a hold of credentials or something like that. Also, this is very much for the warehouse than really for this part of the implementation.
There was a problem hiding this comment.
Isn't that solved in an equally secure way by the password on the KeyStore? Also, @J-N-K had a proposal for isolating bindings from each other in this comment, which wouldn't complicate things for binding developers.
|
@pacive Thank you for the review! I believe I've replied to all of your notes above. Absolutely happy to keep talking about any if you don't feel the answer is solid. As I mentioned above, the goal here is two fold.
|
|
I played around a bit with your PR and wrote a simple test as well. |
|
|
||
| == Source Code | ||
|
|
||
| https://github.com/openhab/openhab-core |
There was a problem hiding this comment.
| https://github.com/openhab/openhab-core | |
| https://github.com/openhab/openhab-core | |
| == Third-party Content | |
| bouncy castle | |
| * License: MIT license | |
| * Project: https://www.bouncycastle.org/ | |
| * Source: https://www.bouncycastle.org/latest_releases.html |
| @@ -18,6 +18,7 @@ | |||
|
|
|||
| <properties> | |||
| <californium.version>2.7.4</californium.version> | |||
| <bouncycastle.version>1.75</bouncycastle.version> | |||
There was a problem hiding this comment.
| <bouncycastle.version>1.75</bouncycastle.version> | |
| <bouncycastle.version>1.77</bouncycastle.version> |
could we switch to 1.77 - or is there a reason to stick with 1.75 for now?
There was a problem hiding this comment.
Yes, we can switch. 1.77 came out after my last push I believe.
| public void loadFromKeyStore(String keystorePassword, byte[] keyString) | ||
| throws GeneralSecurityException, IOException, Exception { | ||
| Key key = convertByteToKey(keyString); | ||
| KeyStore keystore = KeyStore.getInstance("JKS"); |
There was a problem hiding this comment.
Couple of suggestions with regards to keystore type:
- Change from JKS format (which can only store asymmetric keys) to "JCEKS" which can store both asym and symmetric keys.
- Write the keystore type to a file on the filesystem, perhaps alongside the keystore itself. This allows for:
a. migration to a different keystore type in the future if necessary. (There's some drama in the keystore space - Oracle recently change their default keystore from JKS to PCKS12, and JKS seems to be deprecated at this point)
b. There may be cases where a FIPS compliant keystore is required which would warrant a different keystore type. I don't think we need to go all the way down that rabbit hole just yet, but it would be good to have some scaffolding in place
There was a problem hiding this comment.
What would be the use case for storing symmetric keys? In general those are generated as needed for things like IPSEC connections.
I am tracking the Oracle JKS/PKCS12. We could migrate to PKCS12 if we wanted to. I had some issues with it early on and JKS just worked.
There was a problem hiding this comment.
The openhab binding has symmetric (AES) keys for secure communication with the devices. Looks like PKCS12 has basic support for symmetric (AES) keys.
There was a problem hiding this comment.
The AES bits there are just for storing the private key in the memory. It's not used at all for interaction with the device.
There was a problem hiding this comment.
You are referring to the AES key in the CryptoStore. I am referring to the AES keys in the zwave binding. If the CryptoStore is going to be adopted, it needs to support storage of symmetric keys. JKS does not allow that and JKS format is basically deprecated by Oracle.
Use PCKS12 or JCEKS instead. Use <keystore_name>.pkcs12 or <keykstore_name>.jceks so the format is obvious.. in case it needs to change in the future. Best to change the default now instead of just creating a problem to have to solve later
|
We could add a new interface |
| return decrypt(encryptedData, key, this.cipher); | ||
| } | ||
|
|
||
| public String decrypt(String encryptedData, Key key, String cipher) throws Exception { |
There was a problem hiding this comment.
You should be able to change throws Exception to throws GeneralSecurityException. Same for the other occurances
| Cipher decryptionCipher = Cipher.getInstance(cipher); | ||
| Cipher encryptionCipher = this.encryptionCipher; | ||
| if (encryptionCipher != null) { | ||
| GCMParameterSpec spec = new GCMParameterSpec(keySize, encryptionCipher.getIV()); |
There was a problem hiding this comment.
you are using a GCMParameterSpec object here but I don't see one used in the encrypt method. You should probably just remove it from here as the cipher you selected has NoPadding, so IV is irrelevant. Also, invoking encryptionCipher.getIV probably just returns an empty byte array
|
@morph166955 I'll wait with a review until you discussions with @dbadia are finished. I'm not an expert on the crypto-parts, so it's probably better to discuss those issues first. |
Implements (#3289)
Creates the basis for a centralized crypto storage mechanism. Also creates several functions that are used across addon's to simplify addon creation.
Basic usage expectation:
CryptoStore myCryptoStore = new CryptoStore();
Key cryptoStoreKey = CryptoStore.generateEncryptionKey();
cryptoStoreKey is an AES encryption key used to encrypt the private key while in memory. This must be passed to any function which handles the private key.
Option 1) User has private key and certificate to provide (e.g. something provided by a device)
myCryptoStore.setPrivKey(String myPrivKey, cryptoStoreKey);
myCryptoStore.setCert(String cert); OR .setCert(Certificate cert);
Option 2) User needs a new keypair created.
myCryptoStore.generateNewKeyPair(cryptoStoreKey);
Option 3) User already has a java KeyStore stored on disk and imports it.
myCryptoStore.loadFromKeyStore(String keystoreFileName, String keystorePassword, cryptoStoreKey);
User can request a KeyStore via getKeyStore(String keystorePassword, cryptoStoreKey);
User can save the CryptoStore to a java KeyStore for persistence via loadFromKeyStore(String keystoreFileName, String keystorePassword, cryptoStoreKey);
Several other supporting functions exist and will be documented prior to merge.
This is the first part of the centralized crypto pieces. Part two will be the creation of a centralized storage location. Centralized crypto will store the private key in it's encrypted format as well as the certificate. It will also store an encrypted copy of the cryptoStoreKey. Both the store and key will be recovered by the add-on as needed for operation.
Users can also call encrypt(String data, Key key) or decrypt(String encryptedData, Key key) if they want to simply encrypt/decrypt strings for use. Key can be the cryptoStoreKey or a separate key as needed.