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

Refactor NewClientCertificateCredential #15683

Merged
merged 13 commits into from
Oct 28, 2021
Merged

Refactor NewClientCertificateCredential #15683

merged 13 commits into from
Oct 28, 2021

Conversation

chlowell
Copy link
Member

@chlowell chlowell commented Oct 1, 2021

This PR refactors NewClientCertificateCredential to take a slice of certificates and a private key instead of raw certificate data, and adds a helper method to simplify acquiring those in common cases. My motivation is primarily to avoid future problems around encrypted keys. There are many ways to encrypt a certificate's key, and the standard library doesn't support all of them. For example, its method for decrypting legacy PEM encryption is deprecated. Taking only raw cert data may oblige us to support such encryption schemes by writing our own decryption code or adding an external dependency, and asking customers to adopt an ugly workaround in the meantime.

before

certData, err := os.ReadFile("/cert.pem")
cred, err := NewClientCertificateCredential(tenantID, clientID, certData, password, nil)

after

certData, err := os.ReadFile("/cert.pem")
certs, key, err := LoadCerts(certData, password)
cred, err := NewClientCertificateCredential(tenantID, clientID, certs, key, nil)

LoadCerts handles common cases and can become more comprehensive over time; its capabilities are an implementation detail.

@chlowell chlowell requested a review from jhendrixMSFT October 1, 2021 18:42
@chlowell chlowell marked this pull request as ready for review October 4, 2021 22:00
@chlowell chlowell requested a review from RickWinter as a code owner October 4, 2021 22:00
@RickWinter RickWinter added Client This issue points to a problem in the data-plane of the library. Azure.Identity labels Oct 20, 2021
@richardpark-msft
Copy link
Member

Not too familiar with identity. Just some minor comments.

Copy link
Member

@richardpark-msft richardpark-msft left a comment

Choose a reason for hiding this comment

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

Just some comments, looks good to my untrained eye.

@@ -43,7 +43,7 @@ func createClientAssertionJWT(clientID string, audience string, cert *certConten
X5t: base64.RawURLEncoding.EncodeToString(cert.fp),
}
if sendCertificateChain {
headerData.X5c = cert.publicCertificates
headerData.X5c = cert.x5c
Copy link
Member

Choose a reason for hiding this comment

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

I do miss the more descriptive name. I know there's reasons, but still. :)

Copy link
Member

@richardpark-msft richardpark-msft left a comment

Choose a reason for hiding this comment

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

Looks good, one little nit with an error message.

@chlowell chlowell merged commit e89c102 into Azure:main Oct 28, 2021
@chlowell chlowell deleted the certs branch October 28, 2021 17:25
jhendrixMSFT pushed a commit to jhendrixMSFT/azure-sdk-for-go that referenced this pull request Jan 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Azure.Identity Client This issue points to a problem in the data-plane of the library.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants