[aerogear-dev] Push server...master secrets, secrets and some refactoring proposal

[Karel Piwko]

Good stuff. Comments inline:

On Wed, 16 Apr 2014 02:06:35 -0300
Bruno Oliveira <bruno at abstractj.com> wrote:

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

Sorry my ignorance, does it mean that if I restart application server or
redeploy UPS, master secret will be changed? 

For master secret, that's not that big concern, I believe. People just need to
grab master secret from UPS before adding variants from CLI.

But if variant secrets are recomputed as well, all existing application
installations will cease to work!

> 
> 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/a9993c35edc39501eef7d7a26f3d8ea29b38e293#diff-dc07653bd3c83d4e82c45b70d8ad34dfR47
> 

+1

> 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(); 
> ...
> }

+1 to this alternative. @Embedded and (@Embeddable) is a JPA specific stuff -
might be more complicated to migrate to NoSQL DB or to other JPA provides
(EclipseLink) in future.

> 
> 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/a9993c35edc39501eef7d7a26f3d8ea29b38e293)
> between both classes.
> 
> Wdyt?
> 
>