-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Support InputStream as input for ClientCertificateCredential #15814
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
Changes from all commits
542a59f
2668eca
8ee96ea
b28db75
ee63ff1
70121ad
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,8 +3,10 @@ | |
|
|
||
| package com.azure.identity; | ||
|
|
||
| import com.azure.core.util.logging.ClientLogger; | ||
| import com.azure.identity.implementation.util.ValidationUtil; | ||
|
|
||
| import java.io.InputStream; | ||
| import java.util.HashMap; | ||
|
|
||
| /** | ||
|
|
@@ -13,31 +15,59 @@ | |
| * @see ClientCertificateCredential | ||
| */ | ||
| public class ClientCertificateCredentialBuilder extends AadCredentialBuilderBase<ClientCertificateCredentialBuilder> { | ||
| private String clientCertificate; | ||
| private String clientCertificatePath; | ||
| private InputStream clientCertificate; | ||
| private String clientCertificatePassword; | ||
| private final ClientLogger logger = new ClientLogger(ClientCertificateCredentialBuilder.class); | ||
|
|
||
| /** | ||
| * Sets the client certificate for authenticating to AAD. | ||
| * Sets the path of the PEM certificate for authenticating to AAD. | ||
| * | ||
| * @param certificatePath the PEM file containing the certificate | ||
| * @return An updated instance of this builder. | ||
| */ | ||
| public ClientCertificateCredentialBuilder pemCertificate(String certificatePath) { | ||
| ValidationUtil.validateFilePath(getClass().getSimpleName(), certificatePath, "Pem Certificate Path"); | ||
| this.clientCertificate = certificatePath; | ||
| this.clientCertificatePath = certificatePath; | ||
| return this; | ||
| } | ||
|
|
||
| /** | ||
| * Sets the client certificate for authenticating to AAD. | ||
| * Sets the input stream holding the PEM certificate for authenticating to AAD. | ||
| * | ||
| * @param certificate the input stream containing the PEM certificate | ||
| * @return An updated instance of this builder. | ||
| */ | ||
| public ClientCertificateCredentialBuilder pemCertificate(InputStream certificate) { | ||
| this.clientCertificate = certificate; | ||
| return this; | ||
| } | ||
|
|
||
| /** | ||
| * Sets the path and password of the PFX certificate for authenticating to AAD. | ||
| * | ||
| * @param certificatePath the password protected PFX file containing the certificate | ||
| * @param clientCertificatePassword the password protecting the PFX file | ||
| * @return An updated instance of this builder. | ||
| */ | ||
| public ClientCertificateCredentialBuilder pfxCertificate(String certificatePath, String clientCertificatePassword) { | ||
| public ClientCertificateCredentialBuilder pfxCertificate(String certificatePath, | ||
| String clientCertificatePassword) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Per discussion offline - this will be addressed in a future PR. Issue link coming soon There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. After discussing this offline, this is something we should address in a subsequent release.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Issue: #15955 |
||
| ValidationUtil.validateFilePath(getClass().getSimpleName(), certificatePath, "Pfx Certificate Path"); | ||
| this.clientCertificate = certificatePath; | ||
| this.clientCertificatePath = certificatePath; | ||
| this.clientCertificatePassword = clientCertificatePassword; | ||
| return this; | ||
| } | ||
|
|
||
| /** | ||
| * Sets the input stream holding the PFX certificate and its password for authenticating to AAD. | ||
| * | ||
| * @param certificate the input stream containing the password protected PFX certificate | ||
| * @param clientCertificatePassword the password protecting the PFX file | ||
| * @return An updated instance of this builder. | ||
| */ | ||
| public ClientCertificateCredentialBuilder pfxCertificate(InputStream certificate, | ||
| String clientCertificatePassword) { | ||
| this.clientCertificate = certificate; | ||
| this.clientCertificatePassword = clientCertificatePassword; | ||
| return this; | ||
| } | ||
|
|
@@ -86,9 +116,14 @@ public ClientCertificateCredential build() { | |
| ValidationUtil.validate(getClass().getSimpleName(), new HashMap<String, Object>() {{ | ||
| put("clientId", clientId); | ||
| put("tenantId", tenantId); | ||
| put("clientCertificate", clientCertificate); | ||
| put("clientCertificate", clientCertificate == null ? clientCertificatePath : clientCertificate); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The error message from Validation Util, won't be clear for user. We should also throw an exception, if both path and inputstream are configured.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changing the error message will break the current certificate path scenario - people may be depending on the error message.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. okay, then we should atleast add an additive check for the scenario and throw an exception if both path and inputstream are configured. |
||
| }}); | ||
| return new ClientCertificateCredential(tenantId, clientId, clientCertificate, clientCertificatePassword, | ||
| identityClientOptions); | ||
| 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.")); | ||
| } | ||
|
Comment on lines
+121
to
+125
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we are doing this, then maybe the |
||
| return new ClientCertificateCredential(tenantId, clientId, clientCertificatePath, clientCertificate, | ||
| clientCertificatePassword, identityClientOptions); | ||
| } | ||
| } | ||
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.
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.