I need to have a way to export my bind passwords using features. I see that it's not exportable via hook_schema, and I completely understand why, but I think we can find a workable solution. I'm more than willing to write the code, I just wanted to get the maintainer's opinion.

I see 3 possibilties:

  • Just remove the 'no export' from hook_schema. It's the easiest to do, and assumes that the user knows what they're doing.
  • If encryption is enabled, then allow exporting. This is my personal favorite, but I need to make sure that variable_get() is fully bootstrapped when hook_schema() is called.
  • Create a add-on module. I could create a new module, ldap_exportable_passwords that implemented hook_schema_alter() and override it from there.

Thoughts?

CommentFileSizeAuthor
#4 ldap-1206500-4-exportable-bindpw.patch905 bytesjustintime
#2 ldap-1206500-2-exportable-passwords.patch585 bytesjustintime

Comments

johnbarclay’s picture

johnbarclay commented

The first 2 are fine with me. The 3rd seems like overkill. This is definately a must since exportables used for automated builds.

The problem with the second option is the encryption data is also in the exportable data so it gives a false sense of security. Option 1 with a good warning message would be better to me than option 2, but either is fine.

justintime’s picture

justintime commented
StatusFileSize
new ldap-1206500-2-exportable-passwords.patch585 bytes

The problem with the second option is the encryption data is also in the exportable data so it gives a false sense of security.

The key isn't exported with the server by default. The encryption key is stored in a strongarm variable, and that's not included by default when you export a LDAP server. In my case, the key is in a separate feature from the server itself.

In the case of option 1, where would you put the warning? Just in the documentation?

I'm attaching a patch that does #2, hooray for 1-line patches :) If you'd rather go the #1 route, let me know how you want to do it, and I'll whip that up.

johnbarclay’s picture

johnbarclay commented
Status: Active » Fixed

I applied this. Its in head. Thanks.

justintime’s picture

justintime commented
Status: Fixed » Needs review
StatusFileSize
new ldap-1206500-4-exportable-bindpw.patch905 bytes

So, it turns out that patch doesn't work well in most situations. Usually, someone using features doesn't actually have variables set in the DB, they're set with strongarm.

Well, in the case of hook_schema(), when variable_get() is called, strongarm hasn't loaded yet -- it loads via hook_init(). So, if you manually set the variable so it's present in the db, this patch works correctly. However, in most features-based deployements, this won't be the case.

I'm attaching a patch that just makes it exportable by default.

johnbarclay’s picture

johnbarclay commented
Status: Needs review » Fixed

I applied the patch and pushed out to head. thanks.

Another workable approach is to actually have 2 fields: bindpw and bindpw_ecrypted and only make the second exportable. Maybe in 7.x-2.x this could be the way to go. It would also simplify some code.

justintime’s picture

justintime commented

I agree on the 2 fields approach - it would make things much easier.

Status: Fixed » Closed (fixed)

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