Encrypt secrets in database

[rchavik]

This changeset addresses issue #26:

• Enlarging secret columns. I'm not sure what's the correct length for those fields, but I picked 132 since that seem to be the minimum length when used with rijndael in my dev machine.
• While we're changing schema, I added name, created and modified fields for clients
• Extract out the hashing method into OAuthUtility class and trying to be BC. This class would also later be used to implement the oauth-php interface methods.
• Add a new shell to create and list client records. It can display decrypted secrets with --secret. I think this is quite handy.

Note:

Since this change is backward incompatible, ie changing schema and requiring secrets to be reissued, this
should go into a new branch. I think we should tag a version on master first, then create a separate branch for
the version

[rchavik]

Above chunk actually depends on #34

[rchavik]

sigh

needs more work and testing

[thomseddon]

I really like this changeset

• Shell - great idea, this would be significantly more useful than the current "addclient" with no clear method to use it. I know this has caused confusion in the past so I would certainly like to bring this in
• Encryption - I like the implementation in OAuthUtility. Perhaps it would make more sense to encrypt/decrypt the fields in the model, that way if you need/want to use the model you get a consistent interface and you don't have to worry about encrypting/decrypting yourself?

[rchavik]

I've continued working on this and making more changes here. So far, I come to the following conclusion:

• Client.client_secret can be encrypted, while others secrets like access_token, refresh_token, auth_code
  should only be hashed. I've adapted this locally.
• Having OAuth.encrypt might not be a good idea as it will complicate the code further.
  As this is already a BC breaking change, do you think we should simply mark it as such and not keeping
  backward compatibility? In my local branch it's already gone. However, if you require it, I could bring it back.

We don't have test yet, so I'm continually rebasing my branch to tests it against my croogo branch for testing.
I hope to write tests once we completed the refactor.

Encryption - I like the implementation in OAuthUtility. Perhaps it would make more sense to encrypt/decrypt
the fields in the model, that way if you need/want to use the model you get a consistent interface and you don't
have to worry about encrypting/decrypting yourself?

There are two scenarios that I can think of: beforeFind() and afterFind().
Having the following in beforeFind() for models having hashed secrets could be useful:

public function beforeFind($queryData) {
  if (isset($queryData['conditions']['access_token'])) {
    $queryData['conditions']['client_secret'] = OAuthUtilty::hash($....['access_token'])
  }
  return $query;
}

But I'm not to sure of automatically decrypting client_secret in Client::afterFind().
Queries for Client usually is performed against client_id. And decrypting is only needed for viewing secrets in admin pages. Perhaps we should not auto-decrypt for every find call.

Anyway, I hope to an update to this pull this afternoon.

[rchavik]

This depends on the fix-client-secret PR.

The actual diff is: ca91a4a...1913409

The reroll is backward incompatible and contains the following changes:

• No more config OAuth.encrypt. All client_secrets must be reencrypted.
• Compared to the previous attempt, only client_secret is encrypted. auth_code, access_token, and refresh_token are hashed.

Ideally, access_token should also be encrypted. But we'll need to encrypt with with fixed iv which is not good practice afaik. With auto-generated iv, it's difficult to retrieve access_token because the encrypted value will be different each time.

Since, we're only encrypted Client.client_secret, I didn't make it a behavior.

[thomseddon]

But I'm not to sure of automatically decrypting client_secret in Client::afterFind().
Queries for Client usually is performed against client_id. And decrypting is only needed for viewing secrets in admin pages. Perhaps we should not auto-decrypt for every find call.

Very good point but I think we can get round it as we would only decrypt if it was included in the find fields, so the most common code path that would use it is getClientCredentials, and here it doesn't ask for client secret. Hence, afterFind would be more like:

public function afterFind($results, $primary = false) {
    foreach ($results as $key => $val) {
        if (isset($val[$this->alias]['client_secret'])) {
            $results[$key][$this->alias]['client_secret'] = OAuthUtilty::decrypt($val[$this->alias]['client_secret']);
        }
    }
    return $results;
}

Ideally, access_token should also be encrypted. But we'll need to encrypt with with fixed iv which is not good practice afaik. With auto-generated iv, it's difficult to retrieve access_token because the encrypted value will be different each time.

The iv is derived from the key (the salt in the current implementation). The IV does not need to be secret, but does need to be unique (as Cake uses the CBC block cipher) so to get round this, we could either add an auto increment field to the clients table and use that zero padded (e.g. 000000000000001) or we could use something random (and again store it in the table) or use the client_id, although I suspect people override this and the default implementation isn't even very random.

