While working with the module I recognized that the user password used on the endpoint will be saved as plain text in the database (this is already addressed in TODO.txt - conditionally use encrypt) but when you export the deployment with features module the plain text password is also written to the Features files.

That's likely nothing you really want to have because the user on the endpoint has access to nearly anything content related. Somebody could miss this issue and put the configuration in a public repository opening up a security hole on the destination site.

Currently I would not recommend exporting deploy config via Features.

CommentFileSizeAuthor
#50 2837992-diff-42-50.txt3.17 KBvijaycs85
#50 2837992-50.patch19.21 KBvijaycs85
#45 interdiff_37_41.txt473 bytesharshil.maradiya
#42 auth_encrypt-2837992-41.patch18.39 KBharshil.maradiya
#42 interdiff_37_41.txt0 bytesharshil.maradiya
#38 interdiff_34-37.txt832 bytesharshil.maradiya
#38 auth_encrypt-2837992-37.patch18.57 KBharshil.maradiya
#35 interdiff_30_34.txt5.09 KBharshil.maradiya
#35 auth_encrypt-2837992-34.patch18.55 KBharshil.maradiya
#32 interdiff_28_30.patch1.32 KBharshil.maradiya
#32 auth_encrypt-2837992-30.patch17.4 KBharshil.maradiya
#31 interdiff_28_30.txt1.32 KBharshil.maradiya
#30 2837992-30.png138.79 KBvijaycs85
#28 auth_encrypt-2837992-28.patch17.16 KBharshil.maradiya
#25 auth_encrypt-2837992-25.patch17 KBprashantgajare
#21 auth_encrypt-2837992-13.patch10.37 KBharshil.maradiya
#12 auth_encrypt-2837992-12.patch3.26 KBprashantgajare
#7 auth_encrypt-2837992-5.patch3.4 KBharshil.maradiya
#6 auth_encrypt-2837992-4.patch3.35 KBharshil.maradiya
#4 auth_encrypt-2837992-3.patch3.32 KBharshil.maradiya
#2 auth_encrypt-2837992-1.patch3.14 KBharshil.maradiya
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

rogerpfaff created an issue. See original summary.

harshil.maradiya’s picture

harshil.maradiya’s picture

Status: Active » Needs work
harshil.maradiya’s picture

harshil.maradiya’s picture

Status: Needs work » Needs review
harshil.maradiya’s picture

harshil.maradiya’s picture

prashantgajare’s picture

Tested this patch. The patch seems to work fine with both the current dev version and current release.
Please rollout this patch in the next release.

harshil.maradiya’s picture

Status: Needs review » Reviewed & tested by the community
Manuel Garcia’s picture

Version: 7.x-3.0-alpha1 » 7.x-3.x-dev
Status: Reviewed & tested by the community » Needs work

