search

slub / ocrd_kitodo

Docker integration of Kitodo.Production and OCR-D
MIT License
10 stars 6 forks source link

Improve naming of SSH key variable names #73

Open SvenMarcus opened 1 year ago

SvenMarcus commented 1 year ago

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 commented 1 year ago

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 …

…instead?

markusweigelt commented 1 year ago

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 commented 1 year ago

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?