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

tls_private_key resource: adding support for ED25519 key algorithm #151

Merged
merged 22 commits into from
Feb 24, 2022

Conversation

detro
Copy link
Contributor

@detro detro commented Feb 19, 2022

Context: #150

This PR starts from the work of #59 (thank you!) and moves towards the use of upcoming features of x/crypto/ssh (link).

A configuration like this will now be possible:

resource "tls_private_key" "test" {
    algorithm = "ED25519"
}

output "private_key_pem" {
    value = "${tls_private_key.test.private_key_pem}"
    sensitive = true
}

output "private_key_openssh" {                                      #< new
    value = "${tls_private_key.test.private_key_openssh}"
    sensitive = true
}

output "public_key_pem" {
    value = "${tls_private_key.test.public_key_pem}"
}

output "public_key_openssh" {
    value = "${tls_private_key.test.public_key_openssh}"
}

output "public_key_fingerprint_md5" {
    value = "${tls_private_key.test.public_key_fingerprint_md5}"
}

output "public_key_fingerprint_sha256" {.                           #< new
    value = "${tls_private_key.test.public_key_fingerprint_sha256}"
}

Subtasks in this PR:

  • New private_key_openssh for all key algorithm that we can generate OpenSSH PEM format for (i.e. this excludes ECDSA P-224, because of SSH limitations)
  • New public_key_fingerprint_sha256 for all key algorithms that we can generate OpenSSH PEM format for (same as above)
  • Extend test-set to cover the new fields
  • Update tls_private_key documentation
  • Extend testing matrix to include all current minor versions of Terraform

PLEASE NOTE: This PR does NOT cover everything we want to address in #150 but focuses around tls_private_key. This means that we will not be cutting a release until all the items in #150 are addressed.

Copy link
Member

@kmoe kmoe left a comment

Choose a reason for hiding this comment

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

Adding comments from verbal review. Excellent work and LGTM in general with minor changes.

go.mod Outdated Show resolved Hide resolved
internal/openssh/lib_test.go Outdated Show resolved Hide resolved
internal/openssh/lib_test.go Outdated Show resolved Hide resolved
internal/openssh/lib_test.go Outdated Show resolved Hide resolved
internal/provider/resource_private_key.go Outdated Show resolved Hide resolved
internal/provider/util.go Outdated Show resolved Hide resolved
website/docs/r/private_key.html.md Outdated Show resolved Hide resolved
internal/openssh/lib_test.go Outdated Show resolved Hide resolved
internal/openssh/lib_test.go Outdated Show resolved Hide resolved
internal/openssh/lib_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@bflad bflad left a comment

Choose a reason for hiding this comment

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

Looking good so far 😄 Just a few things to consider while we are in the neighborhood.

internal/openssh/lib.go Outdated Show resolved Hide resolved
internal/provider/resource_private_key.go Outdated Show resolved Hide resolved
internal/provider/resource_private_key_test.go Outdated Show resolved Hide resolved
website/docs/r/private_key.html.md Outdated Show resolved Hide resolved
website/docs/r/private_key.html.md Outdated Show resolved Hide resolved
website/docs/r/private_key.html.md Outdated Show resolved Hide resolved
website/docs/r/private_key.html.md Outdated Show resolved Hide resolved
website/docs/r/private_key.html.md Show resolved Hide resolved
website/docs/r/private_key.html.md Show resolved Hide resolved
Copy link

@cipherboy cipherboy left a comment

Choose a reason for hiding this comment

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

Nothing major from my end, just a couple of nits.

internal/openssh/lib.go Outdated Show resolved Hide resolved
internal/openssh/lib.go Outdated Show resolved Hide resolved
internal/provider/resource_private_key.go Outdated Show resolved Hide resolved
website/docs/r/private_key.html.md Outdated Show resolved Hide resolved
website/docs/r/private_key.html.md Show resolved Hide resolved
invidian and others added 16 commits February 22, 2022 17:26
This commit add support for ED25519 algorithm when generating
tls_private_key resource.

Refs #26

Signed-off-by: Mateusz Gozdek <mgozdekof@gmail.com>
This commit adds private_key_openssh attribute, which always contains
private key in format, which is compatible with OpenSSH.

This allows to produce ED25519 private key in OpenSSL compatible format
in private_key_pem attribute and OpenSSH-compatible format in this new
attribute.

Other key types are the same in private_key_pem and private_key_openssh,
as OpenSSH can read them. In the future, this could be changed to produce
all private keys OpenSSH native format.

Refs #26

Signed-off-by: Mateusz Gozdek <mgozdekof@gmail.com>
This is a temporary solution. The content is cherry-picked from Cherry-picking from https://go-review.googlesource.com/c/crypto/+/218620.
Once that is upstreamed, we can remove this and use methods from the official `x/crypto/ssh` module.
The purpose of this is to reduce the reliance on generic `string` and lean a bit more on the compiler.
…matted keys

