Support InputStream as input for ClientCertificateCredential#15814
Support InputStream as input for ClientCertificateCredential#15814jianghaolu merged 6 commits intoAzure:masterfrom
Conversation
ghost
added
the
sdk/identity/azure-identity/src/main/java/com/azure/identity/ClientCertificateCredential.java
Show resolved
Hide resolved
...tity/azure-identity/src/main/java/com/azure/identity/ClientCertificateCredentialBuilder.java
Outdated
Show resolved
Hide resolved
| put("clientId", clientId); | ||
| put("tenantId", tenantId); | ||
| put("clientCertificate", clientCertificate); | ||
| put("clientCertificate", clientCertificate == null ? clientCertificatePath : clientCertificate); |
There was a problem hiding this comment.
The error message from Validation Util, won't be clear for user.
I think, we should custom handle this with the error message "A certificate source as input stream or file path should be configured on the builder."
We should also throw an exception, if both path and inputstream are configured.
There was a problem hiding this comment.
Changing the error message will break the current certificate path scenario - people may be depending on the error message.
There was a problem hiding this comment.
okay, then we should atleast add an additive check for the scenario and throw an exception if both path and inputstream are configured.
sdk/identity/azure-identity/src/main/java/com/azure/identity/EnvironmentCredential.java
Show resolved
Hide resolved
...tity/azure-identity/src/main/java/com/azure/identity/ClientCertificateCredentialBuilder.java
Outdated
Show resolved
Hide resolved
| */ | ||
| public ClientCertificateCredentialBuilder pfxCertificate(String certificatePath, String clientCertificatePassword) { | ||
| public ClientCertificateCredentialBuilder pfxCertificate(String certificatePath, | ||
| String clientCertificatePassword) { |
There was a problem hiding this comment.
It seems like we're requiring a password for PFX and not supporting a password for PEM. If we support password protected certificates, I think we should support them by adding a separate property such as Password or CertificatePassword
There was a problem hiding this comment.
Per discussion offline - this will be addressed in a future PR. Issue link coming soon
There was a problem hiding this comment.
After discussing this offline, this is something we should address in a subsequent release.
| ClientCertificateCredential(String tenantId, String clientId, String certificatePath, String certificatePassword, | ||
| IdentityClientOptions identityClientOptions) { | ||
| Objects.requireNonNull(certificatePath, "'certificatePath' cannot be null."); | ||
| ClientCertificateCredential(String tenantId, String clientId, String certificatePath, InputStream certificate, |
There was a problem hiding this comment.
Why do we have both the path to the file and the InputStream? If the inputstream is given, then we don't need the path right? Maybe have separate overloads - one with input stream and one with file path.
| if (clientCertificate != null && clientCertificatePath != null) { | ||
| throw logger.logExceptionAsWarning(new IllegalArgumentException("Both certificate input stream and " | ||
| + "certificate path are provided in ClientCertificateCredentialBuilder. Only one of them should " | ||
| + "be provided.")); | ||
| } |
There was a problem hiding this comment.
If we are doing this, then maybe the ClientCertificateCredential constructor should only have the input stream arg. If the path is given, the builder can wrap the path with FileInputStream and create the credential instance. This saves multiple if-else checks in ClientCertificateCredential.
4 participants
Fixes #11243