-
Notifications
You must be signed in to change notification settings - Fork 4.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
connect/ca: cease including the common name field in generated certs #10424
Conversation
@@ -0,0 +1,122 @@ | |||
package connect |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can all go away with golang/go#46810
Parts of this can be cleaned up after golang/go#46810 (or something like it) lands. |
@@ -487,8 +471,12 @@ func (c *ConsulProvider) SignIntermediate(csr *x509.CertificateRequest) (string, | |||
effectiveNow := time.Now().Add(-1 * CertificateTimeDriftBuffer) | |||
template := x509.Certificate{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This blob threw me until I found it.
This spot in the code accepts a CSR but does not pass that CSR off to the crypto/x509
package or anything of the sort. The incoming CSR has select fields plucked out of it to assign to the newly generated intermediate cert.
I'd done an overmatch during cleanup and stripped the Subject: csr.Subject,
line from this function which super broke intermediate certs (which only put data in the Subject/CN field). For now I modified this to handle this more like how a CSR should operate and copy the various SAN fields along with the Subject and any provided ExtraExtensions as-is.
We can discuss if the CA certs themselves should be using SAN fields, too, but the CN fields for those things are less awful to construct so it's fine to keep them for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, if we're not going to use CN everywhere else, it probably makes sense to stop using it for the CA and reflect that in the TLS utilities. Is there a reason not to?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AWS PCA requires a CN field to be populated in order to create the Root CA. From what I can tell you can't set SAN fields on the root CA.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting! Seems to align with that I'm finding in https://datatracker.ietf.org/doc/html/rfc5280#section-4.1.2.6 and https://datatracker.ietf.org/doc/html/rfc5280#section-4.1.2.4 where the Subject/CN is required.
It might be nice to document the reasoning for "CN only in the CA", just not sure where.
@@ -487,8 +471,12 @@ func (c *ConsulProvider) SignIntermediate(csr *x509.CertificateRequest) (string, | |||
effectiveNow := time.Now().Add(-1 * CertificateTimeDriftBuffer) | |||
template := x509.Certificate{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, if we're not going to use CN everywhere else, it probably makes sense to stop using it for the CA and reflect that in the TLS utilities. Is there a reason not to?
hashicorp#10424 introduced a regression, because it removed common name also for secondary CA intermediate certificate, which used the same code path. This violates RFC 5280, which mandates that Issuer CN must not be empty and equal to Subject CN of subordinate certificate. Partially revert this patch to fix consul provider, and add a test case to prevent future regressions.
hashicorp#10424 introduced a regression, because it removed common name also for secondary CA intermediate certificate, which used the same code path. This violates RFC 5280, which mandates that Issuer CN must not be empty and equal to Subject CN of subordinate certificate. Partially revert this patch to fix consul provider, and add a test case to prevent future regressions.
hashicorp#10424 introduced a regression, because it removed common name also for secondary CA intermediate certificate, which used the same code path. This violates RFC 5280, which mandates that Issuer CN must not be empty and equal to Subject CN of subordinate certificate. Partially revert this patch to fix consul provider, and add a test case to prevent future regressions.
hashicorp#10424 introduced a regression, because it removed common name also for secondary CA intermediate certificate, which used the same code path. This violates RFC 5280, which mandates that Issuer CN must not be empty and equal to Subject CN of subordinate certificate. Partially revert this patch to fix consul provider, and add a test case to prevent future regressions.
hashicorp#10424 introduced a regression, because it removed common name also for secondary CA intermediate certificate, which used the same code path. This violates RFC 5280, which mandates that Issuer CN must not be empty and equal to Subject CN of subordinate certificate. Partially revert this patch to fix consul provider, and add a test case to prevent future regressions.
hashicorp#10424 introduced a regression, because it removed common name also for secondary CA intermediate certificate, which used the same code path. This violates RFC 5280, which mandates that Issuer CN must not be empty and equal to Subject CN of subordinate certificate. Partially revert this patch to fix consul provider, and add a test case to prevent future regressions.
hashicorp#10424 introduced a regression, because it removed common name also for secondary CA intermediate certificate, which used the same code path. This violates RFC 5280, which mandates that Issuer CN must not be empty and equal to Subject CN of subordinate certificate. Partially revert this patch to fix consul provider, and add a test case to prevent future regressions.
hashicorp#10424 introduced a regression, because it removed common name also for secondary CA intermediate certificate, which used the same code path. This violates RFC 5280, which mandates that Issuer CN must not be empty and equal to Subject CN of subordinate certificate. Partially revert this patch to fix consul provider, and add a test case to prevent future regressions.
hashicorp#10424 introduced a regression, because it removed common name also for secondary CA intermediate certificate, which used the same code path. This violates RFC 5280, which mandates that Issuer CN must not be empty and equal to Subject CN of subordinate certificate. Partially revert this patch to fix consul provider, and add a test case to prevent future regressions.
hashicorp#10424 introduced a regression, because it removed common name also for secondary CA intermediate certificate, which used the same code path. This violates RFC 5280, which mandates that Issuer CN must not be empty and equal to Subject CN of subordinate certificate. Partially revert this patch to fix consul provider, and add a test case to prevent future regressions.
hashicorp#10424 introduced a regression, because it removed common name also for secondary CA intermediate certificate, which used the same code path. This violates RFC 5280, which mandates that Issuer CN must not be empty and equal to Subject CN of subordinate certificate. Partially revert this patch to fix consul provider, and add a test case to prevent future regressions.
hashicorp#10424 introduced a regression, because it removed common name also for secondary CA intermediate certificate, which used the same code path. This violates RFC 5280, which mandates that Issuer CN must not be empty and equal to Subject CN of subordinate certificate. Partially revert this patch to fix consul provider, and add a test case to prevent future regressions.
hashicorp#10424 introduced a regression, because it removed common name also for secondary CA intermediate certificate, which used the same code path. This violates RFC 5280, which mandates that Issuer CN must not be empty and equal to Subject CN of subordinate certificate. Partially revert this patch to fix consul provider, and add a test case to prevent future regressions.
hashicorp#10424 introduced a regression, because it removed common name also for secondary CA intermediate certificate, which used the same code path. This violates RFC 5280, which mandates that Issuer CN must not be empty and equal to Subject CN of subordinate certificate. Partially revert this patch to fix consul provider, and add a test case to prevent future regressions.
hashicorp#10424 introduced a regression, because it removed common name also for secondary CA intermediate certificate, which used the same code path. This violates RFC 5280, which mandates that Issuer CN must not be empty and equal to Subject CN of subordinate certificate. Partially revert this patch to fix consul provider, and add a test case to prevent future regressions.
hashicorp#10424 introduced a regression, because it removed common name also for secondary CA intermediate certificate, which used the same code path. This violates RFC 5280, which mandates that Issuer CN must not be empty and equal to Subject CN of subordinate certificate. Partially revert this patch to fix consul provider, and add a test case to prevent future regressions.
hashicorp#10424 introduced a regression, because it removed common name also for secondary CA intermediate certificate, which used the same code path. This violates RFC 5280, which mandates that Issuer CN must not be empty and equal to Subject CN of subordinate certificate. Partially revert this patch to fix consul provider, and add a test case to prevent future regressions.
hashicorp#10424 introduced a regression, because it removed common name also for secondary CA intermediate certificate, which used the same code path. This violates RFC 5280, which mandates that Issuer CN must not be empty and equal to Subject CN of subordinate certificate. Partially revert this patch to fix consul provider, and add a test case to prevent future regressions.
hashicorp#10424 introduced a regression, because it removed common name also for secondary CA intermediate certificate, which used the same code path. This violates RFC 5280, which mandates that Issuer CN must not be empty and equal to Subject CN of subordinate certificate. Partially revert this patch to fix consul provider, and add a test case to prevent future regressions.
hashicorp#10424 introduced a regression, because it removed common name also for secondary CA intermediate certificate, which used the same code path. This violates RFC 5280, which mandates that Issuer CN must not be empty and equal to Subject CN of subordinate certificate. Partially revert this patch to fix consul provider, and add a test case to prevent future regressions.
hashicorp#10424 introduced a regression, because it removed common name also for secondary CA intermediate certificate, which used the same code path. This violates RFC 5280, which mandates that Issuer CN must not be empty and equal to Subject CN of subordinate certificate. Partially revert this patch to fix consul provider, and add a test case to prevent future regressions.
hashicorp#10424 introduced a regression, because it removed common name also for secondary CA intermediate certificate, which used the same code path. This violates RFC 5280, which mandates that Issuer CN must not be empty and equal to Subject CN of subordinate certificate. Partially revert this patch to fix consul provider, and add a test case to prevent future regressions.
hashicorp#10424 introduced a regression, because it removed common name also for secondary CA intermediate certificate, which used the same code path. This violates RFC 5280, which mandates that Issuer CN must not be empty and equal to Subject CN of subordinate certificate. Partially revert this patch to fix consul provider, and add a test case to prevent future regressions.
hashicorp#10424 introduced a regression, because it removed common name also for secondary CA intermediate certificate, which used the same code path. This violates RFC 5280, which mandates that Issuer CN must not be empty and equal to Subject CN of subordinate certificate. Partially revert this patch to fix consul provider, and add a test case to prevent future regressions.
hashicorp#10424 introduced a regression, because it removed common name also for secondary CA intermediate certificate, which used the same code path. This violates RFC 5280, which mandates that Issuer CN must not be empty and equal to Subject CN of subordinate certificate. Partially revert this patch to fix consul provider, and add a test case to prevent future regressions.
hashicorp#10424 introduced a regression, because it removed common name also for secondary CA intermediate certificate, which used the same code path. This violates RFC 5280, which mandates that Issuer CN must not be empty and equal to Subject CN of subordinate certificate. Partially revert this patch to fix consul provider, and add a test case to prevent future regressions.
hashicorp#10424 introduced a regression, because it removed common name also for secondary CA intermediate certificate, which used the same code path. This violates RFC 5280, which mandates that Issuer CN must not be empty and equal to Subject CN of subordinate certificate. Partially revert this patch to fix consul provider, and add a test case to prevent future regressions.
hashicorp#10424 introduced a regression, because it removed common name also for secondary CA intermediate certificate, which used the same code path. This violates RFC 5280, which mandates that Issuer CN must not be empty and equal to Subject CN of subordinate certificate. Partially revert this patch to fix consul provider, and add a test case to prevent future regressions.
hashicorp#10424 introduced a regression, because it removed common name also for secondary CA intermediate certificate, which used the same code path. This violates RFC 5280, which mandates that Issuer CN must not be empty and equal to Subject CN of subordinate certificate. Partially revert this patch to fix consul provider, and add a test case to prevent future regressions.
As part of this change, we ensure that the SAN extensions are marked as
critical when the subject is empty so that AWS PCA tolerates the loss of
common names well and continues to function as a Connect CA provider.
Parts of this currently hack around a bug in crypto/x509 and can be
removed after https://go-review.googlesource.com/c/go/+/329129 lands in
a Go release.
Note: the AWS PCA tests do not run automatically, but the following
passed locally for me:
The only remaining place that we use a CommonName in certs is for CA certs themselves, since they don't get SAN fields.