shred
commented
1 year ago Thank you for the MR!
I agree that the CN should be optional only. However, there are two minor problems left.
The CSRBuilder.addDomain() javadoc still says that: "The first domain name added will also be the Common Name." The change is breaking this contract. The correct way would be to add a new method like addDomainNoCN() and deprecate the addDomain() method. However I think it would cause more confusion than it helps. It's better to just remove that sentence from the javadocs, since the CN is ignored by CAs anyway.
Also, two CSRBuilderTest unit tests are currently failing because DNS=abc.de is now present twice in the SAN, once from builder.addDomain("abc.de") and once from builder.setCommonName("abc.de"). I think setCommonsName() should not add the value as domain, like the current implementation does. I would propose something like this instead:
public void setCommonName(String cn) {
namebuilder.addRDN(BCStyle.CN, requireNonNull(cn));
}It would then be in line to setOrganization() etc and would have no surprising side effect.
mcpherrinm
This change makes the common name optional in CSRs.
The CN field is length-limited to 64 characters, so trying to request a certificate with the first (or all) domains longer than 64 characters creates an invalid CSR. This isn't the only way to solve this problem, but it's arguably the future-proof option.
The CN, at least for Let's Encrypt, isn't required to be in the CSR at all. The first eligible SAN is automatically promoted by LE.
As well, the Common Name is "not recommended" by the current baseline requirements. It may be removed from all certificates in the future.
One weird side affect, as documented by the final test change, is that you can set two CNs now, which doesn't seem ideal, but maybe isn't worth solving either, as I don't know why anyone would do that.