-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Add support for x.509 Name Serial Number attribute in subject of certificates #4694
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -498,6 +498,29 @@ func parseOtherSANs(others []string) (map[string][]string, error) { | |
return result, nil | ||
} | ||
|
||
func validateSerialNumber(data *dataBundle, serialNumber string) string { | ||
valid := false | ||
if len(data.role.AllowedSerialNumbers) > 0 { | ||
for _, currSerialNumber := range data.role.AllowedSerialNumbers { | ||
if currSerialNumber == "" { | ||
continue | ||
} | ||
|
||
if (strings.Contains(currSerialNumber, "*") && | ||
glob.Glob(currSerialNumber, serialNumber)) || | ||
currSerialNumber == serialNumber { | ||
valid = true | ||
break | ||
} | ||
} | ||
} | ||
if !valid { | ||
return serialNumber | ||
} else { | ||
return "" | ||
} | ||
} | ||
|
||
func generateCert(ctx context.Context, | ||
b *backend, | ||
data *dataBundle, | ||
|
@@ -695,18 +718,26 @@ func generateCreationBundle(b *backend, data *dataBundle) error { | |
|
||
// Read in names -- CN, DNS and email addresses | ||
var cn string | ||
var ridSerialNumber string | ||
dnsNames := []string{} | ||
emailAddresses := []string{} | ||
{ | ||
if data.csr != nil && data.role.UseCSRCommonName { | ||
cn = data.csr.Subject.CommonName | ||
ridSerialNumber = data.csr.Subject.SerialNumber | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sold on this change. The serial number isn't the common name, and that field isn't defined outside of purely the common name. |
||
} | ||
if cn == "" { | ||
cn = data.apiData.Get("common_name").(string) | ||
if cn == "" && data.role.RequireCN { | ||
return errutil.UserError{Err: `the common_name field is required, or must be provided in a CSR with "use_csr_common_name" set to true, unless "require_cn" is set to false`} | ||
} | ||
} | ||
if ridSerialNumber == "" { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like the check that you added but think the order should be reversed -- API-specified values should override CSR values. Part of the reason is that a lot of people are not careful when constructing CSRs and may pick the same value over and over (e.g. just set something in a template) or may not use a sufficiently random source if they aren't intending for it to be something specific. |
||
ridSerialNumberRaw, ok := data.apiData.GetOk("serial_number") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As it's a defined to be a string, you can actually just do a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was failing otherwise since it's not a field common to CA generation/signing. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Feels like it should be? I don't see a reason to not allow specifying the serial number when generating a CA. |
||
if ok { | ||
ridSerialNumber = ridSerialNumberRaw.(string) | ||
} | ||
} | ||
|
||
if data.csr != nil && data.role.UseCSRSANs { | ||
dnsNames = data.csr.DNSNames | ||
|
@@ -774,6 +805,14 @@ func generateCreationBundle(b *backend, data *dataBundle) error { | |
} | ||
} | ||
|
||
if ridSerialNumber != "" { | ||
badName := validateSerialNumber(data, ridSerialNumber) | ||
if len(badName) != 0 { | ||
return errutil.UserError{Err: fmt.Sprintf( | ||
"serial_number %s not allowed by this role", badName)} | ||
} | ||
} | ||
|
||
// Check for bad email and/or DNS names | ||
badName := validateNames(data, dnsNames) | ||
if len(badName) != 0 { | ||
|
@@ -845,6 +884,7 @@ func generateCreationBundle(b *backend, data *dataBundle) error { | |
|
||
subject := pkix.Name{ | ||
CommonName: cn, | ||
SerialNumber: ridSerialNumber, | ||
Country: strutil.RemoveDuplicates(data.role.Country, false), | ||
Organization: strutil.RemoveDuplicates(data.role.Organization, false), | ||
OrganizationalUnit: strutil.RemoveDuplicates(data.role.OU, false), | ||
|
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.
You're one of the first outside contributors to actually use NewTestCluster. Kudos!
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.
I gotta admit, most of this was copy/paste from existing tests 😳
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.
Yes, but, many people copy/paste from existing tests using framework.TestCase and friends, and...we don't like that. (It was a good idea at the time when Vault was very, very simple, and has not at all scaled.) So it's sad when someone comes with a whole ton of TestCase code and we have to decide whether to tell them to redo it all or continue feeding the beast.