-
Notifications
You must be signed in to change notification settings - Fork 300
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
Add token signing certificate resource #968
Conversation
@manicminer @katbyte - Welcoming feedback on this. I am sure there are some things that need addressed, but it works really well right now for adding the cert. We just need to figure out how to add the preferred thumbprint to the service principal. Since it would cause a circular dependency to reference this resource in the service_principal resource, I thought about adding a field to this resource such as |
d6b11b5
to
b2be150
Compare
This adds a new resource called `service_principal_token_signing_certificate` that is used to manage the whole lifecycle of token signing certificates used for SAML authentication. This resource makes use of the `AddTokenSigningCertificate` function that was added to hamilton previously here: manicminer/hamilton#158 MS Graphs Docs: https://learn.microsoft.com/en-us/graph/api/serviceprincipal-addtokensigningcertificate?view=graph-rest-1.0&tabs=http As documented in the docs above, when the `AddTokenSigningCertificate` function is invoked, 3 individual objects are created... - Verify `keyCredential` (Public Cert) - Sign `keyCredential` (Private Key) - `passwordCredential` (Private Key Password) When the object is returned, it includes the thumbprint, the public key pem value, and a `keyId`. However, we found an odd behavior that the `keyId` that is returned is actually for the Sign `keyCredential`. Since the Verify certificate is the one that we acutally care about, we used the `customKeyIdentifier`, which is the same for all 3 values, to get the Verify `keyId`, which we then use in building the resource ID. We additionally had to "calculate" the thumbprint value from the actual value of the Verify cert, as this value is not returned from the API, except after initial creation in the Create step. We did this by getting pem value of the Verify cert by adding the `$select=keyCredential` odata query to the GET of the service principal. By combining this value with the PEM header/footer, we can calculate the SHA-1 fingerprint, which matches up to the appropriate thumbprint. Finally, to delete the certificate, we have to PATCH the service principal with all 3 objects mentioned previously removed. To gather this, we used the `customKeyIdentifier` value in a loop. Closes hashicorp#732 And part of hashicorp#823
b2be150
to
5d99ce1
Compare
Fixed the lint issues. |
Basically, it seems better for terraform resources to be compliant with Microsoft Graph API. I think this is inevitable because the Microsoft Graph API is not well designed. (Since automatic renewal of certificates temporarily disables login to service providers that do not support IdP metadataURL, perhaps the inability to renew is an appropriate process.) In practical terms, the following responses are expected # this is an example
resource "azuread_service_principal_token_signing_certificate" "example" {
service_principal_id = azuread_service_principal.example.id
display_name = "CN=example.com"
end_date = "2023-05-01T01:02:03Z"
provisioner "local-exec" {
command = <<-SHELL
az ad sp update \
--id ${self.service_principal_id} \
--set preferredTokenSigningKeyThumbprint=${self.thumbprint}
SHELL
}
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @tagur87, thanks for this contribution! My changes below are just some minor tweaks, this otherwise looks great and is good to merge 🎸
In terms of setting the preferred certificate, I think perhaps a third resource is the most future-proof approach right now - particularly as the next major version of the provider will see more of these sorts of virtual resources. Something like:
resource "azuread_service_principal_preferred_token_signing_certificate" "example" {
service_principal_id = azuread_service_principal.example.object_id
certificate_id = azuread_service_principal_token_signing_certificate.example.id
}
Whilst that approach doesn't mitigate being able to create overlapping resources, it does reduce the config to a single place and will help with graphing dependencies in a future with more of these split-up virtual resources.
Since that can be added separately, we can still merge this PR. You're welcome to add that in another PR :)
Test results
internal/services/serviceprincipals/service_principal_token_signing_certificate_resource.go
Outdated
Show resolved
Hide resolved
internal/services/serviceprincipals/service_principal_token_signing_certificate_resource.go
Outdated
Show resolved
Hide resolved
tf.LockByName(servicePrincipalResourceName, objectId) | ||
defer tf.UnlockByName(servicePrincipalResourceName, objectId) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We want to move this before any write operations
tf.LockByName(servicePrincipalResourceName, objectId) | |
defer tf.UnlockByName(servicePrincipalResourceName, objectId) |
internal/services/serviceprincipals/service_principal_token_signing_certificate_resource.go
Show resolved
Hide resolved
internal/services/serviceprincipals/service_principal_token_signing_certificate_resource.go
Show resolved
Hide resolved
thumbprint, err := helpers.GetTokenSigningCertificateThumbprint( | ||
[]byte("-----BEGIN CERTIFICATE-----\n" + *credential.Key + "\n-----END CERTIFICATE-----")) | ||
if err != nil { | ||
return tf.ErrorDiagPathF(err, "id", "parsing tokenSigningCertificate key value with ID %q", id.KeyId) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice workaround :)
internal/services/serviceprincipals/service_principal_token_signing_certificate_resource.go
Outdated
Show resolved
Hide resolved
de4aab4
to
8f264a8
Compare
This functionality has been released in v2.33.0 of the Terraform Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you! |
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. |
This adds a new resource called
service_principal_token_signing_certificate
that is used to manage the whole lifecycle of token signing certificates used for SAML authentication.This resource makes use of the
AddTokenSigningCertificate
function that was added to hamilton previously here:manicminer/hamilton#158
MS Graphs Docs: https://learn.microsoft.com/en-us/graph/api/serviceprincipal-addtokensigningcertificate?view=graph-rest-1.0&tabs=http
As documented in the docs above, when the
AddTokenSigningCertificate
function is invoked, 3 individual objects are created...keyCredential
(Public Cert)keyCredential
(Private Key)passwordCredential
(Private Key Password)When the object is returned, it includes the thumbprint, the public key pem value, and a
keyId
. However, we found an odd behavior that thekeyId
that is returned is actually for the SignkeyCredential
.Since the Verify certificate is the one that we acutally care about, we used the
customKeyIdentifier
, which is the same for all 3 values, to get the VerifykeyId
, which we then use in building the resource ID.We additionally had to "calculate" the thumbprint value from the actual value of the Verify cert, as this value is not returned from the API, except after initial creation in the Create step. We did this by getting pem value of the Verify cert by adding the
$select=keyCredential
odata query to the GET of the service principal. By combining this value with the PEM header/footer, we can calculate the SHA-1 fingerprint, which matches up to the appropriate thumbprint.Finally, to delete the certificate, we have to PATCH the service principal with all 3 objects mentioned previously removed. To gather this, we used the
customKeyIdentifier
value in a loop.Closes #732
And part of #823