Implement `IProvideUserSecretBackend` compatibility for per-user encryption #697

summersab · 2023-02-10T00:48:51Z

This PR is intended to implement the functionality from PR #537. The upstream code changed enough that it was easier to close the original PR and submit a new one.

Now that the IProvideUserSecretBackend class has been added to the Nextcloud core with PR nextcloud/server#24837, this PR adds the necessary logic to support per-user SAML provided secrets.

summersab · 2023-02-24T17:29:56Z

@blizzz, you look like one of the top contributors to this repo. Is the CI check broken?

blizzz · 2023-05-12T14:55:07Z

@blizzz, you look like one of the top contributors to this repo. Is the CI check broken?

Only partly, it should be green up to and including stable25. We're working on getting it back for 26 and master.

immerda · 2023-05-20T20:55:41Z

Hey, this is just our yearly reminder that we are still testing this patch in our instance. In fact I just applied this new PR to the newest released nextcloud version and it works perfectly well and is stable enough that we can still decrypt the data from 2.5 years ago when we initially proposed this patch :) We are still eagerly awaiting upstream adoption...

mglants · 2023-11-02T01:35:15Z

Any news, when it will be supported, readlly want to use saml for server-side encryption

Signed-off-by: summersab <[email protected]> perform a little lint cleanup Signed-off-by: summersab <[email protected]> Signed-off-by: Andrew Summers <[email protected]>

blizzz

Thank you for your contribution! Rebased (CI is repaired since) and left a first review.

blizzz · 2023-11-03T17:37:02Z

lib/UserBackend.php

@@ -608,6 +665,21 @@ private function getAttributeArrayValue($name, array $attributes) {
 		return $value;
 	}

