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

hsmdaemon install consistency and accuracy #3710

Merged
merged 5 commits into from
Jun 24, 2021
Merged

hsmdaemon install consistency and accuracy #3710

merged 5 commits into from
Jun 24, 2021

Conversation

jnweiger
Copy link
Contributor

@jnweiger jnweiger commented Jun 18, 2021 •
edited
Loading

This document appears to be generally working.
But it has several smaller defects:

  • sudo is used inconsistently
  • occ is used inconsistently
  • The TIP about analyzing the own PATH shows some misunderstandings. Calling ./hsmdaemon install actually creates a service file with the current PWD hardcoded. No path lookup is ever done during service invocation. It is safer to use the install subcommand with the binary that was placed in the final destination, e.g. /usr/local/bin/hsmdaemon install
  • chmod 750 includes execute permission. This is useless for the toml file. use 640 instead.

As this manual is in the public, I miss some instruction how to actually obtain the hsmdaemon code. It is not opensource. Maybe ooint to the consulting team?

@jnweiger jnweiger requested review from mmattel and IljaN June 18, 2021 21:41
@jnweiger
Copy link
Contributor Author

jnweiger commented Jun 18, 2021 •
edited
Loading

Backports needed.

@jnweiger jnweiger mentioned this pull request Jun 18, 2021
Closed
39 tasks
@mmattel
Copy link
Contributor

mmattel commented Jun 19, 2021

@jnweiger see inline

  • sudo is used inconsistently...
    You have to write sudo on each command where you need raised previledges.
    There is NO and WILL NOT BE any generalisation sentence on top of any page.
  • occ is used inconsistently...
    You MUST use the attribute, no exception.

Please correct your changes accordingly !
There will be no approval without !

@EParzefall FYI

@IljaN
Copy link
Member

IljaN commented Jun 21, 2021

I think I also encountered the issue that when you do the steps described in the docs ownCloud defaults to user-key encryption.

@mmattel
Copy link
Contributor

mmattel commented Jun 21, 2021

I think I also encountered the issue that when you do the steps described in the docs ownCloud defaults to user-key encryption.

Can you bring some light into that what you mean. What needs to be changed where?

@mmattel mmattel mentioned this pull request Jun 21, 2021
Closed
@IljaN
Copy link
Member

IljaN commented Jun 21, 2021

See: owncloud/encryption#284

@mmattel
Copy link
Contributor

mmattel commented Jun 21, 2021

See: owncloud/encryption#284

my proposal - change from user to master key in hsm, see my comment in the issue referenced by @IljaN

@mmattel
Copy link
Contributor

mmattel commented Jun 22, 2021

I am working on a rewrite including the user/master key fix, will push soon.

@mmattel
Copy link
Contributor

mmattel commented Jun 22, 2021

#3717 (Decryption tests for HSM Daemon)

@mmattel mmattel force-pushed the jnweiger-brush-up-hsmdaemon-install branch from 3ae96aa to f998dca Compare June 22, 2021 10:30
@mmattel mmattel requested review from EParzefall and phil-davis June 22, 2021 10:31
@mmattel
Copy link
Contributor

mmattel commented Jun 22, 2021

The last commit is a first rework and needs language checking.
More changes to come because of #3717 (and maybe more what has not identified so far)

EParzefall
EParzefall approved these changes Jun 22, 2021
View reviewed changes
Copy link
Contributor

@EParzefall EParzefall left a comment

Choose a reason for hiding this comment

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

LGTM

modules/admin_manual/pages/configuration/server/security/hsmdaemon/index.adoc Outdated Show resolved Hide resolved
modules/admin_manual/pages/configuration/server/security/hsmdaemon/index.adoc Outdated Show resolved Hide resolved
modules/admin_manual/pages/configuration/server/security/hsmdaemon/index.adoc Outdated Show resolved Hide resolved
modules/admin_manual/pages/configuration/server/security/hsmdaemon/index.adoc Outdated Show resolved Hide resolved
modules/admin_manual/pages/configuration/server/security/hsmdaemon/index.adoc Outdated Show resolved Hide resolved
modules/admin_manual/pages/configuration/server/security/hsmdaemon/index.adoc Outdated Show resolved Hide resolved
modules/admin_manual/pages/configuration/server/security/hsmdaemon/index.adoc Outdated Show resolved Hide resolved
modules/admin_manual/pages/configuration/server/security/hsmdaemon/index.adoc Outdated Show resolved Hide resolved
modules/admin_manual/pages/configuration/server/security/hsmdaemon/index.adoc Outdated Show resolved Hide resolved
modules/admin_manual/pages/configuration/server/security/hsmdaemon/index.adoc Outdated Show resolved Hide resolved
@mmattel mmattel force-pushed the jnweiger-brush-up-hsmdaemon-install branch from 2e59398 to 2d019ef Compare June 22, 2021 16:42
@mmattel
Copy link
Contributor

mmattel commented Jun 22, 2021

Squashed rebased and pushed. Starting next fixes tomorrow.

@mmattel
Copy link
Contributor

mmattel commented Jun 23, 2021

See: owncloud/encryption#284

@IljaN fixed, default now is masterkey

@mmattel
Copy link
Contributor

mmattel commented Jun 23, 2021

#3717 (Decryption tests for HSM Daemon)

Implemented

@mmattel mmattel merged commit b6abe56 into master Jun 24, 2021
@delete-merged-branch delete-merged-branch bot deleted the jnweiger-brush-up-hsmdaemon-install branch June 24, 2021 08:58
mmattel added a commit that referenced this pull request Jun 24, 2021
@mmattel
[10.6] [PR 3710] hsmdaemon install consistency and accuracy
113b743 
Backport of PR #3710
@mmattel mmattel mentioned this pull request Jun 24, 2021
Merged
mmattel added a commit that referenced this pull request Jun 24, 2021
@mmattel
[10.7] [PR 3710] hsmdaemon install consistency and accuracy
a957b53 
Backport of PR #3710
This was referenced Jun 24, 2021
Merged
Closed
@jnweiger jnweiger mentioned this pull request May 25, 2022
Closed
59 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mmattel mmattel mmattel left review comments

@phil-davis phil-davis phil-davis left review comments

@EParzefall EParzefall EParzefall approved these changes

@IljaN IljaN IljaN approved these changes

Assignees
No one assigned
Labels
admin docs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jnweiger @mmattel @IljaN @phil-davis @EParzefall