-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Add support for client certificates to -output-curl-string #13660
Add support for client certificates to -output-curl-string #13660
Conversation
I did not write tests for this feature as -output-curl-string was not already tested and this is a simple change. Because the name of the certificates would be lost once loaded I added fields to Config to keep track of them. I did not add a public method for the user to set them explicitely as I don't think anyone would need this functionnality outside of the Vault CLI. Closes hashicorp#13376
@@ -46,6 +48,22 @@ func (d *OutputStringError) parseRequest() { | |||
if d.Request.Method != "GET" { | |||
d.parsedCurlString = fmt.Sprintf("%s-X %s ", d.parsedCurlString, d.Request.Method) | |||
} | |||
if d.ClientCACert != "" { | |||
clientCACert := strings.Replace(d.ClientCACert, "'", "'\"'\"'", -1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clientCACert := strings.Replace(d.ClientCACert, "'", "'\"'\"'", -1) | |
clientCACert := strings.Replace(d.ClientCACert, "'", `'"'"'`, -1) |
I'm still not sure I understand it even with that change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is so that if the filename contains a special character:
$ ls
clie'nt.crt client.key
then the '
is escaped in the output of -output-curl-string
:
$ ../bin/vault read -client-cert clie\'nt.crt -client-key client.key -output-curl-string foo
curl --cert 'clie'"'"'nt.crt' --key 'client.key' -H "X-Vault-Request: true" -H "X-Vault-Token: $(vault print token)" https://127.0.0.1:8200/v1/foo
The --cert
option is the concatenation of three strings 'clie'
, "'"
and nt.crt
so it gives "clie'nt.crt"
.
For what it's worth this is very naive, it does not escape $(...)
or ``
which would also interpreted by the shell. I don't think this is a solvable problem thought as different shells will interpret different constructs. I used this method to escape '
because it is already used for the body.
b48e5b4
to
a6de90e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm!
I've done some basic testing to verify that the cert and keys are included now in the output-curl-string. I also looked through the callers of ConfigureTLS to double-check if grabbing the modifyLock in the function could cause issues, but it all looked good to me.
Thanks for submitting this PR!
I did not write tests for this feature as -output-curl-string was not
already tested and this is a simple change. Because the name of the
certificates would be lost once loaded I added fields to Config to keep
track of them. I did not add a public method for the user to set them
explicitely as I don't think anyone would need this functionnality
outside of the Vault CLI.
Closes #13376