You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by "greyp9 (via GitHub)" <gi...@apache.org> on 2023/10/13 13:06:30 UTC
[PR] NIFI-12200 - Remove deprecated module nifi-toolkit-tls [nifi]
greyp9 opened a new pull request, #7879:
URL: https://github.com/apache/nifi/pull/7879
# 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 21
### 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
Re: [PR] NIFI-12200 - Remove deprecated module nifi-toolkit-tls [nifi]
Posted by "exceptionfactory (via GitHub)" <gi...@apache.org>.
exceptionfactory closed pull request #7879: NIFI-12200 - Remove deprecated module nifi-toolkit-tls
URL: https://github.com/apache/nifi/pull/7879
--
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
Re: [PR] NIFI-12200 - Remove deprecated module nifi-toolkit-tls [nifi]
Posted by "greyp9 (via GitHub)" <gi...@apache.org>.
greyp9 commented on code in PR #7879:
URL: https://github.com/apache/nifi/pull/7879#discussion_r1364258528
##########
nifi-registry/nifi-registry-core/nifi-registry-web-api/src/test/resources/keys/README.md:
##########
@@ -72,20 +72,10 @@ WD="/tmp/test-keys-$(date +"%Y%m%d-%H%M%S")"
mkdir "$WD"
cd "$WD"
-# copy existing CA key/cert pair to working directory, rename to default tls-toolkit names
+# copy existing CA key/cert pair to working directory, rename to default names
cp /path/to/nifi-registry/nifi-registry-core/nifi-registry-web-api/src/test/resources/keys/ca-key.pem ./nifi-key.key
cp /path/to/nifi-registry/nifi-registry-core/nifi-registry-web-api/src/test/resources/keys/ca-cert.pem ./nifi-cert.pem
Review Comment:
Thanks, I've made that change.
Since the text is wrapped in a Markdown code block, it seems the markdown reformatting is not happening. But probably still a good practice.
https://github.com/greyp9/nifi/blob/NIFI-12200/nifi-registry/nifi-registry-core/nifi-registry-web-api/src/test/resources/keys/README.md
--
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
Re: [PR] NIFI-12200 - Remove deprecated module nifi-toolkit-tls [nifi]
Posted by "ChrisSamo632 (via GitHub)" <gi...@apache.org>.
ChrisSamo632 commented on code in PR #7879:
URL: https://github.com/apache/nifi/pull/7879#discussion_r1358442867
##########
nifi-toolkit/pom.xml:
##########
@@ -23,7 +23,6 @@
<artifactId>nifi-toolkit</artifactId>
<packaging>pom</packaging>
<modules>
- <module>nifi-toolkit-tls</module>
Review Comment:
The `tls-toolkit` is also referenced by the `nifi-toolkit/nifi-toolkit-assembly`'s `docker` build and tests, it will need removing
--
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
Re: [PR] NIFI-12200 - Remove deprecated module nifi-toolkit-tls [nifi]
Posted by "greyp9 (via GitHub)" <gi...@apache.org>.
greyp9 commented on code in PR #7879:
URL: https://github.com/apache/nifi/pull/7879#discussion_r1362711361
##########
nifi-registry/nifi-registry-core/nifi-registry-web-api/src/test/resources/keys/README.md:
##########
@@ -72,20 +72,10 @@ WD="/tmp/test-keys-$(date +"%Y%m%d-%H%M%S")"
mkdir "$WD"
cd "$WD"
-# copy existing CA key/cert pair to working directory, rename to default tls-toolkit names
+# copy existing CA key/cert pair to working directory, rename to default names
cp /path/to/nifi-registry/nifi-registry-core/nifi-registry-web-api/src/test/resources/keys/ca-key.pem ./nifi-key.key
cp /path/to/nifi-registry/nifi-registry-core/nifi-registry-web-api/src/test/resources/keys/ca-cert.pem ./nifi-cert.pem
Review Comment:
Is some sort of blurb needed here to replace the `tls-toolkit` invocation?
--
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
Re: [PR] NIFI-12200 - Remove deprecated module nifi-toolkit-tls [nifi]
Posted by "ChrisSamo632 (via GitHub)" <gi...@apache.org>.
ChrisSamo632 commented on code in PR #7879:
URL: https://github.com/apache/nifi/pull/7879#discussion_r1364231002
##########
nifi-registry/nifi-registry-core/nifi-registry-web-api/src/test/resources/keys/README.md:
##########
@@ -72,20 +72,10 @@ WD="/tmp/test-keys-$(date +"%Y%m%d-%H%M%S")"
mkdir "$WD"
cd "$WD"
-# copy existing CA key/cert pair to working directory, rename to default tls-toolkit names
+# copy existing CA key/cert pair to working directory, rename to default names
cp /path/to/nifi-registry/nifi-registry-core/nifi-registry-web-api/src/test/resources/keys/ca-key.pem ./nifi-key.key
cp /path/to/nifi-registry/nifi-registry-core/nifi-registry-web-api/src/test/resources/keys/ca-cert.pem ./nifi-cert.pem
Review Comment:
To be in keeping with other links within the NiFi docs, I'd suggest using Markdown link notation, e.g. `for more info, see the [walkthrough for securing NiFi with mTLS](https://nifi.apache.org/docs/nifi-docs/html/walkthroughs.html#securing-nifi-with-mtls)`
--
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
Re: [PR] NIFI-12200 - Remove deprecated module nifi-toolkit-tls [nifi]
Posted by "greyp9 (via GitHub)" <gi...@apache.org>.
greyp9 commented on code in PR #7879:
URL: https://github.com/apache/nifi/pull/7879#discussion_r1362674146
##########
nifi-toolkit/pom.xml:
##########
@@ -23,7 +23,6 @@
<artifactId>nifi-toolkit</artifactId>
<packaging>pom</packaging>
<modules>
- <module>nifi-toolkit-tls</module>
Review Comment:
Thanks! Missed those...
--
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
Re: [PR] NIFI-12200 - Remove deprecated module nifi-toolkit-tls [nifi]
Posted by "ChrisSamo632 (via GitHub)" <gi...@apache.org>.
ChrisSamo632 commented on code in PR #7879:
URL: https://github.com/apache/nifi/pull/7879#discussion_r1364278415
##########
nifi-registry/nifi-registry-core/nifi-registry-web-api/src/test/resources/keys/README.md:
##########
@@ -72,20 +72,10 @@ WD="/tmp/test-keys-$(date +"%Y%m%d-%H%M%S")"
mkdir "$WD"
cd "$WD"
-# copy existing CA key/cert pair to working directory, rename to default tls-toolkit names
+# copy existing CA key/cert pair to working directory, rename to default names
cp /path/to/nifi-registry/nifi-registry-core/nifi-registry-web-api/src/test/resources/keys/ca-key.pem ./nifi-key.key
cp /path/to/nifi-registry/nifi-registry-core/nifi-registry-web-api/src/test/resources/keys/ca-cert.pem ./nifi-cert.pem
Review Comment:
Ahh, I'd missed that these are comments within a code block (d'oh)! So yeah, they won't be formated like Markdown links themselves
In which case, I'm happy either way
--
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
Re: [PR] NIFI-12200 - Remove deprecated module nifi-toolkit-tls [nifi]
Posted by "ChrisSamo632 (via GitHub)" <gi...@apache.org>.
ChrisSamo632 commented on code in PR #7879:
URL: https://github.com/apache/nifi/pull/7879#discussion_r1362755726
##########
nifi-registry/nifi-registry-core/nifi-registry-web-api/src/test/resources/keys/README.md:
##########
@@ -72,20 +72,10 @@ WD="/tmp/test-keys-$(date +"%Y%m%d-%H%M%S")"
mkdir "$WD"
cd "$WD"
-# copy existing CA key/cert pair to working directory, rename to default tls-toolkit names
+# copy existing CA key/cert pair to working directory, rename to default names
cp /path/to/nifi-registry/nifi-registry-core/nifi-registry-web-api/src/test/resources/keys/ca-key.pem ./nifi-key.key
cp /path/to/nifi-registry/nifi-registry-core/nifi-registry-web-api/src/test/resources/keys/ca-cert.pem ./nifi-cert.pem
Review Comment:
Some alternative wording here and elsewhere in the docs would seem sensible, e.g. based on the recent [dev mailing list thread](https://lists.apache.org/thread/fny5phr1mq2pqo86hqs1lmsnfrqt06sf) started by @exceptionfactory
I think the important thing is to strike the right balance between pointing administrators at the need for obtaining certificates, but without detailing the multitude of possible ways to do so (as people's needs are often different for different environments)
Do we note a set of minimum requirements and suggest people look to obtain a certificate (in the required format) from a reputable CA, or maybe generate self-signed certificates for development deployments?
--
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
Re: [PR] NIFI-12200 - Remove deprecated module nifi-toolkit-tls [nifi]
Posted by "greyp9 (via GitHub)" <gi...@apache.org>.
greyp9 commented on code in PR #7879:
URL: https://github.com/apache/nifi/pull/7879#discussion_r1362822241
##########
nifi-registry/nifi-registry-core/nifi-registry-web-api/src/test/resources/keys/README.md:
##########
@@ -72,20 +72,10 @@ WD="/tmp/test-keys-$(date +"%Y%m%d-%H%M%S")"
mkdir "$WD"
cd "$WD"
-# copy existing CA key/cert pair to working directory, rename to default tls-toolkit names
+# copy existing CA key/cert pair to working directory, rename to default names
cp /path/to/nifi-registry/nifi-registry-core/nifi-registry-web-api/src/test/resources/keys/ca-key.pem ./nifi-key.key
cp /path/to/nifi-registry/nifi-registry-core/nifi-registry-web-api/src/test/resources/keys/ca-cert.pem ./nifi-cert.pem
Review Comment:
We could point them at this walkthrough (link to target documentation?):
https://github.com/apache/nifi/blob/main/nifi-docs/src/main/asciidoc/walkthroughs.adoc#securing-nifi-with-mtls
... which will be homed here:
https://nifi.apache.org/docs/nifi-docs/html/walkthroughs.html#securing-nifi-with-mtls
Something like:
```
place new keystores / truststores (for more info, see: https://github.com/apache/nifi/blob/main/nifi-docs/src/main/asciidoc/walkthroughs.adoc#securing-nifi-with-mtls)
```
--
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
Re: [PR] NIFI-12200 - Remove deprecated module nifi-toolkit-tls [nifi]
Posted by "greyp9 (via GitHub)" <gi...@apache.org>.
greyp9 commented on code in PR #7879:
URL: https://github.com/apache/nifi/pull/7879#discussion_r1364206817
##########
nifi-registry/nifi-registry-core/nifi-registry-web-api/src/test/resources/keys/README.md:
##########
@@ -72,20 +72,10 @@ WD="/tmp/test-keys-$(date +"%Y%m%d-%H%M%S")"
mkdir "$WD"
cd "$WD"
-# copy existing CA key/cert pair to working directory, rename to default tls-toolkit names
+# copy existing CA key/cert pair to working directory, rename to default names
cp /path/to/nifi-registry/nifi-registry-core/nifi-registry-web-api/src/test/resources/keys/ca-key.pem ./nifi-key.key
cp /path/to/nifi-registry/nifi-registry-core/nifi-registry-web-api/src/test/resources/keys/ca-cert.pem ./nifi-cert.pem
Review Comment:
Added...
--
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
Re: [PR] NIFI-12200 - Remove deprecated module nifi-toolkit-tls [nifi]
Posted by "ChrisSamo632 (via GitHub)" <gi...@apache.org>.
ChrisSamo632 commented on code in PR #7879:
URL: https://github.com/apache/nifi/pull/7879#discussion_r1363113162
##########
nifi-registry/nifi-registry-core/nifi-registry-web-api/src/test/resources/keys/README.md:
##########
@@ -72,20 +72,10 @@ WD="/tmp/test-keys-$(date +"%Y%m%d-%H%M%S")"
mkdir "$WD"
cd "$WD"
-# copy existing CA key/cert pair to working directory, rename to default tls-toolkit names
+# copy existing CA key/cert pair to working directory, rename to default names
cp /path/to/nifi-registry/nifi-registry-core/nifi-registry-web-api/src/test/resources/keys/ca-key.pem ./nifi-key.key
cp /path/to/nifi-registry/nifi-registry-core/nifi-registry-web-api/src/test/resources/keys/ca-cert.pem ./nifi-cert.pem
Review Comment:
Sounds reasonable, I couldn't recall whether I'd seen the new although being added to the docs already, but such a link seems fair
--
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