mgr/cephadm: allow use of authenticated registry#36012
mgr/cephadm: allow use of authenticated registry#36012sebastian-philipp merged 1 commit intoceph:masterfrom
Conversation
|
jenkins test make check |
| | [--apply-spec APPLY_SPEC] | ||
| | [--registry-url REGISTRY_URL] | ||
| | [--registry-username REGISTRY_USERNAME] | ||
| | [--registry-password REGISTRY_PASSWORD] |
There was a problem hiding this comment.
I wonder if sending passwords as command options is a security risk. Note that we are already doing it on --initial-dashboard-password, but by default, the dashboard user will be forced to change the password on the first login.
Maybe allowing the user to choose between --registry-password or --registry-password-stdin/--registry-password-file[1][2] fixes the issue for users that are more concerned about security, but I would prefer to avoid adding any unnecessary complexity, so if this is how cephadm deals with these scenarios, we don't have to change it.
@sebastian-philipp @adk3798 others, any thoughts about this?
[1] https://docs.docker.com/engine/reference/commandline/login/
[2] https://www.mankier.com/1/podman-login
There was a problem hiding this comment.
👍 it is a security risk.
maybe extend this to the existing --config-json arg?
doc/man/8/cephadm.rst
Outdated
| | **cephadm** **install** [-h] [packages [packages ...]] | ||
|
|
||
|
|
||
| | **cephadm** **registry-login** [-h] --registry-url REGISTRY_URL |
There was a problem hiding this comment.
Shouldn't we also have a cephadm logout command?
| Cephadm will attempt to login to that registry and store the login information in the cluster's | ||
| configuration database:: | ||
|
|
||
| cephadm registry-login --registry-url [REGISTRY_URL] --registry-username [USERNAME] |
There was a problem hiding this comment.
podman also have a --tls-verify option that allows login into registries without SSL, but this option is not available on docker.
@sebastian-philipp what it the approach used in cephadm? Only support options that can be used by both podman and docker? Or support options that can only be used by one of them, and throw an error if you use that option on the wrong engine?
There was a problem hiding this comment.
I would think a better approach might would be to configure podman/docker with a self-signed cert and/or as an insecure registery ...
| Cephadm will attempt to login to that registry and store the login information in the cluster's | ||
| configuration database:: | ||
|
|
||
| cephadm registry-login --registry-url [REGISTRY_URL] --registry-username [USERNAME] |
There was a problem hiding this comment.
I would think a better approach might would be to configure podman/docker with a self-signed cert and/or as an insecure registery ...
| | [--apply-spec APPLY_SPEC] | ||
| | [--registry-url REGISTRY_URL] | ||
| | [--registry-username REGISTRY_USERNAME] | ||
| | [--registry-password REGISTRY_PASSWORD] |
There was a problem hiding this comment.
👍 it is a security risk.
maybe extend this to the existing --config-json arg?
|
(btw, would be great to have some teuthology or tox tests for this) |
6d9adc4 to
d7f24bb
Compare
|
I've updated this so that storing the credentials in the cluster is now done with a command in the mgr "ceph cephadm registry-login [url] [username] [password]". The command in the binary is now only responsible for logging the host into the repository and doesn't take any actions with cluster-wide effects. I's be curious to hear more opinions on security risks (discussion started here) #36012 (comment) or whether there should also be a logout command. |
|
I've tried to setup allowing the user to pass their registry login info using a JSON file for the security issue of passing passwords over the command line. It feels a bit messy to me and at the moment I'm also assuming the JSON received will have the indexes URL, Username and Password at the top level but it appears to work as long as that holds. Any feedback on that would be appreciated. |
src/cephadm/cephadm
Outdated
| raise Error("json provided for custom registry login did not include all necessary info. " | ||
| "Please setup json file as\n" | ||
| "{\n" | ||
| " \"URL\": \"REGISTRY_URL\",\n" | ||
| " \"Username\": \"REGISTRY_USERNAME\",\n" | ||
| " \"Password\": \"REGISTRY_PASSWORD\"\n" | ||
| "}\n") |
There was a problem hiding this comment.
can we merge this code with get_parm ?
There was a problem hiding this comment.
Can you clarify this comment? Is this suggesting I make use of get_parm or that I add something to get_parm
|
looks much better! |
sebastian-philipp
left a comment
There was a problem hiding this comment.
lgtm, except some minor nits.
do you want to add a test?
|
Updated with minor improvements. Working on making tests now |
Add option to use custom authenticated registry during bootstrap as well as a registry-login command in order to let user change authenticated registry login info Fixes: https://tracker.ceph.com/issues/44886 Signed-off-by: Adam King <adking@redhat.com>
|
added some python tests for both the changes in the binary and module. They should be usable running tox in the src/cephadm and src/pybind/mgr directories respectively. |
|
jenkins test dashboard backend |
1 similar comment
|
jenkins test dashboard backend |
Add option to use custom authenticated registry during
bootstrap as well as a registry-login command in order
to let user change authenticated registry login info
Fixes: https://tracker.ceph.com/issues/44886
Signed-off-by: Adam King adking@redhat.com