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.
Comment | File | Size | Author |
---|---|---|---|
#50 | 2837992-diff-42-50.txt | 3.17 KB | vijaycs85 |
#50 | 2837992-50.patch | 19.21 KB | vijaycs85 |
Comments
Comment #2
harshil.maradiya CreditAttribution: harshil.maradiya commentedComment #3
harshil.maradiya CreditAttribution: harshil.maradiya commentedComment #4
harshil.maradiya CreditAttribution: harshil.maradiya commentedPlease find the updated patch
Comment #5
harshil.maradiya CreditAttribution: harshil.maradiya commentedComment #6
harshil.maradiya CreditAttribution: harshil.maradiya commentedrectifying errors
Comment #7
harshil.maradiya CreditAttribution: harshil.maradiya commentedupdated with few more fixes
Comment #8
harshil.maradiya CreditAttribution: harshil.maradiya commentedComment #9
prashantgajare CreditAttribution: prashantgajare as a volunteer and commentedTested 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.
Comment #10
harshil.maradiya CreditAttribution: harshil.maradiya commentedComment #11
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedThanks 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:
Indentation is off.
Added spaces on empty line.
@return should leave an empty line before it.
Missing space after if statement
Comment #12
prashantgajare CreditAttribution: prashantgajare as a volunteer and commented@manuel - Fixed the CS issues. Thanks! :)
Comment #13
prashantgajare CreditAttribution: prashantgajare as a volunteer and commentedComment #14
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedThanks @prashantgajare for that, patch is looking good, however I'd like more reviews, and other maintainers' signoff on this before we commit it.
Comment #15
rogerpfaffIf I import this in another site will it then work without doing anything or will the encrypted password be used as new password?
Comment #16
harshil.maradiya CreditAttribution: harshil.maradiya commented@rogerpfaff The password will remain the same , but in database the encrypted password will get store
Comment #18
timmillwoodI 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."?
We don't need this whitespace before the if.
Comment #19
timmillwoodToo many spaces after
=
.Comment #20
harshil.maradiya CreditAttribution: harshil.maradiya at TATA Consultancy Services for Pfizer, Inc. commentedComment #21
harshil.maradiya CreditAttribution: harshil.maradiya at TATA Consultancy Services for Pfizer, Inc. commentedFixing interdiff comments
Comment #22
prashantgajare CreditAttribution: prashantgajare as a volunteer and commentedComment #24
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedLooks like the test bot blew up, lets try again...
Comment #25
prashantgajare CreditAttribution: prashantgajare as a volunteer and at TATA Consultancy Services for Pfizer, Inc. commentedFixing the Interdiff #23
Comment #26
prashantgajare CreditAttribution: prashantgajare as a volunteer and at TATA Consultancy Services for Pfizer, Inc. commentedComment #27
vijaycs85Probably 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.
@file description is missing.
unnecessary empty spaces.
Comment #28
harshil.maradiya CreditAttribution: harshil.maradiya at TATA Consultancy Services for Pfizer, Inc. commented@Vijay, Thanks for feedback I have implemented 1st 3 concerns
Comment #29
harshil.maradiya CreditAttribution: harshil.maradiya at TATA Consultancy Services for Pfizer, Inc. commentedComment #30
vijaycs85Thanks, @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)Comment #31
harshil.maradiya CreditAttribution: harshil.maradiya at TATA Consultancy Services for Pfizer, Inc. commentedDear Vijay,
Thanks for detailed explanation i have made required changes
Comment #32
harshil.maradiya CreditAttribution: harshil.maradiya at TATA Consultancy Services for Pfizer, Inc. commentedDear Vijay,
Please find updated patch and interdiff
Comment #33
vijaycs85It looks good to me. Setting to RTBC to see if we can get a maintainer review.
Comment #34
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedWe 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.
Comment #35
harshil.maradiya CreditAttribution: harshil.maradiya at TATA Consultancy Services for Pfizer, Inc. commentedComment #36
harshil.maradiya CreditAttribution: harshil.maradiya at TATA Consultancy Services for Pfizer, Inc. commentedComment #37
vijaycs85not sure if it's a good idea to delete the variable every time. why can't we set it to false?
if (!variable_get('deploy_php_encryption_state')) {
Use
FALSE
as default value here?Comment #38
harshil.maradiya CreditAttribution: harshil.maradiya at TATA Consultancy Services for Pfizer, Inc. commentedComment #39
vijaycs85Looks 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.
Comment #40
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedThanks for following up on this. I believe at least #34 needs to be addressed still.
Comment #41
vijaycs85#40: Its going to be very tricky because
hook_modules_disabled
andhook_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 functiondecrypt
doesn't exist anymore.Comment #42
harshil.maradiya CreditAttribution: harshil.maradiya at TATA Consultancy Services for Pfizer, Inc. commentedRemoving enable hook so it could not end up in double encryption
Comment #43
harshil.maradiya CreditAttribution: harshil.maradiya at TATA Consultancy Services for Pfizer, Inc. commentedComment #44
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedThanks @harshil.maradiya - looks like the last interdiff didnt come out correct, would you mind providing a valid one to facilitate reviews please?
Comment #45
harshil.maradiya CreditAttribution: harshil.maradiya at TATA Consultancy Services for Pfizer, Inc. commentedThanks, uploading correct inter diff
Comment #46
vijaycs85looks good to go.
Comment #47
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedThe 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.
Comment #48
vijaycs85#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.
Comment #49
dixon_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:
Keeping this issue as "Needs work" because this namespacing issue should be fixed:
Comment #50
vijaycs85Thanks, @dixon_. Here is an update:
FIXED: thanks, added. now both
_deploy_encrypt
and_deploy_decrypt
would check for this variable before fallback to base64.FIXED.
Comment #51
timmillwoodcommitted
Comment #52
timmillwood@vijaycs85 Can you verify all looks good, then we can do a release next week?
Comment #53
timmillwoodCrediting all the people.