Skip to content
This repository has been archived by the owner on Feb 5, 2020. It is now read-only.

metal: add tectonic_ssh_private_key variable #625

Closed
wants to merge 1 commit into from

Conversation

coreypobrien
Copy link
Contributor

Allow users to specify the SSH private key via a variable in cases where
ssh-agent isn't available or configured.

@coreosbot
Copy link

Can one of the admins verify this patch?

1 similar comment
@coreosbot
Copy link

Can one of the admins verify this patch?

@lblackstone
Copy link
Contributor

@coreypobrien You probably want to add an entry similar to https://github.com/coreos/tectonic-installer/blob/master/examples/terraform.tfvars.metal#L25-L27

@@ -185,3 +185,9 @@ SSH public key to use as an authorized key.
Example: `ssh-rsa AAAB3N...`
EOF
}

variable "tectonic_ssh_private_key" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just a concern relating recent discussion. this private key will end up in the state file, rendering its security useless.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hrm, yeah that gets into a more broad discussion of secrets in the state file. Terraform has been wrestling with that issue for a bit: hashicorp/terraform#9556

Since we are already storing secrets like the private keys for the CA, seems like a good discussion to have for the whole repo.

I will go ahead an add a note in the description that the private key will be stored in the state and potentially shared wherever the cluster state is stored.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest we use the file interpolation function and only configure the location of the private key. Since this is only used for bootstrap time and needs not to be persisted I don't think we need to bend the rule here as was done for the ca key.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a clean way to have an optional file? I don't know of one off the top of my head, but I know you've been looking at this as a part of #133 so thought maybe you had some insight?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately I didn't find a good alternative yet. I think using the template_file resource with no template variables defined and a count on it could make this work, but it seems very hacky.

@alexsomesan Do you have any creative suggestion to declare "optionally read file x" semantics?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@coreypobrien ok, I have a silly idea which we already leveraged, namely pointing to /dev/null by default ;-) We have a precedence already when providing optional etcd CA certificates: https://github.com/coreos/tectonic-installer/blob/fff91d9/modules/bootkube/assets.tf#L8-L10

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, nice. I'll head down that path! Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to use a path that defaults to /dev/null

@coreypobrien coreypobrien force-pushed the sshkey branch 2 times, most recently from 353f8c4 to 8c82698 Compare May 17, 2017 13:40
@sym3tri
Copy link
Contributor

sym3tri commented May 24, 2017

bump on review
/cc @s-urbaniak @alexsomesan

@s-urbaniak
Copy link
Contributor

ok to test

@s-urbaniak
Copy link
Contributor

@coreypobrien Hey, sorry for the late response. Do you mind to correct the offending file?

CI says:

misformatted files (run 'terraform fmt .' to fix): platforms/metal/remote.tf

@coreypobrien
Copy link
Contributor Author

Rebased and formatted

alekssaul added a commit to alekssaul/tectonic-installer that referenced this pull request Jun 6, 2017
alekssaul added a commit to alekssaul/tectonic-installer that referenced this pull request Jun 6, 2017
s-urbaniak pushed a commit that referenced this pull request Jun 7, 2017
@@ -200,6 +200,10 @@ tectonic_service_cidr = "10.3.0.0/16"
// Example: `ssh-rsa AAAB3N...`
tectonic_ssh_authorized_key = ""

// (optional) SSH private key file corresponding to tectonic_ssh_authorized_key. If not provided, SSH agent will be used.
// Example: `/root/.ssh/id_rsa`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

based on my testing the key is required to be on PEM format. can we change the line to ~ "Example: /root/.ssh/id_rsa.pem" if that's accurate?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not super excited about that idea because ~/.ssh/id_rsa is a well-known default name for a private key. Curious to hear other opinions, though.

Allow users to specify the SSH private key via a variable in cases where
ssh-agent isn't available or configured.
lblackstone pushed a commit to rcbops/tectonic-installer that referenced this pull request Aug 7, 2017
@cpanato
Copy link
Contributor

cpanato commented Sep 6, 2017

ok to test

@coreosbot
Copy link

Can one of the admins verify this patch?

1 similar comment
@coreosbot
Copy link

Can one of the admins verify this patch?

@s-urbaniak
Copy link
Contributor

let's close this for now. if you feel that feature needs to land, feel free to reopen.

@s-urbaniak s-urbaniak closed this Sep 6, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants