The Townsend Security Key Connection provides an integration between the Encrypt Module and Townsend Security's Alliance Key Manager. This allows for PCI compliant remote key management as well as remote on-board encryption that meets NIST standards. This helps strengthen Drupal servers requiring encryption as they keys are not in the same location as the site files. This helps sites conform to PCI and other regulatory security standards.
More can be found at the sandbox: https://drupal.org/sandbox/CellarDoor/2087261
Git repo: git clone --branch 7.x-1.x git.drupal.org:sandbox/CellarDoor/2087261.git townsend_security_key_connection
Thanks for taking the time to review!
[D7] Commerce Attributes Date
D7 Commerce Postmaster
[D7] Civi Bartik
Comments
Comment #1
PA robot commentedThere are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxCellarDoor2087261git
We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)
Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).
I'm a robot and this is an automated message from Project Applications Scraper.
Comment #2
cellar door commentedMade many of the recommended changes and committed to 7.x-1.x branch
Comment #3
rameshrasaiyan commentedHi,
I cloned your code and did a manual code review.
You have used dpm() to see the output in your .module and .inc files. This will cause some issues if the devel module has not installed.
Note:- First I checked out the 7.x-1.x branch code and I can see only the townsec_key related files. I was able to see all your code after checking out to master. So as per the standard, push all your changes to the 7.x-1.x branch and delete your master branch.
Comment #4
cellar door commentedHi Rameshrasaiyan -
Thanks for taking time to review the code! The master branch has dev code on it and isn't the most up to date. The 7.x-1.x branch is the most up to date. We cleaned up the development code in the master branch and then published it as 7.x-1.x . The 7.x-1.x branch is the one to be considered for review. I will delete the master branch in order to make sure there is no confusion.
Comment #5
Enxebre commentedHi,
becouse your .install file is empty I think you should delete it.
There are still a lot of calls to dpm() function in plugins/encryption_methods/twonsec_aes.inc
I´ve seen you are using user_password(16); function so you should add user module dependency in .info file.
Hope this helps.
Regards!
Comment #6
cellar door commentedThanks for the review - those slipped their way through :) Removed and updated!
Comment #7
cellar door commentedAdded in new README.txt with some basic instructions to fit Drupal best practices and also added the User module to the dependencies in the .info file.
Comment #8
kamleshpatidar commentedHi,
I have manually review your code and i found some issue with Drupal Coding Standard. Although they are minor, but you must adhere to Drupal Coding Standard. You must locally check your project with Coder module for Drupal Best practices and Coding Statndard.If you need help, i can help you out.
Best of Luck.
Comment #9
cellar door commentedAlright - went through and did a coding standards edit. All should be good to go now (only 2 issues but they are minor around using fputs() instead of fwrite() but that's what the manufacturer recommends using)
Comment #9.0
cellar door commentedupdated git repo location
Comment #10
kscheirerThose 2 are minor errors, but they are blocking. Otherwise this issue is RTBC from me.
----
Top Shelf Modules - Crafted, Curated, Contributed.
Comment #11
cellar door commentedThanks for the review Kscheirer! I've updated the project page (didn't know I had to do that prior to launching) and updated the code so that the pareview.sh comes out 100% clean.
Let me know if there's anything else I can do to help this get through! I'll let you RTBC it if you agree it's ready.
Comment #12
cellar door commentedComment #13
kscheirerLooks good, thanks!
----
Top Shelf Modules - Crafted, Curated, Contributed.
Comment #14
cellar door commentedComment #15
klausimanual review:
Otherwise looks good to me, so ...
Thanks for your contribution, Cellar Door!
I promoted this project for you: https://drupal.org/project/townsec_key
Now that this experimental project has been promoted, you'll need to update the URL of your remote repository or reclone it.
Here are some recommended readings to help with excellent maintainership:
You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and stay involved!
Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.
Thanks to the dedicated reviewer(s) as well.
Comment #16
cellar door commentedKlausi -
Thank you for taking the time to review the module and setting it to full status.
To answer your questions:
1 and 2: The two pieces you commented on are due to the way the module handles encryption (16 byte blocks at a time) so it has to be padded with spaces in order to process correctly. I have a patch in dev that allows for greater than 16 characters to be encrypted and am working on patching that in now (wasn't wanting to introduce more code mid-way through review).
As for the not fully vetted part I guess I'm not fully understanding that piece. I understand it's to keep someone with a simple module from having full write permissions but I think I fail to see how it applies in this case. Yes this module only has 4 functions ( I had others for encryption testing etc. but took them out for review) but in those 4 functions you span 350+ lines of code, connect into ctools and the encrypt API and handle some pretty complex processing. So yes this project doesn't meet the 5 function rule (If I had known that I would have just refactored it with a few extra functions). I'm planning on adding a commerce add on to add encryption tie ins there but to be honest if I have to go through the review process again on a module like that It will likely discourage me from going that route.
While I understand the reasoning for having to prove your merit to become a "full contributor" I think if you look over the functions of this module and some of the other contributions I've had elsewhere in the community you'd see I'm not trying to game the system here. For someone who has been a contributor for quite some time to Drupal I have to admit this process is discouraging at best and I can only imagine what it's like for someone not involved like I am. If it's help meet the rules, I'll add back in my testing functions to get over the 5 function limit and get full status (it'd be helpful for me as I'm looking at helping to co-maintain some older versions of modules).