Populate ExtraNames from Names #42
Conversation
This commit will populate the subject ExtraNames in a certificate request from the information present in Names as long as it is not one of the default fields. This way, we can extract non-default fields as well as populate them in the final certificate if necessary. It also uses the proper type for the deprecated email address attribute in the subject. Related to smallstep/certificates#916
x509util/name.go
Outdated
| // attributeTypeNames are the subject attributes managed by Go and this package. | ||
| // newDistinguisedNames will populate .Insecure.CR.ExtraNames with the | ||
| // attributes not present on this map. |
There was a problem hiding this comment.
Perhaps these should be called distinguishedNameAttributeNames to make clear that's what these are? They can be used for both Subject as well as Issuer names.
newDistinguisednames -> newDistinguishedNames. This typo is present several times.
There was a problem hiding this comment.
It's a copy&paste from Go's source code.
There was a problem hiding this comment.
I think it's the right name for this map.
| ExtraNames: []pkix.AttributeTypeAndValue{ | ||
| {Type: asn1.ObjectIdentifier{1, 2, 840, 113549, 1, 9, 1}, Value: "jane@example.com"}, | ||
| Names: []pkix.AttributeTypeAndValue{ | ||
| {Type: asn1.ObjectIdentifier{2, 5, 4, 3}, Value: "The commonName"}, |
There was a problem hiding this comment.
This instance of the The commonName is ignored, right? It may be clearer to use a different value than the one that's already set in CommonName, so that a change in the function will result in a different test outcome.
There was a problem hiding this comment.
I wanted to "imitate" a real case, Names will have all the attributes present in the subject C, O, OU, CN, ... I will add all of them to make sure the map is correct.
x509util/name.go
Outdated
| // newDistinguisedNames returns a list of DistinguishedName with the attributes not | ||
| // present in attributeTypeNames. | ||
| func newDistinguisedNames(atvs []pkix.AttributeTypeAndValue) []DistinguishedName { |
There was a problem hiding this comment.
Maybe the function name can reflect the fact that it excludes the attribute names? It might get overly long doing so though, like newDistinguishedNamesExcludingExistingAttributes. Don't have a better proposal atm 😅
There was a problem hiding this comment.
Fixed with c93d3e3 (newExtraNames)
x509util/name.go
Outdated
| @@ -35,7 +53,7 @@ func newName(n pkix.Name) Name { | |||
| PostalCode: n.PostalCode, | |||
| SerialNumber: n.SerialNumber, | |||
| CommonName: n.CommonName, | |||
| ExtraNames: newDistinguisedNames(n.ExtraNames), | |||
| ExtraNames: newDistinguisedNames(n.Names), | |||
There was a problem hiding this comment.
I think you kept the ExtraNames property for backwards compatibility? Should we maybe also provide the Names property, which could contain the same data? Or doesn't that make any sense from a user's viewpoint?
There was a problem hiding this comment.
The problem with that approach is that it will be more difficult to create a template directly from a CSR. You mentioned in smallstep/certificates#916 a template with "subject": {{ toJson .Insecure.CR.Subject }}, if we do your change, Go will ignore the values of Names when it creates a certificate, so the extra ones from the CSR won't be present in the certificate.
| @@ -35,7 +53,7 @@ func newName(n pkix.Name) Name { | |||
| PostalCode: n.PostalCode, | |||
| SerialNumber: n.SerialNumber, | |||
| CommonName: n.CommonName, | |||
| ExtraNames: newDistinguisedNames(n.ExtraNames), | |||
| ExtraNames: newExtraNames(n.Names), | |||
x509util/name.go
Outdated
| // fromDistinguisedNames converts a list of DistinguisedName to | ||
| // []pkix.AttributeTypeAndValue. Note that this method has a special case to | ||
| // encode the emailAddress deprecated field (1.2.840.113549.1.9.1). | ||
| func fromDistinguisedNames(dns []DistinguishedName) []pkix.AttributeTypeAndValue { |
There was a problem hiding this comment.
Needs update on the calling side too.
| // fromDistinguisedNames converts a list of DistinguisedName to | |
| // []pkix.AttributeTypeAndValue. Note that this method has a special case to | |
| // encode the emailAddress deprecated field (1.2.840.113549.1.9.1). | |
| func fromDistinguisedNames(dns []DistinguishedName) []pkix.AttributeTypeAndValue { | |
| // fromDistinguishedNames converts a list of DistinguishedName to | |
| // []pkix.AttributeTypeAndValue. Note that this method has a special case to | |
| // encode the deprecated emailAddress field (1.2.840.113549.1.9.1). | |
| func fromDistinguishedNames(dns []DistinguishedName) []pkix.AttributeTypeAndValue { |
Description
This commit will populate the subject ExtraNames in a certificate request from the information present in Names as long as it is not one of the default fields. This way, we can extract non-default fields as well as populate them in the final certificate if necessary.
It also uses the proper type for the deprecated email address attribute in the subject.
Related to smallstep/certificates#916