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.

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

Improve naming of SSH key variable names #73

Open
SvenMarcus opened this issue Apr 6, 2023 · 3 comments
Open

Improve naming of SSH key variable names #73

SvenMarcus opened this issue Apr 6, 2023 · 3 comments

Comments

@SvenMarcus
Copy link
Collaborator

I regularly get confused by the naming of the environment variables for SSH keys. For example, when I read MANAGER_KEY, I immediately assume that it's the key to access the manager when it's actually the manager's key to access the controller.
Similarly, there are MANAGER_KEYS and CONTROLLER_KEYS which I wouldn't know how to use either if it weren't for the explanatory comments next to them in the example .env file.

Therefore, I propose the following new names:
MANAGER_KEY -> MANAGER__CONTROLLER_ACCESS_KEY
MANAGER_KEYS -> MANAGER__AUTHORIZED_KEYS
CONTROLLER_KEYS -> CONTROLLER__AUTHORIZED_KEYS

Note that I included double underscores after the name of the service that the respective variables belong to. I find that it especially communicates the purpose of MANAGER__CONTROLLER_ACCESS_KEY more clearly that way.

@bertsky
Copy link
Member

bertsky commented Apr 11, 2023

We currently have

name default comment
APP_KEY ./kitodo/.ssh/id_rsa file path with private SSH key of ocrd user (should match one of MANAGER_KEYS)
MANAGER_KEYS ./ocrd/manager/.ssh/authorized_keys file path with public SSH keys of users allowed to log in
MANAGER_KEY ./ocrd/manager/.ssh/id_rsa file path with private SSH key of internal ocrd user (should match one of CONTROLLER_KEYS)
CONTROLLER_KEYS ./ocrd/controller/.ssh/authorized_keys file path with public SSH keys of users allowed to log in

(from documentation, automatically extracted from .env).

I agree it's a bit cryptic (singular vs. plural keys). But I don't like long variable names, and we should not name the keys after what they are used for, but where they are installed.

So how about …

  • APP_PRIV_KEYFILE
  • MANAGER_PUB_KEYFILE
  • MANAGER_PRIV_KEYFILE
  • CONTROLLER_PUB_KEYFILE

…instead?

@markusweigelt
Copy link
Collaborator

So how about …

  • APP_PRIV_KEYFILE
  • MANAGER_PUB_KEYFILE
  • MANAGER_PRIV_KEYFILE
  • CONTROLLER_PUB_KEYFILE

…instead?

From my side APP_PRIV_KEYFILE and MANAGER_PRIV_KEYFILE is fine. For the others I am more with the initial suggestion without the double underscore.

MANAGER_PUB_KEYFILE -> MANAGER_AUTHORIZED_KEYS, MANAGER_AUTHORIZED_KEYS_FILE
CONTROLLER_PUB_KEYFILE -> CONTROLLER_AUTHORIZED_KEYS, CONTROLLER_AUTHORIZED_KEYS_FILE

Reasons PUB leads to public keyfile which it is not, cause MANAGER_PUB_KEYFILE contains the public key of the Kitodo.Production. In addition, further public keys can be entered here. To avoid confusion, I prefer something with AUTHORIZED_KEY...

@bertsky
Copy link
Member

bertsky commented Apr 14, 2023

Reasons PUB leads to public keyfile which it is not,

I don't understand that.

cause MANAGER_PUB_KEYFILE contains the public key of the Kitodo.Production.

it is the file to the public keys allowed on the Manager, so yes, the public part of the key that Kitodo is using would be part of that. Where's the problem?

In addition, further public keys can be entered here.

And?

bertsky pushed a commit that referenced this issue Mar 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants