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 2021/01/04 13:20:21 UTC
[GitHub] [nifi] exceptionfactory commented on a change in pull request #4709: NIFI-7783: Include CA CN as a SAN entry
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