Add Signing Keys to client resource #66
Conversation
|
@sergiughf - Pointed my changes at the correct repo. |
|
Also pulling in my last comment since it's still relevant. Unsure if it's a reasonable expectation to assume there'd be one signing key based on this - https://github.com/go-auth0/auth0/blob/75aed4b65cb8105ed5466769269322501acbc0d7/management/signing_key.go#L5 Let me know your thoughts. Thanks! |
|
Hey @ccogan-lh thanks a lot for reopening this PR. I'll make sure to finish reviewing it by EOD. |
auth0/resource_auth0_client.go
Outdated
| if l := List(d, "signing_keys", IsNewResource(), HasChange()); l != nil { | ||
| c.SigningKeys = []map[string]string{} | ||
| for _, v := range l.List() { | ||
| mapString := make(map[string]string) | ||
|
|
||
| for key, value := range v.(map[string]interface{}) { | ||
| strKey := fmt.Sprintf("%v", key) | ||
| strValue := fmt.Sprintf("%v", value) | ||
| mapString[strKey] = strValue | ||
| } | ||
| c.SigningKeys = append(c.SigningKeys, mapString) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
The expandClient function is used to prepare the content found within the terraform HCL file to be sent over through the Go SDK to the Management API. If we include signing_keys over here it means that it will try to send it within the request to create or update a client and the request will fail because the schema for those 2 endpoints doesn't allow signing_keys to be within the payload. So this code cannot be here:)
I'm wondering however what is your use case for having the signing_keys on the client? Would help a lot to give a bit more context as to why this feature is needed.
Also if you just need to read it you should use the client data source instead: https://github.com/alexkappa/terraform-provider-auth0/blob/master/docs/datasources/client.md (it's using the same schema as the client resource so we would still need to keep the signing_keys within the client schema but please pay attention to the fact that the signing keys are a []map[string]string).
There was a problem hiding this comment.
I was hoping to treat it like client_id in client. As in a value that's computed only and not overwritten. Does client_id get overwritten?
Would love to avoid the double dip as a data source. I am looking to parse the JWT ( https://github.com/golang-jwt/jwt/blob/2ebb50f957d606de5909fcf9ed49f9af3bc35e97/rsa_utils.go ) like so. Would love to create the client and then stuff the keys where my service can find it.
Looking at this further, I do not see the client_id in the expand client, so perhaps removing that is the right move? I was just following your input from here alexkappa/terraform-provider-auth0#490 (comment)
There was a problem hiding this comment.
I have removed the expandClient piece based on your comment though @sergiughf .
There was a problem hiding this comment.
Hey @ccogan-lh, I cloned this branch locally and started performing some tests. To save you some time I went ahead and applied the needed changes to make this work as tests were failing. Here's a summary of what I changed:
"signing_keys": {
Type: schema.TypeList,
Elem: &schema.Schema{Type: schema.TypeMap},
Computed: true,
},Signing Keys are actually []map[string]string. This is because we can have more than just 1 signing key available on a client. If you rotate your tenant signing keys you should have 2. The new one and the old one (you keep the old one around for old JWTs that are hanging around so you don’t have any downtime). This is why the previous definition of the signing_keys had to be changed as we only accounted for one. Also the definition we have within the Go SDK is SigningKeys []map[string]string json:"signing_keys,omitempty" without mentioning the included fields.
I was able afterwards to also test this manually with the following very minimal terraform config:
provider "auth0" {
debug = true
}
data "auth0_client" "my_client" {
name = "Default App" // the name of an app in my tenant
}
output "signing_keys" {
value = data.auth0_client.my_client.signing_keys
}When running terraform plan we get the following:
Changes to Outputs:
+ signing_keys = [
+ {
+ "cert" = <<-EOT
-----BEGIN CERTIFICATE-----
bunch of characters
-----END CERTIFICATE-----
EOT
+ "pkcs7" = <<-EOT
-----BEGIN PKCS7-----
bunch of characters
-----END PKCS7-----
EOT
+ "subject" = "deprecated"
},
]
Notice that I used the client data source instead of the client resource as I was interested only in the signing key data. This would work as well if you're reusing the client resource to fetch the outputed signing_keys as well (although I suggest the data source for your use case as it minimized the terraform config needed to maintain).
To address some of your previous comments:
Looking at this further, I do not see the client_id in the expand client, so perhaps removing that is the right move? I was just following your input from here alexkappa/terraform-provider-auth0#490 (comment)
Thanks for pointing that out:) this was a mistake on my behalf as I was unaware previously that we cannot modify the signing_keys from within the client post / patch calls. Apologies for the confusion caused 🙏🏻
There was a problem hiding this comment.
Please let me know if you have any questions around this @ccogan-lh :) Happy to explain further if anything's unclear.
|
Note: tests are failing as this contribution is coming from a fork that doesn't have access to our secrets. |
sergiught
force-pushed
the
signing_keys
branch
from
0bded70 to
6151377
Compare
Description
signing_keystoreadClientSigningKeys is already supported in management client, so exposing this.
Checklist
Note: Checklist required to be completed before a PR is considered to be reviewable.
Auth0 Code of Conduct
Auth0 General Contribution Guidelines
Changes include test coverage?
Does the description provide the correct amount of context?
Have you updated the documentation?
Is this code ready for production?