Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Allow SSO authentication to provide a user secret #27929

Merged
merged 1 commit into from
Jul 18, 2022
Merged

Allow SSO authentication to provide a user secret #27929

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions lib/composer/composer/autoload_classmap.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@
'OCP\\Authentication\\Exceptions\\PasswordUnavailableException' => $baseDir . '/lib/public/Authentication/Exceptions/PasswordUnavailableException.php',
'OCP\\Authentication\\IAlternativeLogin' => $baseDir . '/lib/public/Authentication/IAlternativeLogin.php',
'OCP\\Authentication\\IApacheBackend' => $baseDir . '/lib/public/Authentication/IApacheBackend.php',
'OCP\\Authentication\\IProvideUserSecretBackend' => $baseDir . '/lib/public/Authentication/IProvideUserSecretBackend.php',
'OCP\\Authentication\\LoginCredentials\\ICredentials' => $baseDir . '/lib/public/Authentication/LoginCredentials/ICredentials.php',
'OCP\\Authentication\\LoginCredentials\\IStore' => $baseDir . '/lib/public/Authentication/LoginCredentials/IStore.php',
'OCP\\Authentication\\TwoFactorAuth\\ALoginSetupController' => $baseDir . '/lib/public/Authentication/TwoFactorAuth/ALoginSetupController.php',
Expand Down
1 change: 1 addition & 0 deletions lib/composer/composer/autoload_static.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,7 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2
'OCP\\Authentication\\Exceptions\\PasswordUnavailableException' => __DIR__ . '/../../..' . '/lib/public/Authentication/Exceptions/PasswordUnavailableException.php',
'OCP\\Authentication\\IAlternativeLogin' => __DIR__ . '/../../..' . '/lib/public/Authentication/IAlternativeLogin.php',
'OCP\\Authentication\\IApacheBackend' => __DIR__ . '/../../..' . '/lib/public/Authentication/IApacheBackend.php',
'OCP\\Authentication\\IProvideUserSecretBackend' => __DIR__ . '/../../..' . '/lib/public/Authentication/IProvideUserSecretBackend.php',
'OCP\\Authentication\\LoginCredentials\\ICredentials' => __DIR__ . '/../../..' . '/lib/public/Authentication/LoginCredentials/ICredentials.php',
'OCP\\Authentication\\LoginCredentials\\IStore' => __DIR__ . '/../../..' . '/lib/public/Authentication/LoginCredentials/IStore.php',
'OCP\\Authentication\\TwoFactorAuth\\ALoginSetupController' => __DIR__ . '/../../..' . '/lib/public/Authentication/TwoFactorAuth/ALoginSetupController.php',
Expand Down
8 changes: 6 additions & 2 deletions lib/private/legacy/OC_User.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,11 @@ public static function loginWithApache(\OCP\Authentication\IApacheBackend $backe
}
$userSession->setLoginName($uid);
$request = OC::$server->getRequest();
$userSession->createSessionToken($request, $uid, $uid);
$password = null;
if ($backend instanceof \OCP\Authentication\IProvideUserSecretBackend) {
$password = $backend->getCurrentUserSecret();
}
$userSession->createSessionToken($request, $uid, $uid, $password);
$userSession->createRememberMeToken($userSession->getUser());
// setup the filesystem
OC_Util::setupFS($uid);
Expand All @@ -191,7 +195,7 @@ public static function loginWithApache(\OCP\Authentication\IApacheBackend $backe
'post_login',
[
'uid' => $uid,
'password' => null,
'password' => $password,
MichaIng marked this conversation as resolved.
Show resolved Hide resolved
'isTokenLogin' => false,
]
);
Expand Down
41 changes: 41 additions & 0 deletions lib/public/Authentication/IProvideUserSecretBackend.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
<?php
/**
* @copyright Copyright (c) 2021, MichaIng <[email protected]>
*
* @author MichaIng <[email protected]>
*
* @license AGPL-3.0
*
* This code is free software: you can redistribute it and/or modify
* it under the terms of the GNU Affero General Public License, version 3,
* as published by the Free Software Foundation.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU Affero General Public License for more details.
*
* You should have received a copy of the GNU Affero General Public License, version 3,
* along with this program. If not, see <http://www.gnu.org/licenses/>
*
*/
// use OCP namespace for all classes that are considered public.
// This means that they should be used by apps instead of the internal ownCloud classes

namespace OCP\Authentication;

/**
* Interface IProvideUserSecretBackend
*
* @since 23.0.0
*/
interface IProvideUserSecretBackend {

/**
* Optionally returns a stable per-user secret. This secret is for
* instance used to secure file encryption keys.
* @return string
* @since 23.0.0
*/
public function getCurrentUserSecret(): string;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would making this nullable make sense? Especially as $password = null; is the default.

Suggested change
public function getCurrentUserSecret(): string;
public function getCurrentUserSecret(): ?string;

Copy link
Member Author

@MichaIng MichaIng Aug 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the idea was to explicitly not allow a null value here, so that checks can be omitted when the interface is implemented. Backends then need to always provide a string.

Ref: #27929 (comment)

But I see this is inconsistent with the description "Optionally returns a stable per-user secret.", which just has not been changed when I copy&pasted it from IApacheBackend (original idea, where it needed to be optional, of course) to its own interface. Shall I replace with:

* Returns a stable per-user secret e.g. used to secure file encryption keys.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I suggested to introduce the non-nullable return value earlier. To me it seems more reasonable as the user secret would be used for the user key encryption where I'd argue that if a backend provides user secrets there should be always one set for the user.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two comments/issues:

  1. I've tried the most recent version of this PR with the IProvideUserSecretBackend in conjunction with the associated user_saml plugin PR:
    support for per-user encryption user_saml#537
    .
    Since users logging in with this method would be using the user_saml backend, lib/private/legacy/OC_User.php:177 never gets executed. Therefore, the password is never set. Am I missing something?
  2. With the code from the original PR, there seems to be a problem. After a few minutes, heartbeat requests start returning 401 responses. Then, any getstoragestats request triggers a page refresh after a 401 response because the session is no longer valid. I have yet to determine what is causing the session to fail, but it is a problem.

Any thoughts? I have a local Keycloak -> NC dev setup working, and I'm happy to test.

Copy link
Member Author

@MichaIng MichaIng Sep 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without a password, getCurrentUserSecret() will return null. So, this suggested change should either be reversed (probably the recommended solution) or a try/catch should be added

Is it possible that user_saml "provides" the IProvideUserSecretBackend only when a user secret is actually set? In theory this would then reduce some overhead, when the backend is used without it and would make allowing/checking for null obsolete, right? But I clearly lack knowledge about theses things, so probably it's nonsense 😄.

Copy link

@immerda immerda Nov 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a proposal to get us unstuck here. How about we make IProvideUserSecretBackend::getCurrentUserSecret() nullable (or conditionally implementing IProvideUserSecretBackend is fine too -- anything that gets us unstuck...), but then do the following:

$password = null;
if ($backend instanceof \OCP\Authentication\IProvideUserSecretBackend) {
  $password = $backend->getCurrentUserSecret();
}
if ($password !== null) {
  $userSession->createSessionToken($request, $uid, $uid, $password);
} else {
  $userSession->createSessionToken($request, $uid, $uid);
}

This would allow us to disentangle the two problems, preserve the identical behavior for the non-userSecret case and let createSessionToken decide what to do when there is no password. This solution would also be more robust, as it removes that dependency on the decision if token passwords are nullable or not and allows us to define an interface that is independent of that.

Copy link
Member Author

@MichaIng MichaIng Nov 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't it exactly the same outcome whether passing $password = null or no argument at all? The createSessionToken has it's fourth argument null in both cases, isn't it?

The main question is indeed whether we want to be more explicit and force usage of a password when the backend is implemented or whether we want to allow implementing the backend without the need to provide a password. Generally I'm fans of being more explicit, but I understand that it may make the backend implementation more complicated.

I think it is agreed that we can or even should merge this PR first, before doing so at the backend, especially since the interfaces are defined here, so the above is really the last open question. I think we need someone who is more in charge to simply do a decision and we'll be able to deal with the result. But we need to get unstuck 😄.

}