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:
[email protected]
