From b226d1878f729bad02cb8b0bba35629b06455521 Mon Sep 17 00:00:00 2001 From: Rachman Chavik Date: Mon, 19 Aug 2013 08:44:38 +0700 Subject: [PATCH 1/6] Migrations: enlarge columns for encrypted values --- .../Migration/1376749679_enlarge_secrets.php | 59 +++++++++++++++++++ 1 file changed, 59 insertions(+) create mode 100644 Config/Migration/1376749679_enlarge_secrets.php diff --git a/Config/Migration/1376749679_enlarge_secrets.php b/Config/Migration/1376749679_enlarge_secrets.php new file mode 100644 index 0000000..95b8ac2 --- /dev/null +++ b/Config/Migration/1376749679_enlarge_secrets.php @@ -0,0 +1,59 @@ + array( + 'alter_field' => array( + 'clients' => array( + 'client_secret' => array('type' => 'string', 'null' => false, 'default' => NULL, 'length' => 132, 'collate' => 'utf8_general_ci', 'charset' => 'utf8', 'after' => 'client_id'), + ), + ), + ), + + 'down' => array( + 'alter_field' => array( + 'clients' => array( + 'client_secret' => array('type' => 'string', 'null' => false, 'default' => NULL, 'length' => 40, 'collate' => 'utf8_general_ci', 'charset' => 'utf8', 'after' => 'client_id'), + ), + ), + ), + ); + +/** + * Before migration callback + * + * @param string $direction, up or down direction of migration process + * @return boolean Should process continue + * @access public + */ + public function before($direction) { + return true; + } + +/** + * After migration callback + * + * @param string $direction, up or down direction of migration process + * @return boolean Should process continue + * @access public + */ + public function after($direction) { + return true; + } + +} From b89c0b33c9cc6cc535bb41563c166c0d98a58353 Mon Sep 17 00:00:00 2001 From: Rachman Chavik Date: Mon, 19 Aug 2013 09:36:34 +0700 Subject: [PATCH 2/6] Migrations: additional fields to clients table --- ...376879144_clients_add_name_date_fields.php | 80 +++++++++++++++++++ 1 file changed, 80 insertions(+) create mode 100644 Config/Migration/1376879144_clients_add_name_date_fields.php diff --git a/Config/Migration/1376879144_clients_add_name_date_fields.php b/Config/Migration/1376879144_clients_add_name_date_fields.php new file mode 100644 index 0000000..ed12008 --- /dev/null +++ b/Config/Migration/1376879144_clients_add_name_date_fields.php @@ -0,0 +1,80 @@ + array( + 'create_field' => array( + 'clients' => array( + 'name' => array( + 'type' => 'string', + 'null' => false, + 'default' => null, + 'length' => 256, + 'collate' => + 'utf8_general_ci', + 'charset' => 'utf8', + 'after' => 'client_id', + ), + 'created' => array( + 'type' => 'datetime', + 'after' => 'user_id', + 'null' => true, + ), + 'modified' => array( + 'type' => 'datetime', + 'after' => 'created', + 'null' => true, + ), + ), + ), + ), + + 'down' => array( + 'drop_field' => array( + 'clients' => array( + 'name', + 'created', + 'modified', + ), + ), + ), + ); + +/** + * Before migration callback + * + * @param string $direction, up or down direction of migration process + * @return boolean Should process continue + * @access public + */ + public function before($direction) { + return true; + } + +/** + * After migration callback + * + * @param string $direction, up or down direction of migration process + * @return boolean Should process continue + * @access public + */ + public function after($direction) { + return true; + } + +} From c35420358d5b0bc10be14b6c8fc1d054fb088f7c Mon Sep 17 00:00:00 2001 From: Rachman Chavik Date: Sat, 17 Aug 2013 20:39:53 +0700 Subject: [PATCH 3/6] Adding OAuthUtility class Currently, this class implements the shortcut for [en|de]cryption methods. In the future, this class would be used for the oauth-php interface implementations. --- Lib/OAuthUtility.php | 41 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) create mode 100644 Lib/OAuthUtility.php diff --git a/Lib/OAuthUtility.php b/Lib/OAuthUtility.php new file mode 100644 index 0000000..5b93a62 --- /dev/null +++ b/Lib/OAuthUtility.php @@ -0,0 +1,41 @@ + Date: Sat, 17 Aug 2013 20:52:26 +0700 Subject: [PATCH 4/6] Encrypt secrets instead of hashing --- Controller/Component/OAuthComponent.php | 12 +++++++----- Model/Behavior/HashedFieldBehavior.php | 6 +++--- Model/Client.php | 19 +++++++++++-------- README.markdown | 7 ++++++- 4 files changed, 27 insertions(+), 17 deletions(-) diff --git a/Controller/Component/OAuthComponent.php b/Controller/Component/OAuthComponent.php index df1fd6e..a7ec7bf 100644 --- a/Controller/Component/OAuthComponent.php +++ b/Controller/Component/OAuthComponent.php @@ -399,15 +399,17 @@ public function __call($name, $arguments) { */ public function checkClientCredentials($client_id, $client_secret = null) { $conditions = array('client_id' => $client_id); - if ($client_secret) { - $conditions['client_secret'] = $client_secret; - } $client = $this->Client->find('first', array( 'conditions' => $conditions, 'recursive' => -1 )); - if ($client) { - return $client['Client']; + if ($client && $client_secret) { + $decrypted = self::decrypt($client['Client']['client_secret']); + if ($decrypted == $client_secret) { + return $client['Client']; + } else { + return false; + } }; return false; } diff --git a/Model/Behavior/HashedFieldBehavior.php b/Model/Behavior/HashedFieldBehavior.php index 8401761..a6dc16e 100644 --- a/Model/Behavior/HashedFieldBehavior.php +++ b/Model/Behavior/HashedFieldBehavior.php @@ -1,7 +1,7 @@ data[$Model->alias])) { $data = $Model->data[$Model->alias][$field]; - $Model->data[$Model->alias][$field] = Security::hash($data, null, true); + $Model->data[$Model->alias][$field] = OAuthUtility::hash($data); } } return true; @@ -52,7 +52,7 @@ public function beforeFind(Model $Model, $queryData) { } if (isset($queryField)) { $data = $conditions[$queryField]; - $conditions[$queryField] = Security::hash($data, null, true); + $conditions[$queryField] = OAuthUtility::hash($data); } } return $queryData; diff --git a/Model/Client.php b/Model/Client.php index 1331cbb..5b56f88 100644 --- a/Model/Client.php +++ b/Model/Client.php @@ -1,6 +1,7 @@ array( - 'fields' => array( - 'client_secret' - ), - ), - ); - /** * hasMany associations * @@ -164,6 +157,16 @@ public function newClientSecret() { return OAuthComponent::hash($str); } +/** + * Encrypt client secret + */ + public function beforeSave($options) { + if (isset($this->data[$this->alias]['client_secret'])) { + $this->data[$this->alias]['client_secret'] = OAuthUtility::encrypt($this->data[$this->alias]['client_secret']); + } + return true; + } + public function afterSave($created) { if ($this->addClientSecret) { $this->data['Client']['client_secret'] = $this->addClientSecret; diff --git a/README.markdown b/README.markdown index 806a7bf..c50e947 100644 --- a/README.markdown +++ b/README.markdown @@ -121,7 +121,12 @@ Array( The method includes various schemes for generating client id's, [pick your favourite](https://github.com/thomseddon/cakephp-oauth-server/blob/master/Model/Client.php#L122). -**NOTE:** This convenience method will generate a random client secret __and hash it__ for security before storage. Although it will pass back the actual raw client secret when you first add a new client, it is not possible to ever determine this from the hash stored in the database. So if the client forgets their secret, [a new one will have to be issued](https://github.com/thomseddon/cakephp-oauth-server/blob/master/Model/Client.php#L139). +**NOTE:** This convenience method will generate a random client secret +__and encrypt it__ for security before storage using `Security::rijndael()` +method. The default behavior is to use `Security.salt` as encryption key. +Thus, it's important to treat it as __extremely sensitive data__. + +Use `OAuthUtility::decrypt()` to obtain the client_secret. ### Included Endpoints From b2b59114839267916177048236e27ae2396fddb9 Mon Sep 17 00:00:00 2001 From: Rachman Chavik Date: Wed, 21 Aug 2013 07:49:52 +0700 Subject: [PATCH 5/6] Deprecate OAuthComponent::hash() --- Controller/Component/OAuthComponent.php | 1 + Model/Client.php | 2 +- Model/OAuthAppModel.php | 2 ++ 3 files changed, 4 insertions(+), 1 deletion(-) diff --git a/Controller/Component/OAuthComponent.php b/Controller/Component/OAuthComponent.php index a7ec7bf..c15e472 100644 --- a/Controller/Component/OAuthComponent.php +++ b/Controller/Component/OAuthComponent.php @@ -326,6 +326,7 @@ public function user($field = null, $token = null) { * * @param string $password * @return string Hashed password + * @deprecated Will be removed in future version */ public static function hash($password) { return Security::hash($password, null, true); diff --git a/Model/Client.php b/Model/Client.php index 5b56f88..9a0e639 100644 --- a/Model/Client.php +++ b/Model/Client.php @@ -154,7 +154,7 @@ public function newClientSecret() { while ($length--) { $str .= $chars[mt_rand(0, $count - 1)]; } - return OAuthComponent::hash($str); + return OAuthUtility::hash($str); } /** diff --git a/Model/OAuthAppModel.php b/Model/OAuthAppModel.php index 48201f4..a971dd8 100644 --- a/Model/OAuthAppModel.php +++ b/Model/OAuthAppModel.php @@ -1,5 +1,7 @@ Date: Mon, 19 Aug 2013 10:49:13 +0700 Subject: [PATCH 6/6] ClientsShell for creating/listing client records Easier to test/list clients records --- Console/Command/ClientsShell.php | 130 +++++++++++++++++++++++++++++++ 1 file changed, 130 insertions(+) create mode 100644 Console/Command/ClientsShell.php diff --git a/Console/Command/ClientsShell.php b/Console/Command/ClientsShell.php new file mode 100644 index 0000000..09a4918 --- /dev/null +++ b/Console/Command/ClientsShell.php @@ -0,0 +1,130 @@ +description('Client Utility') + ->addSubCommand('list', array( + 'help' => 'List existing client records', + 'parser' => array( + 'options' => array( + 'secret' => array( + 'help' => 'Display secrets', + 'short' => 's', + 'boolean' => true, + ), + ), + ), + )) + ->addSubCommand('add', array( + 'help' => 'Add a new client', + 'parser' => array( + 'options' => array( + 'name' => array( + 'required' => true, + 'help' => 'Client Name', + 'short' => 'n', + ), + 'redirect_uri' => array( + 'required' => true, + 'help' => 'Redirect URI', + 'short' => 'u', + ), + ), + ), + )); + } + +/** + * Shell entry point + */ + public function main() { + $method = null; + if (isset($this->args[0])) { + $method = $this->args[0]; + } + + switch ($method) { + case 'list': + $this->_clients(); + break; + + default: + $this->_displayHelp(); + break; + } + } + +/** + * List all client records + */ + protected function _clients() { + $clients = $this->Client->find('all', array( + 'recursive' => -1, + )); + $this->out(""); + foreach ($clients as $data) { + $client = $data['Client']; + $this->out(sprintf('%-15s: %s', 'Client Id', $client['client_id'])); + $this->out(sprintf('%-15s: %s', 'Client Name', $client['name'])); + if ($this->params['secret']) { + $secret = OAuthUtility::decrypt($client['client_secret']); + $this->out(sprintf('%-15s: %s', 'Client Secret', $secret)); + } + $this->out(sprintf('%-15s: %s', 'Redirect URI', $client['redirect_uri'])); + $this->out(""); + } + $this->out(sprintf('%d record(s) found', count($clients))); + } + +/** + * Add a new client record + */ + public function add() { + if (empty($this->params['name'])) { + return $this->error('Please provide `name`'); + } + if (empty($this->params['redirect_uri'])) { + return $this->error('Please provide `redirect_uri`'); + } + if (!Validation::url($this->params['redirect_uri'])) { + return $this->error('Please provide a valid `redirect_uri`'); + } + $client = $this->Client->create(array( + 'name' => $this->params['name'], + 'redirect_uri' => $this->params['redirect_uri'], + )); + $client = $this->Client->add($client); + if (!$client) { + $this->err('Unable to add client record'); + if (isset($this->Client->validationErrors)) { + $this->error('Validation error', print_r($this->Client->validationErrors, true)); + } + return; + } + $this->out("Client successfully added:\n"); + $this->out(sprintf("\tClient id: %s", $client['Client']['client_id'])); + $this->out(sprintf("\tClient name: %s", $client['Client']['name'])); + $this->out(sprintf("\tClient secret: %s", $this->Client->addClientSecret)); + $this->out(); + } + +}