Uploaded image for project: 'Syncope'
  1. Syncope
  2. SYNCOPE-505

Support propagating non-cleartext passwords to external resources

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.2.0-M1
    • Component/s: core
    • Labels:
      None

      Description

      Similarly to SYNCOPE-313 during synchronization, it seems feasible to provide some Propagation Actions classes (say DBPasswordPropagationActions and LDAPPasswordPropagationActions that will propagate non-cleartext password values to external resources.

      This might require some changes in the related connector bundles.

        Issue Links

          Activity

          Hide
          ilgrosso Francesco Chicchiriccò added a comment -

          Bulk close for 1.2.0-M1

          Show
          ilgrosso Francesco Chicchiriccò added a comment - Bulk close for 1.2.0-M1
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 1606720 from Colm O hEigeartaigh in branch 'syncope/trunk'
          [ https://svn.apache.org/r1606720 ]

          SYNCOPE-505 - Adding integration tests

          Show
          jira-bot ASF subversion and git services added a comment - Commit 1606720 from Colm O hEigeartaigh in branch 'syncope/trunk' [ https://svn.apache.org/r1606720 ] SYNCOPE-505 - Adding integration tests
          Hide
          ilgrosso Francesco Chicchiriccò added a comment -

          Any plan for adding an integration test case for this feature?

          Show
          ilgrosso Francesco Chicchiriccò added a comment - Any plan for adding an integration test case for this feature?
          Show
          ilgrosso Francesco Chicchiriccò added a comment - Here you go: https://connid.atlassian.net/browse/LDAP-13
          Hide
          ilgrosso Francesco Chicchiriccò added a comment -

          Colm O hEigeartaigh, I've taken a look at LDAPPropagationActions and it looks good, nice job!
          Now, as you suggest, we need to open an issue on the LDAP connector for not hashing password values when on the form

          {HASH}value
          

          with HASH different from CLEARTEXT: it shouldn't be an hard modification, actually.

          Show
          ilgrosso Francesco Chicchiriccò added a comment - Colm O hEigeartaigh , I've taken a look at LDAPPropagationActions and it looks good, nice job! Now, as you suggest, we need to open an issue on the LDAP connector for not hashing password values when on the form {HASH}value with HASH different from CLEARTEXT : it shouldn't be an hard modification, actually.
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 1604848 from Colm O hEigeartaigh in branch 'syncope/trunk'
          [ https://svn.apache.org/r1604848 ]

          SYNCOPE-505 - Adding initial LDAPPropagationActions class

          Show
          jira-bot ASF subversion and git services added a comment - Commit 1604848 from Colm O hEigeartaigh in branch 'syncope/trunk' [ https://svn.apache.org/r1604848 ] SYNCOPE-505 - Adding initial LDAPPropagationActions class
          Hide
          coheigea Colm O hEigeartaigh added a comment -

          Hi Francesco,

          I have this working for LDAP as well but wanted to validate my approach.

          For the DB Connector we are sending an extra attribute over to tell the Connector that the password is already hashed. For LDAP, we could do this as well, but I implemented the "reverse" of the Sync Action. So if the configured cipher algorithm of the user matches that of the LDAP Connector, it takes the user password, "de-hexes" it + Base64 encodes it. It then writes out the same type of value that it syncs, i.e. "

          {sha}

          XYZ...==".

          The LDAP Connector needs a change to detect a password of this form "

          {" + matching digest + "}

          " + "rest-of-password", and if so it simply stores the received password "as is".

          WDYT?

          Colm.

          Show
          coheigea Colm O hEigeartaigh added a comment - Hi Francesco, I have this working for LDAP as well but wanted to validate my approach. For the DB Connector we are sending an extra attribute over to tell the Connector that the password is already hashed. For LDAP, we could do this as well, but I implemented the "reverse" of the Sync Action. So if the configured cipher algorithm of the user matches that of the LDAP Connector, it takes the user password, "de-hexes" it + Base64 encodes it. It then writes out the same type of value that it syncs, i.e. " {sha} XYZ...==". The LDAP Connector needs a change to detect a password of this form " {" + matching digest + "} " + "rest-of-password", and if so it simply stores the received password "as is". WDYT? Colm.
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 1604144 from Colm O hEigeartaigh in branch 'syncope/trunk'
          [ https://svn.apache.org/r1604144 ]

          SYNCOPE-505 - Finished with DB Propagation Action

          Show
          jira-bot ASF subversion and git services added a comment - Commit 1604144 from Colm O hEigeartaigh in branch 'syncope/trunk' [ https://svn.apache.org/r1604144 ] SYNCOPE-505 - Finished with DB Propagation Action
          Hide
          ilgrosso Francesco Chicchiriccò added a comment -

          A quick query on this - in DBPasswordSyncActions I can get access to the Connector via AbstractSyncopeResultHandler.getConnector().getActiveConnInstance(). How can I do the same in DBPasswordPropagationActions?

          Use PropagationTask.getResource().getConnector() that returns a ConnInstance object.

          Show
          ilgrosso Francesco Chicchiriccò added a comment - A quick query on this - in DBPasswordSyncActions I can get access to the Connector via AbstractSyncopeResultHandler.getConnector().getActiveConnInstance(). How can I do the same in DBPasswordPropagationActions? Use PropagationTask.getResource().getConnector() that returns a ConnInstance object.
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 1603800 from Colm O hEigeartaigh in branch 'syncope/trunk'
          [ https://svn.apache.org/r1603800 ]

          SYNCOPE-505 - Updating the DB Propagation Actions class

          Show
          jira-bot ASF subversion and git services added a comment - Commit 1603800 from Colm O hEigeartaigh in branch 'syncope/trunk' [ https://svn.apache.org/r1603800 ] SYNCOPE-505 - Updating the DB Propagation Actions class
          Hide
          coheigea Colm O hEigeartaigh added a comment -

          BTW, writing out the password only if SyncopeUser#getCipherAlgorithm matches the configured value for the DB Connector hash algorithm (e.g. the same logic of SYNCOPE-313) seems correct to me.

          A quick query on this - in DBPasswordSyncActions I can get access to the Connector via AbstractSyncopeResultHandler.getConnector().getActiveConnInstance(). How can I do the same in DBPasswordPropagationActions?

          Colm.

          Show
          coheigea Colm O hEigeartaigh added a comment - BTW, writing out the password only if SyncopeUser#getCipherAlgorithm matches the configured value for the DB Connector hash algorithm (e.g. the same logic of SYNCOPE-313 ) seems correct to me. A quick query on this - in DBPasswordSyncActions I can get access to the Connector via AbstractSyncopeResultHandler.getConnector().getActiveConnInstance(). How can I do the same in DBPasswordPropagationActions? Colm.
          Hide
          ilgrosso Francesco Chicchiriccò added a comment -

          Shouldn't it default to "false" when missing?

          Ouch, you are clearly right! o_O

          Show
          ilgrosso Francesco Chicchiriccò added a comment - Shouldn't it default to "false" when missing? Ouch, you are clearly right! o_O
          Hide
          coheigea Colm O hEigeartaigh added a comment -

          A special HASHED_PASSWORD boolean attribute - defaults to true when missing - could be added to the DBTable connector configuration: sounds good.

          Shouldn't it default to "false" when missing? I.e. "HASHED_PASSWORD" being present and "true" means that the value under PASSWORD should be treated as hashed + not subsequently hashed with the configured Connector hash algorithm. Otherwise, the Connector hash algorithm applies.

          Colm.

          Show
          coheigea Colm O hEigeartaigh added a comment - A special HASHED_PASSWORD boolean attribute - defaults to true when missing - could be added to the DBTable connector configuration: sounds good. Shouldn't it default to "false" when missing? I.e. " HASHED_PASSWORD " being present and "true" means that the value under PASSWORD should be treated as hashed + not subsequently hashed with the configured Connector hash algorithm. Otherwise, the Connector hash algorithm applies. Colm.
          Hide
          ilgrosso Francesco Chicchiriccò added a comment -

          Could we have a new (boolean) attribute _HASHED_PASSWORD_ or something? Alternatively, we could use a predefined prefix/suffix on the _PASSWORD_. Any preferences?

          A special _HASHED_PASSWORD_ boolean attribute - defaults to true when missing - could be added to the DBTable connector configuration: sounds good.
          We need to open an issue on ConnId's JIRA then, targeted to DBTable connector 2.1.7.

          One query would be whether we should also follow this logic if the DB Connector has a CLEARTEXT value? I think we should, but want to verify it.

          Agree.

          Show
          ilgrosso Francesco Chicchiriccò added a comment - Could we have a new (boolean) attribute _ HASHED_PASSWORD _ or something? Alternatively, we could use a predefined prefix/suffix on the _ PASSWORD _. Any preferences? A special _ HASHED_PASSWORD _ boolean attribute - defaults to true when missing - could be added to the DBTable connector configuration: sounds good. We need to open an issue on ConnId's JIRA then, targeted to DBTable connector 2.1.7. One query would be whether we should also follow this logic if the DB Connector has a CLEARTEXT value? I think we should, but want to verify it. Agree.
          Hide
          coheigea Colm O hEigeartaigh added a comment -

          We should find a way then to instruct the connector that the specific password value we are passing is already hashed: unfortunately, connector configuration properties are only evaluated when creating a connector instance, so they cannot be changed on-the-fly.

          Could we have a new (boolean) attribute (_HASHED_PASSWORD) or something? Alternatively, we could use a predefined prefix/suffix on the _PASSWORD. Any preferences?

          BTW, writing out the password only if SyncopeUser#getCipherAlgorithm matches the configured value for the DB Connector hash algorithm (e.g. the same logic of SYNCOPE-313) seems correct to me.

          Ok, sounds good. One query would be whether we should also follow this logic if the DB Connector has a CLEARTEXT value? I think we should, but want to verify it.

          Colm.

          Show
          coheigea Colm O hEigeartaigh added a comment - We should find a way then to instruct the connector that the specific password value we are passing is already hashed: unfortunately, connector configuration properties are only evaluated when creating a connector instance, so they cannot be changed on-the-fly. Could we have a new (boolean) attribute (_ HASHED_PASSWORD ) or something? Alternatively, we could use a predefined prefix/suffix on the _PASSWORD . Any preferences? BTW, writing out the password only if SyncopeUser#getCipherAlgorithm matches the configured value for the DB Connector hash algorithm (e.g. the same logic of SYNCOPE-313 ) seems correct to me. Ok, sounds good. One query would be whether we should also follow this logic if the DB Connector has a CLEARTEXT value? I think we should, but want to verify it. Colm.
          Hide
          ilgrosso Francesco Chicchiriccò added a comment -

          The logic for hashing the password value according to the relevant configuration lays in DBTable connector's code, not Syncope's.
          This means that normally Syncope passes the password as clear text (wrapped in GuardedString) and then the connector will hash it according to the configured algorithm before writing to the underlying db table.

          We should find a way then to instruct the connector that the specific password value we are passing is already hashed: unfortunately, connector configuration properties are only evaluated when creating a connector instance, so they cannot be changed on-the-fly.

          BTW, writing out the password only if SyncopeUser#getCipherAlgorithm matches the configured value for the DB Connector hash algorithm (e.g. the same logic of SYNCOPE-313) seems correct to me.

          Show
          ilgrosso Francesco Chicchiriccò added a comment - The logic for hashing the password value according to the relevant configuration lays in DBTable connector's code, not Syncope's. This means that normally Syncope passes the password as clear text (wrapped in GuardedString ) and then the connector will hash it according to the configured algorithm before writing to the underlying db table. We should find a way then to instruct the connector that the specific password value we are passing is already hashed: unfortunately, connector configuration properties are only evaluated when creating a connector instance, so they cannot be changed on-the-fly. BTW, writing out the password only if SyncopeUser#getCipherAlgorithm matches the configured value for the DB Connector hash algorithm (e.g. the same logic of SYNCOPE-313 ) seems correct to me.
          Hide
          coheigea Colm O hEigeartaigh added a comment -

          Well in the DBPasswordPropagationActions we know that the password is hashed/encrypted as we are calling getPassword() and not getClearPassword(), right? I suppose we could only write out the password if SyncopeUser.getCipherAlgorithm() matches the configured value for the DB Connector hash algorithm?

          Colm.

          Show
          coheigea Colm O hEigeartaigh added a comment - Well in the DBPasswordPropagationActions we know that the password is hashed/encrypted as we are calling getPassword() and not getClearPassword(), right? I suppose we could only write out the password if SyncopeUser.getCipherAlgorithm() matches the configured value for the DB Connector hash algorithm? Colm.
          Hide
          ilgrosso Francesco Chicchiriccò added a comment -

          As you can see, I've just committed some changes to the class in order to use some known constants.

          About the proposal of adding a property to the DBTable connector, is there any safe method to understand whether a string is "plaintext" or not?

          Show
          ilgrosso Francesco Chicchiriccò added a comment - As you can see, I've just committed some changes to the class in order to use some known constants. About the proposal of adding a property to the DBTable connector, is there any safe method to understand whether a string is "plaintext" or not?
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 1603184 from Francesco Chicchiriccò in branch 'syncope/trunk'
          [ https://svn.apache.org/r1603184 ]

          SYNCOPE-505 Using known constants

          Show
          jira-bot ASF subversion and git services added a comment - Commit 1603184 from Francesco Chicchiriccò in branch 'syncope/trunk' [ https://svn.apache.org/r1603184 ] SYNCOPE-505 Using known constants
          Hide
          coheigea Colm O hEigeartaigh added a comment -

          I added an initial prototype implementation for DBPasswordPropagationActions. It checks to see if there is a mandatory missing attribute that corresponds to password, and then just writes out the password from SyncopeUser "as is" in this case. What do you think about this approach?

          I've tested the prototype + it works. One issue is that it only works if the Connector uses "CLEARTEXT", as otherwise the supplied password gets hashed. Should we add another Connector property so that we can tell it to only hash/encrypt if the supplied password is "plaintext"?

          Colm.

          Show
          coheigea Colm O hEigeartaigh added a comment - I added an initial prototype implementation for DBPasswordPropagationActions. It checks to see if there is a mandatory missing attribute that corresponds to password, and then just writes out the password from SyncopeUser "as is" in this case. What do you think about this approach? I've tested the prototype + it works. One issue is that it only works if the Connector uses "CLEARTEXT", as otherwise the supplied password gets hashed. Should we add another Connector property so that we can tell it to only hash/encrypt if the supplied password is "plaintext"? Colm.
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 1603171 from Colm O hEigeartaigh in branch 'syncope/trunk'
          [ https://svn.apache.org/r1603171 ]

          SYNCOPE-505 - Adding an initial PropagationActions implementation for DBs

          Show
          jira-bot ASF subversion and git services added a comment - Commit 1603171 from Colm O hEigeartaigh in branch 'syncope/trunk' [ https://svn.apache.org/r1603171 ] SYNCOPE-505 - Adding an initial PropagationActions implementation for DBs
          Hide
          ilgrosso Francesco Chicchiriccò added a comment -

          The use case you mention above is one of use cases this issue should cover, but not the only one.

          Syncope requires an input password when subscribing an user to a new resource, unless AES is used or that resource does not define a password mapping entry.
          With this issue, password could be actually propagated to a resource - even from internal storage - when plugging-in the *PasswordPropagationActions: for this reason I think it is important to take care not to overwrite any password already prepared for propagation by the PropagationManager, in the *PasswordPropagationActions code.

          Show
          ilgrosso Francesco Chicchiriccò added a comment - The use case you mention above is one of use cases this issue should cover, but not the only one. Syncope requires an input password when subscribing an user to a new resource, unless AES is used or that resource does not define a password mapping entry. With this issue, password could be actually propagated to a resource - even from internal storage - when plugging-in the *PasswordPropagationActions : for this reason I think it is important to take care not to overwrite any password already prepared for propagation by the PropagationManager , in the *PasswordPropagationActions code.
          Hide
          coheigea Colm O hEigeartaigh added a comment -

          Hi Francesco,

          I'm just wondering what the use-case is here: Is it when we have users synchronized into Syncope from one resource with non-cleartext passwords, that we then want to propagate to another resource? When we create users in Syncope, then we can just propagate out the password hashed according to the connector property, right?

          Colm.

          Show
          coheigea Colm O hEigeartaigh added a comment - Hi Francesco, I'm just wondering what the use-case is here: Is it when we have users synchronized into Syncope from one resource with non-cleartext passwords, that we then want to propagate to another resource? When we create users in Syncope, then we can just propagate out the password hashed according to the connector property, right? Colm.

            People

            • Assignee:
              coheigea Colm O hEigeartaigh
              Reporter:
              ilgrosso Francesco Chicchiriccò
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development