+	private function getUserSecret($uid, array $attributes) {


$uid is unused and can be removed.

blizzz · 2023-11-03T17:38:03Z

lib/UserBackend.php

@@ -608,6 +665,21 @@ private function getAttributeArrayValue($name, array $attributes) {
 		return $value;
 	}

+	private function getUserSecret($uid, array $attributes) {


Suggested change

private function getUserSecret($uid, array $attributes) {

private function getUserSecret(array $attributes): ?string {

blizzz · 2023-11-03T17:42:52Z

lib/UserBackend.php

 			$this->initializeHomeDir($uid);
 		}
 	}

+	private function getUserSecretHash($uid) {


Suggested change

private function getUserSecretHash($uid) {

private function getUserSecretHashes($uid): array {

may return up to ten, should be reflected in the name. Is 10 an arbitrary number, or where does it come from?

blizzz · 2023-11-03T17:44:41Z

lib/UserBackend.php

+		return $data;
+	}
+
+	private function checkUserSecretHash($uid, $userSecret) {


Suggested change

private function checkUserSecretHash($uid, $userSecret) {

private function checkUserSecretHash($uid, $userSecret): bool {

blizzz · 2023-11-03T17:46:08Z

lib/UserBackend.php

+		$data = $this->getUserSecretHash($uid);
+		foreach($data as $row) {
+			$storedHash = $row['token'];
+			if (\OC::$server->getHasher()->verify($userSecret, $storedHash, $newHash)) {


please use ' \OCP\Server::get(OCP\Security\IHasher::class)over deprecated and private\OC::$server->getHasher()`. Also on in the other places.

blizzz · 2023-11-03T17:46:59Z

lib/UserBackend.php

+					'name' => $qb->createNamedParameter('sso_secret_hash'),
+				]);
+		}
+		return $qb->execute();


Suggested change

return $qb->execute();

return $qb->executeStatement() > 0;

blizzz · 2023-11-03T17:47:34Z

lib/UserBackend.php

+		return false;
+	}
+
+	private function updateUserSecretHash($uid, $userSecret, $exists = false) {


Suggested change

private function updateUserSecretHash($uid, $userSecret, $exists = false) {

private function updateUserSecretHash($uid, $userSecret, $exists = false): bool {

blizzz · 2023-11-03T17:51:31Z

lib/UserBackend.php

+			if ($userSecret !== null) {
+				$this->updateUserSecretHash($uid, $userSecret);
+				// Emit a post login action to initialize the encryption module with the user secret provided by the idp.
+				\OC_Hook::emit('OC_User', 'post_login', ['run' => true, 'uid' => $uid, 'password' => $userSecret, 'isTokenLogin' => false]);


please dispatch the typed event \OCP\User\Events\PostLoginEvent instead of deprecated OC_Hook::emit(…).

blizzz · 2023-11-03T17:53:53Z

lib/UserBackend.php

+			$storedHash = $row['token'];
+			if (\OC::$server->getHasher()->verify($userSecret, $storedHash, $newHash)) {
+				if (!empty($newHash)) {
+					$this->updateUserSecretHash($uid, $userSecret, true);


mh, the function is said to check the hash, not update it.

summersab force-pushed the IProvideUserSecretBackend branch 2 times, most recently from 2835706 to cd0c669 Compare February 10, 2023 20:18

summersab marked this pull request as draft July 11, 2023 13:54

summersab force-pushed the IProvideUserSecretBackend branch from cd0c669 to d5e4825 Compare July 11, 2023 14:24

summersab marked this pull request as ready for review July 11, 2023 14:25

blizzz added enhancement 3. to review labels Jul 14, 2023

summersab force-pushed the IProvideUserSecretBackend branch from d5e4825 to 17b1210 Compare August 24, 2023 16:42

summersab requested review from blizzz and juliusknorr as code owners August 24, 2023 16:42

summersab force-pushed the IProvideUserSecretBackend branch 2 times, most recently from becfef3 to f5de408 Compare August 24, 2023 16:52

summersab force-pushed the IProvideUserSecretBackend branch from f5de408 to 128a9d3 Compare September 3, 2023 18:24

implement IProvideUserSecretBackend compatibility

6e1548a

Signed-off-by: summersab <[email protected]> perform a little lint cleanup Signed-off-by: summersab <[email protected]> Signed-off-by: Andrew Summers <[email protected]>

blizzz force-pushed the IProvideUserSecretBackend branch from 128a9d3 to 6e1548a Compare November 3, 2023 17:35

blizzz requested changes Nov 3, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement `IProvideUserSecretBackend` compatibility for per-user encryption #697

Implement `IProvideUserSecretBackend` compatibility for per-user encryption #697

summersab commented Feb 10, 2023

summersab commented Feb 24, 2023

blizzz commented May 12, 2023

immerda commented May 20, 2023

mglants commented Nov 2, 2023

blizzz left a comment

blizzz Nov 3, 2023

blizzz Nov 3, 2023

blizzz Nov 3, 2023

blizzz Nov 3, 2023

blizzz Nov 3, 2023

blizzz Nov 3, 2023

blizzz Nov 3, 2023

blizzz Nov 3, 2023

blizzz Nov 3, 2023

	private function getUserSecret($uid, array $attributes) {
	private function getUserSecret(array $attributes): ?string {

	private function getUserSecretHash($uid) {
	private function getUserSecretHashes($uid): array {

	private function checkUserSecretHash($uid, $userSecret) {
	private function checkUserSecretHash($uid, $userSecret): bool {

	private function updateUserSecretHash($uid, $userSecret, $exists = false) {
	private function updateUserSecretHash($uid, $userSecret, $exists = false): bool {

Implement IProvideUserSecretBackend compatibility for per-user encryption #697

Are you sure you want to change the base?

Implement IProvideUserSecretBackend compatibility for per-user encryption #697

Conversation

summersab commented Feb 10, 2023

summersab commented Feb 24, 2023

blizzz commented May 12, 2023

immerda commented May 20, 2023

mglants commented Nov 2, 2023

blizzz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Implement `IProvideUserSecretBackend` compatibility for per-user encryption #697

Implement `IProvideUserSecretBackend` compatibility for per-user encryption #697