Skip to content

Commit 2b7f693

Browse files
hslatmanhslatman
committed
Change Subject Common Name verification
Subject Common Names can now also be configured to be allowed or denied, similar to SANs. When a Subject Common Name is not explicitly allowed or denied, its type will be determined and its value will be validated according to the constraints for that type of name (i.e. URI).
1 parent 74a6e59 commit 2b7f693

File tree

14 files changed

+246
-245
lines changed

14 files changed

+246
-245
lines changed

‎api/read/read_test.go

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@ import (
44
"encoding/json"
55
"errors"
66
"io"
7-
"io/ioutil"
87
"net/http"
98
"net/http/httptest"
109
"reflect"
@@ -146,7 +145,7 @@ func Test_badProtoJSONError_Render(t *testing.T) {
146145
res := w.Result()
147146
defer res.Body.Close()
148147

149-
data, err := ioutil.ReadAll(res.Body)
148+
data, err := io.ReadAll(res.Body)
150149
assert.NoError(t, err)
151150

152151
v := struct {

‎authority/admin/api/policy_test.go

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -312,7 +312,6 @@ func TestPolicyAdminResponder_CreateAuthorityPolicy(t *testing.T) {
312312
Allow: &linkedca.X509Names{
313313
Dns: []string{"*.local"},
314314
},
315-
DisableSubjectCommonNameVerification: false,
316315
},
317316
}
318317
body, err := protojson.Marshal(policy)
@@ -1030,7 +1029,6 @@ func TestPolicyAdminResponder_CreateProvisionerPolicy(t *testing.T) {
10301029
Allow: &linkedca.X509Names{
10311030
Dns: []string{"*.local"},
10321031
},
1033-
DisableSubjectCommonNameVerification: false,
10341032
},
10351033
}
10361034
body, err := protojson.Marshal(policy)

‎authority/policy.go

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -288,6 +288,9 @@ func policyToCertificates(p *linkedca.Policy) *authPolicy.Options {
288288
if allow.Uris != nil {
289289
opts.X509.AllowedNames.URIDomains = allow.Uris
290290
}
291+
if allow.CommonNames != nil {
292+
opts.X509.AllowedNames.CommonNames = allow.CommonNames
293+
}
291294
}
292295
if deny := x509.GetDeny(); deny != nil {
293296
opts.X509.DeniedNames = &authPolicy.X509NameOptions{}
@@ -303,10 +306,12 @@ func policyToCertificates(p *linkedca.Policy) *authPolicy.Options {
303306
if deny.Uris != nil {
304307
opts.X509.DeniedNames.URIDomains = deny.Uris
305308
}
309+
if deny.CommonNames != nil {
310+
opts.X509.DeniedNames.CommonNames = deny.CommonNames
311+
}
306312
}
307313

308314
opts.X509.AllowWildcardLiteral = x509.AllowWildcardLiteral
309-
opts.X509.DisableCommonNameVerification = x509.DisableSubjectCommonNameVerification
310315
}
311316

312317
// fill ssh policy configuration

‎authority/policy/options.go

Lines changed: 3 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,6 @@ type X509PolicyOptionsInterface interface {
3131
GetAllowedNameOptions() *X509NameOptions
3232
GetDeniedNameOptions() *X509NameOptions
3333
IsWildcardLiteralAllowed() bool
34-
ShouldVerifyCommonName() bool
3534
}
3635

3736
// X509PolicyOptions is a container for x509 allowed and denied
@@ -47,15 +46,11 @@ type X509PolicyOptions struct {
4746
// such as *.example.com and @example.com are allowed. Defaults
4847
// to false.
4948
AllowWildcardLiteral bool `json:"allowWildcardLiteral,omitempty"`
50-
51-
// DisableCommonNameVerification indicates if the Subject Common Name
52-
// is verified in addition to the SANs. Defaults to false, resulting in
53-
// Common Names being verified.
54-
DisableCommonNameVerification bool `json:"disableCommonNameVerification,omitempty"`
5549
}
5650

5751
// X509NameOptions models the X509 name policy configuration.
5852
type X509NameOptions struct {
53+
CommonNames []string `json:"cn,omitempty"`
5954
DNSDomains []string `json:"dns,omitempty"`
6055
IPRanges []string `json:"ip,omitempty"`
6156
EmailAddresses []string `json:"email,omitempty"`
@@ -65,7 +60,8 @@ type X509NameOptions struct {
6560
// HasNames checks if the AllowedNameOptions has one or more
6661
// names configured.
6762
func (o *X509NameOptions) HasNames() bool {
68-
return len(o.DNSDomains) > 0 ||
63+
return len(o.CommonNames) > 0 ||
64+
len(o.DNSDomains) > 0 ||
6965
len(o.IPRanges) > 0 ||
7066
len(o.EmailAddresses) > 0 ||
7167
len(o.URIDomains) > 0
@@ -96,15 +92,6 @@ func (o *X509PolicyOptions) IsWildcardLiteralAllowed() bool {
9692
return o.AllowWildcardLiteral
9793
}
9894

99-
// ShouldVerifyCommonName returns whether the authority
100-
// should verify the Subject Common Name in addition to the SANs.
101-
func (o *X509PolicyOptions) ShouldVerifyCommonName() bool {
102-
if o == nil {
103-
return false
104-
}
105-
return !o.DisableCommonNameVerification
106-
}
107-
10895
// SSHPolicyOptionsInterface is an interface for providers of
10996
// SSH user and host name policy configuration.
11097
type SSHPolicyOptionsInterface interface {

‎authority/policy/options_test.go

Lines changed: 0 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -43,43 +43,3 @@ func TestX509PolicyOptions_IsWildcardLiteralAllowed(t *testing.T) {
4343
})
4444
}
4545
}
46-
47-
func TestX509PolicyOptions_ShouldVerifySubjectCommonName(t *testing.T) {
48-
tests := []struct {
49-
name string
50-
options *X509PolicyOptions
51-
want bool
52-
}{
53-
{
54-
name: "nil-options",
55-
options: nil,
56-
want: false,
57-
},
58-
{
59-
name: "not-set",
60-
options: &X509PolicyOptions{},
61-
want: true,
62-
},
63-
{
64-
name: "set-true",
65-
options: &X509PolicyOptions{
66-
DisableCommonNameVerification: true,
67-
},
68-
want: false,
69-
},
70-
{
71-
name: "set-false",
72-
options: &X509PolicyOptions{
73-
DisableCommonNameVerification: false,
74-
},
75-
want: true,
76-
},
77-
}
78-
for _, tt := range tests {
79-
t.Run(tt.name, func(t *testing.T) {
80-
if got := tt.options.ShouldVerifyCommonName(); got != tt.want {
81-
t.Errorf("X509PolicyOptions.ShouldVerifySubjectCommonName() = %v, want %v", got, tt.want)
82-
}
83-
})
84-
}
85-
}

‎authority/policy/policy.go

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@ func NewX509PolicyEngine(policyOptions X509PolicyOptionsInterface) (X509Policy,
2828
allowed := policyOptions.GetAllowedNameOptions()
2929
if allowed != nil && allowed.HasNames() {
3030
options = append(options,
31+
policy.WithPermittedCommonNames(allowed.CommonNames...),
3132
policy.WithPermittedDNSDomains(allowed.DNSDomains...),
3233
policy.WithPermittedIPsOrCIDRs(allowed.IPRanges...),
3334
policy.WithPermittedEmailAddresses(allowed.EmailAddresses...),
@@ -38,6 +39,7 @@ func NewX509PolicyEngine(policyOptions X509PolicyOptionsInterface) (X509Policy,
3839
denied := policyOptions.GetDeniedNameOptions()
3940
if denied != nil && denied.HasNames() {
4041
options = append(options,
42+
policy.WithExcludedCommonNames(denied.CommonNames...),
4143
policy.WithExcludedDNSDomains(denied.DNSDomains...),
4244
policy.WithExcludedIPsOrCIDRs(denied.IPRanges...),
4345
policy.WithExcludedEmailAddresses(denied.EmailAddresses...),
@@ -50,10 +52,6 @@ func NewX509PolicyEngine(policyOptions X509PolicyOptionsInterface) (X509Policy,
5052
return nil, nil
5153
}
5254

53-
if policyOptions.ShouldVerifyCommonName() {
54-
options = append(options, policy.WithSubjectCommonNameVerification())
55-
}
56-
5755
if policyOptions.IsWildcardLiteralAllowed() {
5856
options = append(options, policy.WithAllowLiteralWildcardNames())
5957
}

‎authority/policy_test.go

Lines changed: 23 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -218,17 +218,15 @@ func Test_policyToCertificates(t *testing.T) {
218218
Allow: &linkedca.X509Names{
219219
Dns: []string{"*.local"},
220220
},
221-
AllowWildcardLiteral: false,
222-
DisableSubjectCommonNameVerification: false,
221+
AllowWildcardLiteral: false,
223222
},
224223
},
225224
want: &policy.Options{
226225
X509: &policy.X509PolicyOptions{
227226
AllowedNames: &policy.X509NameOptions{
228227
DNSDomains: []string{"*.local"},
229228
},
230-
AllowWildcardLiteral: false,
231-
DisableCommonNameVerification: false,
229+
AllowWildcardLiteral: false,
232230
},
233231
},
234232
},
@@ -237,19 +235,20 @@ func Test_policyToCertificates(t *testing.T) {
237235
policy: &linkedca.Policy{
238236
X509: &linkedca.X509Policy{
239237
Allow: &linkedca.X509Names{
240-
Dns: []string{"step"},
241-
Ips: []string{"127.0.0.1/24"},
242-
Emails: []string{"*.example.com"},
243-
Uris: []string{"https://*.local"},
238+
Dns: []string{"step"},
239+
Ips: []string{"127.0.0.1/24"},
240+
Emails: []string{"*.example.com"},
241+
Uris: []string{"https://*.local"},
242+
CommonNames: []string{"some name"},
244243
},
245244
Deny: &linkedca.X509Names{
246-
Dns: []string{"bad"},
247-
Ips: []string{"127.0.0.30"},
248-
Emails: []string{"badhost.example.com"},
249-
Uris: []string{"https://badhost.local"},
245+
Dns: []string{"bad"},
246+
Ips: []string{"127.0.0.30"},
247+
Emails: []string{"badhost.example.com"},
248+
Uris: []string{"https://badhost.local"},
249+
CommonNames: []string{"another name"},
250250
},
251-
AllowWildcardLiteral: true,
252-
DisableSubjectCommonNameVerification: false,
251+
AllowWildcardLiteral: true,
253252
},
254253
Ssh: &linkedca.SSHPolicy{
255254
Host: &linkedca.SSHHostPolicy{
@@ -283,15 +282,16 @@ func Test_policyToCertificates(t *testing.T) {
283282
IPRanges: []string{"127.0.0.1/24"},
284283
EmailAddresses: []string{"*.example.com"},
285284
URIDomains: []string{"https://*.local"},
285+
CommonNames: []string{"some name"},
286286
},
287287
DeniedNames: &policy.X509NameOptions{
288288
DNSDomains: []string{"bad"},
289289
IPRanges: []string{"127.0.0.30"},
290290
EmailAddresses: []string{"badhost.example.com"},
291291
URIDomains: []string{"https://badhost.local"},
292+
CommonNames: []string{"another name"},
292293
},
293-
AllowWildcardLiteral: true,
294-
DisableCommonNameVerification: false,
294+
AllowWildcardLiteral: true,
295295
},
296296
SSH: &policy.SSHPolicyOptions{
297297
Host: &policy.SSHHostCertificateOptions{
@@ -369,8 +369,7 @@ func TestAuthority_reloadPolicyEngines(t *testing.T) {
369369
DeniedNames: &policy.X509NameOptions{
370370
DNSDomains: []string{"badhost.local"},
371371
},
372-
AllowWildcardLiteral: true,
373-
DisableCommonNameVerification: false,
372+
AllowWildcardLiteral: true,
374373
},
375374
}
376375

@@ -429,8 +428,7 @@ func TestAuthority_reloadPolicyEngines(t *testing.T) {
429428
DeniedNames: &policy.X509NameOptions{
430429
DNSDomains: []string{"badhost.local"},
431430
},
432-
AllowWildcardLiteral: true,
433-
DisableCommonNameVerification: false,
431+
AllowWildcardLiteral: true,
434432
},
435433
SSH: &policy.SSHPolicyOptions{
436434
Host: &policy.SSHHostCertificateOptions{
@@ -488,8 +486,7 @@ func TestAuthority_reloadPolicyEngines(t *testing.T) {
488486
DeniedNames: &policy.X509NameOptions{
489487
DNSDomains: []string{"badhost.local"},
490488
},
491-
AllowWildcardLiteral: true,
492-
DisableCommonNameVerification: false,
489+
AllowWildcardLiteral: true,
493490
},
494491
SSH: &policy.SSHPolicyOptions{
495492
Host: &policy.SSHHostCertificateOptions{
@@ -700,8 +697,7 @@ func TestAuthority_reloadPolicyEngines(t *testing.T) {
700697
DeniedNames: &policy.X509NameOptions{
701698
DNSDomains: []string{"badhost.local"},
702699
},
703-
AllowWildcardLiteral: true,
704-
DisableCommonNameVerification: false,
700+
AllowWildcardLiteral: true,
705701
},
706702
},
707703
},
@@ -800,8 +796,7 @@ func TestAuthority_reloadPolicyEngines(t *testing.T) {
800796
DeniedNames: &policy.X509NameOptions{
801797
DNSDomains: []string{"badhost.local"},
802798
},
803-
AllowWildcardLiteral: true,
804-
DisableCommonNameVerification: false,
799+
AllowWildcardLiteral: true,
805800
},
806801
SSH: &policy.SSHPolicyOptions{
807802
Host: &policy.SSHHostCertificateOptions{
@@ -916,8 +911,7 @@ func TestAuthority_reloadPolicyEngines(t *testing.T) {
916911
Deny: &linkedca.X509Names{
917912
Dns: []string{"badhost.local"},
918913
},
919-
AllowWildcardLiteral: true,
920-
DisableSubjectCommonNameVerification: false,
914+
AllowWildcardLiteral: true,
921915
},
922916
Ssh: &linkedca.SSHPolicy{
923917
Host: &linkedca.SSHHostPolicy{
@@ -982,8 +976,7 @@ func TestAuthority_reloadPolicyEngines(t *testing.T) {
982976
Deny: &linkedca.X509Names{
983977
Dns: []string{"badhost.local"},
984978
},
985-
AllowWildcardLiteral: true,
986-
DisableSubjectCommonNameVerification: false,
979+
AllowWildcardLiteral: true,
987980
},
988981
}, nil
989982
},

‎authority/provisioner/options.go

Lines changed: 0 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -70,11 +70,6 @@ type X509Options struct {
7070
// such as *.example.com and @example.com are allowed. Defaults
7171
// to false.
7272
AllowWildcardLiteral bool `json:"-"`
73-
74-
// DisableCommonNameVerification indicates if the Subject Common Name
75-
// is verified in addition to the SANs. Defaults to false, resulting
76-
// in Common Names to be verified.
77-
DisableCommonNameVerification bool `json:"-"`
7873
}
7974

8075
// HasTemplate returns true if a template is defined in the provisioner options.
@@ -107,13 +102,6 @@ func (o *X509Options) IsWildcardLiteralAllowed() bool {
107102
return o.AllowWildcardLiteral
108103
}
109104

110-
func (o *X509Options) ShouldVerifyCommonName() bool {
111-
if o == nil {
112-
return false
113-
}
114-
return !o.DisableCommonNameVerification
115-
}
116-
117105
// TemplateOptions generates a CertificateOptions with the template and data
118106
// defined in the ProvisionerOptions, the provisioner generated data, and the
119107
// user data provided in the request. If no template has been provided,

‎authority/provisioner/options_test.go

Lines changed: 0 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -322,38 +322,3 @@ func TestX509Options_IsWildcardLiteralAllowed(t *testing.T) {
322322
})
323323
}
324324
}
325-
326-
func TestX509Options_ShouldVerifySubjectCommonName(t *testing.T) {
327-
tests := []struct {
328-
name string
329-
options *X509Options
330-
want bool
331-
}{
332-
{
333-
name: "nil-options",
334-
options: nil,
335-
want: false,
336-
},
337-
{
338-
name: "set-true",
339-
options: &X509Options{
340-
DisableCommonNameVerification: true,
341-
},
342-
want: false,
343-
},
344-
{
345-
name: "set-false",
346-
options: &X509Options{
347-
DisableCommonNameVerification: false,
348-
},
349-
want: true,
350-
},
351-
}
352-
for _, tt := range tests {
353-
t.Run(tt.name, func(t *testing.T) {
354-
if got := tt.options.ShouldVerifyCommonName(); got != tt.want {
355-
t.Errorf("X509PolicyOptions.ShouldVerifySubjectCommonName() = %v, want %v", got, tt.want)
356-
}
357-
})
358-
}
359-
}

‎authority/tls_test.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -701,9 +701,9 @@ ZYtQ9Ot36qc=
701701
options := &policy.Options{
702702
X509: &policy.X509PolicyOptions{
703703
AllowedNames: &policy.X509NameOptions{
704-
DNSDomains: []string{"*.smallstep.com"},
704+
CommonNames: []string{"smallstep test"},
705+
DNSDomains: []string{"*.smallstep.com"},
705706
},
706-
DisableCommonNameVerification: true, // TODO(hs): allows "smallstep test"; do we want to keep it like this?
707707
},
708708
}
709709
engine, err := policy.New(options)

0 commit comments

Comments
 (0)