-
Notifications
You must be signed in to change notification settings - Fork 305
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
PAYARA-3324 APPSERV-16 APPSERV-23 Allow Encryption of Web Session Data and SFSBs Stored in Hazelcast #4433
PAYARA-3324 APPSERV-16 APPSERV-23 Allow Encryption of Web Session Data and SFSBs Stored in Hazelcast #4433
Conversation
Jenkins test please |
Something I hadn't tested before creating this PR: what happens if you change the master password after generating the key? Answer: it breaks the key and you get Hazelcast exceptions on startup (but doesn't shut down the server or prevent joining the data grid) |
I'll add a message to the "change-master-password" command that informs the user that they'll need to generate a new key - I can't change the key on the fly. I'll also make it so the server fails to start if it can't read the key during initialisation. |
jenkins test please |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As you are still working on some details I submit some comments as a round 1 of reviewing the PR.
I did run your tests and they work (on linux).
Once I got
[ERROR] fish.payara.samples.datagridencryption.sfsb.SfsbEncryptionTest Time elapsed: 35.02 s <<< ERROR!
javax.ws.rs.ProcessingException: Failed to write asset to output: /WEB-INF/classes/fish/payara/samples/datagridencryption/sfsb/SfsbEncryptionTest.class
Caused by: org.jboss.shrinkwrap.api.exporter.ArchiveExportException: Failed to write asset to output: /WEB-INF/classes/fish/payara/samples/datagridencryption/sfsb/SfsbEncryptionTest.class
Caused by: java.lang.IllegalArgumentException: fish/payara/samples/datagridencryption/sfsb/SfsbEncryptionTest.class not found in classloader sun.misc.Launcher$AppClassLoader@7f31245a
but I think this is an arquillian/test runner issue.
I did not test the feature manually. Will do that later or in connection with round 2 of the review.
...a/ha-hazelcast-store/src/main/java/fish/payara/ha/hazelcast/store/HazelcastBackingStore.java
Outdated
Show resolved
Hide resolved
...-passivation/src/main/java/fish/payara/samples/datagridencryption/sfsb/TestEjbEndpoints.java
Outdated
Show resolved
Hide resolved
...rver/tests/payara-samples/test-utils/src/main/java/fish/payara/samples/ServerOperations.java
Outdated
Show resolved
Hide resolved
...mt/src/main/java/com/sun/enterprise/admin/servermgmt/cli/ChangeMasterPasswordCommandDAS.java
Outdated
Show resolved
Hide resolved
.../admin/server-mgmt/src/main/java/fish/payara/admin/servermgmt/cli/GenerateEncryptionKey.java
Show resolved
Hide resolved
...ast-bootstrap/src/main/java/fish/payara/nucleus/hazelcast/encryption/SymmetricEncryptor.java
Outdated
Show resolved
Hide resolved
.../main/java/fish/payara/nucleus/hazelcast/encryption/PayaraHazelcastEncryptedValueHolder.java
Outdated
Show resolved
Hide resolved
...ast-bootstrap/src/main/java/fish/payara/nucleus/hazelcast/encryption/SymmetricEncryptor.java
Outdated
Show resolved
Hide resolved
...ast-bootstrap/src/main/java/fish/payara/nucleus/hazelcast/encryption/SymmetricEncryptor.java
Outdated
Show resolved
Hide resolved
...yara-modules/hazelcast-bootstrap/src/main/java/fish/payara/nucleus/store/ClusteredStore.java
Show resolved
Hide resolved
...a-modules/hazelcast-bootstrap/src/main/java/fish/payara/nucleus/hazelcast/HazelcastCore.java
Outdated
Show resolved
Hide resolved
Had a second review. Main question mark for me is the use of salt in the actual encryption. This is more a question for @rdebusscher . I'm still not sure about multi-threading safety for fields in HZ core service but if you think this is fine I am happy with that. Not sure if I get to running the manual testing today. Otherwise it will be on Thursday when I am back. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some small improvements suggested
|
||
private byte[] generateAndEncryptKey(char[] masterpasswordChars) throws CommandException { | ||
byte[] saltBytes = new byte[20]; | ||
random.nextBytes(saltBytes); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the salt is random there is no way to redo this even if we would know the master password. With random salt I assume there is no way to verify that an encryption key belongs to a certain master password?
@jbee Correct, but we also store the salt when we need the symmetric key. This implementation is correct. Lets recap the process.
- We need a Symmetric Key for the encryption with the Data Grid (call it DataGridSymKey).
- This key needs to be stored on disk because it needs to be transferred to to other nodes/instances. So we need to encrypt it when stored on disk.
- We need to encrypt it with something user/domain specific which is already safely stored, like the master password
- Password are bad for encryption because they are mostly too short, hence the use of the Key Derivation function.
- PBKDF needs a salt and password to generate an enhanced 'password' (which is again a Symmetric Key EncryptionSymKey, which can be used to encrypt the DataGridSymKey.
- When decoding to retrieve the DataGridSymKey, we need the EncryptionSymKey. This can be generated again using the Master password and the salt.
- That is the reason why the salt is also part of the output (see line 165 and next)
So the salt is used to generate a unique key, although 2 systems have the same master password, the key is different .
if (attribute == null) { | ||
return false; | ||
} | ||
return Boolean.valueOf(attribute.getValue()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With Boolean.parseBoolean
there is no autoboxing in the code.
@Override | ||
public <K extends Serializable, V extends Serializable> BackingStore<K, V> createBackingStore(BackingStoreConfiguration<K, V> bsc) throws BackingStoreException { | ||
return new HazelcastBackingStore<K, V>(this, bsc.getStoreName(), core); | ||
return new HazelcastBackingStore<K, V>(this, bsc.getStoreName(), clusteredStore); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Explicit types not needed.
throw new HazelcastException("Error reading encrypted key", ioe); | ||
} | ||
|
||
if (encryptedBytes == null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
encryptedBytes is never null as Files.readAllBytes
never returns null.
Remove unnecessary autoboxing and null check, and use diamond operator
Jenkins test please |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested successful manually as well as running provided tests locally.
Only point I already mentioned in chat is that user feedback on active encryption in form of a log entry at startup and maybe something in the admin-console would be nice.
Jenkins test please |
Description
This is a new feature to replace the encryption that used to be provided by Shoal (GMS).
In short, this is done by generating a symmetric encryption key, propagating it to each instance in the domain (in the same manner as the keystore), and changing the Hazelcast backing store used by the web and ejb containers to use the clustered store service (rather than accessing Hazelcast directly themselves) which checks whether or not it should encrypt/decode the values it stores/retrieves respectively (keys are not encrypted).
Values are stored in a new value-holder class to prevent Hazelcast jumping in front with its own non-overridable serialiser.
This PR brings in a new Asadmin CLI command,
generate-encryption-key
, which requires the master password and the domain to not be running. Encryption is enabled using theset-hazelcast-configuration
command with a new option--encryptdatagrid
: this only applies after a restart, regardless of whether or not--dynamic
was specified. This is also a domain-wide option, it is not config specific.Testing
New tests
New test added under
appserver/tests/payara-samples/samples/datagrid-encryption/sfsb-passivation
.This test has a very simple EJB that stores data, which is poked a number of times to store data. The bean is then passivated by spamming a lookup of 1200 EJBs (default cache size is 512) before requesting it again to check if the data is still there and correct.
As part of the test set-up it generates a symmetric key, enables encryption, and restarts the domain to apply the changes - these changes are removed once the test has completed.
Testing Performed
The above test, plus essentially these two blogs:
Configuring Sticky Sessions for Payara Server with Apache Web Server & Session Replication in Payara Server with Hazelcast
Datagrid encryption was enabled before adding session data to the clusterjsp application noted in the blog. One instance was then killed to force failover, before starting it back up again and killing the other to force a second failover (checking that the data was correct on each instance between failovers).
This is a bit tricky to reliably automate in a portable fashion!
I also manually tested that the changes on whether encryption should or should not occur are only applied after a restart.
Test suites executed
Testing Environment
Windows 10 64-bit, Zulu JDK 8u212.
Documentation
https://github.com/payara/Payara-Server-Documentation/pull/696