-
Notifications
You must be signed in to change notification settings - Fork 850
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
NewClientCertificateCredential requires certificate bytes instead of path #15604
Conversation
@@ -56,35 +53,18 @@ type ClientCertificateCredential struct { | |||
// NewClientCertificateCredential creates an instance of ClientCertificateCredential with the details needed to authenticate against Azure Active Directory with the specified certificate. | |||
// tenantID: The Azure Active Directory tenant (directory) ID of the service principal. | |||
// clientID: The client (application) ID of the service principal. | |||
// certificatePath: The path to the client certificate used to authenticate the client. Supported formats are PEM and PFX. | |||
// certificateData: The bytes of a certificate in PEM or PKCS12 format, including the private key. |
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.
Presumably this is just the bytes from a call to ioutil.ReadAll()
or something similar. Assuming that's the case, it would be good to add an example demonstrating this.
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.
Another option would be to take an io.Reader
instead which might cut down on the amount of code the caller has to write.
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.
I picked the slice because it's the lowest common denominator and we need all the data in memory to deserialize it anyway. I suppose the question is whether a caller is more likely to have []byte
or io.Reader
. Do you think we create more struggle overall by requiring the slice?
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.
I don't think it creates more struggle (as you mentioned, it's a trivial amount of code), I'm just wondering if an io.Reader
will provide more flexibility in the future.
The key vault example is an interesting one. I don't know what the return type looks like for that when retrieving a cert, but if it's just binary data it would be returned as an io.ReadCloser
. In that case we could simply pass the result to this constructor without requiring the caller to read it themselves.
Do you know what other languages accept here?
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.
The value from Key Vault would be a plain string
(i.e. not binary). .NET and Python expose it as string
.
Every SDK's ClientCertificateCredential
has a constructor taking a path (but I think consistency in that is not important for us because we only get one "good" constructor). Everyone but JS takes an alternative as well: for .NET it's X509Certificate2
; for Java, a byte stream; for Python, non-streaming bytes.
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.
Interesting, surprises me it's a string. Is the underlying data base-64 encoded?
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.
Only for PKCS12 certs. So to consume the value from Key Vault directly, we would have to consider that cert data not in PEM format might be base64 encoded PKCS12 content. For Python, I left the decoding up to applications and added a sample showing how to do it.
cert, err = extractFromPFXFile(certData, options.Password, options.SendCertificateChain) | ||
} else { | ||
err = errors.New("only PEM and PFX files are supported") | ||
cert, err := extractFromPEMFile(certificateData, options.Password, options.SendCertificateChain) |
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.
I assume that the contents will always be sufficient to disambiguate the various kinds of supported certificate formats (not that the path was much better beyond file extension)?
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.
That's correct. We won't get an unusable PKCS12 cert from PEM content or vice versa; deserialization will just fail. I'll add some tests using a PKCS12 cert to validate that. Considering that PEM format is obvious, the constructor could be smarter. I'll tweak it to try PKCS12 only when a cert definitely isn't in PEM format. Edit: pem.Decode
is cheap when input isn't in PEM format, so I'll keep the simpler constructor code.
928d8e5
to
c8d4d0b
Compare
c8d4d0b
to
e066862
Compare
We need a ClientCertificateCredential constructor that takes certificate bytes because sometimes that's all applications have, e.g. when they get certs from Key Vault, and it's sometimes impossible to write them to disk before creating a credential. Rather than add a second constructor, this PR has the existing one require a
[]byte
. I don't think the path parameter is worth keeping, because it only saves callers from writing a couple lines of trivial code.