Change key transfering method #815
Conversation
fix debug test update debug merge func merge func update syntax update
takara9
force-pushed
the
improve-deadlock2
branch
from
61a9d65 to
f19768d
Compare
| }) | ||
| return err | ||
| } | ||
|
|
There was a problem hiding this comment.
We may need to defer here. If an error occurs until line 205, the file may remain.
defer os.Remove(fifo)
There was a problem hiding this comment.
Moved "defer os.Remove(pipeFilename)" after the error check.
pkg/ckecli/cmd/ssh.go
Outdated
| if err != nil { | ||
| log.Error("failed to create the named pipe", map[string]interface{}{ | ||
| log.FnError: err, | ||
| "fifo name": pipeFilename, |
There was a problem hiding this comment.
When an error occurs in createFifo(), pipeFileName is empty. There is no meaning in outputting here.
pkg/ckecli/cmd/ssh.go
Outdated
| } | ||
|
|
||
| func writeToFifo(fifo string, data string) error { | ||
| f, err := os.OpenFile(fifo, os.O_WRONLY|os.O_CREATE|os.O_APPEND, os.ModeNamedPipe) |
There was a problem hiding this comment.
Why use os.O_CREATE? The file is created in createFifo() as far as I understand.
There was a problem hiding this comment.
Deleted "os.O_WRONLY|"
pkg/ckecli/cmd/ssh.go
Outdated
| }) | ||
| return err | ||
| } | ||
| log.Info("killed ssh-agent", map[string]interface{}{ |
There was a problem hiding this comment.
-
This info log is not output.
Please check the usage ofcybozu-go/log. The usable field keys are limited.
https://github.com/cybozu-go/log/blob/v1.7.0/formatter.go#L43You can check the behavior of
cybozu-go/logusing the following command.package main import ( "github.com/cybozu-go/log" ) func main() { log.Info("hello1", nil) log.Info("hello2", map[string]interface{}{"message": ""}) log.Info("hello3", map[string]interface{}{"foo bar": "hoge"}) log.Info("hello4", map[string]interface{}{"Foo": "hoge"}) log.Info("hello5", map[string]interface{}{"foo": "hoge"}) } -
Please change this log to the debug level.
There was a problem hiding this comment.
Thank you kindness the explanation.
I changed appropirate key and level.
pkg/ckecli/cmd/ssh.go
Outdated
| sshArgs := []string{ | ||
| "-s", | ||
| } | ||
|
|
||
| return fifo, nil | ||
| cmd := exec.CommandContext(ctx, "ssh-agent", sshArgs...) |
There was a problem hiding this comment.
This option is short, so there's no need to make a variable.
IMO, this variable decreases readability. And Unnecessary line breaks have been inserted.
| sshArgs := []string{ | |
| "-s", | |
| } | |
| return fifo, nil | |
| cmd := exec.CommandContext(ctx, "ssh-agent", sshArgs...) | |
| cmd := exec.CommandContext(ctx, "ssh-agent", "-s") |
There was a problem hiding this comment.
Deleted sshArgs, and put "-s" after "ssh-agent"
pkg/ckecli/cmd/scp.go
Outdated
| if err = writeToFifo(pipeFilename, pirvateKey); err != nil { | ||
| log.Error("failed to write the named pipe", map[string]interface{}{ | ||
| log.FnError: err, | ||
| "named pipe": pipeFilename, |
There was a problem hiding this comment.
This field key is not valid.
| }) | ||
| return err | ||
| } | ||
|
|
There was a problem hiding this comment.
It needs to defer here.
| defer os.Remove(pipeFilename) |
pkg/ckecli/cmd/ssh.go
Outdated
| sshArgs2 := []string{ | ||
| privateKeyFile, | ||
| } | ||
| cmd0 := exec.CommandContext(ctx, "ssh-add", sshArgs2...) | ||
| _, err = cmd0.CombinedOutput() |
There was a problem hiding this comment.
- There is no need to make the option a variable.
- I cannot understand the naming conventions of local variables in this function. Why does
cmd0come aftercmd? It might just be better to reusecmd.
| sshArgs2 := []string{ | |
| privateKeyFile, | |
| } | |
| cmd0 := exec.CommandContext(ctx, "ssh-add", sshArgs2...) | |
| _, err = cmd0.CombinedOutput() | |
| cmd = exec.CommandContext(ctx, "ssh-add", privateKeyFile) | |
| _, err = cmd0.CombinedOutput() |
| }) | ||
| } | ||
| }() | ||
|
|
There was a problem hiding this comment.
If writeToFifo() fails, ssh-agent is not killed.
| defer killSshAgent(ctx) |
| }) | ||
| } | ||
| }() | ||
|
|
There was a problem hiding this comment.
It needs to defer here.
| defer killSshAgent(ctx) |
Changes to the Internal Structure of ckecli ssh
The method of retrieving the private key from Vault, writing it three times via a named pipe, and passing it to the ssh command has been discontinued.
Stability is achieved by passing the private key to the ssh command via ssh-agent.
https://bozuman.cybozu.com/k/#/space/3801/thread/76755/5837513/12062963