You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by GitBox <gi...@apache.org> on 2021/02/25 16:24:28 UTC
[GitHub] [nifi] exceptionfactory commented on a change in pull request #4843: NIFI-8258: Add support for Service Principal authentication in ADLS p…
exceptionfactory commented on a change in pull request #4843:
URL: https://github.com/apache/nifi/pull/4843#discussion_r582955753
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/AbstractAzureDataLakeStorageProcessor.java
##########
@@ -140,17 +150,39 @@ public static DataLakeServiceClient getStorageClient(PropertyContext context, Fl
storageClient = new DataLakeServiceClientBuilder().endpoint(endpoint).sasToken(sasToken)
.buildClient();
} else if (accessToken != null) {
- TokenCredential credential = tokenRequestContext -> Mono.just(accessToken);
+ final TokenCredential credential = tokenRequestContext -> Mono.just(accessToken);
storageClient = new DataLakeServiceClientBuilder().endpoint(endpoint).credential(credential)
- .buildClient();
- } else if(useManagedIdentity){
- final ManagedIdentityCredential misCrendential = new ManagedIdentityCredentialBuilder()
- .build();
- storageClient = new DataLakeServiceClientBuilder()
- .endpoint(endpoint)
- .credential(misCrendential)
- .buildClient();
+ .buildClient();
+ } else if (useManagedIdentity) {
+ final ManagedIdentityCredential misCredential = new ManagedIdentityCredentialBuilder()
+ .build();
+ storageClient = new DataLakeServiceClientBuilder()
+ .endpoint(endpoint)
+ .credential(misCredential)
+ .buildClient();
+ } else if (servicePrincipalTenantId != null && servicePrincipalClientId != null && servicePrincipalClientSecret != null) {
+ final ClientSecretCredential credential = new ClientSecretCredentialBuilder()
+ .tenantId(servicePrincipalTenantId)
+ .clientId(servicePrincipalClientId)
+ .clientSecret(servicePrincipalClientSecret)
+ .build();
+
+ storageClient = new DataLakeServiceClientBuilder()
+ .endpoint(endpoint)
+ .credential(credential)
+ .buildClient();
+ } else if (servicePrincipalTenantId != null && servicePrincipalClientId != null && servicePrincipalClientCertificatePath != null && servicePrincipalClientCertificatePassword != null) {
Review comment:
Using `StringUtils.isNoneBlank()` would make this a bit more concise:
```suggestion
} else if (StringUtils.isNoneBlank(servicePrincipalTenantId, servicePrincipalClientId, servicePrincipalClientCertificatePath, servicePrincipalClientCertificatePassword)) {
```
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/services/azure/storage/ADLSCredentialsControllerService.java
##########
@@ -74,12 +75,56 @@
.addValidator(StandardValidators.BOOLEAN_VALIDATOR)
.build();
+ public static final PropertyDescriptor SERVICE_PRINCIPAL_TENANT_ID = new PropertyDescriptor.Builder()
+ .name("service-principal-tenant-id")
+ .displayName("Service Principal Tenant ID")
+ .description("Tenant ID of the Azure Active Directory hosting the Service Principal. The property is required when Service Principal authentication is used.")
+ .sensitive(true)
+ .required(false)
+ .addValidator(StandardValidators.NON_BLANK_VALIDATOR)
+ .expressionLanguageSupported(ExpressionLanguageScope.FLOWFILE_ATTRIBUTES)
+ .build();
+
+ public static final PropertyDescriptor SERVICE_PRINCIPAL_CLIENT_ID = new PropertyDescriptor.Builder()
+ .name("service-principal-client-id")
+ .displayName("Service Principal Client ID")
+ .description("Client ID (or Application ID) of the Client/Application having the Service Principal. The property is required when Service Principal authentication is used. " +
+ "Also 'Service Principal Client Secret' or 'Service Principal Client Certificate' must be specified in this case.")
+ .sensitive(true)
+ .required(false)
+ .addValidator(StandardValidators.NON_BLANK_VALIDATOR)
+ .expressionLanguageSupported(ExpressionLanguageScope.FLOWFILE_ATTRIBUTES)
+ .build();
+
+ public static final PropertyDescriptor SERVICE_PRINCIPAL_CLIENT_SECRET = new PropertyDescriptor.Builder()
+ .name("service-principal-client-Secret")
+ .displayName("Service Principal Client Secret")
+ .description("Password of the Client/Application.")
+ .sensitive(true)
+ .required(false)
+ .addValidator(StandardValidators.NON_BLANK_VALIDATOR)
+ .expressionLanguageSupported(ExpressionLanguageScope.FLOWFILE_ATTRIBUTES)
+ .build();
+
+ public static final PropertyDescriptor SERVICE_PRINCIPAL_CLIENT_CERTIFICATE = new PropertyDescriptor.Builder()
+ .name("service-principal-client-certificate")
+ .displayName("Service Principal Client Certificate")
+ .description("SSL Context Service referencing the keystore with the client certificate of the Client/Application. Only PKCS12 (.pfx) keystore type is supported. " +
+ "The keystore must contain a single key and the password of the keystore and the key must be the same.")
+ .identifiesControllerService(SSLContextService.class)
Review comment:
The property name is somewhat confusing in light of this property referencing the `SSLContextService`. Given that SSLContextService is not really used to provide an `SSLContext` object, what about changing this property to be just the file path? That would also avoid the need for introducing the additional dependency on `nifi-ssl-context-service-api`. In that case, the File Path Validator could be used to ensure that the file exists.
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/services/azure/storage/ADLSCredentialsControllerService.java
##########
@@ -74,12 +75,56 @@
.addValidator(StandardValidators.BOOLEAN_VALIDATOR)
.build();
+ public static final PropertyDescriptor SERVICE_PRINCIPAL_TENANT_ID = new PropertyDescriptor.Builder()
+ .name("service-principal-tenant-id")
+ .displayName("Service Principal Tenant ID")
+ .description("Tenant ID of the Azure Active Directory hosting the Service Principal. The property is required when Service Principal authentication is used.")
+ .sensitive(true)
+ .required(false)
+ .addValidator(StandardValidators.NON_BLANK_VALIDATOR)
+ .expressionLanguageSupported(ExpressionLanguageScope.FLOWFILE_ATTRIBUTES)
Review comment:
If this property is considered sensitive, should it support expression language? Retrieving a sensitive property from flow file attributes would expose the value in provenance events. This same question applies to the other properties marked as sensitive.
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/services/azure/storage/ADLSCredentialsControllerService.java
##########
@@ -74,12 +75,56 @@
.addValidator(StandardValidators.BOOLEAN_VALIDATOR)
.build();
+ public static final PropertyDescriptor SERVICE_PRINCIPAL_TENANT_ID = new PropertyDescriptor.Builder()
+ .name("service-principal-tenant-id")
+ .displayName("Service Principal Tenant ID")
+ .description("Tenant ID of the Azure Active Directory hosting the Service Principal. The property is required when Service Principal authentication is used.")
+ .sensitive(true)
+ .required(false)
+ .addValidator(StandardValidators.NON_BLANK_VALIDATOR)
+ .expressionLanguageSupported(ExpressionLanguageScope.FLOWFILE_ATTRIBUTES)
+ .build();
+
+ public static final PropertyDescriptor SERVICE_PRINCIPAL_CLIENT_ID = new PropertyDescriptor.Builder()
+ .name("service-principal-client-id")
+ .displayName("Service Principal Client ID")
+ .description("Client ID (or Application ID) of the Client/Application having the Service Principal. The property is required when Service Principal authentication is used. " +
+ "Also 'Service Principal Client Secret' or 'Service Principal Client Certificate' must be specified in this case.")
+ .sensitive(true)
+ .required(false)
+ .addValidator(StandardValidators.NON_BLANK_VALIDATOR)
+ .expressionLanguageSupported(ExpressionLanguageScope.FLOWFILE_ATTRIBUTES)
+ .build();
+
+ public static final PropertyDescriptor SERVICE_PRINCIPAL_CLIENT_SECRET = new PropertyDescriptor.Builder()
+ .name("service-principal-client-Secret")
+ .displayName("Service Principal Client Secret")
+ .description("Password of the Client/Application.")
+ .sensitive(true)
+ .required(false)
+ .addValidator(StandardValidators.NON_BLANK_VALIDATOR)
+ .expressionLanguageSupported(ExpressionLanguageScope.FLOWFILE_ATTRIBUTES)
Review comment:
Based on the approach of some other components, it seems best to avoid supporting expression language for password properties. This can still be parameterized using parameter contexts, which preserves the ability to use sensitive property encryption, and also supports passwords that may look like expression language strings.
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/services/azure/storage/ADLSCredentialsControllerService.java
##########
@@ -74,12 +75,56 @@
.addValidator(StandardValidators.BOOLEAN_VALIDATOR)
.build();
+ public static final PropertyDescriptor SERVICE_PRINCIPAL_TENANT_ID = new PropertyDescriptor.Builder()
+ .name("service-principal-tenant-id")
+ .displayName("Service Principal Tenant ID")
+ .description("Tenant ID of the Azure Active Directory hosting the Service Principal. The property is required when Service Principal authentication is used.")
+ .sensitive(true)
+ .required(false)
+ .addValidator(StandardValidators.NON_BLANK_VALIDATOR)
+ .expressionLanguageSupported(ExpressionLanguageScope.FLOWFILE_ATTRIBUTES)
+ .build();
+
+ public static final PropertyDescriptor SERVICE_PRINCIPAL_CLIENT_ID = new PropertyDescriptor.Builder()
+ .name("service-principal-client-id")
+ .displayName("Service Principal Client ID")
+ .description("Client ID (or Application ID) of the Client/Application having the Service Principal. The property is required when Service Principal authentication is used. " +
+ "Also 'Service Principal Client Secret' or 'Service Principal Client Certificate' must be specified in this case.")
+ .sensitive(true)
+ .required(false)
+ .addValidator(StandardValidators.NON_BLANK_VALIDATOR)
+ .expressionLanguageSupported(ExpressionLanguageScope.FLOWFILE_ATTRIBUTES)
+ .build();
+
+ public static final PropertyDescriptor SERVICE_PRINCIPAL_CLIENT_SECRET = new PropertyDescriptor.Builder()
+ .name("service-principal-client-Secret")
+ .displayName("Service Principal Client Secret")
+ .description("Password of the Client/Application.")
+ .sensitive(true)
+ .required(false)
+ .addValidator(StandardValidators.NON_BLANK_VALIDATOR)
+ .expressionLanguageSupported(ExpressionLanguageScope.FLOWFILE_ATTRIBUTES)
+ .build();
+
+ public static final PropertyDescriptor SERVICE_PRINCIPAL_CLIENT_CERTIFICATE = new PropertyDescriptor.Builder()
+ .name("service-principal-client-certificate")
+ .displayName("Service Principal Client Certificate")
+ .description("SSL Context Service referencing the keystore with the client certificate of the Client/Application. Only PKCS12 (.pfx) keystore type is supported. " +
Review comment:
In light of the description and the Azure SDK allowing only PKCS12 certificates, it would be helpful to add a check in the `customValidate` method to ensure that the file provided is actually a PKCS12. Leveraging the Azure `ClientCertificateCredentialBuilder` to load the certificate with the password provided would be a good way to ensure that the configured properties meet the requirements described.
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/services/azure/storage/ADLSCredentialsControllerService.java
##########
@@ -97,16 +142,38 @@
boolean sasTokenSet = StringUtils.isNotBlank(validationContext.getProperty(AzureStorageUtils.PROP_SAS_TOKEN).getValue());
boolean useManagedIdentitySet = validationContext.getProperty(USE_MANAGED_IDENTITY).asBoolean();
- if (!onlyOneSet(accountKeySet, sasTokenSet, useManagedIdentitySet)) {
- StringJoiner options = new StringJoiner(", ")
- .add(AzureStorageUtils.ACCOUNT_KEY.getDisplayName())
- .add(AzureStorageUtils.PROP_SAS_TOKEN.getDisplayName())
- .add(USE_MANAGED_IDENTITY.getDisplayName());
+ boolean servicePrincipalTenantIdSet = StringUtils.isNotBlank(validationContext.getProperty(SERVICE_PRINCIPAL_TENANT_ID).getValue());
+ boolean servicePrincipalClientIdSet = StringUtils.isNotBlank(validationContext.getProperty(SERVICE_PRINCIPAL_CLIENT_ID).getValue());
+ boolean servicePrincipalClientSecretSet = StringUtils.isNotBlank(validationContext.getProperty(SERVICE_PRINCIPAL_CLIENT_SECRET).getValue());
+ boolean servicePrincipalClientCertificateSet = validationContext.getProperty(SERVICE_PRINCIPAL_CLIENT_CERTIFICATE).isSet();
+
+ boolean servicePrincipalSet = servicePrincipalTenantIdSet || servicePrincipalClientIdSet || servicePrincipalClientSecretSet || servicePrincipalClientCertificateSet;
Review comment:
Instead of using the presence of any of these properties to imply Service Principal Authentication, what about introducing one more property named something like `Authentication Type`? The value of that property could take one of an enumerated list of values. With that property in place, the remaining Service Principal properties could use the `dependsOn` feature of Property Descriptors. This would provide a cleaner user experience and should also make the determination of authentication type easier to follow.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org