Geronimo
  1. Geronimo
  2. GERONIMO-3003

Encrypt password strings in deployment plans

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: Wish List
    • Fix Version/s: 2.1.5, 2.2.1, 3.0.0
    • Component/s: deployment
    • Security Level: public (Regular issues)
    • Labels:
      None

      Description

      Geronimo currently has a feature where password strings in the config.xml get encrypted using the org.apache.geronimo.util.EncryptionManager. This encryption is performed in the org.apache.geronimo.system.configuration.GBeanOverride class.

      It would be desirable to have the same encryption applied to the password strings in deployment plans (e.g. datasource or JMS deployment plans within an EAR). Even though the plans are only used during the deployment process, and not at runtime, the plans are left with plaintext password strings sitting in them. It would be nice if the deployment process could internally encrypt the strings and then write back out the deployment plan to the file system. Also, this means that the deployment process will require the ability to decrypt strings that are already in encrypted format in the plan (in the case of redeployment, for example).

      More discussion of this feature can be found in the following mailing list thread:

      http://www.mail-archive.com/user@geronimo.apache.org/msg05859.html

      I would suggest that an appropriate spot to perform the encryption is in the org.apache.geronimo.j2ee.deployment.EARConfigBuilder class, perhaps in the following code just before the file is written to a temporary file:


      if (gerModule.isSetAltDd()) {
      // the the url of the alt dd
      try

      { altVendorDDs.put(path, DeploymentUtil.toTempFile(earFile, gerModule.getAltDd().getStringValue())); }

      catch (IOException e)

      { throw new DeploymentException("Invalid alt vendor dd url: " + gerModule.getAltDd().getStringValue(), e); }

      However, somebody more familiar with the design might be able to suggest a better solution.

      1. GERONIMO-3003.patch
        47 kB
        Jack Cai
      2. GERONIMO-3003_21.patch
        38 kB
        Jack Cai
      3. GERONIMO-3003-2.2-2.patch
        68 kB
        David Jencks
      4. GERONIMO-3003.cmd.21.patch
        12 kB
        Jack Cai
      5. GERONIMO-3003.cmd.22.patch
        12 kB
        Jack Cai
      6. GERONIMO-3003.gshell.cmd 30.patch
        2 kB
        Vanessa Wang

        Activity

        Hide
        Aman Nanner added a comment -

        I am out of the office this week, and returning Monday, May 18th

        Show
        Aman Nanner added a comment - I am out of the office this week, and returning Monday, May 18th
        Hide
        Jack Cai added a comment -

        Creating a patch for the trunk. If it is accepted well, I'll create a patch for 2.1 branch too.

        The current design is -
        1. Developers can specify a String type attribute of a GBean as "encrypted", either through API or annotation. Encrypted attributes are encrypted when marshalled, e.g., serialized to config.ser. It will also get encrypted when saved into the server's config.xml.

        2. By default, GBean attributes are not encrypted except those whose name contains the string "password" (ignore case) and whose type is java.lang.String.

        Since the patch is pretty big and affects quite a few files, please help to review and commit it soon. I hope it will make into G2.2 release. Thanks!

        Show
        Jack Cai added a comment - Creating a patch for the trunk. If it is accepted well, I'll create a patch for 2.1 branch too. The current design is - 1. Developers can specify a String type attribute of a GBean as "encrypted", either through API or annotation. Encrypted attributes are encrypted when marshalled, e.g., serialized to config.ser. It will also get encrypted when saved into the server's config.xml. 2. By default, GBean attributes are not encrypted except those whose name contains the string "password" (ignore case) and whose type is java.lang.String. Since the patch is pretty big and affects quite a few files, please help to review and commit it soon. I hope it will make into G2.2 release. Thanks!
        Hide
        Aman Nanner added a comment -

        I am out of the office this week, and returning Tuesday, July 28th.

        Show
        Aman Nanner added a comment - I am out of the office this week, and returning Tuesday, July 28th.
        Hide
        Jack Cai added a comment -

        Created a patch for 2.1 branch.

        I did both a few unit testing and manual testing, for both 2.2 and 2.1. All looks good.

        Please help to review and commit. Thanks!

        Show
        Jack Cai added a comment - Created a patch for 2.1 branch. I did both a few unit testing and manual testing, for both 2.2 and 2.1. All looks good. Please help to review and commit. Thanks!
        Hide
        Ivan added a comment -

        The patch looks good to me, if no objection, I will first try to apply it to 2.1 branch.

        Show
        Ivan added a comment - The patch looks good to me, if no objection, I will first try to apply it to 2.1 branch.
        Hide
        Kevan Miller added a comment -

        I looked at the 2.1 patch. Looks pretty good and build/tests seem to be working well.

        David J, maybe you could have a look at the patch for trunk? I think this would be a good feature for 2.2.

        Show
        Kevan Miller added a comment - I looked at the 2.1 patch. Looks pretty good and build/tests seem to be working well. David J, maybe you could have a look at the patch for trunk? I think this would be a good feature for 2.2.
        Hide
        David Jencks added a comment -

        I reworked the 2.2. patch extensively to

        • use the command pattern to put the encrypt/decrypt login in the EncryptionSession enumeration
        • store the encrypted values in the GBeanOverride and only encrypt/decrypt when we deal with a GBeanData object. This means we always have the GAttibuteInfo available that tells us what to do.

        I plan to review this again tonight before committing it.

        Show
        David Jencks added a comment - I reworked the 2.2. patch extensively to use the command pattern to put the encrypt/decrypt login in the EncryptionSession enumeration store the encrypted values in the GBeanOverride and only encrypt/decrypt when we deal with a GBeanData object. This means we always have the GAttibuteInfo available that tells us what to do. I plan to review this again tonight before committing it.
        Hide
        Jack Cai added a comment -

        Thanks Ivan, Kevan and David!

        I think it makes good sense to simply store encrypted values in GBeanOverride. I reviewed David's revised patch and suggest to add the below lines to the patch so that GBeanOverride.applyOverride() will encrypt attribute values which are not encrypted in config.xml but should be encrypted. This is to handle scenarios in which users change a password in config.xml and want it get encrypted when the server writes back to config.xml.

        @@ -357,6 +402,19 @@
                     }
                     String valueString = entry.getValue();
                     Object value = getValue(attributeInfo, valueString, configName, gbeanName, classLoader);
        +            /*
        +             * Make sure encrypted attributes are encrypted even if they are in
        +             * plain text originally.
        +             */
        +            if (valueString != null
        +                    && attributeInfo.getEncryptedSetting() == EncryptionSetting.ENCRYPTED) {
        +                String decryptedValue = (String) attributeInfo
        +                        .getEncryptedSetting().decrypt(valueString);
        +                if (valueString.equals(decryptedValue)) {
        +                    entry.setValue(attributeInfo.getEncryptedSetting().encrypt(
        +                            valueString));
        +                }
        +            }
                     data.setAttribute(attributeName, value);
                 }
         
        
        Show
        Jack Cai added a comment - Thanks Ivan, Kevan and David! I think it makes good sense to simply store encrypted values in GBeanOverride. I reviewed David's revised patch and suggest to add the below lines to the patch so that GBeanOverride.applyOverride() will encrypt attribute values which are not encrypted in config.xml but should be encrypted. This is to handle scenarios in which users change a password in config.xml and want it get encrypted when the server writes back to config.xml. @@ -357,6 +402,19 @@ } String valueString = entry.getValue(); Object value = getValue(attributeInfo, valueString, configName, gbeanName, classLoader); + /* + * Make sure encrypted attributes are encrypted even if they are in + * plain text originally. + */ + if (valueString != null + && attributeInfo.getEncryptedSetting() == EncryptionSetting.ENCRYPTED) { + String decryptedValue = ( String ) attributeInfo + .getEncryptedSetting().decrypt(valueString); + if (valueString.equals(decryptedValue)) { + entry.setValue(attributeInfo.getEncryptedSetting().encrypt( + valueString)); + } + } data.setAttribute(attributeName, value); }
        Hide
        David Jencks added a comment -

        I applied my revised patch in rev 798794 before I saw Jac's suggestion. I think the use case there is a good idea, and I don't see a better way to implement it. I wish we didn't have to rely on encrypt and decrupt being idempotent.

        If no one gets there first I'll apply jack's suggestion in the morning.

        Show
        David Jencks added a comment - I applied my revised patch in rev 798794 before I saw Jac's suggestion. I think the use case there is a good idea, and I don't see a better way to implement it. I wish we didn't have to rely on encrypt and decrupt being idempotent. If no one gets there first I'll apply jack's suggestion in the morning.
        Hide
        Jack Cai added a comment -

        David, looks like you forgot to commit EncryptionSetting.java.

        Show
        Jack Cai added a comment - David, looks like you forgot to commit EncryptionSetting.java.
        Hide
        David Jencks added a comment -

        As several people noiced I left out a bunch of stuff in the first commit.

        Additional files from first commit in rev.799023

        Modified version of Jacks' suggestion above in rev 799037.

        I think this is all we need for trunk. I would advise using jack's patch for 2.1 directly but if someone wants to try to port my changes it's fine with me.

        Show
        David Jencks added a comment - As several people noiced I left out a bunch of stuff in the first commit. Additional files from first commit in rev.799023 Modified version of Jacks' suggestion above in rev 799037. I think this is all we need for trunk. I would advise using jack's patch for 2.1 directly but if someone wants to try to port my changes it's fine with me.
        Hide
        Ivan added a comment -

        Commit the patch to 2.1 At revision: 889880.

        Show
        Ivan added a comment - Commit the patch to 2.1 At revision: 889880.
        Hide
        Jack Cai added a comment -

        In addition to what is already done for this requirement, guess the original requirement raised in this JIRA is not resolved. However, instead of doing what is proposed originally (rewrite the deployment plan to encrypt the password), another way to achieve the security goal is to -

        1) accept encrypted passwords in deployment plan, and
        2) provide an encrypt utility so users could generate encrypted strings that could then be pasted into the deployment plan

        Comments?

        Show
        Jack Cai added a comment - In addition to what is already done for this requirement, guess the original requirement raised in this JIRA is not resolved. However, instead of doing what is proposed originally (rewrite the deployment plan to encrypt the password), another way to achieve the security goal is to - 1) accept encrypted passwords in deployment plan, and 2) provide an encrypt utility so users could generate encrypted strings that could then be pasted into the deployment plan Comments?
        Hide
        Jack Cai added a comment -

        I believe Requirement (1) is already done in the previous patch. The TODO is to provide a utility to generate encrypted strings. A simple way to do this is to let the user optionally specify a key file and then generate the string, or use the default key if no key file is provided. A more robust way is to call the running server to do the encryption, e.g., extend the org.apache.geronimo.system.util.ConfiguredEncryption GBean to provide such operation. I'd prefer the latter solution. If no objection, I'll create a patch.

        Show
        Jack Cai added a comment - I believe Requirement (1) is already done in the previous patch. The TODO is to provide a utility to generate encrypted strings. A simple way to do this is to let the user optionally specify a key file and then generate the string, or use the default key if no key file is provided. A more robust way is to call the running server to do the encryption, e.g., extend the org.apache.geronimo.system.util.ConfiguredEncryption GBean to provide such operation. I'd prefer the latter solution. If no objection, I'll create a patch.
        Hide
        Jack Cai added a comment -

        Attaching the patch to add a command to the deploy commandline to do an encryption. Typical usage:

        Use a running server to do the encryption so that the encryption setting of that server will be used:
        deploy(.sh|.bat) -h localhost -p 1099 encrypt sometext

        Use the common simple encrption so that no running server is required
        deploy(.sh|.bat) -o encrypt sometext

        Tested with 2.2 branch.

        Show
        Jack Cai added a comment - Attaching the patch to add a command to the deploy commandline to do an encryption. Typical usage: Use a running server to do the encryption so that the encryption setting of that server will be used: deploy(.sh|.bat) -h localhost -p 1099 encrypt sometext Use the common simple encrption so that no running server is required deploy(.sh|.bat) -o encrypt sometext Tested with 2.2 branch.
        Hide
        Jack Cai added a comment -

        Add a bit error check code and tested with 2.1 branch.

        Show
        Jack Cai added a comment - Add a bit error check code and tested with 2.1 branch.
        Hide
        Jack Cai added a comment -

        Commited the encrypt command patch to 2.1 branch at Rev 896103 and 2.2 branch at Rev 896316.

        TODO:
        1. Trunk update
        2. GShell equivalent
        3. Doc update

        Show
        Jack Cai added a comment - Commited the encrypt command patch to 2.1 branch at Rev 896103 and 2.2 branch at Rev 896316. TODO: 1. Trunk update 2. GShell equivalent 3. Doc update
        Hide
        Vanessa Wang added a comment -

        GShell Command patch for EncryptCommand in Geronimo 3.0

        Show
        Vanessa Wang added a comment - GShell Command patch for EncryptCommand in Geronimo 3.0
        Hide
        Shawn Jiang added a comment -

        1, Migrated encrypt command to trunk.
        2, committed the patch from Vanessa to add GShell equivalent for karaf commands.

        TODO:

        1, testing
        2, doc update.

        Show
        Shawn Jiang added a comment - 1, Migrated encrypt command to trunk. 2, committed the patch from Vanessa to add GShell equivalent for karaf commands. TODO: 1, testing 2, doc update.
        Hide
        Shawn Jiang added a comment -

        This should be resloved after applying the patch from Venessa Wang.

        Show
        Shawn Jiang added a comment - This should be resloved after applying the patch from Venessa Wang.
        Hide
        Rex Wang added a comment -

        closing it

        Show
        Rex Wang added a comment - closing it

          People

          • Assignee:
            Shawn Jiang
            Reporter:
            Aman Nanner
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development