debug: update ssh-keygen algorithm #21542
Conversation
k8s-ci-robot
added
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: medyagh The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
|
@medyagh: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
There was a problem hiding this comment.
This seems to do 2 changes:
- use ed25519 instead of rsa - looks good, matching the default algorithm in Fedora and macOS
- Keep the key in /root/.ssh instead of /docker.ssh - seems untreated to this change.
It is nice to keep the key in a common user that will never change instead of docker user that seems to be the wrong name for minikube vm user, but not clear why this is mixed with the algorithm change. Mixing unrelated changes make it harder to review and manage, for example reverting the user change if it turns out problematic.
| @@ -51,7 +51,7 @@ const ( | |||
| nineP = "9p" | |||
| defaultMount9PVersion = "9p2000.L" | |||
| mount9PVersionDescription = "Specify the 9p version that the mount should use" | |||
| defaultMountGID = "docker" | |||
| defaultMountGID = "root" | |||
There was a problem hiding this comment.
Why do we change mount command user? how is this related to changing ssh key algorithm?
| @@ -210,6 +210,6 @@ func validateArgs(src, dst *remotePath) { | |||
| // if node name not explicitly specified in both of source and target, | |||
| // consider target node is control-plane for backward compatibility. | |||
| if src.node == "" && dst.node == "" && !strings.HasPrefix(dst.path, "/") { | |||
| exit.Message(reason.Usage, `Target <remote file path> must be an absolute Path. Relative Path is not allowed (example: "minikube:/home/docker/copied.txt")`) | |||
| exit.Message(reason.Usage, `Target <remote file path> must be an absolute Path. Relative Path is not allowed (example: "minikube:/root/copied.txt")`) | |||
There was a problem hiding this comment.
Why change the location of the file?
| @@ -208,7 +208,7 @@ func startKicServiceTunnel(services service.URLs, configName, driverName string) | |||
| exit.Error(reason.DrvPortForward, "error getting ssh port", err) | |||
| } | |||
| sshPort := strconv.Itoa(port) | |||
| sshKey := filepath.Join(localpath.MiniPath(), "machines", configName, "id_rsa") | |||
| sshKey := filepath.Join(localpath.MiniPath(), "machines", configName, "id_ed25519") | |||
There was a problem hiding this comment.
This looks fine, but why do we have to hard code the algorithm? why not use the default algorithm on the host? With this code we will have to change again when the default changes next time.
| @@ -67,7 +67,7 @@ func appendKnownHelper(nodeName string, appendKnown bool) { | |||
| } | |||
| } | |||
|
|
|||
| scanArgs := []string{"-t", "rsa"} | |||
| scanArgs := []string{"-t", "ed25519"} | |||
There was a problem hiding this comment.
We use this in too many places - add a constant so we can have the algorithm exactly once in the project.
| @@ -262,6 +262,8 @@ RUN echo '%sudo ALL=(ALL) NOPASSWD:ALL' >> /etc/sudoers | |||
| USER docker | |||
| RUN mkdir /home/docker/.ssh | |||
| USER root | |||
| # prepare root ssh directory for minikube access | |||
| RUN mkdir /root/.ssh | |||
No description provided.