You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by kevdoran <gi...@git.apache.org> on 2017/12/07 18:24:03 UTC
[GitHub] nifi-registry pull request #51: NIFIREG-61 Add support for encrypted config ...
GitHub user kevdoran opened a pull request:
https://github.com/apache/nifi-registry/pull/51
NIFIREG-61 Add support for encrypted config files
Allows sensitive property values to be encrypted in the following configuration files:
- nifi-registry.properties
- identity-providers.xml
- authorizers.xml
A decryption key can be configured in bootstrap.conf that allows decrypting protected properties at runtime.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/kevdoran/nifi-registry NIFIREG-61
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/nifi-registry/pull/51.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #51
----
commit 8c0cd912c19889e32f8962d20c450c109a8901f1
Author: Kevin Doran <kd...@gmail.com>
Date: 2017-12-05T19:44:24Z
NIFIREG-61 Add support for encrypted config files
Allows sensitive property values to be encrypted in the following
configuration files:
- nifi-registry.properties
- identity-providers.xml
- authorizers.xml
A decryption key can be configured in bootstrap.conf that allows
decrypting protected properties at runtime.
----
---
[GitHub] nifi-registry issue #51: NIFIREG-61 Add support for encrypted config files
Posted by bbende <gi...@git.apache.org>.
Github user bbende commented on the issue:
https://github.com/apache/nifi-registry/pull/51
Spoke with @kevdoran and was able to test the functionality in this PR using a pre-encrypted conf directory, verified the app started and worked successfully when encrypted values were present in nifi-registry.properties, identity-providers.xml, and authorizers.xml... will squash the commits and merge, thanks!
---
[GitHub] nifi-registry issue #51: NIFIREG-61 Add support for encrypted config files
Posted by alopresto <gi...@git.apache.org>.
Github user alopresto commented on the issue:
https://github.com/apache/nifi-registry/pull/51
Will review.
---
[GitHub] nifi-registry issue #51: NIFIREG-61 Add support for encrypted config files
Posted by alopresto <gi...@git.apache.org>.
Github user alopresto commented on the issue:
https://github.com/apache/nifi-registry/pull/51
I am reviewing this. As for squashing/merging, I too prefer the historical advantage of not squashing, but Apache NiFi policy is to perform the squash. I recommend creating a branch on your local fork which maintains this history in case it is needed, while the `master` branch will be more compact for community readability & maintenance (example -- [NIFI-3024-rebased-squashed](https://github.com/alopresto/nifi/tree/NIFI-3024-rebased-squashed) vs. [NIFI-3024](https://github.com/alopresto/nifi/tree/NIFI-3024)).
---
[GitHub] nifi-registry issue #51: NIFIREG-61 Add support for encrypted config files
Posted by kevdoran <gi...@git.apache.org>.
Github user kevdoran commented on the issue:
https://github.com/apache/nifi-registry/pull/51
Thanks @alopresto - updated that log level. Also rebased off the latest master. Let me know if you see anything else to be changed.
---
[GitHub] nifi-registry issue #51: NIFIREG-61 Add support for encrypted config files
Posted by kevdoran <gi...@git.apache.org>.
Github user kevdoran commented on the issue:
https://github.com/apache/nifi-registry/pull/51
Had another followup conversation with @alopresto to confirm the desired behavior. We are on the same page now and I'm going to push one more update to this that simplifies the code path for loading the master key value from the configuration location. Please hold reviews/merge until that change is ready, which should be soon. @alopresto, I'll ping you when this is updated as we discussed. Thanks again for the help in getting this feature into Registry!
---
[GitHub] nifi-registry issue #51: NIFIREG-61 Add support for encrypted config files
Posted by alopresto <gi...@git.apache.org>.
Github user alopresto commented on the issue:
https://github.com/apache/nifi-registry/pull/51
Discussed with @kevdoran . He is going to make a small change so the `CryptoKeyHolder` doesn't maintain the master key value in memory for the lifetime of the application, but rather makes available to each servlet context the capability to retrieve the key from the `bootstrap.conf` file when necessary. Over the lifetime of the application, this value should be needed at most 3 times (NiFi Registry properties read, Identity provider read, and Authorizer read).
He will also add some Javadoc and test cases.
---
[GitHub] nifi-registry issue #51: NIFIREG-61 Add support for encrypted config files
Posted by kevdoran <gi...@git.apache.org>.
Github user kevdoran commented on the issue:
https://github.com/apache/nifi-registry/pull/51
Roger that, thanks. I'll maintain the history on my fork. It's rebased on current master, so feel free to squash all commits on merge. Or I can create a squashed branch if you prefer.
---
[GitHub] nifi-registry pull request #51: NIFIREG-61 Add support for encrypted config ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/nifi-registry/pull/51
---
[GitHub] nifi-registry issue #51: NIFIREG-61 Add support for encrypted config files
Posted by kevdoran <gi...@git.apache.org>.
Github user kevdoran commented on the issue:
https://github.com/apache/nifi-registry/pull/51
@alopresto, I made the changes we discussed and I believe this branch is now ready to merge pending final review.
To @alopresto, @bbende, or whomever merges this: please do not squash on merge. The interim commit contains a useful history of the approaches @alopresto and I discussed, and the final commit removes an approach that may be useful to refer back to down the road should we ever revisit this.
---
[GitHub] nifi-registry issue #51: NIFIREG-61 Add support for encrypted config files
Posted by alopresto <gi...@git.apache.org>.
Github user alopresto commented on the issue:
https://github.com/apache/nifi-registry/pull/51
To be honest, if a "non-you" actor performs the merge, you don't have to worry about this. Your fork's branch will maintain history automatically. The master will be the squashed commit (usually cherry-picked onto the master branch).
---
[GitHub] nifi-registry pull request #51: NIFIREG-61 Add support for encrypted config ...
Posted by alopresto <gi...@git.apache.org>.
Github user alopresto commented on a diff in the pull request:
https://github.com/apache/nifi-registry/pull/51#discussion_r157324448
--- Diff: nifi-registry-properties/src/main/java/org/apache/nifi/registry/security/crypto/CryptoKeyLoader.java ---
@@ -0,0 +1,87 @@
+/*
+ * 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.registry.security.crypto;
+
+import org.apache.commons.lang3.StringUtils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Paths;
+import java.util.Optional;
+import java.util.stream.Stream;
+
+public class CryptoKeyLoader {
+
+ private static final Logger logger = LoggerFactory.getLogger(CryptoKeyLoader.class);
+
+ private static final String BOOTSTRAP_KEY_PREFIX = "nifi.registry.bootstrap.sensitive.key=";
+
+ /**
+ * Returns the key (if any) used to encrypt sensitive properties.
+ * The key extracted from the bootstrap.conf file at the specified location.
+ *
+ * @param bootstrapPath the path to the bootstrap file
+ * @return the key in hexadecimal format, or {@link CryptoKeyProvider#EMPTY_KEY} if the key is null or empty
+ * @throws IOException if the file is not readable
+ */
+ public static String extractKeyFromBootstrapFile(String bootstrapPath) throws IOException {
+ File bootstrapFile;
+ if (StringUtils.isBlank(bootstrapPath)) {
+ logger.error("Cannot read from bootstrap.conf file to extract encryption key; location not specified");
+ throw new IOException("Cannot read from bootstrap.conf without file location");
+ } else {
+ bootstrapFile = new File(bootstrapPath);
+ }
+
+ String keyValue;
+ if (bootstrapFile.exists() && bootstrapFile.canRead()) {
+ try (Stream<String> stream = Files.lines(Paths.get(bootstrapFile.getAbsolutePath()))) {
+ Optional<String> keyLine = stream.filter(l -> l.startsWith(BOOTSTRAP_KEY_PREFIX)).findFirst();
+ if (keyLine.isPresent()) {
+ keyValue = keyLine.get().split("=", 2)[1];
+ keyValue = checkHexKey(keyValue);
+ } else {
+ keyValue = CryptoKeyProvider.EMPTY_KEY;
+ }
+ } catch (IOException e) {
+ logger.error("Cannot read from bootstrap.conf file at {} to extract encryption key", bootstrapFile.getAbsolutePath());
+ throw new IOException("Cannot read from bootstrap.conf", e);
+ }
+ } else {
+ logger.error("Cannot read from bootstrap.conf file at {} to extract encryption key -- file is missing or permissions are incorrect", bootstrapFile.getAbsolutePath());
+ throw new IOException("Cannot read from bootstrap.conf");
+ }
+
+ if (CryptoKeyProvider.EMPTY_KEY.equals(keyValue)) {
+ logger.warn("No encryption key present in the bootstrap.conf file at {}", bootstrapFile.getAbsolutePath());
--- End diff --
I don't think this should be `WARN` level -- `INFO` is fine.
---