New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generation of Role keys & Share keys #4

Merged
merged 10 commits into from Jun 22, 2016

Conversation

Projects
None yet
3 participants
@talhaparacha
Copy link
Contributor

talhaparacha commented Jun 12, 2016

  • Implements a key provider.
  • The key provider doesn't store a key value (i.e. role key) in its actual form, but encrypts the key value with public keys of all users from a specified role and then stores these multiple encrypted copies (i.e. share keys) in the configuration system.
  • The key provider retrieves the actual key value by using the temporarily stored Private key of a user.
  • Generates a role key for each role upon module installation, updates the role key upon a new user addition/removal from that role, adds a role key upon role creation, removes the role key upon role removal.

Updates over the PR till 6/16:

  • All users with "administer permissions" permission are given complete control over all Role keys.
  • Tests for Role keys management have been added. Testing is not very thorough at this moment.
@@ -60,4 +60,7 @@ function pubkey_encrypt_install() {
->removeComponent('field_private_key')
->removeComponent('field_private_key_protected')
->save();

// Generate Role keys.
$pubkey_encrypt_manager->generateRoleKeys();

This comment has been minimized.

@nerdstein

nerdstein Jun 12, 2016

Contributor

This should be injtializeRoleKeys since it will do all roles

This comment has been minimized.

@talhaparacha

talhaparacha Jun 12, 2016

Author Contributor

Agreed. Will do it.

This comment has been minimized.

@talhaparacha

talhaparacha Jun 14, 2016

Author Contributor

Done.

$roleOptions[$role->id()] = $role->label();
}
unset($roleOptions[AccountInterface::ANONYMOUS_ROLE]);
unset($roleOptions[AccountInterface::AUTHENTICATED_ROLE]);

This comment has been minimized.

@nerdstein

nerdstein Jun 12, 2016

Contributor

So this does not apply for authenticated users?

