8 Replies Latest reply on Apr 6, 2006 5:35 PM by anil.saldhana

    Deep Copy of Subject Sets

    anil.saldhana

      http://jira.jboss.com/jira/browse/JBAS-2657

      The copy of the Subject made by the JaasSecurityService is just a shallow copy of the various subject sets. This is fine for immutable objects, but if there are mutable objects that are destroyed when the cache is flushed and the login module executes this can have side effects on other threads. The only way this can be avoided is to make a deep copy using clone for objects that implement Cloneable and override the Object.clone method to correctly implement a deep copy. We should add such a deep copy option.



      I want to dedicate this forum thread for discussion of this JIRA issue.

      Scott, can you please provide further insight into this?

        • 1. Re: Deep Copy of Subject Sets
          anil.saldhana

          I think Scott is referring to the following line of code in the updateCache method.

          SubjectActions.copySubject(subject, info.subject, true);
          


          • 2. Re: Deep Copy of Subject Sets
            anil.saldhana

            Scott, need some guidance on this.

            If I am going to clone the subject sets, I will have to clone the subject contents in the org.jboss.security.plugins.SubjectAction class. But the contents can be derivatives of org.jboss.security.SimplePrincipal(SimpleGroup, NestedPrincipal, NestedGroup etc). So there is a mismatch in the packages.

            How does the package protection semantics work when a security manager is installed, in this case?

            • 3. Re: Deep Copy of Subject Sets
              anil.saldhana

              Think there should be no issue as the SubjectActions class already imports org.jboss.security.SecurityAssociation

              • 4. Re: Deep Copy of Subject Sets
                starksm64

                The package names are just part of the type system so either one class loader is involved and it works, or it does not. This is independent of a security manager being present. The SimplePrincipal does not implement Clonable (and does not need to as its immutable) so it would not matter.

                • 5. Re: Deep Copy of Subject Sets
                  anil.saldhana

                  SimplePrincipal is immutable, but many of the derivatives like NestedGroup, NestedPrincipal are not, as they carry internal collections like an HashMap or a LinkedList. I guess we will have to do deep copy for these cases.

                  Also, in SubjectActions, where we do the deep copy, what would be the best choice:
                  a) use reflection or
                  b) Do instanceof checks for the known principals and clone them.

                  Rather than using cloning, we can do a serialized copy of the subject, which have correct semantics, but will performance hit plus the need for the objects to implement Serializable.

                  • 6. Re: Deep Copy of Subject Sets
                    starksm64

                    The check needs to be (instanceof Cloneable). If the crap in the Subject does not implement Cloneable, then its not deep copied.

                    • 7. Re: Deep Copy of Subject Sets
                      anil.saldhana

                      The implementation looks as follows:

                      private static class CopySubjectAction implements PrivilegedAction
                       {
                       ...
                       public Object run()
                       {
                       Set principals = fromSubject.getPrincipals();
                       Set principals2 = toSubject.getPrincipals();
                       Iterator iter = principals.iterator();
                       while( iter.hasNext() )
                       principals2.add(getClonedObjectIfAvailable(iter.next()));
                       Set privateCreds = fromSubject.getPrivateCredentials();
                       Set privateCreds2 = toSubject.getPrivateCredentials();
                       iter = privateCreds.iterator();
                       while( iter.hasNext() )
                       privateCreds2.add(getClonedObjectIfAvailable(iter.next()));
                       Set publicCreds = fromSubject.getPublicCredentials();
                       Set publicCreds2 = toSubject.getPublicCredentials();
                       iter = publicCreds.iterator();
                       while( iter.hasNext() )
                       publicCreds2.add(getClonedObjectIfAvailable(iter.next()));
                       if( setReadOnly == true )
                       toSubject.setReadOnly();
                       return null;
                       }
                      
                       /** Check if the Object implements Cloneable and return cloned object */
                       private Object getClonedObjectIfAvailable(Object obj)
                       {
                       Object clonedObject = null;
                       if(obj instanceof Cloneable)
                       {
                       Class clazz = obj.getClass();
                       try
                       {
                       Method cloneMethod = clazz.getMethod("clone", null);
                       clonedObject = cloneMethod.invoke(obj, null);
                       }
                       catch (Exception e)
                       {//Ignore non-cloneable issues
                       }
                       }
                       if(clonedObject == null)
                       clonedObject = obj;
                       return clonedObject;
                       }
                       }
                      


                      The Cloneable interface is a marker interface (with no clone method). So I cannot cast any object to Cloneable and call clone on it.

                      I have the unit tests for cloning the various JBossSX principals/groups( that need cloning) ready. I have to write a test for the deep copy done by the JaasSecurityManager.

                      A clarification I seek is: The JIRA issue mentions the following at the end.
                      We should add such a deep copy option.


                      Does it mean:
                      a) configurable option on the JaasSecurityManager? or
                      b) make the deep copy the default behavior of the subject copy action. (I would like this behavior)

                      • 8. Re: Deep Copy of Subject Sets
                        anil.saldhana

                        Confirmed from Scott that this should be a configurable option on the security manager service, because of the fact that there can be excessing copying.

                        I have a testcase that tests the cloning of the subject sets on the serverside via a mutable principal, under the two flags - when deepcopy is enabled and disabled.