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 2022/11/04 14:21:48 UTC
[GitHub] [nifi] nandorsoma opened a new pull request, #6619: NIFI-10760 Add Api key authentication option to ElasticSearchClientServiceImpl
nandorsoma opened a new pull request, #6619:
URL: https://github.com/apache/nifi/pull/6619
<!-- Licensed to the Apache Software Foundation (ASF) under one or more -->
<!-- contributor license agreements. See the NOTICE file distributed with -->
<!-- this work for additional information regarding copyright ownership. -->
<!-- The ASF licenses this file to You under the Apache License, Version 2.0 -->
<!-- (the "License"); you may not use this file except in compliance with -->
<!-- the License. You may obtain a copy of the License at -->
<!-- http://www.apache.org/licenses/LICENSE-2.0 -->
<!-- Unless required by applicable law or agreed to in writing, software -->
<!-- distributed under the License is distributed on an "AS IS" BASIS, -->
<!-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -->
<!-- See the License for the specific language governing permissions and -->
<!-- limitations under the License. -->
# Summary
[NIFI-10760](https://issues.apache.org/jira/browse/NIFI-10760)
# Tracking
Please complete the following tracking steps prior to pull request creation.
### Issue Tracking
- [x] [Apache NiFi Jira](https://issues.apache.org/jira/browse/NIFI) issue created
### Pull Request Tracking
- [x] Pull Request title starts with Apache NiFi Jira issue number, such as `NIFI-00000`
- [x] Pull Request commit message starts with Apache NiFi Jira issue number, as such `NIFI-00000`
### Pull Request Formatting
- [x] Pull Request based on current revision of the `main` branch
- [x] Pull Request refers to a feature branch with one commit containing changes
# Verification
Please indicate the verification steps performed prior to pull request creation.
### Build
- [x] Build completed using `mvn clean install -P contrib-check`
- [x] JDK 8
- [ ] JDK 11
- [ ] JDK 17
### Licensing
- [ ] New dependencies are compatible with the [Apache License 2.0](https://apache.org/licenses/LICENSE-2.0) according to the [License Policy](https://www.apache.org/legal/resolved.html)
- [ ] New dependencies are documented in applicable `LICENSE` and `NOTICE` files
### Documentation
- [ ] Documentation formatting appears as expected in rendered files
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] nandorsoma commented on a diff in pull request #6619: NIFI-10760 Add Api key authentication option to ElasticSearchClientServiceImpl
Posted by GitBox <gi...@apache.org>.
nandorsoma commented on code in PR #6619:
URL: https://github.com/apache/nifi/pull/6619#discussion_r1014953245
##########
nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-client-service-api/src/main/java/org/apache/nifi/elasticsearch/ElasticSearchClientService.java:
##########
@@ -52,23 +54,63 @@ public interface ElasticSearchClientService extends ControllerService {
.addValidator(Validator.VALID)
.build();
PropertyDescriptor PROXY_CONFIGURATION_SERVICE = ProxyConfiguration.createProxyConfigPropertyDescriptor(false, ProxySpec.HTTP);
+
+ AllowableValue ALLOWABLE_VALUE_BASIC_AUTH = new AllowableValue(AuthorizationScheme.BASIC.name(), AuthorizationScheme.BASIC.getDisplayName());
+ AllowableValue ALLOWABLE_VALUE_API_KEY = new AllowableValue(AuthorizationScheme.API_KEY.name(), AuthorizationScheme.API_KEY.getDisplayName());
+
+ PropertyDescriptor AUTHORIZATION_SCHEME = new PropertyDescriptor.Builder()
+ .name("authorization-scheme")
+ .displayName("Authorization Scheme")
+ .description("Authorization Scheme used for the authorization.")
+ .allowableValues(ALLOWABLE_VALUE_BASIC_AUTH, ALLOWABLE_VALUE_API_KEY)
Review Comment:
Good idea! I didn't know about that interface. I'm wondering, wouldn't it make sense to create a matching signature for the `dependsOn` and `defaultValue` methods? If so I'd be happy to create a followup on that.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] nandorsoma commented on a diff in pull request #6619: NIFI-10760 Add Api key authentication option to ElasticSearchClientServiceImpl
Posted by GitBox <gi...@apache.org>.
nandorsoma commented on code in PR #6619:
URL: https://github.com/apache/nifi/pull/6619#discussion_r1014952186
##########
nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-client-service-api/src/main/java/org/apache/nifi/elasticsearch/ElasticSearchClientService.java:
##########
@@ -52,23 +54,63 @@ public interface ElasticSearchClientService extends ControllerService {
.addValidator(Validator.VALID)
.build();
PropertyDescriptor PROXY_CONFIGURATION_SERVICE = ProxyConfiguration.createProxyConfigPropertyDescriptor(false, ProxySpec.HTTP);
+
+ AllowableValue ALLOWABLE_VALUE_BASIC_AUTH = new AllowableValue(AuthorizationScheme.BASIC.name(), AuthorizationScheme.BASIC.getDisplayName());
+ AllowableValue ALLOWABLE_VALUE_API_KEY = new AllowableValue(AuthorizationScheme.API_KEY.name(), AuthorizationScheme.API_KEY.getDisplayName());
+
+ PropertyDescriptor AUTHORIZATION_SCHEME = new PropertyDescriptor.Builder()
+ .name("authorization-scheme")
+ .displayName("Authorization Scheme")
+ .description("Authorization Scheme used for the authorization.")
+ .allowableValues(ALLOWABLE_VALUE_BASIC_AUTH, ALLOWABLE_VALUE_API_KEY)
+ .defaultValue(ALLOWABLE_VALUE_BASIC_AUTH.getValue())
+ .required(true)
+ .addValidator(StandardValidators.NON_EMPTY_VALIDATOR)
+ .build();
+
PropertyDescriptor USERNAME = new PropertyDescriptor.Builder()
.name("el-cs-username")
.displayName("Username")
.description("The username to use with XPack security.")
+ .dependsOn(AUTHORIZATION_SCHEME, ALLOWABLE_VALUE_BASIC_AUTH)
.required(false)
.expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
.addValidator(StandardValidators.NON_EMPTY_VALIDATOR)
.build();
+
PropertyDescriptor PASSWORD = new PropertyDescriptor.Builder()
.name("el-cs-password")
.displayName("Password")
.description("The password to use with XPack security.")
+ .dependsOn(AUTHORIZATION_SCHEME, ALLOWABLE_VALUE_BASIC_AUTH)
.required(false)
.sensitive(true)
.expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
.addValidator(StandardValidators.NON_EMPTY_VALIDATOR)
.build();
+
+ PropertyDescriptor API_KEY_ID = new PropertyDescriptor.Builder()
+ .name("api-key-id")
+ .displayName("API key id")
+ .description("Unique id of the API key")
+ .dependsOn(AUTHORIZATION_SCHEME, ALLOWABLE_VALUE_API_KEY)
+ .required(false)
+ .sensitive(false)
+ .expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
+ .addValidator(StandardValidators.NON_EMPTY_VALIDATOR)
+ .build();
+
+ PropertyDescriptor API_KEY = new PropertyDescriptor.Builder()
+ .name("api-key")
+ .displayName("API key")
+ .description("API key")
+ .dependsOn(AUTHORIZATION_SCHEME, ALLOWABLE_VALUE_API_KEY)
+ .required(false)
+ .sensitive(true)
+ .expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
Review Comment:
I added that because username/password supports that, and I thought it's better not to make a difference to avoid confusion. But I accept!
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] nandorsoma commented on pull request #6619: NIFI-10760 Add Api key authentication option to ElasticSearchClientServiceImpl
Posted by GitBox <gi...@apache.org>.
nandorsoma commented on PR #6619:
URL: https://github.com/apache/nifi/pull/6619#issuecomment-1307066467
Hey @exceptionfactory, @ChrisSamo632!
Thank you for the reviews.
Follow up Jiras (I didn't add NONE as a separate one, but mentioned it in the PKI one):
[NIFI-10775](https://issues.apache.org/jira/browse/NIFI-10775) - Improve support for DescribedValue in PropertyDescriptor.Builder
[NIFI-10776](https://issues.apache.org/jira/browse/NIFI-10776) - Add support for PKI Authentication in ElasticSearchClientServiceImpl
[NIFI-10777](https://issues.apache.org/jira/browse/NIFI-10777) - Add support for JWT Authentication in ElasticSeachClientServiceImpl
[NIFI-10778](https://issues.apache.org/jira/browse/NIFI-10778) - Add support for Kerberos Authentication in ElasticSearchClientServiceImpl
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] nandorsoma commented on a diff in pull request #6619: NIFI-10760 Add Api key authentication option to ElasticSearchClientServiceImpl
Posted by GitBox <gi...@apache.org>.
nandorsoma commented on code in PR #6619:
URL: https://github.com/apache/nifi/pull/6619#discussion_r1014948735
##########
nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-client-service-api/src/main/java/org/apache/nifi/elasticsearch/AuthorizationScheme.java:
##########
@@ -0,0 +1,32 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.nifi.elasticsearch;
+
+public enum AuthorizationScheme {
+ BASIC("Basic"),
Review Comment:
Since the API uses the Authorization header from the HTTP protocol, I thought it would make sense to follow that naming convention. I have changed the property descriptions, though. Could you check whether it is good now?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] nandorsoma commented on pull request #6619: NIFI-10760 Add Api key authentication option to ElasticSearchClientServiceImpl
Posted by GitBox <gi...@apache.org>.
nandorsoma commented on PR #6619:
URL: https://github.com/apache/nifi/pull/6619#issuecomment-1305000287
Thank you for the reviews @exceptionfactory and @ChrisSamo632!
I tried to address your comments, please check my followup commits and comments.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on a diff in pull request #6619: NIFI-10760 Add Api key authentication option to ElasticSearchClientServiceImpl
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on code in PR #6619:
URL: https://github.com/apache/nifi/pull/6619#discussion_r1014086673
##########
nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-client-service-api/src/main/java/org/apache/nifi/elasticsearch/ElasticSearchClientService.java:
##########
@@ -52,23 +54,63 @@ public interface ElasticSearchClientService extends ControllerService {
.addValidator(Validator.VALID)
.build();
PropertyDescriptor PROXY_CONFIGURATION_SERVICE = ProxyConfiguration.createProxyConfigPropertyDescriptor(false, ProxySpec.HTTP);
+
+ AllowableValue ALLOWABLE_VALUE_BASIC_AUTH = new AllowableValue(AuthorizationScheme.BASIC.name(), AuthorizationScheme.BASIC.getDisplayName());
+ AllowableValue ALLOWABLE_VALUE_API_KEY = new AllowableValue(AuthorizationScheme.API_KEY.name(), AuthorizationScheme.API_KEY.getDisplayName());
+
+ PropertyDescriptor AUTHORIZATION_SCHEME = new PropertyDescriptor.Builder()
+ .name("authorization-scheme")
+ .displayName("Authorization Scheme")
+ .description("Authorization Scheme used for the authorization.")
+ .allowableValues(ALLOWABLE_VALUE_BASIC_AUTH, ALLOWABLE_VALUE_API_KEY)
+ .defaultValue(ALLOWABLE_VALUE_BASIC_AUTH.getValue())
+ .required(true)
+ .addValidator(StandardValidators.NON_EMPTY_VALIDATOR)
+ .build();
+
PropertyDescriptor USERNAME = new PropertyDescriptor.Builder()
.name("el-cs-username")
.displayName("Username")
.description("The username to use with XPack security.")
+ .dependsOn(AUTHORIZATION_SCHEME, ALLOWABLE_VALUE_BASIC_AUTH)
.required(false)
.expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
.addValidator(StandardValidators.NON_EMPTY_VALIDATOR)
.build();
+
PropertyDescriptor PASSWORD = new PropertyDescriptor.Builder()
.name("el-cs-password")
.displayName("Password")
.description("The password to use with XPack security.")
+ .dependsOn(AUTHORIZATION_SCHEME, ALLOWABLE_VALUE_BASIC_AUTH)
.required(false)
.sensitive(true)
.expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
.addValidator(StandardValidators.NON_EMPTY_VALIDATOR)
.build();
+
+ PropertyDescriptor API_KEY_ID = new PropertyDescriptor.Builder()
+ .name("api-key-id")
+ .displayName("API key id")
Review Comment:
Property Display Names should capitalize all words:
```suggestion
.displayName("API Key ID")
```
##########
nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-client-service-api/src/main/java/org/apache/nifi/elasticsearch/ElasticSearchClientService.java:
##########
@@ -52,23 +54,63 @@ public interface ElasticSearchClientService extends ControllerService {
.addValidator(Validator.VALID)
.build();
PropertyDescriptor PROXY_CONFIGURATION_SERVICE = ProxyConfiguration.createProxyConfigPropertyDescriptor(false, ProxySpec.HTTP);
+
+ AllowableValue ALLOWABLE_VALUE_BASIC_AUTH = new AllowableValue(AuthorizationScheme.BASIC.name(), AuthorizationScheme.BASIC.getDisplayName());
+ AllowableValue ALLOWABLE_VALUE_API_KEY = new AllowableValue(AuthorizationScheme.API_KEY.name(), AuthorizationScheme.API_KEY.getDisplayName());
+
+ PropertyDescriptor AUTHORIZATION_SCHEME = new PropertyDescriptor.Builder()
+ .name("authorization-scheme")
+ .displayName("Authorization Scheme")
+ .description("Authorization Scheme used for the authorization.")
+ .allowableValues(ALLOWABLE_VALUE_BASIC_AUTH, ALLOWABLE_VALUE_API_KEY)
+ .defaultValue(ALLOWABLE_VALUE_BASIC_AUTH.getValue())
+ .required(true)
+ .addValidator(StandardValidators.NON_EMPTY_VALIDATOR)
+ .build();
+
PropertyDescriptor USERNAME = new PropertyDescriptor.Builder()
.name("el-cs-username")
.displayName("Username")
.description("The username to use with XPack security.")
+ .dependsOn(AUTHORIZATION_SCHEME, ALLOWABLE_VALUE_BASIC_AUTH)
.required(false)
.expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
.addValidator(StandardValidators.NON_EMPTY_VALIDATOR)
.build();
+
PropertyDescriptor PASSWORD = new PropertyDescriptor.Builder()
.name("el-cs-password")
.displayName("Password")
.description("The password to use with XPack security.")
+ .dependsOn(AUTHORIZATION_SCHEME, ALLOWABLE_VALUE_BASIC_AUTH)
.required(false)
.sensitive(true)
.expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
.addValidator(StandardValidators.NON_EMPTY_VALIDATOR)
.build();
+
+ PropertyDescriptor API_KEY_ID = new PropertyDescriptor.Builder()
+ .name("api-key-id")
+ .displayName("API key id")
+ .description("Unique id of the API key")
Review Comment:
```suggestion
.description("Unique identifier of the API key")
```
##########
nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-client-service-api/src/main/java/org/apache/nifi/elasticsearch/ElasticSearchClientService.java:
##########
@@ -52,23 +54,63 @@ public interface ElasticSearchClientService extends ControllerService {
.addValidator(Validator.VALID)
.build();
PropertyDescriptor PROXY_CONFIGURATION_SERVICE = ProxyConfiguration.createProxyConfigPropertyDescriptor(false, ProxySpec.HTTP);
+
+ AllowableValue ALLOWABLE_VALUE_BASIC_AUTH = new AllowableValue(AuthorizationScheme.BASIC.name(), AuthorizationScheme.BASIC.getDisplayName());
+ AllowableValue ALLOWABLE_VALUE_API_KEY = new AllowableValue(AuthorizationScheme.API_KEY.name(), AuthorizationScheme.API_KEY.getDisplayName());
+
+ PropertyDescriptor AUTHORIZATION_SCHEME = new PropertyDescriptor.Builder()
+ .name("authorization-scheme")
+ .displayName("Authorization Scheme")
+ .description("Authorization Scheme used for the authorization.")
+ .allowableValues(ALLOWABLE_VALUE_BASIC_AUTH, ALLOWABLE_VALUE_API_KEY)
+ .defaultValue(ALLOWABLE_VALUE_BASIC_AUTH.getValue())
+ .required(true)
+ .addValidator(StandardValidators.NON_EMPTY_VALIDATOR)
+ .build();
+
PropertyDescriptor USERNAME = new PropertyDescriptor.Builder()
.name("el-cs-username")
.displayName("Username")
.description("The username to use with XPack security.")
+ .dependsOn(AUTHORIZATION_SCHEME, ALLOWABLE_VALUE_BASIC_AUTH)
.required(false)
.expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
.addValidator(StandardValidators.NON_EMPTY_VALIDATOR)
.build();
+
PropertyDescriptor PASSWORD = new PropertyDescriptor.Builder()
.name("el-cs-password")
.displayName("Password")
.description("The password to use with XPack security.")
+ .dependsOn(AUTHORIZATION_SCHEME, ALLOWABLE_VALUE_BASIC_AUTH)
.required(false)
.sensitive(true)
.expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
.addValidator(StandardValidators.NON_EMPTY_VALIDATOR)
.build();
+
+ PropertyDescriptor API_KEY_ID = new PropertyDescriptor.Builder()
+ .name("api-key-id")
+ .displayName("API key id")
+ .description("Unique id of the API key")
+ .dependsOn(AUTHORIZATION_SCHEME, ALLOWABLE_VALUE_API_KEY)
+ .required(false)
+ .sensitive(false)
+ .expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
+ .addValidator(StandardValidators.NON_EMPTY_VALIDATOR)
+ .build();
+
+ PropertyDescriptor API_KEY = new PropertyDescriptor.Builder()
+ .name("api-key")
+ .displayName("API key")
Review Comment:
```suggestion
.displayName("API Key")
```
##########
nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-client-service-api/src/main/java/org/apache/nifi/elasticsearch/ElasticSearchClientService.java:
##########
@@ -52,23 +54,63 @@ public interface ElasticSearchClientService extends ControllerService {
.addValidator(Validator.VALID)
.build();
PropertyDescriptor PROXY_CONFIGURATION_SERVICE = ProxyConfiguration.createProxyConfigPropertyDescriptor(false, ProxySpec.HTTP);
+
+ AllowableValue ALLOWABLE_VALUE_BASIC_AUTH = new AllowableValue(AuthorizationScheme.BASIC.name(), AuthorizationScheme.BASIC.getDisplayName());
+ AllowableValue ALLOWABLE_VALUE_API_KEY = new AllowableValue(AuthorizationScheme.API_KEY.name(), AuthorizationScheme.API_KEY.getDisplayName());
Review Comment:
The `AllowableValue` definitions are not necessary, the `AuthorizationScheme` enum can be used directly.
##########
nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-client-service/src/main/java/org/apache/nifi/elasticsearch/ElasticSearchClientServiceImpl.java:
##########
@@ -97,6 +105,31 @@ protected List<PropertyDescriptor> getSupportedPropertyDescriptors() {
return properties;
}
+ @Override
+ protected Collection<ValidationResult> customValidate(ValidationContext validationContext) {
+ final List<ValidationResult> results = new ArrayList<>(1);
+
+ final boolean usernameSet = validationContext.getProperty(USERNAME).isSet();
+ final boolean passwordSet = validationContext.getProperty(PASSWORD).isSet();
+
+ if ((usernameSet && !passwordSet) || (!usernameSet && passwordSet)) {
+ results.add(new ValidationResult.Builder().subject("Username and Password").valid(false).explanation("if Username or Password is set, both must be set.").build());
+ }
+
+ final boolean apiKeyIdSet = validationContext.getProperty(API_KEY_ID).isSet();
+ final boolean apiKeySet = validationContext.getProperty(API_KEY).isSet();
+
+ if ((apiKeyIdSet && !apiKeySet) || (!apiKeyIdSet && apiKeySet)) {
+ results.add(new ValidationResult.Builder().subject("API key and API key id").valid(false).explanation("if 'API key id' or 'API key' is set, both must be set.").build());
Review Comment:
Recommend replacing the property display names with references to the Property Descriptor and using `String.format()` to keep the message synchronized with the Property Display Name.
##########
nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-client-service/src/main/java/org/apache/nifi/elasticsearch/ElasticSearchClientServiceImpl.java:
##########
@@ -161,6 +197,11 @@ private void setupClient(final ConfigurationContext context) throws MalformedURL
credentialsProvider = addCredentials(null, AuthScope.ANY, username, password);
}
+ if (apiKeyId != null && apiKey != null) {
+ final String apiKeyAuth = Base64.getEncoder().encodeToString((apiKeyId + ":" + apiKey).getBytes(StandardCharsets.UTF_8));
Review Comment:
Recommend separating the string formatting and encoding to separate lines for better readability.
##########
nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-client-service-api/src/main/java/org/apache/nifi/elasticsearch/ElasticSearchClientService.java:
##########
@@ -52,23 +54,63 @@ public interface ElasticSearchClientService extends ControllerService {
.addValidator(Validator.VALID)
.build();
PropertyDescriptor PROXY_CONFIGURATION_SERVICE = ProxyConfiguration.createProxyConfigPropertyDescriptor(false, ProxySpec.HTTP);
+
+ AllowableValue ALLOWABLE_VALUE_BASIC_AUTH = new AllowableValue(AuthorizationScheme.BASIC.name(), AuthorizationScheme.BASIC.getDisplayName());
+ AllowableValue ALLOWABLE_VALUE_API_KEY = new AllowableValue(AuthorizationScheme.API_KEY.name(), AuthorizationScheme.API_KEY.getDisplayName());
+
+ PropertyDescriptor AUTHORIZATION_SCHEME = new PropertyDescriptor.Builder()
+ .name("authorization-scheme")
+ .displayName("Authorization Scheme")
+ .description("Authorization Scheme used for the authorization.")
+ .allowableValues(ALLOWABLE_VALUE_BASIC_AUTH, ALLOWABLE_VALUE_API_KEY)
+ .defaultValue(ALLOWABLE_VALUE_BASIC_AUTH.getValue())
+ .required(true)
+ .addValidator(StandardValidators.NON_EMPTY_VALIDATOR)
+ .build();
+
PropertyDescriptor USERNAME = new PropertyDescriptor.Builder()
.name("el-cs-username")
.displayName("Username")
.description("The username to use with XPack security.")
+ .dependsOn(AUTHORIZATION_SCHEME, ALLOWABLE_VALUE_BASIC_AUTH)
.required(false)
.expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
.addValidator(StandardValidators.NON_EMPTY_VALIDATOR)
.build();
+
PropertyDescriptor PASSWORD = new PropertyDescriptor.Builder()
.name("el-cs-password")
.displayName("Password")
.description("The password to use with XPack security.")
+ .dependsOn(AUTHORIZATION_SCHEME, ALLOWABLE_VALUE_BASIC_AUTH)
.required(false)
.sensitive(true)
.expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
.addValidator(StandardValidators.NON_EMPTY_VALIDATOR)
.build();
+
+ PropertyDescriptor API_KEY_ID = new PropertyDescriptor.Builder()
+ .name("api-key-id")
+ .displayName("API key id")
+ .description("Unique id of the API key")
+ .dependsOn(AUTHORIZATION_SCHEME, ALLOWABLE_VALUE_API_KEY)
+ .required(false)
+ .sensitive(false)
+ .expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
Review Comment:
Recommend removing expression language support since Parameters are preferred.
##########
nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-client-service/src/main/java/org/apache/nifi/elasticsearch/ElasticSearchClientServiceImpl.java:
##########
@@ -130,6 +163,9 @@ private void setupClient(final ConfigurationContext context) throws MalformedURL
final String username = context.getProperty(USERNAME).evaluateAttributeExpressions().getValue();
final String password = context.getProperty(PASSWORD).evaluateAttributeExpressions().getValue();
+ final String apiKeyId = context.getProperty(API_KEY_ID).evaluateAttributeExpressions().getValue();
+ final String apiKey = context.getProperty(API_KEY).evaluateAttributeExpressions().getValue();
Review Comment:
```suggestion
final String apiKeyId = context.getProperty(API_KEY_ID).getValue();
final String apiKey = context.getProperty(API_KEY).getValue();
```
##########
nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-client-service-api/src/main/java/org/apache/nifi/elasticsearch/ElasticSearchClientService.java:
##########
@@ -52,23 +54,63 @@ public interface ElasticSearchClientService extends ControllerService {
.addValidator(Validator.VALID)
.build();
PropertyDescriptor PROXY_CONFIGURATION_SERVICE = ProxyConfiguration.createProxyConfigPropertyDescriptor(false, ProxySpec.HTTP);
+
+ AllowableValue ALLOWABLE_VALUE_BASIC_AUTH = new AllowableValue(AuthorizationScheme.BASIC.name(), AuthorizationScheme.BASIC.getDisplayName());
+ AllowableValue ALLOWABLE_VALUE_API_KEY = new AllowableValue(AuthorizationScheme.API_KEY.name(), AuthorizationScheme.API_KEY.getDisplayName());
+
+ PropertyDescriptor AUTHORIZATION_SCHEME = new PropertyDescriptor.Builder()
+ .name("authorization-scheme")
+ .displayName("Authorization Scheme")
+ .description("Authorization Scheme used for the authorization.")
+ .allowableValues(ALLOWABLE_VALUE_BASIC_AUTH, ALLOWABLE_VALUE_API_KEY)
+ .defaultValue(ALLOWABLE_VALUE_BASIC_AUTH.getValue())
+ .required(true)
+ .addValidator(StandardValidators.NON_EMPTY_VALIDATOR)
+ .build();
+
PropertyDescriptor USERNAME = new PropertyDescriptor.Builder()
.name("el-cs-username")
.displayName("Username")
.description("The username to use with XPack security.")
+ .dependsOn(AUTHORIZATION_SCHEME, ALLOWABLE_VALUE_BASIC_AUTH)
.required(false)
.expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
.addValidator(StandardValidators.NON_EMPTY_VALIDATOR)
.build();
+
PropertyDescriptor PASSWORD = new PropertyDescriptor.Builder()
.name("el-cs-password")
.displayName("Password")
.description("The password to use with XPack security.")
+ .dependsOn(AUTHORIZATION_SCHEME, ALLOWABLE_VALUE_BASIC_AUTH)
.required(false)
.sensitive(true)
.expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
.addValidator(StandardValidators.NON_EMPTY_VALIDATOR)
.build();
+
+ PropertyDescriptor API_KEY_ID = new PropertyDescriptor.Builder()
+ .name("api-key-id")
+ .displayName("API key id")
+ .description("Unique id of the API key")
+ .dependsOn(AUTHORIZATION_SCHEME, ALLOWABLE_VALUE_API_KEY)
+ .required(false)
+ .sensitive(false)
+ .expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
+ .addValidator(StandardValidators.NON_EMPTY_VALIDATOR)
+ .build();
+
+ PropertyDescriptor API_KEY = new PropertyDescriptor.Builder()
+ .name("api-key")
+ .displayName("API key")
+ .description("API key")
+ .dependsOn(AUTHORIZATION_SCHEME, ALLOWABLE_VALUE_API_KEY)
+ .required(false)
+ .sensitive(true)
+ .expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
Review Comment:
Sensitive values should not support expression language, so this should be removed.
##########
nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-client-service-api/src/main/java/org/apache/nifi/elasticsearch/ElasticSearchClientService.java:
##########
@@ -52,23 +54,63 @@ public interface ElasticSearchClientService extends ControllerService {
.addValidator(Validator.VALID)
.build();
PropertyDescriptor PROXY_CONFIGURATION_SERVICE = ProxyConfiguration.createProxyConfigPropertyDescriptor(false, ProxySpec.HTTP);
+
+ AllowableValue ALLOWABLE_VALUE_BASIC_AUTH = new AllowableValue(AuthorizationScheme.BASIC.name(), AuthorizationScheme.BASIC.getDisplayName());
+ AllowableValue ALLOWABLE_VALUE_API_KEY = new AllowableValue(AuthorizationScheme.API_KEY.name(), AuthorizationScheme.API_KEY.getDisplayName());
+
+ PropertyDescriptor AUTHORIZATION_SCHEME = new PropertyDescriptor.Builder()
+ .name("authorization-scheme")
+ .displayName("Authorization Scheme")
+ .description("Authorization Scheme used for the authorization.")
+ .allowableValues(ALLOWABLE_VALUE_BASIC_AUTH, ALLOWABLE_VALUE_API_KEY)
+ .defaultValue(ALLOWABLE_VALUE_BASIC_AUTH.getValue())
+ .required(true)
+ .addValidator(StandardValidators.NON_EMPTY_VALIDATOR)
+ .build();
+
PropertyDescriptor USERNAME = new PropertyDescriptor.Builder()
.name("el-cs-username")
.displayName("Username")
.description("The username to use with XPack security.")
+ .dependsOn(AUTHORIZATION_SCHEME, ALLOWABLE_VALUE_BASIC_AUTH)
.required(false)
.expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
.addValidator(StandardValidators.NON_EMPTY_VALIDATOR)
.build();
+
PropertyDescriptor PASSWORD = new PropertyDescriptor.Builder()
.name("el-cs-password")
.displayName("Password")
.description("The password to use with XPack security.")
+ .dependsOn(AUTHORIZATION_SCHEME, ALLOWABLE_VALUE_BASIC_AUTH)
.required(false)
.sensitive(true)
.expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
.addValidator(StandardValidators.NON_EMPTY_VALIDATOR)
.build();
+
+ PropertyDescriptor API_KEY_ID = new PropertyDescriptor.Builder()
+ .name("api-key-id")
+ .displayName("API key id")
+ .description("Unique id of the API key")
+ .dependsOn(AUTHORIZATION_SCHEME, ALLOWABLE_VALUE_API_KEY)
+ .required(false)
+ .sensitive(false)
+ .expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
+ .addValidator(StandardValidators.NON_EMPTY_VALIDATOR)
+ .build();
+
+ PropertyDescriptor API_KEY = new PropertyDescriptor.Builder()
+ .name("api-key")
+ .displayName("API key")
+ .description("API key")
Review Comment:
```suggestion
.description("Encoded API key")
```
##########
nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-client-service-api/src/main/java/org/apache/nifi/elasticsearch/ElasticSearchClientService.java:
##########
@@ -52,23 +54,63 @@ public interface ElasticSearchClientService extends ControllerService {
.addValidator(Validator.VALID)
.build();
PropertyDescriptor PROXY_CONFIGURATION_SERVICE = ProxyConfiguration.createProxyConfigPropertyDescriptor(false, ProxySpec.HTTP);
+
+ AllowableValue ALLOWABLE_VALUE_BASIC_AUTH = new AllowableValue(AuthorizationScheme.BASIC.name(), AuthorizationScheme.BASIC.getDisplayName());
+ AllowableValue ALLOWABLE_VALUE_API_KEY = new AllowableValue(AuthorizationScheme.API_KEY.name(), AuthorizationScheme.API_KEY.getDisplayName());
+
+ PropertyDescriptor AUTHORIZATION_SCHEME = new PropertyDescriptor.Builder()
+ .name("authorization-scheme")
+ .displayName("Authorization Scheme")
+ .description("Authorization Scheme used for the authorization.")
Review Comment:
```suggestion
.description("Authorization Scheme used for authenticating to Elasticsearch using the HTTP Authorization header")
```
##########
nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-client-service-api/src/main/java/org/apache/nifi/elasticsearch/ElasticSearchClientService.java:
##########
@@ -52,23 +54,63 @@ public interface ElasticSearchClientService extends ControllerService {
.addValidator(Validator.VALID)
.build();
PropertyDescriptor PROXY_CONFIGURATION_SERVICE = ProxyConfiguration.createProxyConfigPropertyDescriptor(false, ProxySpec.HTTP);
+
+ AllowableValue ALLOWABLE_VALUE_BASIC_AUTH = new AllowableValue(AuthorizationScheme.BASIC.name(), AuthorizationScheme.BASIC.getDisplayName());
+ AllowableValue ALLOWABLE_VALUE_API_KEY = new AllowableValue(AuthorizationScheme.API_KEY.name(), AuthorizationScheme.API_KEY.getDisplayName());
+
+ PropertyDescriptor AUTHORIZATION_SCHEME = new PropertyDescriptor.Builder()
+ .name("authorization-scheme")
+ .displayName("Authorization Scheme")
+ .description("Authorization Scheme used for the authorization.")
+ .allowableValues(ALLOWABLE_VALUE_BASIC_AUTH, ALLOWABLE_VALUE_API_KEY)
Review Comment:
```suggestion
.allowableValues(AuthorizationScheme.class)
```
##########
nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-client-service-api/src/main/java/org/apache/nifi/elasticsearch/AuthorizationScheme.java:
##########
@@ -0,0 +1,32 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.nifi.elasticsearch;
+
+public enum AuthorizationScheme {
Review Comment:
This enum should implement `DescribedValue` from `nifi-api` and include a description.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] ChrisSamo632 commented on a diff in pull request #6619: NIFI-10760 Add Api key authentication option to ElasticSearchClientServiceImpl
Posted by GitBox <gi...@apache.org>.
ChrisSamo632 commented on code in PR #6619:
URL: https://github.com/apache/nifi/pull/6619#discussion_r1014662812
##########
nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-client-service-api/src/main/java/org/apache/nifi/elasticsearch/AuthorizationScheme.java:
##########
@@ -0,0 +1,32 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.nifi.elasticsearch;
+
+public enum AuthorizationScheme {
+ BASIC("Basic"),
+ API_KEY("API key");
+
Review Comment:
Should `jwt` and `kerberos` [Elasticsearch realms](https://www.elastic.co/guide/en/elasticsearch/reference/current/realms.html) also be supported?
Happy for this to be raised as new follow-on jira ticket(s) for the future if too much to add now - might need some further consideration as these connection types would likely need to use regularly changing tokens rather than more static credentials set on the controller, so wee might need a way of getting that from the Processor or some other sort of token provider controller if/when they exist in future
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] nandorsoma commented on pull request #6619: NIFI-10760 Add Api key authentication option to ElasticSearchClientServiceImpl
Posted by GitBox <gi...@apache.org>.
nandorsoma commented on PR #6619:
URL: https://github.com/apache/nifi/pull/6619#issuecomment-1304913954
need to force push because of the conflict
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on a diff in pull request #6619: NIFI-10760 Add Api key authentication option to ElasticSearchClientServiceImpl
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on code in PR #6619:
URL: https://github.com/apache/nifi/pull/6619#discussion_r1015876300
##########
nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-client-service-api/src/main/java/org/apache/nifi/elasticsearch/AuthorizationScheme.java:
##########
@@ -0,0 +1,32 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.nifi.elasticsearch;
+
+public enum AuthorizationScheme {
+ BASIC("Basic"),
+ API_KEY("API key");
+
Review Comment:
I agree that a separate issue sounds good, as it would provide more opportunity to outline the implementation approach.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] nandorsoma commented on a diff in pull request #6619: NIFI-10760 Add Api key authentication option to ElasticSearchClientServiceImpl
Posted by GitBox <gi...@apache.org>.
nandorsoma commented on code in PR #6619:
URL: https://github.com/apache/nifi/pull/6619#discussion_r1014947128
##########
nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-client-service-api/src/main/java/org/apache/nifi/elasticsearch/AuthorizationScheme.java:
##########
@@ -0,0 +1,32 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.nifi.elasticsearch;
+
+public enum AuthorizationScheme {
+ BASIC("Basic"),
+ API_KEY("API key");
+
Review Comment:
It would make sense, but as you noted probably in a separate pr.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] ChrisSamo632 commented on a diff in pull request #6619: NIFI-10760 Add Api key authentication option to ElasticSearchClientServiceImpl
Posted by GitBox <gi...@apache.org>.
ChrisSamo632 commented on code in PR #6619:
URL: https://github.com/apache/nifi/pull/6619#discussion_r1014661291
##########
nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-client-service-api/src/main/java/org/apache/nifi/elasticsearch/AuthorizationScheme.java:
##########
@@ -0,0 +1,32 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.nifi.elasticsearch;
+
+public enum AuthorizationScheme {
+ BASIC("Basic"),
+ API_KEY("API key");
Review Comment:
Should there be an entry for connections using 2-way TLS (`pki` Realm in Elasticsearch)?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] wannadream commented on a diff in pull request #6619: NIFI-10760 Add Api key authentication option to ElasticSearchClientServiceImpl
Posted by "wannadream (via GitHub)" <gi...@apache.org>.
wannadream commented on code in PR #6619:
URL: https://github.com/apache/nifi/pull/6619#discussion_r1270058336
##########
nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-client-service/src/main/java/org/apache/nifi/elasticsearch/ElasticSearchClientServiceImpl.java:
##########
@@ -275,6 +318,12 @@ private CredentialsProvider addCredentials(final CredentialsProvider credentials
return cp;
}
+ private BasicHeader createApiKeyAuthorizationHeader(String apiKeyId, String apiKey) {
+ final String apiKeyCredentials = String.format("%s:%s", apiKeyId, apiKey);
+ final String apiKeyAuth = Base64.getEncoder().encodeToString((apiKeyCredentials).getBytes(StandardCharsets.UTF_8));
Review Comment:
Thanks, Chris. Feeling enlightened.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] ChrisSamo632 commented on a diff in pull request #6619: NIFI-10760 Add Api key authentication option to ElasticSearchClientServiceImpl
Posted by GitBox <gi...@apache.org>.
ChrisSamo632 commented on code in PR #6619:
URL: https://github.com/apache/nifi/pull/6619#discussion_r1014662218
##########
nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-client-service-api/src/main/java/org/apache/nifi/elasticsearch/AuthorizationScheme.java:
##########
@@ -0,0 +1,32 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.nifi.elasticsearch;
+
+public enum AuthorizationScheme {
+ BASIC("Basic"),
Review Comment:
Is `BASIC` the most appropriate name here do we think, considering there are lots of [Elasticsearch Authentication Realms](https://www.elastic.co/guide/en/elasticsearch/reference/current/realms.html) that use Username & Password?
It's probably descriptive enough, but is it worth linking to the Elasticsearch docs in the controller documentation? Might not be required, just expect nifi users to figure out for themselves when they need to use `BASIC` to set a Username & Password if required by their Elasticsearch setup
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] ChrisSamo632 commented on a diff in pull request #6619: NIFI-10760 Add Api key authentication option to ElasticSearchClientServiceImpl
Posted by GitBox <gi...@apache.org>.
ChrisSamo632 commented on code in PR #6619:
URL: https://github.com/apache/nifi/pull/6619#discussion_r1015850257
##########
nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-client-service-api/src/main/java/org/apache/nifi/elasticsearch/AuthorizationScheme.java:
##########
@@ -0,0 +1,32 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.nifi.elasticsearch;
+
+public enum AuthorizationScheme {
+ BASIC("Basic"),
+ API_KEY("API key");
Review Comment:
I'd probably stick with `BASIC` being the default as that would mean `Username`/`Password` remain available by default when adding the Processor to the canvas, which matches the current behaviour and therefore is backwards compatible.
If the user selects `NONE` then I guess we'd be hiding the existing Username/Password and new `API Key` property inputs (via `dependsOn`).
If the user selected `PKI` then it would be great if the `SSLContext Controller` become mandatory, but I don't think that's currently possible, so in effect this would have the same effect as `NONE` on the available processor properties (we don't want to hide `SSL Context` if something other than `PKI` is selected here, as `https`/`TLS` might very well still be needed even if it's not used for Auth in Elasticsearch).
One option might be to use some `customerValidation` for this though - if the user has selected `PKI`, they **must** also specify an `SSL Context` in their processor configuration. Similarly, if `NONE` is selected, then we could validate that none of the Username/Password/API Key fields are provided?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] ChrisSamo632 commented on a diff in pull request #6619: NIFI-10760 Add Api key authentication option to ElasticSearchClientServiceImpl
Posted by GitBox <gi...@apache.org>.
ChrisSamo632 commented on code in PR #6619:
URL: https://github.com/apache/nifi/pull/6619#discussion_r1015850257
##########
nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-client-service-api/src/main/java/org/apache/nifi/elasticsearch/AuthorizationScheme.java:
##########
@@ -0,0 +1,32 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.nifi.elasticsearch;
+
+public enum AuthorizationScheme {
+ BASIC("Basic"),
+ API_KEY("API key");
Review Comment:
I'd probably stick with `BASIC` being the default as that would mean `Username`/`Password` remain available by default when adding the Processor to the canvas, which matches the current behaviour and therefore is backwards compatible.
If the user selects `NONE` then I guess we'd be hiding the existing Username/Password and new `API Key` property inputs (via `dependsOn`).
If the user selected `PKI` then it would be great if the `SSLContext Controller` become mandatory, but I don't think that's currently possible, so in effect this would have the same effect as `NONE` on the available processor properties (we don't want to hide `SSL Context` if something other than `PKI` is selected here, as `https`/`TLS` might very well still be needed even if it's not used for Auth in Elasticsearch). One option might be to use some `customerValidation` for this though - if the user has selected `PKI`, they **must** also specify an `SSL Context` in their processor configuration.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] wannadream commented on a diff in pull request #6619: NIFI-10760 Add Api key authentication option to ElasticSearchClientServiceImpl
Posted by "wannadream (via GitHub)" <gi...@apache.org>.
wannadream commented on code in PR #6619:
URL: https://github.com/apache/nifi/pull/6619#discussion_r1269709827
##########
nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-client-service/src/main/java/org/apache/nifi/elasticsearch/ElasticSearchClientServiceImpl.java:
##########
@@ -275,6 +318,12 @@ private CredentialsProvider addCredentials(final CredentialsProvider credentials
return cp;
}
+ private BasicHeader createApiKeyAuthorizationHeader(String apiKeyId, String apiKey) {
+ final String apiKeyCredentials = String.format("%s:%s", apiKeyId, apiKey);
+ final String apiKeyAuth = Base64.getEncoder().encodeToString((apiKeyCredentials).getBytes(StandardCharsets.UTF_8));
Review Comment:
Hi @nandorsoma , I ran into issues with Elastic Search API auth. Standard HTTP client works well for me, but it did not work with this client implementation. I looked into the code then found some questionable lines. What is API key id doing here? It is a required field, but should it be part of the token? And why does it encode the string once more as we already pass in base64 token string. Could you elaborate?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] ChrisSamo632 commented on a diff in pull request #6619: NIFI-10760 Add Api key authentication option to ElasticSearchClientServiceImpl
Posted by GitBox <gi...@apache.org>.
ChrisSamo632 commented on code in PR #6619:
URL: https://github.com/apache/nifi/pull/6619#discussion_r1015852926
##########
nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-client-service-api/src/main/java/org/apache/nifi/elasticsearch/AuthorizationScheme.java:
##########
@@ -0,0 +1,32 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.nifi.elasticsearch;
+
+public enum AuthorizationScheme {
+ BASIC("Basic"),
+ API_KEY("API key");
+
Review Comment:
Are you happy to raise a new Jira ticket for those once this approach has been approved, as the new ticket could point to how this has been implemented?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on a diff in pull request #6619: NIFI-10760 Add Api key authentication option to ElasticSearchClientServiceImpl
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on code in PR #6619:
URL: https://github.com/apache/nifi/pull/6619#discussion_r1015877916
##########
nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-client-service-api/src/main/java/org/apache/nifi/elasticsearch/AuthorizationScheme.java:
##########
@@ -0,0 +1,32 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.nifi.elasticsearch;
+
+public enum AuthorizationScheme {
+ BASIC("Basic"),
+ API_KEY("API key");
Review Comment:
With the introduction of the new enum, supporting TLS as an Authorization Scheme sounds like something that could be handled in a subsequent issue.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on a diff in pull request #6619: NIFI-10760 Add Api key authentication option to ElasticSearchClientServiceImpl
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on code in PR #6619:
URL: https://github.com/apache/nifi/pull/6619#discussion_r1015883790
##########
nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-client-service-api/src/main/java/org/apache/nifi/elasticsearch/ElasticSearchClientService.java:
##########
@@ -52,23 +54,63 @@ public interface ElasticSearchClientService extends ControllerService {
.addValidator(Validator.VALID)
.build();
PropertyDescriptor PROXY_CONFIGURATION_SERVICE = ProxyConfiguration.createProxyConfigPropertyDescriptor(false, ProxySpec.HTTP);
+
+ AllowableValue ALLOWABLE_VALUE_BASIC_AUTH = new AllowableValue(AuthorizationScheme.BASIC.name(), AuthorizationScheme.BASIC.getDisplayName());
+ AllowableValue ALLOWABLE_VALUE_API_KEY = new AllowableValue(AuthorizationScheme.API_KEY.name(), AuthorizationScheme.API_KEY.getDisplayName());
+
+ PropertyDescriptor AUTHORIZATION_SCHEME = new PropertyDescriptor.Builder()
+ .name("authorization-scheme")
+ .displayName("Authorization Scheme")
+ .description("Authorization Scheme used for the authorization.")
+ .allowableValues(ALLOWABLE_VALUE_BASIC_AUTH, ALLOWABLE_VALUE_API_KEY)
Review Comment:
That sounds like a good idea for a subsequent issue.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] exceptionfactory closed pull request #6619: NIFI-10760 Add Api key authentication option to ElasticSearchClientServiceImpl
Posted by GitBox <gi...@apache.org>.
exceptionfactory closed pull request #6619: NIFI-10760 Add Api key authentication option to ElasticSearchClientServiceImpl
URL: https://github.com/apache/nifi/pull/6619
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] nandorsoma commented on a diff in pull request #6619: NIFI-10760 Add Api key authentication option to ElasticSearchClientServiceImpl
Posted by GitBox <gi...@apache.org>.
nandorsoma commented on code in PR #6619:
URL: https://github.com/apache/nifi/pull/6619#discussion_r1014951123
##########
nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-client-service-api/src/main/java/org/apache/nifi/elasticsearch/AuthorizationScheme.java:
##########
@@ -0,0 +1,32 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.nifi.elasticsearch;
+
+public enum AuthorizationScheme {
+ BASIC("Basic"),
+ API_KEY("API key");
Review Comment:
It would make sense, but we can't add it because we need to provide backward compatibility. What would be the default value in that case? I guess `NONE` should be. It would cause confusion if someone upgrades to a newer version and `Basic Authentication` is set.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] ChrisSamo632 commented on pull request #6619: NIFI-10760 Add Api key authentication option to ElasticSearchClientServiceImpl
Posted by GitBox <gi...@apache.org>.
ChrisSamo632 commented on PR #6619:
URL: https://github.com/apache/nifi/pull/6619#issuecomment-1306172013
> Thanks for making the adjustments @nandorsoma, the current version looks good!
>
> Do you have any additional feedback @ChrisSamo632? It sounds like there are some possibilities for follow-on improvements, but the current version looks ready.
If the remaining suggestions are captured as follow-on tickets (so we don't forget them), I've no objection to these change being Approved & merged
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] ChrisSamo632 commented on a diff in pull request #6619: NIFI-10760 Add Api key authentication option to ElasticSearchClientServiceImpl
Posted by GitBox <gi...@apache.org>.
ChrisSamo632 commented on code in PR #6619:
URL: https://github.com/apache/nifi/pull/6619#discussion_r1014661684
##########
nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-client-service-api/src/main/java/org/apache/nifi/elasticsearch/AuthorizationScheme.java:
##########
@@ -0,0 +1,32 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.nifi.elasticsearch;
+
+public enum AuthorizationScheme {
+ BASIC("Basic"),
+ API_KEY("API key");
Review Comment:
And a `NONE` option for unsecured Elasticsearch instances (not that we'd encourage that, but imagine there are plenty of people doing that, particularly in development environments)
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on pull request #6619: NIFI-10760 Add Api key authentication option to ElasticSearchClientServiceImpl
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on PR #6619:
URL: https://github.com/apache/nifi/pull/6619#issuecomment-1307883718
Thanks for the confirmation @ChrisSamo632!
Thanks for the work on this @nandorsoma, and for writing up the follow-on issues!
+1 merging
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] ChrisSamo632 commented on a diff in pull request #6619: NIFI-10760 Add Api key authentication option to ElasticSearchClientServiceImpl
Posted by "ChrisSamo632 (via GitHub)" <gi...@apache.org>.
ChrisSamo632 commented on code in PR #6619:
URL: https://github.com/apache/nifi/pull/6619#discussion_r1269822821
##########
nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-client-service/src/main/java/org/apache/nifi/elasticsearch/ElasticSearchClientServiceImpl.java:
##########
@@ -275,6 +318,12 @@ private CredentialsProvider addCredentials(final CredentialsProvider credentials
return cp;
}
+ private BasicHeader createApiKeyAuthorizationHeader(String apiKeyId, String apiKey) {
+ final String apiKeyCredentials = String.format("%s:%s", apiKeyId, apiKey);
+ final String apiKeyAuth = Base64.getEncoder().encodeToString((apiKeyCredentials).getBytes(StandardCharsets.UTF_8));
Review Comment:
The `id` and `key` would be the values returned by Elasticsearch when you [create an api key](https://www.elastic.co/guide/en/elasticsearch/reference/current/security-api-create-api-key.html#security-api-create-api-key-example) - NiFi requires both of these to construct the `Authorization` (ApiKey) header
From your description, it sounds like you are applying the `encoded` field from the Elasticsearch response (which is the Base64 encoded `id` & `key`) - NiFi doesn't currently use that in its approach - if you require that, you could raise a Jira to request it be added (submit a PR of your able) to make that an alternative to providing these existing fields
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] wannadream commented on a diff in pull request #6619: NIFI-10760 Add Api key authentication option to ElasticSearchClientServiceImpl
Posted by "wannadream (via GitHub)" <gi...@apache.org>.
wannadream commented on code in PR #6619:
URL: https://github.com/apache/nifi/pull/6619#discussion_r1269709827
##########
nifi-nar-bundles/nifi-elasticsearch-bundle/nifi-elasticsearch-client-service/src/main/java/org/apache/nifi/elasticsearch/ElasticSearchClientServiceImpl.java:
##########
@@ -275,6 +318,12 @@ private CredentialsProvider addCredentials(final CredentialsProvider credentials
return cp;
}
+ private BasicHeader createApiKeyAuthorizationHeader(String apiKeyId, String apiKey) {
+ final String apiKeyCredentials = String.format("%s:%s", apiKeyId, apiKey);
+ final String apiKeyAuth = Base64.getEncoder().encodeToString((apiKeyCredentials).getBytes(StandardCharsets.UTF_8));
Review Comment:
Hi @nandorsoma , I ran into issues with Elastic Search API auth. Standard HTTP client works well for me, but it did not work with this client implementation. I looked into the code then found some questionable lines. What is API key id doing here? It is a required field, but should it be part of the token? And why does it encode the string once more as we already passed in base64 token string. Could you elaborate?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org