Notice the "gotchas" around ECDSA elliptic P-224 curves
…data source

This is necessary as the function `readPublicKey()` is shared between resources and data sources.
…s_public_key` data source.

They are both getting updated as they share the `utils.go#readPublicKey()` function.
Co-authored-by: kmoe <5575356+kmoe@users.noreply.github.com>
Co-authored-by: kmoe <5575356+kmoe@users.noreply.github.com>
Co-authored-by: kmoe <5575356+kmoe@users.noreply.github.com>
@detro detro force-pushed the detro/ed25516-tls_private_key branch from 415c170 to c9e8373 Compare February 22, 2022 17:42
@detro detro changed the title Adding support for ED25519 key algorithm to tsl_private_key resource tsl_private_key resource: adding support for ED25519 key algorithm Feb 23, 2022
@detro detro changed the title tsl_private_key resource: adding support for ED25519 key algorithm tls_private_key resource: adding support for ED25519 key algorithm Feb 23, 2022
Copy link

@cipherboy cipherboy left a comment

Choose a reason for hiding this comment

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

Looks like all my changes are addressed.

@detro detro merged commit 99eb433 into main Feb 24, 2022
@detro detro deleted the detro/ed25516-tls_private_key branch February 24, 2022 13:12
@detro detro mentioned this pull request Feb 24, 2022
5 tasks
@detro detro added this to the v3.2.0 milestone Mar 16, 2022
@detro detro self-assigned this Mar 30, 2022
jackivanov pushed a commit to jackivanov/terraform-provider-tls that referenced this pull request Aug 4, 2022
…ashicorp#151)

* r/private_key: Add support for ed25519 algorithm

This commit add support for ED25519 algorithm when generating
tls_private_key resource.

Refs hashicorp#26

Signed-off-by: Mateusz Gozdek <mgozdekof@gmail.com>

* r/private_key: Add private_key_openssh attribute

This commit adds private_key_openssh attribute, which always contains
private key in format, which is compatible with OpenSSH.

This allows to produce ED25519 private key in OpenSSL compatible format
in private_key_pem attribute and OpenSSH-compatible format in this new
attribute.

Other key types are the same in private_key_pem and private_key_openssh,
as OpenSSH can read them. In the future, this could be changed to produce
all private keys OpenSSH native format.

Refs hashicorp#26

Signed-off-by: Mateusz Gozdek <mgozdekof@gmail.com>

* Utility package to marshal `crypto.PrivateKey` to OpenSSH PEM format

This is a temporary solution. The content is cherry-picked from Cherry-picking from https://go-review.googlesource.com/c/crypto/+/218620.
Once that is upstreamed, we can remove this and use methods from the official `x/crypto/ssh` module.

* Removing `marshalED25519PrivateKey` from `util.go` in favour of the (temporary) `openssh` package

* Adding type `Algorithm` to use in maps and signatures

The purpose of this is to reduce the reliance on generic `string` and lean a bit more on the compiler.

* Switching to use the `openssh` package for generating OpenSSH PEM formatted keys

Notice the "gotchas" around ECDSA elliptic P-224 curves

* Adding `public_key_fingerprint_sha256` attribute to `tls_private_key` resource

* Update `tls_private_key` resource testing to reflect all the recent changes

* Adding attribute `public_key_fingerprint_sha256` to `tls_public_key` data source

This is necessary as the function `readPublicKey()` is shared between resources and data sources.

* Updating website documentation for `tls_private_key` resource and `tls_public_key` data source.

They are both getting updated as they share the `utils.go#readPublicKey()` function.

* Update internal/openssh/lib_test.go (typo)

Co-authored-by: kmoe <5575356+kmoe@users.noreply.github.com>

* Update website/docs/r/private_key.html.md

Co-authored-by: kmoe <5575356+kmoe@users.noreply.github.com>

* Update internal/openssh/lib_test.go

Co-authored-by: kmoe <5575356+kmoe@users.noreply.github.com>

* Fixing indentation

* Removing dependency on `testify` as requested by Katy Moe

* Rewarding description for 2 fields

* Moving "types" into "types.go" and out of "util.go"

* Adding input argument validations to `tls_private_key`

* Updating markdown documentation to address PR feedback

* Avoided creating exported constants in `internal/openssh` library as this is a temporary solution

We want to get rid of it as soon as hashicorp#154 becomes actionable

* Fix typo: marshall -> marshal

* Adding a 'copyright header' on the 'internal/openssh/lib.go' file

Co-authored-by: Mateusz Gozdek <mgozdekof@gmail.com>
Co-authored-by: kmoe <5575356+kmoe@users.noreply.github.com>
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 28, 2024
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.

6 participants