My preference would be the zero padded id, but this does add a level of complexity.

[rchavik]

The iv is derived from the key (the salt in the current implementation). The IV does not need to be secret, but does need to be unique (as Cake uses the CBC block cipher) so to get round this, we could either add an auto increment field to the clients table and use that zero padded (e.g. 000000000000001) or we could use something random (and again store it in the table) or use the client_id, although I suspect people override this and the default implementation isn't even very random.

This has been changed recently. The IV will be automatically created using mcrypt_create_iv().

If we want to pursue access tokens encryption, we would need to supply our own encrypt/decrypt methods internally in OAuthUtility with something that uses a fixed IV.

[thomseddon]

Why can't we just use Security::rijndael with the random iv?

And, i'm confused, a previous comment outlines the following behaviours:

• encrypted: client_secret
• hashed: auth_code, access_token, and refresh_token

But this diff only covers client_secret encryption?

[rchavik]

Why can't we just use Security::rijndael with the random iv?

Since using random IV will cause encrypted value of $access_token be different each time, the proposed implementation for OAuthUtility::getAccessToken($oauth_token) will not work because it won't ever find at match:

    public function getAccessToken($oauth_token) {
        $accessToken = $this->AccessToken->find('first', array(
            'conditions' => array('oauth_token' => $oauth_token),
            'recursive' => -1,
        ));
        if ($accessToken) {
            return $accessToken['AccessToken'];
        }
        return null;
    }

We will need to know the client_id to get the encrypted access_tokens.access token.
Then decrypt it to compare against $oauth_token from the request.

And, i'm confused, a previous comment outlines the following behaviours:

encrypted: client_secret
hashed: auth_code, access_token, and refresh_token

But this diff only covers client_secret encryption?

Yes, The hashing changes is backward compatible and has been merged in ca91a4a.

[thomseddon]

We will need to know the client_id to get the encrypted access_tokens.access token.
Then decrypt it to compare against $oauth_token from the request.

The access token is hashed, not encrypted? This would be true for getClientCredentials, however we could modify the implementation to get the client by id, decrypt the secret and check it matches?

[rchavik]

We will need to know the client_id to get the encrypted access_tokens.access token.
Then decrypt it to compare against $oauth_token from the request.

The access token is hashed, not encrypted?

Yes. At the time I made this PR, it's only hashed. This particular PR enable encryption of clients.client_secret.

This would be true for getClientCredentials, however we could modify the implementation to get the client by id,
decrypt the secret and check it matches?

I'm not very familiar with OAuth2 protocol, but my understanding is that a request only need either access_token GET parameter or an Authorization header. Hence, the proposed OAuthAuthenticate::authenticate() (method)[https://github.com/xintesa/cakephp-oauth-server/blob/next/Controller/Component/Auth/OAuthAuthenticate.php#L38] only checks for those parameters. If you follow it down to getUser() and _hasCredentials(), it only has knowledge about access_token and not client_id.

That's basically what I'm having problem with.

If this is allowed by the spec, I can certainly adjust it so that we can encrypt both client_secret and access_token.

[thomseddon]

I think we (me) may have our wires crossed - in my opinion, the ideal would be to encrypt client_secret and hash access_token. Where we are at the moment is that access_token's are hashed (I do not want to change this) and client_secret is encrypted.

If the IV issue has been resolved in cake core (as per your link), then the only thing I think needs changing is to add automatic decryption of client_secret in afterFind (as per my previous comment)

[rchavik]

I think we (me) may have our wires crossed - in my opinion, the ideal would be to encrypt client_secret and hash access_token. Where we are at the moment is that access_token's are hashed (I do not want to change this) and client_secret is encrypted.

Ah, okay then. That's exactly where we will be when this gets in.

If the IV issue has been resolved in cake core (as per your link), then the only thing I think needs changing is to add automatic decryption of client_secret in afterFind (as per my previous comment)

Yes. The IV is fixed in the core. I will adjust this PR as per your comment as soon as time permits.

[thomseddon]

Awesome - sorry for the confusion

[dereuromark]

@rchavik Is this still planned?

[rchavik]

@dereuromark I don't think I'll be able to polish this further to a mergable PR. Also, I'm not sure yet how it would fit/look with the next version that thom had in mind.