exceptionfactory commented on a change in pull request #5034: URL: https://github.com/apache/nifi/pull/5034#discussion_r623084122
########## File path: nifi-commons/nifi-vault-utils/src/main/java/org/apache/nifi/vault/StandardVaultCommunicationService.java ########## @@ -0,0 +1,90 @@ +/* + * 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.vault; + +import org.apache.nifi.util.FormatUtils; +import org.apache.nifi.vault.config.VaultConfiguration; +import org.apache.nifi.vault.config.VaultProperties; +import org.springframework.vault.authentication.SimpleSessionManager; +import org.springframework.vault.client.ClientHttpRequestFactoryFactory; +import org.springframework.vault.core.VaultTemplate; +import org.springframework.vault.support.Ciphertext; +import org.springframework.vault.support.ClientOptions; +import org.springframework.vault.support.Plaintext; +import org.springframework.vault.support.SslConfiguration; + +import java.time.Duration; +import java.util.Optional; +import java.util.concurrent.TimeUnit; + +/** + * Implements the VaultCommunicationService using Spring Vault + */ +public class StandardVaultCommunicationService implements VaultCommunicationService { + private static final String HTTPS = "https"; + + private final VaultConfiguration vaultConfiguration; + private final VaultTemplate vaultTemplate; + + /** + * Creates a VaultCommunicationService that uses Spring Vault. + * @param vaultProperties Properties to configure the service + * @throws VaultConfigurationException If the configuration was invalid + */ + public StandardVaultCommunicationService(final VaultProperties vaultProperties) throws VaultConfigurationException { + this.vaultConfiguration = new VaultConfiguration(vaultProperties); + + final SslConfiguration sslConfiguration = vaultProperties.getUri().contains(HTTPS) + ? vaultConfiguration.sslConfiguration() : SslConfiguration.unconfigured(); + + final ClientOptions clientOptions = getClientOptions(vaultProperties); + + vaultTemplate = new VaultTemplate(vaultConfiguration.vaultEndpoint(), + ClientHttpRequestFactoryFactory.create(clientOptions, sslConfiguration), + new SimpleSessionManager(vaultConfiguration.clientAuthentication())); + } + + private static ClientOptions getClientOptions(VaultProperties vaultProperties) { + final ClientOptions clientOptions = new ClientOptions(); + Duration readTimeoutDuration = clientOptions.getReadTimeout(); + Duration connectionTimeoutDuration = clientOptions.getConnectionTimeout(); + final Optional<String> configuredReadTimeout = vaultProperties.getReadTimeout(); + if (configuredReadTimeout.isPresent()) { + readTimeoutDuration = getDuration(configuredReadTimeout.get()); + } + final Optional<String> configuredConnectionTimeout = vaultProperties.getConnectionTimeout(); + if (configuredConnectionTimeout.isPresent()) { + connectionTimeoutDuration = getDuration(configuredConnectionTimeout.get()); + } + return new ClientOptions(connectionTimeoutDuration, readTimeoutDuration); + } + + private static Duration getDuration(String formattedDuration) { + final double duration = FormatUtils.getPreciseTimeDuration(formattedDuration, TimeUnit.MILLISECONDS); + return Duration.ofMillis(Double.valueOf(duration).longValue()); + } + + @Override + public String encrypt(String transitKey, byte[] plainText) { Review comment: Recommend marking these method parameters and others as `final`. ```suggestion public String encrypt(final String transitKey, final byte[] plainText) { ``` ########## File path: nifi-commons/nifi-vault-utils/src/main/java/org/apache/nifi/vault/config/VaultEnvironment.java ########## @@ -0,0 +1,179 @@ +/* + * 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.vault.config; + +import org.apache.nifi.vault.VaultConfigurationException; +import org.springframework.core.env.Environment; +import org.springframework.core.env.Profiles; + +import java.io.File; +import java.io.FileReader; +import java.io.IOException; +import java.net.URI; +import java.nio.file.Paths; +import java.util.Objects; +import java.util.Optional; +import java.util.Properties; + +/** + * A custom Spring Environment that uses configured POJO properties to provide the expected property values + * for Spring Vault. + */ +public class VaultEnvironment implements Environment { + public static final String VAULT_URI = "vault.uri"; + public static final String VAULT_SSL_KEYSTORE = "vault.ssl.key-store"; + public static final String VAULT_SSL_KEYSTORE_PASSWORD = "vault.ssl.key-store-password"; + public static final String VAULT_SSL_KEYSTORE_TYPE = "vault.ssl.key-store-type"; + public static final String VAULT_SSL_TRUSTSTORE = "vault.ssl.trust-store"; + public static final String VAULT_SSL_TRUSTSTORE_PASSWORD = "vault.ssl.trust-store-password"; + public static final String VAULT_SSL_TRUSTSTORE_TYPE = "vault.ssl.trust-store-type"; + public static final String VAULT_SSL_ENABLED_PROTOCOLS = "vault.ssl.enabled-protocols"; + public static final String VAULT_SSL_ENABLED_CIPHER_SUITES = "vault.ssl.enabled-cipher-suites"; + + private final Properties authProperties; + private final VaultProperties vaultProperties; + + public VaultEnvironment(final VaultProperties vaultProperties) throws VaultConfigurationException { + Objects.requireNonNull(vaultProperties, "Vault Properties are required"); + this.vaultProperties = vaultProperties; + + Optional<Properties> authProperties = this.getAuthProperties(vaultProperties); + if (!authProperties.isPresent()) { + throw new VaultConfigurationException("Vault auth properties file is required"); + } else { + this.authProperties = authProperties.get(); + } Review comment: This could be adjusted to use `Optional.orElseThrow()` ```suggestion this.authProperties = authProperties.orElseThrow(() -> new VaultConfigurationException("Vault auth properties file required")); ``` ########## File path: nifi-commons/nifi-vault-utils/pom.xml ########## @@ -0,0 +1,60 @@ +<project xmlns="http://maven.apache.org/POM/4.0.0"; xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"; xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 https://maven.apache.org/xsd/maven-4.0.0.xsd";> + <!-- + 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. + --> + <modelVersion>4.0.0</modelVersion> + <parent> + <groupId>org.apache.nifi</groupId> + <artifactId>nifi-commons</artifactId> + <version>1.14.0-SNAPSHOT</version> + </parent> + <artifactId>nifi-vault-utils</artifactId> + <dependencies> + <dependency> + <groupId>org.springframework.vault</groupId> + <artifactId>spring-vault-core</artifactId> + <version>2.3.2</version> + </dependency> + <dependency> + <groupId>org.springframework</groupId> + <artifactId>spring-core</artifactId> + <version>5.3.5</version> Review comment: Integrating this utility module in other parts of the framework may necessitate upgrading Spring more widely. This looks fine for now, but something to consider going forward. ########## File path: nifi-commons/nifi-vault-utils/src/main/java/org/apache/nifi/vault/config/VaultEnvironment.java ########## @@ -0,0 +1,179 @@ +/* + * 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.vault.config; + +import org.apache.nifi.vault.VaultConfigurationException; +import org.springframework.core.env.Environment; +import org.springframework.core.env.Profiles; + +import java.io.File; +import java.io.FileReader; +import java.io.IOException; +import java.net.URI; +import java.nio.file.Paths; +import java.util.Objects; +import java.util.Optional; +import java.util.Properties; + +/** + * A custom Spring Environment that uses configured POJO properties to provide the expected property values + * for Spring Vault. + */ +public class VaultEnvironment implements Environment { + public static final String VAULT_URI = "vault.uri"; + public static final String VAULT_SSL_KEYSTORE = "vault.ssl.key-store"; + public static final String VAULT_SSL_KEYSTORE_PASSWORD = "vault.ssl.key-store-password"; + public static final String VAULT_SSL_KEYSTORE_TYPE = "vault.ssl.key-store-type"; + public static final String VAULT_SSL_TRUSTSTORE = "vault.ssl.trust-store"; + public static final String VAULT_SSL_TRUSTSTORE_PASSWORD = "vault.ssl.trust-store-password"; + public static final String VAULT_SSL_TRUSTSTORE_TYPE = "vault.ssl.trust-store-type"; + public static final String VAULT_SSL_ENABLED_PROTOCOLS = "vault.ssl.enabled-protocols"; + public static final String VAULT_SSL_ENABLED_CIPHER_SUITES = "vault.ssl.enabled-cipher-suites"; + + private final Properties authProperties; + private final VaultProperties vaultProperties; + + public VaultEnvironment(final VaultProperties vaultProperties) throws VaultConfigurationException { + Objects.requireNonNull(vaultProperties, "Vault Properties are required"); + this.vaultProperties = vaultProperties; + + Optional<Properties> authProperties = this.getAuthProperties(vaultProperties); + if (!authProperties.isPresent()) { + throw new VaultConfigurationException("Vault auth properties file is required"); + } else { + this.authProperties = authProperties.get(); + } + } + + private Optional<Properties> getAuthProperties(VaultProperties properties) throws VaultConfigurationException { + if (properties.getAuthPropertiesFilename() == null) { + return Optional.empty(); + } + + File authPropertiesFile = Paths.get(properties.getAuthPropertiesFilename()).toFile(); + if (!authPropertiesFile.exists()) { + return Optional.empty(); + } + + Properties authProperties = new Properties(); + try (final FileReader reader = new FileReader(authPropertiesFile)) { + authProperties.load(reader); + } catch (final IOException e) { + final String message = String.format("Failed to read Vault auth properties [%s]", authPropertiesFile); + throw new VaultConfigurationException(message, e); + } + return Optional.of(authProperties); + } + + @Override + public boolean containsProperty(String propertyName) { + return this.getProperty(propertyName) != null; + } + + /** + * This maps the relevant Spring Vault properties to the configured properties from VaultProperties. + * @param key The Spring Vault property name (e.g., vault.uri) + * @return The configured property value + */ + @Override + public String getProperty(String key) { + if (VAULT_URI.equals(key)) { + return vaultProperties.getUri(); + } else if (VAULT_SSL_KEYSTORE.equals(key)) { + return vaultProperties.getKeystore(); + } else if (VAULT_SSL_KEYSTORE_PASSWORD.equals(key)) { + return vaultProperties.getKeystorePassword(); + } else if (VAULT_SSL_KEYSTORE_TYPE.equals(key)) { + return vaultProperties.getKeystoreType(); + } else if (VAULT_SSL_TRUSTSTORE.equals(key)) { + return vaultProperties.getTruststore(); + } else if (VAULT_SSL_TRUSTSTORE_PASSWORD.equals(key)) { + return vaultProperties.getTruststorePassword(); + } else if (VAULT_SSL_TRUSTSTORE_TYPE.equals(key)) { + return vaultProperties.getTruststoreType(); + } else if (VAULT_SSL_ENABLED_CIPHER_SUITES.equals(key)) { + return vaultProperties.getEnabledTlsCipherSuites(); + } else if (VAULT_SSL_ENABLED_PROTOCOLS.equals(key)) { + return vaultProperties.getEnabledTlsProtocols(); + } else { + return authProperties.getProperty(key); + } + } + + @Override + public String getProperty(String key, String defaultValue) { + return this.getProperty(key, String.class, defaultValue); + } + + @Override + public <T> T getProperty(String key, Class<T> targetType) { + return this.getProperty(key, targetType, null); + } + + @Override + public <T> T getProperty(String key, Class<T> targetType, T defaultValue) { + final String value = this.getProperty(key); + if (targetType.isAssignableFrom(URI.class)) { + return value == null ? defaultValue : (T) URI.create(value); + } else { + return (T) (value == null ? defaultValue : value); + } + } + + // The rest of these are not actually called from EnvironmentVaultConfiguration + @Override + public String getRequiredProperty(String s) throws IllegalStateException { + throw new UnsupportedOperationException("Method not supported"); + } + + @Override + public <T> T getRequiredProperty(String s, Class<T> aClass) throws IllegalStateException { + throw new UnsupportedOperationException("Method not supported"); + } + + @Override + public String resolvePlaceholders(String s) { + + Review comment: Some empty lines could be removed. ########## File path: nifi-commons/nifi-vault-utils/src/main/java/org/apache/nifi/vault/StandardVaultCommunicationService.java ########## @@ -0,0 +1,90 @@ +/* + * 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.vault; + +import org.apache.nifi.util.FormatUtils; +import org.apache.nifi.vault.config.VaultConfiguration; +import org.apache.nifi.vault.config.VaultProperties; +import org.springframework.vault.authentication.SimpleSessionManager; +import org.springframework.vault.client.ClientHttpRequestFactoryFactory; +import org.springframework.vault.core.VaultTemplate; +import org.springframework.vault.support.Ciphertext; +import org.springframework.vault.support.ClientOptions; +import org.springframework.vault.support.Plaintext; +import org.springframework.vault.support.SslConfiguration; + +import java.time.Duration; +import java.util.Optional; +import java.util.concurrent.TimeUnit; + +/** + * Implements the VaultCommunicationService using Spring Vault + */ +public class StandardVaultCommunicationService implements VaultCommunicationService { + private static final String HTTPS = "https"; + + private final VaultConfiguration vaultConfiguration; + private final VaultTemplate vaultTemplate; + + /** + * Creates a VaultCommunicationService that uses Spring Vault. + * @param vaultProperties Properties to configure the service + * @throws VaultConfigurationException If the configuration was invalid + */ + public StandardVaultCommunicationService(final VaultProperties vaultProperties) throws VaultConfigurationException { + this.vaultConfiguration = new VaultConfiguration(vaultProperties); + + final SslConfiguration sslConfiguration = vaultProperties.getUri().contains(HTTPS) + ? vaultConfiguration.sslConfiguration() : SslConfiguration.unconfigured(); + + final ClientOptions clientOptions = getClientOptions(vaultProperties); + + vaultTemplate = new VaultTemplate(vaultConfiguration.vaultEndpoint(), + ClientHttpRequestFactoryFactory.create(clientOptions, sslConfiguration), + new SimpleSessionManager(vaultConfiguration.clientAuthentication())); + } + + private static ClientOptions getClientOptions(VaultProperties vaultProperties) { + final ClientOptions clientOptions = new ClientOptions(); + Duration readTimeoutDuration = clientOptions.getReadTimeout(); + Duration connectionTimeoutDuration = clientOptions.getConnectionTimeout(); + final Optional<String> configuredReadTimeout = vaultProperties.getReadTimeout(); + if (configuredReadTimeout.isPresent()) { + readTimeoutDuration = getDuration(configuredReadTimeout.get()); + } + final Optional<String> configuredConnectionTimeout = vaultProperties.getConnectionTimeout(); + if (configuredConnectionTimeout.isPresent()) { + connectionTimeoutDuration = getDuration(configuredConnectionTimeout.get()); + } + return new ClientOptions(connectionTimeoutDuration, readTimeoutDuration); + } + + private static Duration getDuration(String formattedDuration) { + final double duration = FormatUtils.getPreciseTimeDuration(formattedDuration, TimeUnit.MILLISECONDS); + return Duration.ofMillis(Double.valueOf(duration).longValue()); + } + + @Override + public String encrypt(String transitKey, byte[] plainText) { + return vaultTemplate.opsForTransit().encrypt(transitKey, Plaintext.of(plainText)).getCiphertext(); Review comment: The `opsForTransit()` method creates a new `VaultTransitTemplate`, which seems unnecessary if the transitKey is the same for multiple invocations. With the `VaultCommunicationService` being focused on encryption and decryption operations, would it be better to have `transitKey` as a constructor argument and simplify the interface methods? That would probably also necessitate renaming `VaultCommunicationService` to something more narrow, such as `VaultEncryptionService`. ########## File path: nifi-commons/nifi-vault-utils/src/main/java/org/apache/nifi/vault/config/VaultEnvironment.java ########## @@ -0,0 +1,179 @@ +/* + * 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.vault.config; + +import org.apache.nifi.vault.VaultConfigurationException; +import org.springframework.core.env.Environment; +import org.springframework.core.env.Profiles; + +import java.io.File; +import java.io.FileReader; +import java.io.IOException; +import java.net.URI; +import java.nio.file.Paths; +import java.util.Objects; +import java.util.Optional; +import java.util.Properties; + +/** + * A custom Spring Environment that uses configured POJO properties to provide the expected property values + * for Spring Vault. + */ +public class VaultEnvironment implements Environment { + public static final String VAULT_URI = "vault.uri"; + public static final String VAULT_SSL_KEYSTORE = "vault.ssl.key-store"; + public static final String VAULT_SSL_KEYSTORE_PASSWORD = "vault.ssl.key-store-password"; + public static final String VAULT_SSL_KEYSTORE_TYPE = "vault.ssl.key-store-type"; + public static final String VAULT_SSL_TRUSTSTORE = "vault.ssl.trust-store"; + public static final String VAULT_SSL_TRUSTSTORE_PASSWORD = "vault.ssl.trust-store-password"; + public static final String VAULT_SSL_TRUSTSTORE_TYPE = "vault.ssl.trust-store-type"; + public static final String VAULT_SSL_ENABLED_PROTOCOLS = "vault.ssl.enabled-protocols"; + public static final String VAULT_SSL_ENABLED_CIPHER_SUITES = "vault.ssl.enabled-cipher-suites"; + + private final Properties authProperties; + private final VaultProperties vaultProperties; + + public VaultEnvironment(final VaultProperties vaultProperties) throws VaultConfigurationException { + Objects.requireNonNull(vaultProperties, "Vault Properties are required"); + this.vaultProperties = vaultProperties; + + Optional<Properties> authProperties = this.getAuthProperties(vaultProperties); + if (!authProperties.isPresent()) { + throw new VaultConfigurationException("Vault auth properties file is required"); + } else { + this.authProperties = authProperties.get(); + } + } + + private Optional<Properties> getAuthProperties(VaultProperties properties) throws VaultConfigurationException { + if (properties.getAuthPropertiesFilename() == null) { + return Optional.empty(); + } + + File authPropertiesFile = Paths.get(properties.getAuthPropertiesFilename()).toFile(); + if (!authPropertiesFile.exists()) { + return Optional.empty(); + } + + Properties authProperties = new Properties(); + try (final FileReader reader = new FileReader(authPropertiesFile)) { + authProperties.load(reader); + } catch (final IOException e) { + final String message = String.format("Failed to read Vault auth properties [%s]", authPropertiesFile); + throw new VaultConfigurationException(message, e); + } + return Optional.of(authProperties); + } + + @Override + public boolean containsProperty(String propertyName) { + return this.getProperty(propertyName) != null; + } + + /** + * This maps the relevant Spring Vault properties to the configured properties from VaultProperties. + * @param key The Spring Vault property name (e.g., vault.uri) + * @return The configured property value + */ + @Override + public String getProperty(String key) { + if (VAULT_URI.equals(key)) { + return vaultProperties.getUri(); + } else if (VAULT_SSL_KEYSTORE.equals(key)) { + return vaultProperties.getKeystore(); + } else if (VAULT_SSL_KEYSTORE_PASSWORD.equals(key)) { + return vaultProperties.getKeystorePassword(); + } else if (VAULT_SSL_KEYSTORE_TYPE.equals(key)) { + return vaultProperties.getKeystoreType(); + } else if (VAULT_SSL_TRUSTSTORE.equals(key)) { + return vaultProperties.getTruststore(); + } else if (VAULT_SSL_TRUSTSTORE_PASSWORD.equals(key)) { + return vaultProperties.getTruststorePassword(); + } else if (VAULT_SSL_TRUSTSTORE_TYPE.equals(key)) { + return vaultProperties.getTruststoreType(); + } else if (VAULT_SSL_ENABLED_CIPHER_SUITES.equals(key)) { + return vaultProperties.getEnabledTlsCipherSuites(); + } else if (VAULT_SSL_ENABLED_PROTOCOLS.equals(key)) { + return vaultProperties.getEnabledTlsProtocols(); + } else { + return authProperties.getProperty(key); + } Review comment: This seems a bit painful to maintain, as well as the other unimplemented methods. Is it possible to use a different approach for translating the custom `VaultProperties` to an environment configuration? Perhaps using annotating `VaultProperties` with Spring's `@ConfigurationProperties` and providing it to the custom Application Context? Leveraging Spring's ability to support a variety of configuration options makes sense, but it would be helpful to evaluate other options for streamlining the 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. For queries about this service, please contact Infrastructure at: [email protected]
