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

[Sebastien Blanc]

Hi Bruno !
Cool stuff some comments inline


On Wed, Apr 16, 2014 at 7:06 AM, 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


I tried out your branch and created some applications.
This is what I see now when browsing to my app details, is this the valid
ID/Secret pair that the application owner / administrator can use to send
psuh notifications ?

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


Nice improvement

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

Make sense


>
> 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?
>
>
> --
> abstractj
>
>
>
>
> _______________________________________________
> aerogear-dev mailing list
> aerogear-dev at lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/aerogear-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.jboss.org/pipermail/aerogear-dev/attachments/20140416/ece5f37b/attachment.html