Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Won't Fix
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: Java
    • Security Level: Public (Public issues, viewable by everyone)
    • Labels:
      None
    • Environment:
      Operating System: Windows NT
      Platform: PC

      Description

      Created an attachment (id=25614)
      source code patch and new junit

      The constructor of XMLCipher is private which makes it impossible to subclass XMLCipher. Furthermore, much of the work to construct the XMLCipher instance is located in the getInstance() or getProviderInstance() methods. That would force a subclass to duplicate that code once again.

      The goal of this effort is to experiment with per KeyInfo KeyResolvers to resolve the Key Encryption Key dynamically based on the EncryptedKey/KeyInfo carried in the message. The junit in attachment shows how to achieve it through subclassing. This might not be the most obvious use of the API, but at least it proves that it can be done. It has the advantage that none of the API changes are controversial.

      The solution involves:

      • Making the XMLCipher constructor protected. Callers must still call one of the getInstance() or getProviderInstance() methods.
      • pushing all the construction code in getInstance() and getProviderInstance() into the real constructor.
      • relaxing the requirement that provider must not be null. Passing null for the provider in getProviderInstance() gives the same result as using the equivalent getInstance() method.
      • Adding createKeyInfo() and createEncryptedKeyResolver() factory methods in XMLCipher.
      • Changing XMLCipher to use the new factory methods when creating internal KeyInfo or EncryptedKeyResolver objects.
      • Adding the method createXMLCipher() to EncryptedKeyResolver.
      • Also added some test to @return keywords that were empty in XMLCipher

        Issue Links

          Activity

          Hide
          sean.mullan added a comment -

          I only skimmed the patch but it seems to me a much simpler approach would be to add a new overloaded XMLCipher.init method that takes a KeyResolver instead of a Key. Did you consider that solution?

          Show
          sean.mullan added a comment - I only skimmed the patch but it seems to me a much simpler approach would be to add a new overloaded XMLCipher.init method that takes a KeyResolver instead of a Key. Did you consider that solution?
          Hide
          Clement Pellerin added a comment -

          I agree subclassing does not fit the design of the static factory methods XMLCipher.getInstance(). There is bound to be a better solution if we can consider more visible changes to the API.

          Your suggestion is promising but not sufficient in itself.

          The essence of the problem occurs in EncryptedKeyResolver.engineLookupAndResolveSecretKey():
          XMLCipher cipher = XMLCipher.getInstance();
          cipher.init(XMLCipher.UNWRAP_MODE, _kek);
          This XMLCipher object is created internally.
          The user does not control its options.
          We need to attach the KeyResolvers from the outer XMLCipher into this one.
          Usually this is done by registering the KeyResolvers globally.
          The whole point of my effort is to allow different KeyResolvers on
          different XMLCipher objects so this is not applicable.

          An elegant solution would be to pass a context around but
          we are not designing JSR-106 from scratch here.

          A possible solution would be to pass the outer XMLCipher in the
          EncryptedKeyResolver when it is created in XMLCipher.decryptToByteArray()
          This is the code that needs modifications:
          ki.registerInternalKeyResolver(
          new EncryptedKeyResolver(
          encryptedData.getEncryptionMethod().getAlgorithm(),
          _kek));
          Then we could pass the outer XMLCipher KeyResolvers to the inner XMLCipher
          inside EncryptedKeyResolver.

          This is a simplified view since XMLCipher does not hold
          the KeyResolvers directly. Instead, the KeyResolvers are added
          to KeyInfo with the call KeyInfo.registerInternalKeyResolver().

          Whatever the final solution becomes, I still think the clean up of
          the XMLCipher constructor and getInstance() methods stands on its own
          (if we keep the constructor private).

          Let's continue the discussion to discover what could be the
          best solution with reasonable changes to the public API.

          Show
          Clement Pellerin added a comment - I agree subclassing does not fit the design of the static factory methods XMLCipher.getInstance(). There is bound to be a better solution if we can consider more visible changes to the API. Your suggestion is promising but not sufficient in itself. The essence of the problem occurs in EncryptedKeyResolver.engineLookupAndResolveSecretKey(): XMLCipher cipher = XMLCipher.getInstance(); cipher.init(XMLCipher.UNWRAP_MODE, _kek); This XMLCipher object is created internally. The user does not control its options. We need to attach the KeyResolvers from the outer XMLCipher into this one. Usually this is done by registering the KeyResolvers globally. The whole point of my effort is to allow different KeyResolvers on different XMLCipher objects so this is not applicable. An elegant solution would be to pass a context around but we are not designing JSR-106 from scratch here. A possible solution would be to pass the outer XMLCipher in the EncryptedKeyResolver when it is created in XMLCipher.decryptToByteArray() This is the code that needs modifications: ki.registerInternalKeyResolver( new EncryptedKeyResolver( encryptedData.getEncryptionMethod().getAlgorithm(), _kek)); Then we could pass the outer XMLCipher KeyResolvers to the inner XMLCipher inside EncryptedKeyResolver. This is a simplified view since XMLCipher does not hold the KeyResolvers directly. Instead, the KeyResolvers are added to KeyInfo with the call KeyInfo.registerInternalKeyResolver(). Whatever the final solution becomes, I still think the clean up of the XMLCipher constructor and getInstance() methods stands on its own (if we keep the constructor private). Let's continue the discussion to discover what could be the best solution with reasonable changes to the public API.
          Hide
          Colm O hEigeartaigh added a comment -

          Thanks for the patch....I committed some of the changes you suggested for XMLCipher, namely cleaning up the constructor and getInstance methods, as well as various spelling corrections for the Javadoc.

          Author: coheigea
          Date: Tue Oct 12 16:24:15 2010
          New Revision: 1021829

          URL: http://svn.apache.org/viewvc?rev=1021829&view=rev
          Log:
          Committed some of the improvements to XMLCipher as suggested in BUG 49465 - impossible to subclass XMLCipher.

          Modified:
          santuario/trunk/CHANGELOG.txt
          santuario/trunk/src/org/apache/xml/security/encryption/XMLCipher.java

          Thanks,

          Colm.

          Show
          Colm O hEigeartaigh added a comment - Thanks for the patch....I committed some of the changes you suggested for XMLCipher, namely cleaning up the constructor and getInstance methods, as well as various spelling corrections for the Javadoc. Author: coheigea Date: Tue Oct 12 16:24:15 2010 New Revision: 1021829 URL: http://svn.apache.org/viewvc?rev=1021829&view=rev Log: Committed some of the improvements to XMLCipher as suggested in BUG 49465 - impossible to subclass XMLCipher. Modified: santuario/trunk/CHANGELOG.txt santuario/trunk/src/org/apache/xml/security/encryption/XMLCipher.java Thanks, Colm.
          Hide
          Clement Pellerin added a comment -

          I think it is intentional that XMLCipher cannot be subclassed. This is similar to KeyStore which would not normally be subclassed.
          The problem with the EncryptedKeyResolver is a duplicate of SANTUARIO-305.
          I recommend this bug be closed at the same time SANTUARIO-305 is fixed.

          Show
          Clement Pellerin added a comment - I think it is intentional that XMLCipher cannot be subclassed. This is similar to KeyStore which would not normally be subclassed. The problem with the EncryptedKeyResolver is a duplicate of SANTUARIO-305 . I recommend this bug be closed at the same time SANTUARIO-305 is fixed.
          Hide
          Colm O hEigeartaigh added a comment -


          This issue is superceded by SANTUARIO-305.

          Show
          Colm O hEigeartaigh added a comment - This issue is superceded by SANTUARIO-305 .

            People

            • Assignee:
              Unassigned
              Reporter:
              Clement Pellerin
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development