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

[V2] Use PFX Certificates for Client Auth instead of PEM #327

Closed
wants to merge 0 commits into from

Conversation

JenGoldstrich
Copy link
Contributor

@JenGoldstrich JenGoldstrich commented Aug 7, 2023

In the new SDK we are using in v2 client certificates behave the way they do for the Terraform Provider https://registry.terraform.io/providers/hashicorp/azuread/latest/docs/guides/service_principal_client_certificate using .pfx files only. This also allows user to use password protected certificates which is preferred for security. This will require users to generate new certificates sadly which is a breaking change

Adds client_cert_password field, as most pfx files are password protected and the SDK supports this

Removes client_cert_timeout field, which is not supported in the new SDK

This PR has a messy diff since its on top of my fork with all of V2 migration, but the main changers are in the azure_authorizer file, and in the common client config

Closes #46

@JenGoldstrich JenGoldstrich changed the title [V2] Implement client certificate authentication logic [V2] Use PFX Certificates for Client Auth instead of PEM Aug 7, 2023
@JenGoldstrich JenGoldstrich marked this pull request as ready for review August 10, 2023 19:09
@JenGoldstrich JenGoldstrich requested a review from a team as a code owner August 10, 2023 19:09
Copy link
Contributor

@lbajolet-hashicorp lbajolet-hashicorp left a comment

Choose a reason for hiding this comment

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

Still gotta work through the V2 PR (up next!), but the cert logic looks good to me. The TODO test needs to be written still, I'll circle back on this when it's done

@JenGoldstrich
Copy link
Contributor Author

@lbajolet-hashicorp I messaged you in slack about this but just to close the loop that test is not planned to be written as part of this PR, and is intended to document a pre-exsisting missing test case we want to circle back on when we have acceptance tests orchestrated in CI with Terraform

@nywilken nywilken added enhancement breaking-change version/bump major A PR that breaks backwards compatibility. security Security issues/fixes. labels Aug 11, 2023
Copy link
Contributor

@nywilken nywilken left a comment

Choose a reason for hiding this comment

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

Change wise this looks good. Please rebase and drop the rest of v2 before merging. Or you can drop and retarget to the v2 branch so that things get merged in order.

Since there are fields being removed and there is a new flow. Do we need to add more documentation for how to migrate to the new cert?

To not create too much conflict with the docs. Please include upgrade notes in the release notes on what users should do to migrate to the new PFX certificate.

JenGoldstrich added a commit that referenced this pull request Aug 11, 2023
* Implement v2 Certificate logic

* Make generate

* Add cert password to dtl and chroot

* add unimplemented test stub based on Wilken's feedback to track the creation of this test

* remove todo

* Regenerate docs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change enhancement security Security issues/fixes. version/bump major A PR that breaks backwards compatibility.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Azure Builder: certificate in .pem format is required for authentication using client_cert_path
3 participants