added certificate authentication for ssh authentication #18017
Conversation
sherodtaylor
changed the title
|
@apparentlymart is there anyway we can get this into the next release. We need this to provision our environment. I updated the tests and it works perfectly in our staging environment. |
sherodtaylor
force-pushed
the
feature/ca-ssh
branch
from
b84f909 to
fd94446
Compare
|
@apparentlymart any update on this? We really need it. |
remove debug logs update err log add debug logs logging
sherodtaylor
force-pushed
the
feature/ca-ssh
branch
from
c01e456 to
6fc76bf
Compare
|
@catsby or any other contributor can this get a look. We have a production deployment coming up and this would be really nice to get in the next release :) |
There was a problem hiding this comment.
Question about unit testing the new method.
Additionally, what is the user experience if the user supplies a certificate but does not supply the private key? The documentation says that to use certificate they must also provide the private key, is there a way we can check this in code, rather than just failing during the connection attempt?
| @@ -254,6 +269,31 @@ func buildSSHClientConfig(opts sshClientConfigOpts) (*ssh.ClientConfig, error) { | |||
| return conf, nil | |||
| } | |||
|
|
|||
| // Create a Cert Signer and return ssh.AuthMethod | |||
| func signCertWithPrivateKey(pk string, certificate string) (ssh.AuthMethod, error) { | |||
There was a problem hiding this comment.
Is this something we can add unit tests for? My knowledge of SSH and certificates is fairly shallow, so forgive me if I'm asking a silly question
There was a problem hiding this comment.
I don’t know how the unit tests work for this provisioner.
| @@ -105,6 +110,7 @@ func TestProvisioner_connInfoHostname(t *testing.T) { | |||
| "user": "root", | |||
| "password": "supersecret", | |||
| "private_key": "someprivatekeycontents", | |||
| "certificate": "somecertificate", | |||
There was a problem hiding this comment.
Let's remove somecertificate from one of these 3 tests, and use that test to verify that conf.Certificate remains an empty string. Because it's optional, I'd like to see at least one test verify the exclusion still works as expected.
|
@catsby the fail check is handled in line of provisioner.go 139 |
|
Hi @sherodtaylor! Thanks for working on this, and sorry for the slow response on my part. It looks like you've already been working on some feedback with @catsby here, which is great! I just wanted to let you know that your PR here has a small conflict with a bigger development branch we have in progress for updating the configuration language in the next major release, and so I'd like to hold on merging this until we've merged that branch into At that point we can also give some pointers on ways to test this code... the Sorry again for the slow response here. The current configuration language project work is taking our attention a lot more than usual due to how many of Terraform's subsystems need small updates to work with it. |
|
@apparentlymart thanks for the response! The bulk of the changes where made in the provisioner.go I was looking at the communicator. Is it better for me to add or move a test from communicator_test.go to provision_test.go? Or I was thinking I could do a unit test on the function signCertWithPrivateKey to test if it returns a valid public key certificate. I don't know which is better. |
|
@apparentlymart how long until the development branch gets merged? Is there any guidance you can give me on the unit testing aspect? |
|
@apparentlymart See the above pr ^^. I've added some better unit tests for communicator_test.go |
|
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
ghost
locked and limited conversation to collaborators
This pull request addresses #14413. It adds certificate support for the ssh provisioner. It adds a new parameter
certificatewhich is used to authenticate ssh sessions. I updated the provisioner_test and docs to reflect this.