From b6b0713c3534ebcbe1c0d959ccc2555ad11d16a3 Mon Sep 17 00:00:00 2001 From: Kit Haines Date: Wed, 25 Jan 2023 13:13:54 -0500 Subject: [PATCH] Vault 9406 enablement certs need userid handling in role (#18397) * The fields. * UserID set, add to certificate * Changelog. * Fix test (set default). * Add UserID constant to certutil, revert extension changes Signed-off-by: Alexander Scheel * Add user_ids as field for leaf signing Presumably, this isn't necessary for CAs, given that CAs probably don't have a user ID corresponding to them. Signed-off-by: Alexander Scheel * Support setting multiple user_ids in Subject Signed-off-by: Alexander Scheel * Allow any User ID with sign-verbatim Signed-off-by: Alexander Scheel * Add tests for User IDs in PKI Signed-off-by: Alexander Scheel * Add docs about user_ids, allowed_user_ids Signed-off-by: Alexander Scheel Signed-off-by: Alexander Scheel Co-authored-by: Alexander Scheel --- builtin/logical/pki/backend_test.go | 257 ++++++++++++++++++++++++ builtin/logical/pki/ca_util.go | 1 + builtin/logical/pki/cert_util.go | 62 ++++++ builtin/logical/pki/fields.go | 10 + builtin/logical/pki/path_issue_sign.go | 1 + builtin/logical/pki/path_roles.go | 9 + changelog/18397.txt | 3 + sdk/helper/certutil/types.go | 3 + website/content/api-docs/secret/pki.mdx | 19 ++ 9 files changed, 365 insertions(+) create mode 100644 changelog/18397.txt diff --git a/builtin/logical/pki/backend_test.go b/builtin/logical/pki/backend_test.go index f542048e43..da04223456 100644 --- a/builtin/logical/pki/backend_test.go +++ b/builtin/logical/pki/backend_test.go @@ -3621,6 +3621,7 @@ func TestReadWriteDeleteRoles(t *testing.T) { "code_signing_flag": false, "issuer_ref": "default", "cn_validations": []interface{}{"email", "hostname"}, + "allowed_user_ids": []interface{}{}, } if diff := deep.Equal(expectedData, resp.Data); len(diff) > 0 { @@ -6082,6 +6083,262 @@ func TestPKI_TemplatedAIAs(t *testing.T) { require.Error(t, err) } +func requireSubjectUserIDAttr(t *testing.T, cert string, target string) { + xCert := parseCert(t, cert) + + for _, attr := range xCert.Subject.Names { + var userID string + if attr.Type.Equal(certutil.SubjectPilotUserIDAttributeOID) { + if target == "" { + t.Fatalf("expected no UserID (OID: %v) subject attributes in cert:\n%v", certutil.SubjectPilotUserIDAttributeOID, cert) + } + + switch aValue := attr.Value.(type) { + case string: + userID = aValue + case []byte: + userID = string(aValue) + default: + t.Fatalf("unknown type for UserID attribute: %v\nCert: %v", attr, cert) + } + + if userID == target { + return + } + } + } + + if target != "" { + t.Fatalf("failed to find UserID (OID: %v) matching %v in cert:\n%v", certutil.SubjectPilotUserIDAttributeOID, target, cert) + } +} + +func TestUserIDsInLeafCerts(t *testing.T) { + t.Parallel() + b, s := CreateBackendWithStorage(t) + + // 1. Setup root issuer. + resp, err := CBWrite(b, s, "root/generate/internal", map[string]interface{}{ + "common_name": "Vault Root CA", + "key_type": "ec", + "ttl": "7200h", + }) + requireSuccessNonNilResponse(t, resp, err, "failed generating root issuer") + + // 2. Allow no user IDs. + resp, err = CBWrite(b, s, "roles/testing", map[string]interface{}{ + "allowed_user_ids": "", + "key_type": "ec", + }) + requireSuccessNonNilResponse(t, resp, err, "failed setting up role") + + // - Issue cert without user IDs should work. + resp, err = CBWrite(b, s, "issue/testing", map[string]interface{}{ + "common_name": "localhost", + }) + requireSuccessNonNilResponse(t, resp, err, "failed issuing leaf cert") + requireSubjectUserIDAttr(t, resp.Data["certificate"].(string), "") + + // - Issue cert with user ID should fail. + resp, err = CBWrite(b, s, "issue/testing", map[string]interface{}{ + "common_name": "localhost", + "user_ids": "humanoid", + }) + require.Error(t, err) + require.True(t, resp.IsError()) + + // 3. Allow any user IDs. + resp, err = CBWrite(b, s, "roles/testing", map[string]interface{}{ + "allowed_user_ids": "*", + "key_type": "ec", + }) + requireSuccessNonNilResponse(t, resp, err, "failed setting up role") + + // - Issue cert without user IDs. + resp, err = CBWrite(b, s, "issue/testing", map[string]interface{}{ + "common_name": "localhost", + }) + requireSuccessNonNilResponse(t, resp, err, "failed issuing leaf cert") + requireSubjectUserIDAttr(t, resp.Data["certificate"].(string), "") + + // - Issue cert with one user ID. + resp, err = CBWrite(b, s, "issue/testing", map[string]interface{}{ + "common_name": "localhost", + "user_ids": "humanoid", + }) + requireSuccessNonNilResponse(t, resp, err, "failed issuing leaf cert") + requireSubjectUserIDAttr(t, resp.Data["certificate"].(string), "humanoid") + + // - Issue cert with two user IDs. + resp, err = CBWrite(b, s, "issue/testing", map[string]interface{}{ + "common_name": "localhost", + "user_ids": "humanoid,robot", + }) + requireSuccessNonNilResponse(t, resp, err, "failed issuing leaf cert") + requireSubjectUserIDAttr(t, resp.Data["certificate"].(string), "humanoid") + requireSubjectUserIDAttr(t, resp.Data["certificate"].(string), "robot") + + // 4. Allow one specific user ID. + resp, err = CBWrite(b, s, "roles/testing", map[string]interface{}{ + "allowed_user_ids": "humanoid", + "key_type": "ec", + }) + requireSuccessNonNilResponse(t, resp, err, "failed setting up role") + + // - Issue cert without user IDs. + resp, err = CBWrite(b, s, "issue/testing", map[string]interface{}{ + "common_name": "localhost", + }) + requireSuccessNonNilResponse(t, resp, err, "failed issuing leaf cert") + requireSubjectUserIDAttr(t, resp.Data["certificate"].(string), "") + + // - Issue cert with approved ID. + resp, err = CBWrite(b, s, "issue/testing", map[string]interface{}{ + "common_name": "localhost", + "user_ids": "humanoid", + }) + requireSuccessNonNilResponse(t, resp, err, "failed issuing leaf cert") + requireSubjectUserIDAttr(t, resp.Data["certificate"].(string), "humanoid") + + // - Issue cert with non-approved user ID should fail. + resp, err = CBWrite(b, s, "issue/testing", map[string]interface{}{ + "common_name": "localhost", + "user_ids": "robot", + }) + require.Error(t, err) + require.True(t, resp.IsError()) + + // - Issue cert with one approved and one non-approved should also fail. + resp, err = CBWrite(b, s, "issue/testing", map[string]interface{}{ + "common_name": "localhost", + "user_ids": "humanoid,robot", + }) + require.Error(t, err) + require.True(t, resp.IsError()) + + // 5. Allow two specific user IDs. + resp, err = CBWrite(b, s, "roles/testing", map[string]interface{}{ + "allowed_user_ids": "humanoid,robot", + "key_type": "ec", + }) + requireSuccessNonNilResponse(t, resp, err, "failed setting up role") + + // - Issue cert without user IDs. + resp, err = CBWrite(b, s, "issue/testing", map[string]interface{}{ + "common_name": "localhost", + }) + requireSuccessNonNilResponse(t, resp, err, "failed issuing leaf cert") + requireSubjectUserIDAttr(t, resp.Data["certificate"].(string), "") + + // - Issue cert with one approved ID. + resp, err = CBWrite(b, s, "issue/testing", map[string]interface{}{ + "common_name": "localhost", + "user_ids": "humanoid", + }) + requireSuccessNonNilResponse(t, resp, err, "failed issuing leaf cert") + requireSubjectUserIDAttr(t, resp.Data["certificate"].(string), "humanoid") + + // - Issue cert with other user ID. + resp, err = CBWrite(b, s, "issue/testing", map[string]interface{}{ + "common_name": "localhost", + "user_ids": "robot", + }) + requireSuccessNonNilResponse(t, resp, err, "failed issuing leaf cert") + requireSubjectUserIDAttr(t, resp.Data["certificate"].(string), "robot") + + // - Issue cert with unknown user ID will fail. + resp, err = CBWrite(b, s, "issue/testing", map[string]interface{}{ + "common_name": "localhost", + "user_ids": "robot2", + }) + require.Error(t, err) + require.True(t, resp.IsError()) + + // - Issue cert with both should succeed. + resp, err = CBWrite(b, s, "issue/testing", map[string]interface{}{ + "common_name": "localhost", + "user_ids": "humanoid,robot", + }) + requireSuccessNonNilResponse(t, resp, err, "failed issuing leaf cert") + requireSubjectUserIDAttr(t, resp.Data["certificate"].(string), "humanoid") + requireSubjectUserIDAttr(t, resp.Data["certificate"].(string), "robot") + + // 6. Use a glob. + resp, err = CBWrite(b, s, "roles/testing", map[string]interface{}{ + "allowed_user_ids": "human*", + "key_type": "ec", + "use_csr_sans": true, // setup for further testing. + }) + requireSuccessNonNilResponse(t, resp, err, "failed setting up role") + + // - Issue cert without user IDs. + resp, err = CBWrite(b, s, "issue/testing", map[string]interface{}{ + "common_name": "localhost", + }) + requireSuccessNonNilResponse(t, resp, err, "failed issuing leaf cert") + requireSubjectUserIDAttr(t, resp.Data["certificate"].(string), "") + + // - Issue cert with approved ID. + resp, err = CBWrite(b, s, "issue/testing", map[string]interface{}{ + "common_name": "localhost", + "user_ids": "humanoid", + }) + requireSuccessNonNilResponse(t, resp, err, "failed issuing leaf cert") + requireSubjectUserIDAttr(t, resp.Data["certificate"].(string), "humanoid") + + // - Issue cert with another approved ID. + resp, err = CBWrite(b, s, "issue/testing", map[string]interface{}{ + "common_name": "localhost", + "user_ids": "human", + }) + requireSuccessNonNilResponse(t, resp, err, "failed issuing leaf cert") + requireSubjectUserIDAttr(t, resp.Data["certificate"].(string), "human") + + // - Issue cert with literal glob. + resp, err = CBWrite(b, s, "issue/testing", map[string]interface{}{ + "common_name": "localhost", + "user_ids": "human*", + }) + requireSuccessNonNilResponse(t, resp, err, "failed issuing leaf cert") + requireSubjectUserIDAttr(t, resp.Data["certificate"].(string), "human*") + + // - Still no robotic certs are allowed; will fail. + resp, err = CBWrite(b, s, "issue/testing", map[string]interface{}{ + "common_name": "localhost", + "user_ids": "robot", + }) + require.Error(t, err) + require.True(t, resp.IsError()) + + // Create a CSR and validate it works with both sign/ and sign-verbatim. + csrTemplate := x509.CertificateRequest{ + Subject: pkix.Name{ + CommonName: "localhost", + ExtraNames: []pkix.AttributeTypeAndValue{ + { + Type: certutil.SubjectPilotUserIDAttributeOID, + Value: "humanoid", + }, + }, + }, + } + _, _, csrPem := generateCSR(t, &csrTemplate, "ec", 256) + + // Should work with role-based signing. + resp, err = CBWrite(b, s, "sign/testing", map[string]interface{}{ + "csr": csrPem, + }) + requireSuccessNonNilResponse(t, resp, err, "failed issuing leaf cert") + requireSubjectUserIDAttr(t, resp.Data["certificate"].(string), "humanoid") + + // - Definitely will work with sign-verbatim. + resp, err = CBWrite(b, s, "sign-verbatim", map[string]interface{}{ + "csr": csrPem, + }) + requireSuccessNonNilResponse(t, resp, err, "failed issuing leaf cert") + requireSubjectUserIDAttr(t, resp.Data["certificate"].(string), "humanoid") +} + var ( initTest sync.Once rsaCAKey string diff --git a/builtin/logical/pki/ca_util.go b/builtin/logical/pki/ca_util.go index 018e0fff98..c7dd02299a 100644 --- a/builtin/logical/pki/ca_util.go +++ b/builtin/logical/pki/ca_util.go @@ -58,6 +58,7 @@ func getGenerationParams(sc *storageContext, data *framework.FieldData) (exporte AllowedURISANs: []string{"*"}, AllowedOtherSANs: []string{"*"}, AllowedSerialNumbers: []string{"*"}, + AllowedUserIDs: []string{"*"}, OU: data.Get("ou").([]string), Organization: data.Get("organization").([]string), Country: data.Get("country").([]string), diff --git a/builtin/logical/pki/cert_util.go b/builtin/logical/pki/cert_util.go index 9a388233e3..22d045fb9c 100644 --- a/builtin/logical/pki/cert_util.go +++ b/builtin/logical/pki/cert_util.go @@ -642,6 +642,33 @@ func parseOtherSANs(others []string) (map[string][]string, error) { return result, nil } +// Returns bool stating whether the given UserId is Valid +func validateUserId(data *inputBundle, userId string) bool { + allowedList := data.role.AllowedUserIDs + + if len(allowedList) == 0 { + // Nothing is allowed. + return false + } + + if strutil.StrListContainsCaseInsensitive(allowedList, userId) { + return true + } + + for _, rolePattern := range allowedList { + if rolePattern == "" { + continue + } + + if strings.Contains(rolePattern, "*") && glob.Glob(rolePattern, userId) { + return true + } + } + + // No matches. + return false +} + func validateSerialNumber(data *inputBundle, serialNumber string) string { valid := false if len(data.role.AllowedSerialNumbers) > 0 { @@ -1391,6 +1418,41 @@ func generateCreationBundle(b *backend, data *inputBundle, caSign *certutil.CAIn } } + // Add UserIDs into the Subject, if the request type supports it. + if _, present := data.apiData.Schema["user_ids"]; present { + rawUserIDs := data.apiData.Get("user_ids").([]string) + + // Only take UserIDs from CSR if one was not supplied via API. + if len(rawUserIDs) == 0 && csr != nil { + for _, attr := range csr.Subject.Names { + if attr.Type.Equal(certutil.SubjectPilotUserIDAttributeOID) { + switch aValue := attr.Value.(type) { + case string: + rawUserIDs = append(rawUserIDs, aValue) + case []byte: + rawUserIDs = append(rawUserIDs, string(aValue)) + default: + return nil, nil, errutil.UserError{Err: "unknown type for user_id attribute in CSR's Subject"} + } + } + } + } + + // Check for bad userIDs and add to the subject. + if len(rawUserIDs) > 0 { + for _, value := range rawUserIDs { + if !validateUserId(data, value) { + return nil, nil, errutil.UserError{Err: fmt.Sprintf("user_id %v is not allowed by this role", value)} + } + + subject.ExtraNames = append(subject.ExtraNames, pkix.AttributeTypeAndValue{ + Type: certutil.SubjectPilotUserIDAttributeOID, + Value: value, + }) + } + } + } + creation := &certutil.CreationBundle{ Params: &certutil.CreationParameters{ Subject: subject, diff --git a/builtin/logical/pki/fields.go b/builtin/logical/pki/fields.go index 1f3022ab42..695c8ed781 100644 --- a/builtin/logical/pki/fields.go +++ b/builtin/logical/pki/fields.go @@ -152,6 +152,16 @@ The value format should be given in UTC format YYYY-MM-ddTHH:MM:SSZ`, of the ca_chain field.`, } + fields["user_ids"] = &framework.FieldSchema{ + Type: framework.TypeCommaStringSlice, + Description: `The requested user_ids value to place in the subject, +if any, in a comma-delimited list. Restricted by allowed_user_ids. +Any values are added with OID 0.9.2342.19200300.100.1.1.`, + DisplayAttrs: &framework.DisplayAttributes{ + Name: "User ID(s)", + }, + } + fields = addIssuerRefField(fields) return fields diff --git a/builtin/logical/pki/path_issue_sign.go b/builtin/logical/pki/path_issue_sign.go index 7203d56c73..ccb1251b63 100644 --- a/builtin/logical/pki/path_issue_sign.go +++ b/builtin/logical/pki/path_issue_sign.go @@ -214,6 +214,7 @@ func (b *backend) pathSignVerbatim(ctx context.Context, req *logical.Request, da AllowedOtherSANs: []string{"*"}, AllowedSerialNumbers: []string{"*"}, AllowedURISANs: []string{"*"}, + AllowedUserIDs: []string{"*"}, CNValidations: []string{"disabled"}, GenerateLease: new(bool), KeyUsage: data.Get("key_usage").([]string), diff --git a/builtin/logical/pki/path_roles.go b/builtin/logical/pki/path_roles.go index 93d0a53baa..9137f2b9fe 100644 --- a/builtin/logical/pki/path_roles.go +++ b/builtin/logical/pki/path_roles.go @@ -180,6 +180,11 @@ Any valid URI is accepted, these values support globbing.`, Description: `If set, an array of allowed serial numbers to put in Subject. These values support globbing.`, }, + "allowed_user_ids": { + Type: framework.TypeCommaStringSlice, + Description: `If set, an array of allowed user-ids to put in user system login name specified here: https://www.rfc-editor.org/rfc/rfc1274#section-9.3.1`, + }, + "server_flag": { Type: framework.TypeBool, Default: true, @@ -698,6 +703,7 @@ func (b *backend) pathRoleCreate(ctx context.Context, req *logical.Request, data RequireCN: data.Get("require_cn").(bool), CNValidations: data.Get("cn_validations").([]string), AllowedSerialNumbers: data.Get("allowed_serial_numbers").([]string), + AllowedUserIDs: data.Get("allowed_user_ids").([]string), PolicyIdentifiers: getPolicyIdentifier(data, nil), BasicConstraintsValidForNonCA: data.Get("basic_constraints_valid_for_non_ca").(bool), NotBeforeDuration: time.Duration(data.Get("not_before_duration").(int)) * time.Second, @@ -896,6 +902,7 @@ func (b *backend) pathRolePatch(ctx context.Context, req *logical.Request, data RequireCN: getWithExplicitDefault(data, "require_cn", oldEntry.RequireCN).(bool), CNValidations: getWithExplicitDefault(data, "cn_validations", oldEntry.CNValidations).([]string), AllowedSerialNumbers: getWithExplicitDefault(data, "allowed_serial_numbers", oldEntry.AllowedSerialNumbers).([]string), + AllowedUserIDs: getWithExplicitDefault(data, "allowed_user_ids", oldEntry.AllowedUserIDs).([]string), PolicyIdentifiers: getPolicyIdentifier(data, &oldEntry.PolicyIdentifiers), BasicConstraintsValidForNonCA: getWithExplicitDefault(data, "basic_constraints_valid_for_non_ca", oldEntry.BasicConstraintsValidForNonCA).(bool), NotBeforeDuration: getTimeWithExplicitDefault(data, "not_before_duration", oldEntry.NotBeforeDuration), @@ -1098,6 +1105,7 @@ type roleEntry struct { CNValidations []string `json:"cn_validations"` AllowedOtherSANs []string `json:"allowed_other_sans"` AllowedSerialNumbers []string `json:"allowed_serial_numbers"` + AllowedUserIDs []string `json:"allowed_user_ids"` AllowedURISANs []string `json:"allowed_uri_sans"` AllowedURISANsTemplate bool `json:"allowed_uri_sans_template"` PolicyIdentifiers []string `json:"policy_identifiers"` @@ -1147,6 +1155,7 @@ func (r *roleEntry) ToResponseData() map[string]interface{} { "no_store": r.NoStore, "allowed_other_sans": r.AllowedOtherSANs, "allowed_serial_numbers": r.AllowedSerialNumbers, + "allowed_user_ids": r.AllowedUserIDs, "allowed_uri_sans": r.AllowedURISANs, "require_cn": r.RequireCN, "cn_validations": r.CNValidations, diff --git a/changelog/18397.txt b/changelog/18397.txt new file mode 100644 index 0000000000..aafb9d71e6 --- /dev/null +++ b/changelog/18397.txt @@ -0,0 +1,3 @@ +```release-note:improvement +secrets/pki: Allow UserID Field (https://www.rfc-editor.org/rfc/rfc1274#section-9.3.1) to be set on Certificates when +allowed by role``` diff --git a/sdk/helper/certutil/types.go b/sdk/helper/certutil/types.go index 15b816f0c8..8a806c6f86 100644 --- a/sdk/helper/certutil/types.go +++ b/sdk/helper/certutil/types.go @@ -1013,3 +1013,6 @@ func CreatePolicyInformationExtensionFromStorageStrings(policyIdentifiers []stri Value: asn1Bytes, }, nil } + +// Subject Attribute OIDs +var SubjectPilotUserIDAttributeOID = asn1.ObjectIdentifier{0, 9, 2342, 19200300, 100, 1, 1} diff --git a/website/content/api-docs/secret/pki.mdx b/website/content/api-docs/secret/pki.mdx index d7edaf7123..79b4fe0d83 100644 --- a/website/content/api-docs/secret/pki.mdx +++ b/website/content/api-docs/secret/pki.mdx @@ -296,6 +296,11 @@ It is suggested to limit access to the path-overridden issue endpoint (on `YYYY-MM-ddTHH:MM:SSZ`. Supports the Y10K end date for IEEE 802.1AR-2018 standard devices, `9999-12-31T23:59:59Z`. +- `user_ids` `(string: "")` - Specifies the comma-separated list of requested + User ID (OID 0.9.2342.19200300.100.1.1) Subject values to be placed on the + signed certificate. This field is validated against `allowed_user_ids` on + the role. + #### Sample Payload ```json @@ -417,6 +422,11 @@ It is suggested to limit access to the path-overridden sign endpoint (on field will not include any self-signed CA certificates. Useful if end-users already have the root CA in their trust store. +- `user_ids` `(string: "")` - Specifies the comma-separated list of requested + User ID (OID 0.9.2342.19200300.100.1.1) Subject values to be placed on the + signed certificate. This field is validated against `allowed_user_ids` on + the role. + #### Sample Payload ```json @@ -802,6 +812,10 @@ have access.** field will not include any self-signed CA certificates. Useful if end-users already have the root CA in their trust store. +- `user_ids` `(string: "")` - Specifies the comma-separated list of requested + User ID (OID 0.9.2342.19200300.100.1.1) Subject values to be placed on the + signed certificate. No validation on names is performed using this endpoint. + #### Sample Payload ```json @@ -2816,6 +2830,11 @@ request is denied. correctness validation around email addresses and domain names). This allows non-standard CNs to be used verbatim from the request. +- `allowed_user_ids` `(string: "")` - Comma separated, globbing list of User ID + Subject components to allow on requests. By default, no user IDs are allowed. + Use the bare wildcard `*` value to allow any value. See also the `user_ids` + request parameter. + #### Sample Payload ```json