Add pubkey generation support #11

FrenchBen · 2018-02-15T01:57:36Z

Although this may not be very common, I often find myself having to feed a private key to Terraform, but then needing its public key for provider specific deployments.

This adds an equivalent public key RSA generator to the current private key, and moves the publicKey function to the public key file.

The input is a private key string, or file.

Checking acceptance tests:

$ make testacc
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test $(go list ./... |grep -v 'vendor') -v  -timeout 120m
?   	github.com/terraform-providers/terraform-provider-tls	[no test files]
=== RUN   TestProvider
--- PASS: TestProvider (0.00s)
=== RUN   TestCertRequest
--- PASS: TestCertRequest (0.04s)
=== RUN   TestLocallySignedCert
--- PASS: TestLocallySignedCert (0.02s)
=== RUN   TestPrivateKeyRSA
--- PASS: TestPrivateKeyRSA (3.21s)
=== RUN   TestPrivateKeyECDSA
--- PASS: TestPrivateKeyECDSA (0.05s)
=== RUN   TestPublicKeyRSA
--- PASS: TestPublicKeyRSA (4.69s)
=== RUN   TestSelfSignedCert
--- PASS: TestSelfSignedCert (0.04s)
PASS
ok  	github.com/terraform-providers/terraform-provider-tls/tls	8.077s

Signed-off-by: French Ben <frenchben@docker.com>

FrenchBen · 2018-02-15T01:59:56Z

There's a good amount of duplicate code with the tls_private_key module, perhaps we can simply add support to the generation to have a private key string submitted, in which case it's not generated?

vancluever · 2018-02-15T02:51:54Z

Hey @FrenchBen, this might also answer your second question as well, but I think this would be better as a data source. Main reason being is that a private key already needs to exist to extract the public key from, so nothing is actually being created by the resource.

More specifically answering your question about reuse - you could probably take the extraction code starting here and throw it into a helper that both tls_private_key (the resource) and tls_public_key (the data source) could use. The code may need a bit of re-arrangement so that the resulting helper is only exporting the public fields, leaving the private fields to the resource only.

Signed-off-by: French Ben <frenchben@docker.com>

FrenchBen · 2018-02-17T01:55:52Z

Updated to use a data_source instead, as recommended:

$ make testacc
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test $(go list ./... |grep -v 'vendor') -v  -timeout 120m
?   	github.com/terraform-providers/terraform-provider-tls	[no test files]
=== RUN   TestAccPublicKey_dataSource
--- PASS: TestAccPublicKey_dataSource (0.02s)
=== RUN   TestProvider
--- PASS: TestProvider (0.00s)
=== RUN   TestCertRequest
--- PASS: TestCertRequest (0.04s)
=== RUN   TestLocallySignedCert
--- PASS: TestLocallySignedCert (0.03s)
=== RUN   TestPrivateKeyRSA
--- PASS: TestPrivateKeyRSA (4.10s)
=== RUN   TestPrivateKeyECDSA
--- PASS: TestPrivateKeyECDSA (0.05s)
=== RUN   TestSelfSignedCert
--- PASS: TestSelfSignedCert (0.04s)
PASS
ok  	github.com/terraform-providers/terraform-provider-tls/tls	4.300s

vancluever

@FrenchBen great job so far! Just got some review comments that I'd like to see changed before we merge, and also the resource is lacking documentation. Can you make sure you add that as well?

You can see some examples on existing documentation for this provider in the website directory, and if you want to preview it you can clone the terraform-website repository and follow the instructions there to get set up.

Thanks!

vancluever · 2018-02-17T16:20:05Z

tls/data_source_public_key.go

