geronimo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jack Cai <greensi...@gmail.com>
Subject Re: Encrypt password in deployment plans and config.ser
Date Wed, 13 May 2009 08:04:38 GMT
Thanks Jarek for your comments! I should have provided more description
about the fix -

1. setAttribute() is usually called by the unmarshaller, so it should
actually do the decryption. Since the decryption will automatically bypass
unecrypted values, it won't do any harm if the passed-in attribute value is
not encrypted.

2. getAttribute() is usually called by various consumers that wants to read
the real value (decrypted), so it just returns what is stored in the
GBeanData (already decrypted value).

3. The current patch does do encryption for writeExternal(). The reason
decryption is not done for readExternal() directly is because readExternal()
calls setAttribute() which does the decryption.

I agree that it's not optimal to hard-code to encrypt "password" attributes,
and better to use the GAttributeInfo to make this configurable. My
suggestion is to set encryption on for all attributes whose name contains
"password" and off for the rest. We can thus eliminate the hard-code logic
in GBeanOverride too (for encrption in config.xml).

I'm going to create a patch if no objection.

-Jack

2009/5/13 Jarek Gawor <jgawor@gmail.com>

> Jack,
>
> I was thinking about this a while ago and had a slightly different
> approach. I was thinking of adding a 'protected' or 'encrypted'
> attribute to the GAttributeInfo (for example, just like the
> 'persistent' attribute) to indicate that this attribute should be
> "encrypted" in the serialized file. I wanted to allow for any
> attribute to be encrypted even if it doesn't have the "password" name
> in it. And I think with this way we could also expose that setting via
> our GBean annotations.
>
> Btw, hmm... shouldn't encrypt be called in setAttribute() and decrypt
> in getAttribute() or at least encrypt in writeExternal() and decrypt
> in readExternal() ?
>
> Jarek
>
> On Tue, May 12, 2009 at 4:06 AM, Jack Cai <greensight@gmail.com> wrote:
> > Recently someone pointed out to us that password specified in the
> deployment
> > plan is stored in clear text in the config.ser after deployment, for
> > example, when deploying a datasource with the database password specified
> in
> > the deployment plan. I notice that there was another user mentioning
> exactly
> > the same problem in the geronimo-user list two years ago [1].
> >
> > I did a little more dig and also found this JIRA [2] along with this
> > discussion [3] on encrypting passwords in deployment plans.
> >
> > I understand that there are different arguments on what is "real"
> security.
> > But I also well appreciate users' concerns on having clear text password
> > appearing in the system. In China, we have a saying "guard against the
> good
> > guys but not the bad guys", meaning the guard is there just to prevent
> the
> > good guys from doing bad. Taking the same example as in the old thread
> [3],
> > if we lock a bicycle, then the good guys won't steal it, while the bad
> guys
> > with the intention to steal it can still find ways to steal it. But if we
> > leave the bicycle unlocked, then the good guys are tempted to steal the
> > bicycle too, because it's so easy.
> >
> > Back to JIRA [2], I think an alternative is to let user input the
> password
> > in encrypted form in the deployment plan at the very beginning. We can
> > provide a small command line tool to let user ecrypt the password
> > beforehands. If this is acceptable, then there is a very simple way to
> > satisfy requirement [1] & [2]. We can simply add a little encryption
> logic
> > in the class org.apache.geronimo.gbean.GBeanData [4], similar to what we
> did
> > in GBeanOverride for config.xml.
> >
> > Comments are welcome.
> >
> > -Jack
> >
> >
> > [1]
> http://www.nabble.com/plaintext-password-in-config.ser-to9834211.html
> > [2] http://issues.apache.org/jira/browse/GERONIMO-3003
> > [3]
> >
> http://www.nabble.com/Plaintext-passwords-in-Geronimo-plans-and-config-files-td9100565s134.html
> > [4]
> > Index:
> >
> D:/Dev/s/wasce_v2.1.0.1/server/framework/modules/geronimo-kernel/src/main/java/org/apache/geronimo/gbean/GBeanData.java
> > ===================================================================
> > ---
> >
> D:/Dev/s/wasce_v2.1.0.1/server/framework/modules/geronimo-kernel/src/main/java/org/apache/geronimo/gbean/GBeanData.java
> > (revision 111111)
> > +++
> >
> D:/Dev/s/wasce_v2.1.0.1/server/framework/modules/geronimo-kernel/src/main/java/org/apache/geronimo/gbean/GBeanData.java
> > (working copy)
> > @@ -27,6 +27,8 @@
> >  import java.util.Map;
> >  import java.util.Set;
> >
> > +import org.apache.geronimo.crypto.EncryptionManager;
> > +
> >  /**
> >   * @version $Rev: 556119 $ $Date: 2007-07-13 15:34:02 -0400 (Fri, 13 Jul
> > 2007) $
> >   */
> > @@ -112,6 +114,10 @@
> >      }
> >
> >      public void setAttribute(String name, Object value) {
> > +        if (name.toLowerCase().indexOf("password") > -1
> > +                && value instanceof String) {
> > +            value = EncryptionManager.decrypt((String) value);
> > +        }
> >          attributes.put(name, value);
> >      }
> >
> > @@ -207,6 +213,10 @@
> >          for (Map.Entry<String, Object> entry : attributes.entrySet()) {
> >              String name = entry.getKey();
> >              Object value = entry.getValue();
> > +            if (name.toLowerCase().indexOf("password") > -1
> > +                    && value instanceof String) {
> > +                value = EncryptionManager.encrypt((String) value);
> > +            }
> >              try {
> >                  out.writeObject(name);
> >                  out.writeObject(value);
> >
> >
> >
>

Mime
View raw message