Store encrypted OAuth2 client secrets

[julien-nc]

This PR includes:

• A migration step that encrypts existing client secrets and changes the length of the secret column in oc_oauth2_clients
• 2 new methods (OCA\OAuth2\Db\Client::getRawSecret and OCA\OAuth2\Db\Client::setRawSecret) in the Client DB entity which are used in the oauth2 app instead of OCA\OAuth2\Db\Client::getSecret and OCA\OAuth2\Db\Client::setSecret

Maybe there's a more straightforward way to implement this.
I gave up on trying to redefine OCA\OAuth2\Db\Client::getSecret and OCA\OAuth2\Db\Client::setSecret because setSecret is used to build the Client entity so the value we get from the DB is encrypted again by setSecret and then stored in the entity attribute 😁.

Remaining doubts/questions:

• Is 256 chars long enough for an encrypted 64 chars string?
• Is it fine that this breaks any other apps directly using the OCA\OAuth2\Db\Client::getSecret and OCA\OAuth2\Db\Client::setSecret methods?

[marcelklehr]

Is it fine that this breaks any other apps directly using the OCA\OAuth2\Db\Client::getSecret and OCA\OAuth2\Db\Client::setSecret methods?

Quick search in nextcloud org only reveals: https://github.com/nextcloud/integration_openproject

[julien-nc]

Quick search in nextcloud org only reveals: https://github.com/nextcloud/integration_openproject

Yeah that's the one I found by searching for getSecret usages in my IDE.

[ChristophWurst]

For the software architecture I would suggest to leave the db layer like before and do the encryption/description in the service layer (controller in the oauth app case). That avoids adding logic and a non-testable service locator to the entity class.

[ChristophWurst]

Is it fine that this breaks any other apps directly using the OCA\OAuth2\Db\Client::getSecret and OCA\OAuth2\Db\Client::setSecret methods?

OCA is a private app namespace. We can change it at any time. Other apps are not supposed to access other apps this way but go through OCP

[ChristophWurst]

performance nitpicks

[julien-nc]

Rebased and fixed the failing SettingsControllerTest.
CI failures are not related IMO.

[julien-nc]

/backport to stable27

[julien-nc]

/backport to stable26

[julien-nc]

/backport to stable25

[backportbot-nextcloud]

The backport to stable27 failed. Please do this backport manually.

# Switch to the target branch and update it
git checkout stable27
git pull origin/stable27
# Create the new backport branch
git checkout -b fix/foo-stable27
# Cherry pick the change from the commit sha1 of the change against the default branch# This might cause conflicts. Resolve them.
git cherry-pick abc123
# Push the cherry pick commit to the remote repository and open a pull request
git push origin fix/foo-stable27

More info at https://docs.nextcloud.com/server/latest/developer_manual/getting_started/development_process.html#manual-backport

[backportbot-nextcloud]

The backport to stable26 failed. Please do this backport manually.

# Switch to the target branch and update it
git checkout stable26
git pull origin/stable26
# Create the new backport branch
git checkout -b fix/foo-stable26
# Cherry pick the change from the commit sha1 of the change against the default branch# This might cause conflicts. Resolve them.
git cherry-pick abc123
# Push the cherry pick commit to the remote repository and open a pull request
git push origin fix/foo-stable26

More info at https://docs.nextcloud.com/server/latest/developer_manual/getting_started/development_process.html#manual-backport

[backportbot-nextcloud]

The backport to stable25 failed. Please do this backport manually.

# Switch to the target branch and update it
git checkout stable25
git pull origin/stable25
# Create the new backport branch
git checkout -b fix/foo-stable25
# Cherry pick the change from the commit sha1 of the change against the default branch# This might cause conflicts. Resolve them.
git cherry-pick abc123
# Push the cherry pick commit to the remote repository and open a pull request
git push origin fix/foo-stable25

More info at https://docs.nextcloud.com/server/latest/developer_manual/getting_started/development_process.html#manual-backport

[szaimen]

@julien-nc
I think these are related:

[julien-nc]

@szaimen Thanks, I'm on it.

[ChristophWurst]

Is 256 chars long enough for an encrypted 64 chars string?

Apparently it is not 🥲

An exception occurred while executing a query: SQLSTATE[22001]: String data, right truncated: 1406 Data too long for column 'secret' at row 1

With mysql on my dev env. The secret string is 324 bytes long.

[ChristophWurst]

let's widen to 512?

[julien-nc]

👍 Thanks a lot.
I'll create another PR for master and adjust the backport PRs.
No idea about the max potential length of a 64 B string encrypted with OC\Security\Crypto. Let's discuss that in #38770

[ChristophWurst]

My application secret is 48 chars long