+	return &schema.Resource{
+		Read: dataSourcePublicKeyRead,
+		Schema: map[string]*schema.Schema{
+			"private_key": &schema.Schema{


Can this be changed to private_key_pem to bring it in line with the other resources in this provider?

vancluever · 2018-02-17T16:20:52Z

tls/data_source_public_key.go

+				Type:        schema.TypeString,
+				Optional:    true,
+				Description: "File path of the PEM formatted string to use as the private key",
+				ForceNew:    true,


ForceNew is not needed in data sources. This generally controls deletion/creation workflows, both of which data sources lack.

vancluever · 2018-02-17T16:21:44Z

tls/data_source_public_key.go

+				Description: "PEM formatted string to use as the private key",
+			},
+
+			"private_key_path": &schema.Schema{


I'm actually wondering if maybe this key should just be removed - this removes a bit of complexity from the resource, and if someone needs to load a file from a local path, they can just use the file interpolation function.

Great point! It'll make things a lot easier.

vancluever · 2018-02-17T16:24:36Z

tls/data_source_public_key.go

+
+func dataSourcePublicKeyRead(d *schema.ResourceData, meta interface{}) error {
+	// Read private key
+	bytes := []byte(d.Get("private_key").(string))


You could also use GetOk or GetOkExists here, which are specifically the exact ways you check for a value defined in Terraform.

vancluever · 2018-02-17T16:25:37Z

tls/data_source_public_key.go

+	keyPemBlock, _ := pem.Decode(bytes)
+
+	if keyPemBlock == nil || (keyPemBlock.Type != "RSA PRIVATE KEY" && keyPemBlock.Type != "EC PRIVATE KEY") {
+		return fmt.Errorf("failed to decode PEM block containing RSA private key")


The error message seems to indicate that you are only attempting to decode an RSA private key even though the key can be that or an EC private key.

vancluever · 2018-02-17T16:27:05Z

tls/data_source_public_key.go

+		return fmt.Errorf("error converting key to rsa %s", err)
+	}
+
+	d.Set("private_key_pem", keyPem)


Looks like you have the field here named private_key_pem even though the field is just private_key in the schema. I'm recommending the schema be fixed, of course. :)

vancluever · 2018-02-17T16:29:53Z

tls/util.go

@@ -74,3 +75,29 @@ func parseCertificateRequest(d *schema.ResourceData, pemKey string) (*x509.Certi

 	return certReq, nil
 }
+
+func parsePublicKey(d *schema.ResourceData, rsaKey interface{}) error {


Minor can we change this to readPublicKey to imply that it's doing a bit more than just parsing the public key?

vancluever · 2018-02-17T16:38:07Z

tls/data_source_public_key_test.go

+}
+
+const testAccDataSourcePublicKeyConfig = `
+data "tls_public_key" "test" {


PS: You could probably make this test self-hosting but using the tls_private_key resource to generate a key. Then you could use TestCheckResourceAttrPair to just compare the two values.

I'll add it as another test. I like the fact that we're taking a private key without it's matching Public Key and confirming that its output is what we expected.
The TestCheckResourceAttrPair seems redundant, as they both use the same function, thus if there's a mistake, it'll come be a consistent one.

@FrenchBen fair enough, and I actually didn't notice the testPrivateKey fixture being used in other tests. Will wait for the second test!

Signed-off-by: French Ben <frenchben@docker.com>

FrenchBen · 2018-02-27T02:23:07Z

@vancluever PTAL at the latest changes :)

vancluever · 2018-02-27T16:35:41Z

@FrenchBen sorry for leaving this so long - I'll have a look sometime within the next couple of days for sure. Were you going to work on adding the second test as mentioned?

FrenchBen · 2018-02-28T00:52:57Z

@vancluever already did:
https://github.com/terraform-providers/terraform-provider-tls/pull/11/files#diff-db53aa662810ae9abc7cf890db96dc69R50

The 2 tests are:

Given a known private key, verify that the public key matches expected result.
Generate a private key, verify that the public key matches that of the generated public key (they use the same function, so it's a bit of an odd test).

Anything else you think I should test for?

vancluever · 2018-02-28T00:54:32Z

Great - I didn't see that!

Barring anything serious, I think this can be merged tomorrow when I give it another once over 🙂

FrenchBen · 2018-02-28T00:55:42Z

I think it's my fault - Looks like spacing is a little off, I'll get this fixed.

Signed-off-by: French Ben <frenchben@docker.com>

vancluever

Hey @FrenchBen, one more comment about the tests, in addition to the following smoke test that failed:

This config:

resource "tls_private_key" "key" {
  algorithm   = "ECDSA"
  ecdsa_curve = "P384"
}

data "tls_public_key" "pub" {
  private_key_pem = "${tls_private_key.key.private_key_pem}"
}

output "data_public_key_pem" {
  value = "${data.tls_public_key.pub.public_key_pem}"
}

output "data_public_key_openssh" {
  value = "${data.tls_public_key.pub.public_key_openssh}"
}

Gives this error:

Error: Error applying plan:

1 error(s) occurred:

* data.tls_public_key.pub: data.tls_public_key.pub: error converting key to rsa asn1: structure error: tags don't match (2 vs {class:0 tag:4 length:48 isCompound:false}) {optional:false explicit:false application:false defaultValue:<nil> tag:<nil> stringType:0 timeType:0 set:false omitEmpty:false}  @5

This works with RSA, so not too sure what's happening here, but the P384 test is what is in our examples for tls_private_key, and the public keys dump in that resource just fine.

If you can fix that it would be great (might be good to add another test with that config specifically), also you will need to pull as I just fixed up some test naming and docs, and was about to approve this before the smoke test failed.

Thanks!

vancluever · 2018-03-01T01:05:01Z

tls/data_source_public_key_test.go

+					resource "tls_private_key" "test" {
+						algorithm = "RSA"
+					}
+					output "private_key_pem" {


Are these outputs actually being tested anywhere? I don't see any checks for them...

Can this test also be broken out into another test? As it's pretty much a completely different test case (manually supplied key versus generated one).

The output can be removed indeed.
Since the test was for all Public Key, I thought they belonged within the same function, but as separate steps. Is that not the right approach?

Signed-off-by: French Ben <frenchben@docker.com>

FrenchBen · 2018-03-02T04:03:57Z

Added the extra test - Different algo = different parser (of course 🤦‍♂️ ) - Moved to using the default private key parser in the certs to make things cleaner.

vancluever · 2018-03-03T01:26:52Z

@FrenchBen this looks good! I've just split out the tests so that it's more clear what exactly we are testing in each step.

Otherwise, this fixed everything - smoke test works now, so I think we are good to merge!

Thank you very much for you contribution and your patience through the process!

github-actions · 2024-05-30T08:52:54Z

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.

French Ben added 5 commits February 6, 2018 17:54

Start of public key resource

429f8a4

Signed-off-by: French Ben <frenchben@docker.com>

Created initial pubkey gen code

9bce802

Signed-off-by: French Ben <frenchben@docker.com>

Added test for RSA public key

249d0bf

Signed-off-by: French Ben <frenchben@docker.com>

Added missing tests

1ccb617

Signed-off-by: French Ben <frenchben@docker.com>

Removed duplicate test

f495897

Signed-off-by: French Ben <frenchben@docker.com>

vancluever added the enhancement label Feb 15, 2018

French Ben added 2 commits February 16, 2018 17:36

Migrated resource to data source

ea7bb3a

Signed-off-by: French Ben <frenchben@docker.com>

Added DataSource test

a52fab1

Signed-off-by: French Ben <frenchben@docker.com>

vancluever suggested changes Feb 17, 2018

View reviewed changes

French Ben added 2 commits February 20, 2018 11:33

Tweaks following the suggestions from vancluever

aadd061

Signed-off-by: French Ben <frenchben@docker.com>

Added docs around the data source

b9d2daa

Signed-off-by: French Ben <frenchben@docker.com>

Fixed spacing to be consistent

de96279

Signed-off-by: French Ben <frenchben@docker.com>

FrenchBen force-pushed the add-pubkey branch from b762db2 to de96279 Compare February 28, 2018 00:57

vancluever suggested changes Mar 1, 2018

View reviewed changes

Extra test and use built-in parser

d8ba204

Signed-off-by: French Ben <frenchben@docker.com>

vancluever approved these changes Mar 3, 2018

View reviewed changes

vancluever merged commit 7f8e015 into hashicorp:master Mar 3, 2018

FrenchBen deleted the add-pubkey branch March 3, 2018 02:30

github-actions bot locked as resolved and limited conversation to collaborators May 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add pubkey generation support #11

Add pubkey generation support #11

FrenchBen commented Feb 15, 2018

FrenchBen commented Feb 15, 2018

vancluever commented Feb 15, 2018 •

edited

Loading

FrenchBen commented Feb 17, 2018 •

edited

Loading

vancluever left a comment

vancluever Feb 17, 2018

vancluever Feb 17, 2018

vancluever Feb 17, 2018 •

edited

Loading

FrenchBen Feb 20, 2018

vancluever Feb 17, 2018

vancluever Feb 17, 2018

vancluever Feb 17, 2018

vancluever Feb 17, 2018

vancluever Feb 17, 2018

FrenchBen Feb 20, 2018 •

edited

Loading

vancluever Feb 20, 2018

FrenchBen commented Feb 27, 2018

vancluever commented Feb 27, 2018

FrenchBen commented Feb 28, 2018 •

edited

Loading

vancluever commented Feb 28, 2018

FrenchBen commented Feb 28, 2018

vancluever left a comment •

edited

Loading

vancluever Mar 1, 2018 •

edited

Loading

FrenchBen Mar 2, 2018 •

edited

Loading

FrenchBen commented Mar 2, 2018

vancluever commented Mar 3, 2018

github-actions bot commented May 30, 2024

Add pubkey generation support #11

Add pubkey generation support #11

Conversation

FrenchBen commented Feb 15, 2018

FrenchBen commented Feb 15, 2018

vancluever commented Feb 15, 2018 • edited Loading

FrenchBen commented Feb 17, 2018 • edited Loading

vancluever left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vancluever Feb 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FrenchBen Feb 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FrenchBen commented Feb 27, 2018

vancluever commented Feb 27, 2018

FrenchBen commented Feb 28, 2018 • edited Loading

vancluever commented Feb 28, 2018

FrenchBen commented Feb 28, 2018

vancluever left a comment • edited Loading

Choose a reason for hiding this comment

vancluever Mar 1, 2018 • edited Loading

Choose a reason for hiding this comment

FrenchBen Mar 2, 2018 • edited Loading

Choose a reason for hiding this comment

FrenchBen commented Mar 2, 2018

vancluever commented Mar 3, 2018

github-actions bot commented May 30, 2024

vancluever commented Feb 15, 2018 •

edited

Loading

FrenchBen commented Feb 17, 2018 •

edited

Loading

vancluever Feb 17, 2018 •

edited

Loading

FrenchBen Feb 20, 2018 •

edited

Loading

FrenchBen commented Feb 28, 2018 •

edited

Loading

vancluever left a comment •

edited

Loading

vancluever Mar 1, 2018 •

edited

Loading

FrenchBen Mar 2, 2018 •

edited

Loading