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