Skip to content
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

Allowlist non-sensitive data on Terraform state #598

Merged
merged 2 commits into from
Jun 18, 2021

Conversation

0x2b3bfa0
Copy link
Member

@0x2b3bfa0 0x2b3bfa0 commented Jun 16, 2021

To avoid future issues, we're migrating to a safer approach where we explicitly choose what to display instead of what to hide.

The cloudSshPrivateVisible option was introduced as a transition measure, but isn't necessary: users can always specify a custom key with --cloud-ssh-private

@0x2b3bfa0 0x2b3bfa0 added bug Something isn't working p0-critical Max priority (ASAP) security Sensitive flaws labels Jun 16, 2021
@0x2b3bfa0 0x2b3bfa0 self-assigned this Jun 16, 2021
@0x2b3bfa0 0x2b3bfa0 temporarily deployed to internal June 16, 2021 19:46 Inactive
@0x2b3bfa0 0x2b3bfa0 marked this pull request as ready for review June 16, 2021 19:46
@0x2b3bfa0 0x2b3bfa0 temporarily deployed to internal June 16, 2021 19:47 Inactive
@0x2b3bfa0 0x2b3bfa0 force-pushed the allowlist-non-sensitive-data branch from d6bf54e to 0fa0a6f Compare June 16, 2021 19:55
@0x2b3bfa0 0x2b3bfa0 temporarily deployed to internal June 16, 2021 19:56 Inactive
@@ -379,11 +387,6 @@ const opts = yargs
'cloud-ssh-private',
'Custom private RSA SSH key. If not provided an automatically generated throwaway key will be used'
)
.boolean('cloud-ssh-private-visible')
Copy link
Contributor

@DavidGOrtega DavidGOrtega Jun 18, 2021

Choose a reason for hiding this comment

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

@0x2b3bfa0 if we remove the functionality of showing the ssh we HAVE to remove the capability of creating the SSH automatically with all the friction that it generates. Makes no sense that we are able to create SSH keys and nt being able to give it back to them.
I think that giving them the capability of showing it at their own risk is acceptable

Let's do it. They can specify the SSH so it might be fine. The rationale of my original thought is because there are some users that might not know how to generate SSH keys (i found them), so I wanted to provide them it back

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's do it. They can specify the SSH so it might be fine.

Awesome! In my opinion, specifying the SSH private key is, if not the best solution, at least a good short term compromise between printing keys to the logs and having users do everything manually.

The rationale of my original thought is because there are some users that might not know how to generate SSH keys (I found them), so I wanted to provide them it back.

If that's the case, let's document the process:

  1. Generate2 a new SSH RSA (PEM) private key:

    $ ssh-keygen -t rsa -m pem -b 4096 -f key.pem
  2. Pass the --cloud-ssh-private to cml-runner including the contents1 of newly genereated private key file:

    $ cml-runner --cloud ··· --cloud-ssh-private="$(cat key.pem)"

1 Would be a bit more involved if we don't resort to the changes introduced with #602. Using perl or sed for this is not precisely an user-friendly solution.
2 Hardcoding key bit lengths in documentation is not future-proof, but perhaps better than recommending the default 3072 bit length.

Copy link
Member Author

Choose a reason for hiding this comment

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

🔔 @casperdcl, should we add this to the wiki under the cml-runner reference section?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would say no. What you are doing with that is making users generate the "useless" keys by themselves so the inbuilt feature is not needed anymore

Copy link
Member Author

Choose a reason for hiding this comment

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

if we remove the functionality of showing the ssh we HAVE to remove the capability of creating the SSH automatically with all the friction that it generates. Makes no sense that we are able to create SSH keys and nt being able to give it back to them.

It does make sense as long as we keep using them internally for log retrieval in some platforms as per iterative/terraform-provider-iterative#70. Anyhow.

I think that giving them the capability of showing it at their own risk is acceptable.

I don't want to be one of these cryptographers that develop popular libraries1 and explicitly forbid users from using certain functions, even if they know what they're doing.

Nevertheless, providing this functionality, for the reasons exposed in #598 (comment), could be equivalent to adding a yellow Tweet my Private Keys button in your favorite crypto wallet. 😉

Copy link
Member Author

@0x2b3bfa0 0x2b3bfa0 Jun 18, 2021

Choose a reason for hiding this comment

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

👌🏼 As long as they get it right...

Copy link
Contributor

Choose a reason for hiding this comment

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

should we add this to the wiki

please do!

Copy link
Member Author

Choose a reason for hiding this comment

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

Done! It leaves room for improvement, but at least it won't get lost.

Copy link
Contributor

Choose a reason for hiding this comment

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

I do not fully understand why we need that. Docs says:

Custom private RSA SSH key. If not provided an
                               automatically generated throwaway key will be
                               used

Additionally we are teaching them how to create a throwaway key? @casperdcl @0x2b3bfa0

Copy link
Member Author

Choose a reason for hiding this comment

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

If users take no action, the Terraform provider will automatically generate a throwaway key for internal use. This key won't be available to users and will only be used to retrieve machine logs and determine the runner status.

If users need to access the instance on their own, they will need to know how to provide their own key. As you can guess, we are also teaching them how to generate a new one because using their default key from .ssh/id_rsa:

  1. Might not be an option, as it's usually in OpenSSH format, not in PEM format.
  2. Would expose the user private key beyond what's acceptable in many scenarios.

@casperdcl casperdcl temporarily deployed to internal June 18, 2021 12:48 Inactive
@casperdcl casperdcl merged commit c51e870 into master Jun 18, 2021
@casperdcl casperdcl deleted the allowlist-non-sensitive-data branch June 18, 2021 13:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working p0-critical Max priority (ASAP) security Sensitive flaws
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants