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 2020/12/04 10:39:36 UTC
[GitHub] [nifi] CefBoud opened a new pull request #4709: NIFI-7783: Include CA CN as a SAN entry
CefBoud opened a new pull request #4709:
URL: https://github.com/apache/nifi/pull/4709
Thank you for submitting a contribution to Apache NiFi.
Please provide a short description of the PR here:
Include CN as a SAN entry when generating a self-signed certificate.
_Enables X functionality; fixes bug NIFI-YYYY._
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
- [ ] Does your PR title start with **NIFI-XXXX** where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
- [ ] Has your PR been rebased against the latest commit within the target branch (typically `main`)?
- [ ] Is your initial contribution a single, squashed commit? _Additional commits in response to PR reviewer feedback should be made on this branch and pushed to allow change tracking. Do not `squash` or use `--force` when pushing to allow for clean monitoring of changes._
### For code changes:
- [ ] Have you ensured that the full suite of tests is executed via `mvn -Pcontrib-check clean install` at the root `nifi` folder?
- [ ] Have you written or updated unit tests to verify your changes?
- [ ] Have you verified that the full build is successful on JDK 8?
- [ ] Have you verified that the full build is successful on JDK 11?
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
- [ ] If applicable, have you updated the `LICENSE` file, including the main `LICENSE` file under `nifi-assembly`?
- [ ] If applicable, have you updated the `NOTICE` file, including the main `NOTICE` file found under `nifi-assembly`?
- [ ] If adding new Properties, have you added `.displayName` in addition to .name (programmatic access) for each of the new properties?
### For documentation related changes:
- [ ] Have you ensured that format looks appropriate for the output in which it is rendered?
### Note:
Please ensure that once the PR is submitted, you check GitHub Actions CI for build issues and submit an update to your PR as soon as possible.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on a change in pull request #4709: NIFI-7783: Include CA CN as a SAN entry
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on a change in pull request #4709:
URL: https://github.com/apache/nifi/pull/4709#discussion_r551308940
##########
File path: nifi-commons/nifi-security-utils/src/main/java/org/apache/nifi/security/util/CertificateUtils.java
##########
@@ -469,6 +472,14 @@ public static X509Certificate generateSelfSignedX509Certificate(KeyPair keyPair,
// (2) extendedKeyUsage extension
certBuilder.addExtension(Extension.extendedKeyUsage, false, new ExtendedKeyUsage(new KeyPurposeId[]{KeyPurposeId.id_kp_clientAuth, KeyPurposeId.id_kp_serverAuth}));
+ // (3) subjectAlternativeName extension. Include CN as a SAN entry.
+ try {
+ final String cn = IETFUtils.valueToString(new X500Name(dn).getRDNs(BCStyle.CN)[0].getFirst().getValue());
Review comment:
Although all current uses of `generateSelfSignedX509Certificate()` appear to include a common name in the distinguished name string, common name is not necessarily required. Perhaps creating a separate method along the lines of `String getCommonName(String dn)` would allow for checking whether `getRDNs(BCStyle.CN)` returns at least one value. The return of that method would inform this method as to whether the SAN should be 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on pull request #4709: NIFI-7783: Include CA CN as a SAN entry
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on pull request #4709:
URL: https://github.com/apache/nifi/pull/4709#issuecomment-771161070
Thanks for the work on this PR @CefBoud! +1
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] CefBoud commented on pull request #4709: NIFI-7783: Include CA CN as a SAN entry
Posted by GitBox <gi...@apache.org>.
CefBoud commented on pull request #4709:
URL: https://github.com/apache/nifi/pull/4709#issuecomment-771042727
Thank you for taking the time to review, @exceptionfactory. And kudos for the attention to detail. I updated per your recommendations.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] asfgit closed pull request #4709: NIFI-7783: Include CA CN as a SAN entry
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #4709:
URL: https://github.com/apache/nifi/pull/4709
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] CefBoud commented on a change in pull request #4709: NIFI-7783: Include CA CN as a SAN entry
Posted by GitBox <gi...@apache.org>.
CefBoud commented on a change in pull request #4709:
URL: https://github.com/apache/nifi/pull/4709#discussion_r559224571
##########
File path: nifi-commons/nifi-security-utils/src/main/java/org/apache/nifi/security/util/CertificateUtils.java
##########
@@ -469,6 +472,14 @@ public static X509Certificate generateSelfSignedX509Certificate(KeyPair keyPair,
// (2) extendedKeyUsage extension
certBuilder.addExtension(Extension.extendedKeyUsage, false, new ExtendedKeyUsage(new KeyPurposeId[]{KeyPurposeId.id_kp_clientAuth, KeyPurposeId.id_kp_serverAuth}));
+ // (3) subjectAlternativeName extension. Include CN as a SAN entry.
+ try {
+ final String cn = IETFUtils.valueToString(new X500Name(dn).getRDNs(BCStyle.CN)[0].getFirst().getValue());
+ certBuilder.addExtension(Extension.subjectAlternativeName, false, new GeneralNames(new GeneralName(GeneralName.dNSName, cn)));
+ } catch (Exception e) {
Review comment:
Creating the `String getCommonName(String dn)` method allowed to avoid having the exception all together by returning null in case the common name is not present in the DN.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] CefBoud commented on pull request #4709: NIFI-7783: Include CA CN as a SAN entry
Posted by GitBox <gi...@apache.org>.
CefBoud commented on pull request #4709:
URL: https://github.com/apache/nifi/pull/4709#issuecomment-761870143
Thank you for the insightful review @exceptionfactory.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on a change in pull request #4709: NIFI-7783: Include CA CN as a SAN entry
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on a change in pull request #4709:
URL: https://github.com/apache/nifi/pull/4709#discussion_r568006661
##########
File path: nifi-commons/nifi-security-utils/src/main/java/org/apache/nifi/security/util/CertificateUtils.java
##########
@@ -469,6 +472,12 @@ public static X509Certificate generateSelfSignedX509Certificate(KeyPair keyPair,
// (2) extendedKeyUsage extension
certBuilder.addExtension(Extension.extendedKeyUsage, false, new ExtendedKeyUsage(new KeyPurposeId[]{KeyPurposeId.id_kp_clientAuth, KeyPurposeId.id_kp_serverAuth}));
+ // (3) subjectAlternativeName extension. Include CN as a SAN entry if it exists.
+ final String cn = getCommonName(dn);
+ if(cn != null) {
Review comment:
Recommend adjusting this check to use `StringUtils.isNotBlank()` and adding a space after `if`:
```suggestion
if (StringUtils.isNotBlank(cn)) {
```
##########
File path: nifi-commons/nifi-security-utils/src/main/java/org/apache/nifi/security/util/CertificateUtils.java
##########
@@ -625,6 +634,20 @@ public static boolean isTlsError(Throwable e) {
}
}
+ /**
+ *Extracts the common name from the given DN.
Review comment:
Recommend adding a space after the asterisk to improve readability.
```suggestion
* Extracts the common name from the given DN.
```
##########
File path: nifi-commons/nifi-security-utils/src/main/java/org/apache/nifi/security/util/CertificateUtils.java
##########
@@ -625,6 +634,20 @@ public static boolean isTlsError(Throwable e) {
}
}
+ /**
+ *Extracts the common name from the given DN.
+ *
+ * @param dn the distinguished name to evaluate
+ * @return the common name if it exists, null otherwise.
+ */
+ public static String getCommonName(String dn){
Review comment:
Recommend declaring the parameter as `final`:
```suggestion
public static String getCommonName(final String dn) {
```
##########
File path: nifi-commons/nifi-security-utils/src/main/java/org/apache/nifi/security/util/CertificateUtils.java
##########
@@ -625,6 +634,20 @@ public static boolean isTlsError(Throwable e) {
}
}
+ /**
+ *Extracts the common name from the given DN.
+ *
+ * @param dn the distinguished name to evaluate
+ * @return the common name if it exists, null otherwise.
+ */
+ public static String getCommonName(String dn){
+ RDN[] rdns = new X500Name(dn).getRDNs(BCStyle.CN);
+ if(rdns.length == 0) {
Review comment:
Recommend adding a space after the `if`:
```suggestion
if (rdns.length == 0) {
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on a change in pull request #4709: NIFI-7783: Include CA CN as a SAN entry
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on a change in pull request #4709:
URL: https://github.com/apache/nifi/pull/4709#discussion_r568005942
##########
File path: nifi-commons/nifi-security-utils/src/main/java/org/apache/nifi/security/util/CertificateUtils.java
##########
@@ -469,6 +472,14 @@ public static X509Certificate generateSelfSignedX509Certificate(KeyPair keyPair,
// (2) extendedKeyUsage extension
certBuilder.addExtension(Extension.extendedKeyUsage, false, new ExtendedKeyUsage(new KeyPurposeId[]{KeyPurposeId.id_kp_clientAuth, KeyPurposeId.id_kp_serverAuth}));
+ // (3) subjectAlternativeName extension. Include CN as a SAN entry.
+ try {
+ final String cn = IETFUtils.valueToString(new X500Name(dn).getRDNs(BCStyle.CN)[0].getFirst().getValue());
+ certBuilder.addExtension(Extension.subjectAlternativeName, false, new GeneralNames(new GeneralName(GeneralName.dNSName, cn)));
Review comment:
After evaluating the valid characters for a DNS name, it looks like it would be difficult to incorporate an appropriate regular expression pattern to cover all cases, so it is probably best to just introduce a check for a string that is not blank.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] CefBoud commented on a change in pull request #4709: NIFI-7783: Include CA CN as a SAN entry
Posted by GitBox <gi...@apache.org>.
CefBoud commented on a change in pull request #4709:
URL: https://github.com/apache/nifi/pull/4709#discussion_r559230043
##########
File path: nifi-commons/nifi-security-utils/src/main/java/org/apache/nifi/security/util/CertificateUtils.java
##########
@@ -469,6 +472,14 @@ public static X509Certificate generateSelfSignedX509Certificate(KeyPair keyPair,
// (2) extendedKeyUsage extension
certBuilder.addExtension(Extension.extendedKeyUsage, false, new ExtendedKeyUsage(new KeyPurposeId[]{KeyPurposeId.id_kp_clientAuth, KeyPurposeId.id_kp_serverAuth}));
+ // (3) subjectAlternativeName extension. Include CN as a SAN entry.
+ try {
+ final String cn = IETFUtils.valueToString(new X500Name(dn).getRDNs(BCStyle.CN)[0].getFirst().getValue());
+ certBuilder.addExtension(Extension.subjectAlternativeName, false, new GeneralNames(new GeneralName(GeneralName.dNSName, cn)));
Review comment:
How about *.domain.tld ? or *.sub.domain.tld?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on a change in pull request #4709: NIFI-7783: Include CA CN as a SAN entry
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on a change in pull request #4709:
URL: https://github.com/apache/nifi/pull/4709#discussion_r551308940
##########
File path: nifi-commons/nifi-security-utils/src/main/java/org/apache/nifi/security/util/CertificateUtils.java
##########
@@ -469,6 +472,14 @@ public static X509Certificate generateSelfSignedX509Certificate(KeyPair keyPair,
// (2) extendedKeyUsage extension
certBuilder.addExtension(Extension.extendedKeyUsage, false, new ExtendedKeyUsage(new KeyPurposeId[]{KeyPurposeId.id_kp_clientAuth, KeyPurposeId.id_kp_serverAuth}));
+ // (3) subjectAlternativeName extension. Include CN as a SAN entry.
+ try {
+ final String cn = IETFUtils.valueToString(new X500Name(dn).getRDNs(BCStyle.CN)[0].getFirst().getValue());
Review comment:
Although all current uses of `generateSelfSignedX509Certificate()` appear to include a common name in the distinguished name string, common name is not necessarily required. Perhaps creating a separate method along the lines of `String getCommonName(String dn)` would allow for checking whether `getRDNs(BCStyle.CN)` returns at least one value. The return of method would inform this method as to whether the SAN should be added.
##########
File path: nifi-commons/nifi-security-utils/src/main/java/org/apache/nifi/security/util/CertificateUtils.java
##########
@@ -469,6 +472,14 @@ public static X509Certificate generateSelfSignedX509Certificate(KeyPair keyPair,
// (2) extendedKeyUsage extension
certBuilder.addExtension(Extension.extendedKeyUsage, false, new ExtendedKeyUsage(new KeyPurposeId[]{KeyPurposeId.id_kp_clientAuth, KeyPurposeId.id_kp_serverAuth}));
+ // (3) subjectAlternativeName extension. Include CN as a SAN entry.
+ try {
+ final String cn = IETFUtils.valueToString(new X500Name(dn).getRDNs(BCStyle.CN)[0].getFirst().getValue());
+ certBuilder.addExtension(Extension.subjectAlternativeName, false, new GeneralNames(new GeneralName(GeneralName.dNSName, cn)));
+ } catch (Exception e) {
Review comment:
Should this `Exception` be more specifically typed? Also recommend including a unit test case to handle the exception case. If logic is added to make inclusion of the SAN conditional, then unit tests should be updated accordingly.
##########
File path: nifi-commons/nifi-security-utils/src/main/java/org/apache/nifi/security/util/CertificateUtils.java
##########
@@ -469,6 +472,14 @@ public static X509Certificate generateSelfSignedX509Certificate(KeyPair keyPair,
// (2) extendedKeyUsage extension
certBuilder.addExtension(Extension.extendedKeyUsage, false, new ExtendedKeyUsage(new KeyPurposeId[]{KeyPurposeId.id_kp_clientAuth, KeyPurposeId.id_kp_serverAuth}));
+ // (3) subjectAlternativeName extension. Include CN as a SAN entry.
+ try {
+ final String cn = IETFUtils.valueToString(new X500Name(dn).getRDNs(BCStyle.CN)[0].getFirst().getValue());
+ certBuilder.addExtension(Extension.subjectAlternativeName, false, new GeneralNames(new GeneralName(GeneralName.dNSName, cn)));
Review comment:
Most uses of common name use it as a DNS name, but some test classes use a generic description instead of a DNS name for the common name field. Although it may not cover every use case, introducing a simple regular expression pattern to check for what appears to be a valid hostname could be used to determine whether or not to add the SAN. The pattern should support unqualified hostnames, like `localhost` as well as qualified names like `localhost.localdomain`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org