// Retrieve the actual key value from the Share key of user.
$shareKeys = $this->configuration['share_keys'];
if (isset($shareKeys[$currentUserId])) {
$shareKey = base64_decode($shareKeys[$currentUserId]);

This comment has been minimized.

@nerdstein

nerdstein Jun 12, 2016

Contributor

Surprised to see this decoding here, why is that?

This comment has been minimized.

@talhaparacha

talhaparacha Jun 14, 2016

Author Contributor

Base64 encoding/decoding a stored is completely unnecessary here. I did it only because then viewing the key values from database/configuration system looked a bit nice if the keys are base64_encoded. We should remove it if it is affecting performance. What do you say?

This comment has been minimized.

@nerdstein

nerdstein Jun 16, 2016

Contributor

I would remove it to avoid confusion

This comment has been minimized.

@nerdstein

nerdstein Jun 16, 2016

Contributor

One less dependent thing

foreach ($users as $user) {
$userId = $user->get('uid')->getString();
$publicKey = $user->get('field_public_key')->getString();
openssl_public_encrypt($key_value, $shareKey, $publicKey);

This comment has been minimized.

@nerdstein

nerdstein Jun 12, 2016

Contributor

Still need to strip this out into a plugin

This comment has been minimized.

@nerdstein

nerdstein Jun 12, 2016

Contributor

I would have a parameter for the plugin selected so other modules can do so

This comment has been minimized.

@talhaparacha

talhaparacha Jun 14, 2016

Author Contributor

Yes. The Key Generator plugin will need to implement these three functions: generatePublicPrivateKeyPair() , encryptWithPublicKey() , decryptWithPrivateKey(). Once a plugin system is implemented, this logic would be encapsulated.

Umm hmm I'll see to it.

/*
* Generate a Role key.
*/
public function generateRoleKey(Role $role) {

This comment has been minimized.

@nerdstein

nerdstein Jun 12, 2016

Contributor

Possibly pass the plugin ID to help select the algorithm for key generation

This comment has been minimized.

@talhaparacha

talhaparacha Jun 14, 2016

Author Contributor

Indeed.

/*
* Generate Role keys upon module installation.
*/
public function generateRoleKeys() {

This comment has been minimized.

@nerdstein

nerdstein Jun 12, 2016

Contributor

Pass plugin ID for algorithm

This comment has been minimized.

@talhaparacha

talhaparacha Jun 14, 2016

Author Contributor

Indeed. Will add it once work on Key generator plugin system is started.

* Update a Role key.
*/
public function updateRoleKey(Role $role) {
// Since only root user has complete control over all keys, so allow for

This comment has been minimized.

@nerdstein

nerdstein Jun 12, 2016

Contributor

Also pass plugin Id for algorithm

This comment has been minimized.

@talhaparacha

talhaparacha Jun 14, 2016

Author Contributor

Noted.

@colans

This comment has been minimized.

Copy link
Member

colans commented Jun 13, 2016

The root user (uid = 1) is currently given complete control over all keys. It is assumed that this user and only this user will be adding/removing people from a role. Hence only this user has the complete control over all keys. If someone other than the root user modifies a role by adding/removing people from it, the corresponding Role key update doesn't take place.

Often these duties are delegated to others in an administrator role. So the behaviour should be the same for everyone in that role. In other words, role-key updates should also happen for these other users when they modify a role. See /admin/config/people/accounts for details, the "Administrator role" setting.

* Implements hook_user_role_delete().
*/
function pubkey_encrypt_user_role_delete(\Drupal\user\RoleInterface $role) {
// Cater for Role deletion.

This comment has been minimized.

@colans

colans Jun 13, 2016

Member

A better comment here would be something like: "Delete the role keys." as it explains exactly what you're doing.

This comment has been minimized.

@talhaparacha

talhaparacha Jun 14, 2016

Author Contributor

Done

* Implements hook_user_role_insert().
*/
function pubkey_encrypt_user_role_insert(\Drupal\user\RoleInterface $role) {
// Cater for Role creation.

This comment has been minimized.

@colans

colans Jun 13, 2016

Member

How about something like "Add keys for the new role."?

This comment has been minimized.

@talhaparacha

talhaparacha Jun 14, 2016

Author Contributor

Done

->getStorage('user')
->loadByProperties(['roles' => $role]);
// Allow root user control over all keys irrespective of his role.
// @todiscuss WE COULD GIVE THIS PREVILIGE TO ALL USERS WITH "ADMINISTER_KEYS" PERMISSION.

This comment has been minimized.

@colans

colans Jun 13, 2016

Member

Yes, that's a great idea as delegation is always a possibility.

public function generateRoleKeys() {
// Generate a Role key per role.
foreach (Role::loadMultiple() as $role) {
if ($role->id() != AccountInterface::ANONYMOUS_ROLE && $role->id() != AccountInterface::AUTHENTICATED_ROLE) {

This comment has been minimized.

@colans

colans Jun 13, 2016

Member

Same question as earlier. Why not authenticated?

public function updateRoleKey(Role $role) {
// Since only root user has complete control over all keys, so allow for
// Role key updates only if the root user is logged in.
if (\Drupal::currentUser()->id() == '1') {

This comment has been minimized.

@colans

colans Jun 13, 2016

Member

Should be expanded to other admin users with the correct permission.

// Role key updates only if the root user is logged in.
if (\Drupal::currentUser()->id() == '1') {
// Since we don't have a Role key for "authenticated" role.
if ($role->id() != AccountInterface::AUTHENTICATED_ROLE) {

This comment has been minimized.

@colans

colans Jun 13, 2016

Member

This could use an explanation.

@nerdstein nerdstein merged commit aa690f7 into d8-contrib-modules:8.x Jun 22, 2016

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details

@talhaparacha talhaparacha deleted the talhaparacha:integrate-with-key branch Jul 24, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment