-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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.
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
Proceed encrypt-all after one of the encryption mode is selected #31598
Closed
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, there is
$this->encryptionManager->isEnabled()
a few lines above. That already checksencryption_enabled
anduseMasterKey
. Maybe it also needs to checkuserSpecificKey
.What is the bug you are trying to solve? It seems that when user based encryption was enabled user individual or master key based encryption should have been selected.
So, to properly enable encryption you now need to
before you can
encryption:encryptall
... shouldn'tencryption:enable
also set the encryption type? Currently, it neither setsuseMasterKey
, noruserSpecificKey
. Presumably, because core shouldn't know anything about the inner workings of the encryption app, such as which encryption type is used. The encryption login hook will initialize user individual keys unlessuseMasterKey
is set, so implicitly user individual keys are used.in the login hook check if
useMasterKey
oruserSpecificKey
are set. If not setuserSpecificKey
. That will make user individual keys explicit. do it in a separat PR, as it will change behavior. Currently you can just use user individual keys and then switch to master key based encryption, even if some users use user individual keys.add an issue to add an API to explicitly enable an encryption module. That will allow us to call eg
\OC::$server->getEncryptionManager()->enable()
to tell an encryption that it has been enabled and it con do things like create eg the master key. We might pass input and output but that remains to be discussed in the new issue.the IEncryptionModule API should have an enable(), isEnabled() and disable() method. Maybe add a new interface in a new PR so that existing code can work, but we can call these methods on an app when they are being enabled with
occ app:enable
. Best would be ifenable()
could take options as a parameter, so the app can configure itself. Otherwise configuration is a separate step before an app can be used. encryption, ldap and user_shibboleth all need configuration before they are really enalbled.with that api core must not know about config variables used in apps
core/lib/private/Encryption/Manager.php
Line 91 in 5f0af49
We need to check if the documentation contains all three steps to enable the encryption and make sure
userSpecificKey
is set.maybe related: https://github.com/owncloud/encryption/blob/9f576ab02af0578265134c1e728b6703f14d5a7d/lib/Command/RecreateMasterKey.php#L197-L202
does not delete the
userSpecificKey
config variable. open a new PR if it is not.can you add a new issue to add a proper method to the encryption api to check what encryption is used? checking the config bit by bit is tedious and error prone. There are other places where the same functionality is needed:
https://github.com/owncloud/encryption/blob/407c630e03605cf5050a64d6cf3a2b8ac58c7a7d/lib/Command/SelectEncryptionType.php#L91-L95
https://github.com/owncloud/encryption/blob/407c630e03605cf5050a64d6cf3a2b8ac58c7a7d/appinfo/Migrations/Version20170913113840.php#L38-L41
https://github.com/owncloud/encryption/blob/407c630e03605cf5050a64d6cf3a2b8ac58c7a7d/templates/settings-admin.php#L15-L20
https://github.com/owncloud/encryption/blob/407c630e03605cf5050a64d6cf3a2b8ac58c7a7d/lib/Session.php#L63-L65
https://github.com/owncloud/encryption/blob/4992583c5a1f75067a6ccecb54644d39373af47d/templates/settings-admin.php#L15-L20
Arg what a mess ... why did I even look at this ... in light of all this, this PR is good enough.