Thanks for the patch! - I am not entirely sure this is the best solution for this problem. We could look into using key module, but I doubt we've got the momentum on the 7.x branch to do such a thing. So for now here's a CS review:

  1. +++ b/modules/deploy_ui/plugins/export_ui/deploy_ui_endpoint.class.php
    @@ -145,7 +145,12 @@ class deploy_ui_endpoint extends ctools_export_ui {
    +        $item->authenticator_config = $form_state['values']['authenticator_config'];
    

    Indentation is off.

  2. +++ b/plugins/DeployAuthenticatorSession.inc
    @@ -26,6 +26,13 @@ class DeployAuthenticatorSession implements DeployAuthenticator {
    +    ¶
    

    Added spaces on empty line.

  3. +++ b/deploy.module
    @@ -792,3 +792,31 @@ function deploy_plan_entity_label($type, $entity, $rev = NULL) {
    + *  @return boolean
    

    @return should leave an empty line before it.

+++ b/deploy.install
@@ -400,3 +400,13 @@ function deploy_update_7003(&$sandbox) {
+  if(_deploy_encrypt_endpoints()) {

+++ b/deploy.module
@@ -792,3 +792,31 @@ function deploy_plan_entity_label($type, $entity, $rev = NULL) {
+  if(module_exists('encrypt')) {
...
+      if(!empty($endpoint->authenticator_config['username']) && !empty($endpoint->authenticator_config['password'])) {

+++ b/plugins/DeployAuthenticatorSession.inc
@@ -26,6 +26,13 @@ class DeployAuthenticatorSession implements DeployAuthenticator {
+      if(!empty($config['username']) && !empty($config['password'])) {

Missing space after if statement

prashantgajare’s picture

@manuel - Fixed the CS issues. Thanks! :)

prashantgajare’s picture

Status: Needs work » Needs review
Manuel Garcia’s picture

Thanks @prashantgajare for that, patch is looking good, however I'd like more reviews, and other maintainers' signoff on this before we commit it.

rogerpfaff’s picture

If I import this in another site will it then work without doing anything or will the encrypted password be used as new password?

harshil.maradiya’s picture

@rogerpfaff The password will remain the same , but in database the encrypted password will get store

Status: Needs review » Needs work

The last submitted patch, 12: auth_encrypt-2837992-12.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

timmillwood’s picture

  1. +++ b/deploy.install
    @@ -400,3 +400,13 @@ function deploy_update_7003(&$sandbox) {
    +  return t("Nothing to update as encrypt module is not there.");
    

    I don't think this message is clear enough. Encrypt module is not where? Also encrypt should have a capital E as it's the name of a module.

    Maybe something like "Updated skipped: This update only applies when using the Encrypt module."?

  2. +++ b/deploy.module
    @@ -792,3 +792,33 @@ function deploy_plan_entity_label($type, $entity, $rev = NULL) {
    +
    

    We don't need this whitespace before the if.

timmillwood’s picture

+++ b/modules/deploy_ui/plugins/export_ui/deploy_ui_endpoint.class.php
@@ -146,6 +146,11 @@ class deploy_ui_endpoint extends ctools_export_ui {
+        $item->authenticator_config =  $encrypt;

Too many spaces after =.

harshil.maradiya’s picture

harshil.maradiya’s picture

prashantgajare’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 21: auth_encrypt-2837992-13.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Manuel Garcia’s picture

Status: Needs work » Needs review

Looks like the test bot blew up, lets try again...

prashantgajare’s picture

prashantgajare’s picture

vijaycs85’s picture

Status: Needs review » Needs work
  1. +++ b/deploy.install
    @@ -400,3 +401,13 @@ function deploy_update_7003(&$sandbox) {
    +function deploy_update_7004() {
    
    +++ b/deploy.module
    @@ -792,3 +793,33 @@ function deploy_plan_entity_label($type, $entity, $rev = NULL) {
    + * Implements hook_modules_enabled().
    ...
    +function deploy_modules_enabled($modules) {
    

    Probably should update hook_install for the new sites as hook_modules_enabled() might not cover few scenarios like:

    1. encrypt module is already enabled and installing deploy for the first time: Since the encrypt already enabled, deploy_modules_enabled() wouldn't do anything and hook_update_N wouldn't run on install.

  2. +++ b/modules/deploy_ui/deploy_ui.module
    @@ -1,5 +1,9 @@
    +  * @file
    

    @file description is missing.

  3. +++ b/modules/deploy_ui/plugins/export_ui/deploy_ui_plan.class.php
    @@ -396,6 +398,7 @@ class deploy_ui_plan extends ctools_export_ui {
    +  ¶
    

    unnecessary empty spaces.

  4. One final concern is, it is worth to maintain the state of the encryption somewhere in a variable for example. This we don't just check if the encrypt module enabled or not. This would help to assure that the multiple installs/uninstalls of the encrypt module wouldn't cause any problem.
harshil.maradiya’s picture

@Vijay, Thanks for feedback I have implemented 1st 3 concerns

harshil.maradiya’s picture

Status: Needs work » Needs review
vijaycs85’s picture

FileSize
138.79 KB

Thanks, @harshil.maradiya. Please make sure to provide interdiff next time which would helps a lot to review. I still see the empty line with space (see attached dreditor UI). It's trivial though. So if you reroll the patch for any other fix make sure to include it.

I still think #27.4 is a valid item to fix. Here is a sample scenario:
1. Install & enable encryption module -> Would trigger deploy_modules_enabled() and does the encription of username (e.g. foo to bar) and password.
2. Disable & uninstall encryption module
3. Enable encryption module again -> Now the deploy_modules_enabled() would double encrypt the username (i.e. bar to baz)

harshil.maradiya’s picture

FileSize
1.32 KB

Dear Vijay,
Thanks for detailed explanation i have made required changes

harshil.maradiya’s picture

vijaycs85’s picture

Status: Needs review » Reviewed & tested by the community

It looks good to me. Setting to RTBC to see if we can get a maintainer review.

Manuel Garcia’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/deploy.module
@@ -792,3 +793,35 @@ function deploy_plan_entity_label($type, $entity, $rev = NULL) {
+/**
+ * Implements hook_modules_enabled().
+ */
+function deploy_modules_enabled($modules) {
+  if (in_array('encrypt', $modules)) {
+    _deploy_encrypt_endpoints();
+  }
+}

We should also decrypt the endpoints' username and password in the case that encrypt module is being disabled, otherwise sites will end up with unusable endpoints credentials.

harshil.maradiya’s picture

harshil.maradiya’s picture

Status: Needs work » Needs review
vijaycs85’s picture

Status: Needs review » Needs work
  1. +    if (module_exists('encrypt')) {
    +      // Remove defaulr php encryption state.
    +      if (_get_deploy_php_encryption_state()) {
    +        variable_del('deploy_php_encryption_state');

    not sure if it's a good idea to delete the variable every time. why can't we set it to false?

  2. if (!variable_get('deploy_php_encryption_state')) {
    Use FALSE as default value here?
harshil.maradiya’s picture

vijaycs85’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me. Though I feel the introduction of new functions making the code bit complicated, I don't have any strong objection. +1 to RTBC.

Manuel Garcia’s picture

Status: Reviewed & tested by the community » Needs work

Thanks for following up on this. I believe at least #34 needs to be addressed still.

vijaycs85’s picture

#40: Its going to be very tricky because hook_modules_disabled and hook_modules_uninstalled are executed aftera module being disabled/unistalled. So by that time when we disable/uninstall encrypt, our data is already in a deadlock as the function decrypt doesn't exist anymore.

harshil.maradiya’s picture

Removing enable hook so it could not end up in double encryption

harshil.maradiya’s picture

Status: Needs work » Needs review
Manuel Garcia’s picture

Thanks @harshil.maradiya - looks like the last interdiff didnt come out correct, would you mind providing a valid one to facilitate reviews please?

harshil.maradiya’s picture

FileSize
473 bytes

Thanks, uploading correct inter diff

vijaycs85’s picture

Status: Needs review » Reviewed & tested by the community

looks good to go.

Manuel Garcia’s picture

Status: Reviewed & tested by the community » Needs work

The patch would now prevent double encryption, thanks for that.

However the problem described on #34 is still valid:
1. You enable deploy while having encrypt enabled, or you run this update hook while having encrypt enabled, so the endpoints get encrypted.
2. Later you disable encrypt module, and since its not a dependency of deploy, we cannot prevent this as far as I know.
3. Result: your endpoints are still encrypted, you have no way of decrypting them, and your enpdoints are unusable.

I have not tested this scenario manually but by reading the code I believe this would be the case.

vijaycs85’s picture

#47: True, but I don't see any way we could prevent this as explained in #41. Even though it is possible in Drupal 8, seems it causes more issues. see #2899478: Uninstall validator makes uninstall impossible in some circumstances.

@Manuel Garcia, I am open for any alternative solution here.

dixon_’s picture

Patch looks good generally speaking. I'm ok with being pragmatic and ignoring the issue raised in #34 if no one can think of a solution :)

If some does want to fix it, here's one approach:

if (module_enabled('encrypt') {
  variable_set('encrypt_module_was_enabled', TRUE);
}
[...]
if (variable_get('encrypt_module_was_enabled') && !module_enabled('encrypt')) {
  drupal_set_message('Your endpoints are broken');
}

Keeping this issue as "Needs work" because this namespacing issue should be fixed:

+/**
+ * Set default php encryption state.
+ */
+function _set_deploy_php_encryption_state() {
+  if (!variable_get('deploy_php_encryption_state', FALSE)) {
+    variable_set('deploy_php_encryption_state', TRUE);
+  }
+}
+
+/**
+ * Get default php encryption state.
+ */
+function _get_deploy_php_encryption_state() {
+  if (variable_get('deploy_php_encryption_state')) {
+    return TRUE;
+  }
+  return FALSE;
+}
vijaycs85’s picture

Thanks, @dixon_. Here is an update:

If some does want to fix it, here's one approach:

FIXED: thanks, added. now both _deploy_encrypt and _deploy_decrypt would check for this variable before fallback to base64.

this namespacing issue should be fixed

FIXED.

timmillwood’s picture

Status: Needs review » Fixed

committed

timmillwood’s picture

@vijaycs85 Can you verify all looks good, then we can do a release next week?

timmillwood’s picture

Crediting all the people.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.