Good morning, while I was working on "that" passphrase encryption thing, I found some improvements for UPS. But would like to discuss before. 1. Currently UPS stores master secret and secrets (from Variants) into the database. IMO this information should never be persisted into the database and such kind of information only belongs to the owner of that application. How? https://github.com/abstractj/aerogear-unifiedpush-server/tree/master-secret 2. Master secret and Secret make use of UUID from Java, not truly a PRNG once some attributes are predictable, you are not collision free. "WTF are you saying Bruno!?". From JDK 7: |public static UUID randomUUID() { SecureRandom ng = numberGenerator; if (ng == null) { numberGenerator = ng = new SecureRandom(); } byte[] randomBytes = new byte[16]; ng.nextBytes(randomBytes); randomBytes[6] &= 0x0f; /* clear version */ randomBytes[6] |= 0x40; /* set to version 4 */ randomBytes[8] &= 0x3f; /* clear variant */ randomBytes[8] |= 0x80; /* set to IETF variant */ return new UUID(randomBytes); }| Solution: https://github.com/abstractj/aerogear-unifiedpush-server/commit/a9993c35e... 3. I noticed the fact that PushApplication and Variant have some duplicated attributes with different names. public abstract class Variant extends BaseModel { private String variantID = UUID.randomUUID().toString(); private String secret = UUID.randomUUID().toString(); .... } public class PushApplication extends BaseModel { private String pushApplicationID = UUID.randomUUID().toString(); private String masterSecret = UUID.randomUUID().toString(); ... } I can see two alternatives to avoid it: - Move these attributes to an |Embeddable| class, something like: |@Embeddable *public class ApplicationKey*{ | private String id = UUID.randomUUID().toString(); private String secret = UUID.randomUUID().toString(); | }| public abstract class Variant extends BaseModel { | @Embedded @AttributeOverrides( { @AttributeOverride(name="variantID")), @AttributeOverride(name="secret")) }) *private ApplicationKey*key;| .... } public class PushApplication extends BaseModel { |@Embedded @AttributeOverrides( { @AttributeOverride(name="pushApplicationID")), @AttributeOverride(name="masterSecret")) }) *private ApplicationKey*key;| ... } - Second alternative (more simple) pull up the attributes to BaseModel public abstract class BaseModel implements Serializable { private String id = UUID.randomUUID().toString(); private String secret = UUID.randomUUID().toString(); ... } Why this is important? 1. Avoid sensitive data to be stored into the database, make use of KDF functions to store secrets, master secrets.... 2. Avoid code duplication. With the inclusion of encryption for passphrases and other kind of sensitive data, I pretty much have to duplicate code like this (https://github.com/abstractj/aerogear-unifiedpush-server/commit/a9993c35e...) between both classes. Wdyt? -- abstractj