Skip to content

Commit 9b6a4f4

Browse files
openshift-merge-robotopenshift-merge-robot
authored
Merge pull request #21922 from stlaz/o_auth_validate
Add validation for Authentication and OAuth CRs
2 parents 85f1906 + c9ec23a commit 9b6a4f4

19 files changed

Lines changed: 1919 additions & 11 deletions

‎hack/import-restrictions.json‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -151,6 +151,8 @@
151151
"github.com/openshift/origin/pkg/apiserver/authentication/oauth",
152152
"github.com/openshift/origin/pkg/oauth/apis/oauth/validation",
153153

154+
"github.com/openshift/origin/pkg/admission/customresourcevalidation/oauth",
155+
154156
"github.com/openshift/origin/pkg/cmd/openshift-integrated-oauth-server",
155157
"github.com/openshift/origin/pkg/cmd/openshift-kube-apiserver/openshiftkubeapiserver",
156158
"github.com/openshift/origin/pkg/cmd/server/origin",

‎pkg/admission/customresourcevalidation/oauth/helpers.go‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
package oauth
2+
3+
import (
4+
"net"
5+
6+
kvalidation "k8s.io/apimachinery/pkg/util/validation"
7+
"k8s.io/apimachinery/pkg/util/validation/field"
8+
9+
configv1 "github.com/openshift/api/config/v1"
10+
crvalidation "github.com/openshift/origin/pkg/admission/customresourcevalidation"
11+
"github.com/openshift/origin/pkg/cmd/server/apis/config/validation/common"
12+
)
13+
14+
func isValidHostname(hostname string) bool {
15+
return len(kvalidation.IsDNS1123Subdomain(hostname)) == 0 || net.ParseIP(hostname) != nil
16+
}
17+
18+
func ValidateRemoteConnectionInfo(remoteConnectionInfo configv1.OAuthRemoteConnectionInfo, fldPath *field.Path) field.ErrorList {
19+
allErrs := field.ErrorList{}
20+
21+
if len(remoteConnectionInfo.URL) == 0 {
22+
allErrs = append(allErrs, field.Required(fldPath.Child("url"), ""))
23+
} else {
24+
_, urlErrs := common.ValidateSecureURL(remoteConnectionInfo.URL, fldPath.Child("url"))
25+
allErrs = append(allErrs, urlErrs...)
26+
}
27+
28+
allErrs = append(allErrs, crvalidation.ValidateConfigMapReference(fldPath.Child("ca"), remoteConnectionInfo.CA, false)...)
29+
allErrs = append(allErrs, crvalidation.ValidateSecretReference(fldPath.Child("tlsClientCert"), remoteConnectionInfo.TLSClientCert, false)...)
30+
allErrs = append(allErrs, crvalidation.ValidateSecretReference(fldPath.Child("tlsClientKey"), remoteConnectionInfo.TLSClientKey, false)...)
31+
32+
return allErrs
33+
}

‎pkg/admission/customresourcevalidation/oauth/validate_github.go‎

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,69 @@
1+
package oauth
2+
3+
import (
4+
"strings"
5+
6+
"k8s.io/apimachinery/pkg/util/validation/field"
7+
8+
configv1 "github.com/openshift/api/config/v1"
9+
crvalidation "github.com/openshift/origin/pkg/admission/customresourcevalidation"
10+
)
11+
12+
func ValidateGitHubIdentityProvider(provider *configv1.GitHubIdentityProvider, mappingMethod configv1.MappingMethodType, fieldPath *field.Path) field.ErrorList {
13+
errs := field.ErrorList{}
14+
if provider == nil {
15+
errs = append(errs, field.Required(fieldPath, ""))
16+
return errs
17+
}
18+
19+
errs = append(errs, ValidateOAuthIdentityProvider(provider.ClientID, provider.ClientSecret, fieldPath.Child("provider"))...)
20+
21+
if len(provider.Teams) > 0 && len(provider.Organizations) > 0 {
22+
errs = append(errs, field.Invalid(fieldPath.Child("organizations"), provider.Organizations, "specify organizations or teams, not both"))
23+
errs = append(errs, field.Invalid(fieldPath.Child("teams"), provider.Teams, "specify organizations or teams, not both"))
24+
}
25+
26+
// only check that there are some teams/orgs if not GitHub Enterprise Server
27+
if len(provider.Hostname) == 0 && len(provider.Teams) == 0 && len(provider.Organizations) == 0 && mappingMethod != configv1.MappingMethodLookup {
28+
errs = append(errs, field.Invalid(fieldPath, nil, "one of organizations or teams must be specified unless hostname is set or lookup is used"))
29+
}
30+
for i, organization := range provider.Organizations {
31+
if strings.Contains(organization, "/") {
32+
errs = append(errs, field.Invalid(fieldPath.Child("organizations").Index(i), organization, "cannot contain /"))
33+
}
34+
if len(organization) == 0 {
35+
errs = append(errs, field.Required(fieldPath.Child("organizations").Index(i), "cannot be empty"))
36+
}
37+
}
38+
for i, team := range provider.Teams {
39+
if split := strings.Split(team, "/"); len(split) != 2 {
40+
errs = append(errs, field.Invalid(fieldPath.Child("teams").Index(i), team, "must be in the format <org>/<team>"))
41+
} else if org, t := split[0], split[1]; len(org) == 0 || len(t) == 0 {
42+
errs = append(errs, field.Invalid(fieldPath.Child("teams").Index(i), team, "must be in the format <org>/<team>"))
43+
}
44+
}
45+
46+
if hostname := provider.Hostname; len(hostname) != 0 {
47+
hostnamePath := fieldPath.Child("hostname")
48+
49+
if hostname == "github.com" || strings.HasSuffix(hostname, ".github.com") {
50+
errs = append(errs, field.Invalid(hostnamePath, hostname, "cannot equal [*.]github.com"))
51+
}
52+
53+
if !isValidHostname(hostname) {
54+
errs = append(errs, field.Invalid(hostnamePath, hostname, "must be a valid DNS subdomain or IP address"))
55+
}
56+
}
57+
58+
if caFile := provider.CA; len(caFile.Name) != 0 {
59+
caPath := fieldPath.Child("ca")
60+
61+
errs = append(errs, crvalidation.ValidateConfigMapReference(caPath, caFile, true)...)
62+
63+
if len(provider.Hostname) == 0 {
64+
errs = append(errs, field.Invalid(caPath, caFile, "cannot be specified when hostname is empty"))
65+
}
66+
}
67+
68+
return errs
69+
}

‎pkg/admission/customresourcevalidation/oauth/validate_github_test.go‎

Lines changed: 249 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,249 @@
1+
package oauth
2+
3+
import (
4+
"reflect"
5+
"testing"
6+
7+
"k8s.io/apimachinery/pkg/util/validation/field"
8+
9+
configv1 "github.com/openshift/api/config/v1"
10+
)
11+
12+
func TestValidateGitHubIdentityProvider(t *testing.T) {
13+
type args struct {
14+
provider *configv1.GitHubIdentityProvider
15+
mappingMethod configv1.MappingMethodType
16+
fieldPath *field.Path
17+
}
18+
tests := []struct {
19+
name string
20+
args args
21+
errors field.ErrorList
22+
}{
23+
{
24+
name: "cannot use GH as hostname",
25+
args: args{
26+
provider: &configv1.GitHubIdentityProvider{
27+
ClientID: "client",
28+
ClientSecret: configv1.SecretNameReference{Name: "secret"},
29+
Organizations: []string{"org1"},
30+
Teams: nil,
31+
Hostname: "github.com",
32+
CA: configv1.ConfigMapNameReference{Name: "caconfigmap"},
33+
},
34+
mappingMethod: "",
35+
},
36+
errors: field.ErrorList{
37+
{Type: field.ErrorTypeInvalid, Field: "hostname", BadValue: "github.com", Detail: "cannot equal [*.]github.com"},
38+
},
39+
},
40+
{
41+
name: "cannot use GH subdomain as hostname",
42+
args: args{
43+
provider: &configv1.GitHubIdentityProvider{
44+
ClientID: "client",
45+
ClientSecret: configv1.SecretNameReference{Name: "secret"},
46+
Organizations: []string{"org1"},
47+
Teams: nil,
48+
Hostname: "foo.github.com",
49+
CA: configv1.ConfigMapNameReference{Name: "caconfigmap"},
50+
},
51+
mappingMethod: "",
52+
},
53+
errors: field.ErrorList{
54+
{Type: field.ErrorTypeInvalid, Field: "hostname", BadValue: "foo.github.com", Detail: "cannot equal [*.]github.com"},
55+
},
56+
},
57+
{
58+
name: "valid domain hostname",
59+
args: args{
60+
provider: &configv1.GitHubIdentityProvider{
61+
ClientID: "client",
62+
ClientSecret: configv1.SecretNameReference{Name: "secret"},
63+
Organizations: []string{"org1"},
64+
Teams: nil,
65+
Hostname: "company.com",
66+
CA: configv1.ConfigMapNameReference{Name: "caconfigmap"},
67+
},
68+
mappingMethod: "",
69+
},
70+
},
71+
{
72+
name: "valid ip hostname",
73+
args: args{
74+
provider: &configv1.GitHubIdentityProvider{
75+
ClientID: "client",
76+
ClientSecret: configv1.SecretNameReference{Name: "secret"},
77+
Organizations: []string{"org1"},
78+
Teams: nil,
79+
Hostname: "192.168.8.1",
80+
CA: configv1.ConfigMapNameReference{Name: "caconfigmap"},
81+
},
82+
mappingMethod: "",
83+
},
84+
},
85+
{
86+
name: "invalid ip hostname with port",
87+
args: args{
88+
provider: &configv1.GitHubIdentityProvider{
89+
ClientID: "client",
90+
ClientSecret: configv1.SecretNameReference{Name: "secret"},
91+
Organizations: []string{"org1"},
92+
Teams: nil,
93+
Hostname: "192.168.8.1:8080",
94+
CA: configv1.ConfigMapNameReference{Name: "caconfigmap"},
95+
},
96+
mappingMethod: "",
97+
},
98+
errors: field.ErrorList{
99+
{Type: field.ErrorTypeInvalid, Field: "hostname", BadValue: "192.168.8.1:8080", Detail: "must be a valid DNS subdomain or IP address"},
100+
},
101+
},
102+
{
103+
name: "invalid domain hostname",
104+
args: args{
105+
provider: &configv1.GitHubIdentityProvider{
106+
ClientID: "client",
107+
ClientSecret: configv1.SecretNameReference{Name: "secret"},
108+
Organizations: []string{"org1"},
109+
Teams: nil,
110+
Hostname: "google-.com",
111+
CA: configv1.ConfigMapNameReference{Name: "caconfigmap"},
112+
},
113+
mappingMethod: "",
114+
},
115+
errors: field.ErrorList{
116+
{Type: field.ErrorTypeInvalid, Field: "hostname", BadValue: "google-.com", Detail: "must be a valid DNS subdomain or IP address"},
117+
},
118+
},
119+
{
120+
name: "invalid name in ca ref and no hostname",
121+
args: args{
122+
provider: &configv1.GitHubIdentityProvider{
123+
ClientID: "client",
124+
ClientSecret: configv1.SecretNameReference{Name: "secret"},
125+
Organizations: []string{"org1"},
126+
Teams: nil,
127+
Hostname: "",
128+
CA: configv1.ConfigMapNameReference{Name: "ca&config-map"},
129+
},
130+
mappingMethod: "",
131+
},
132+
errors: field.ErrorList{
133+
{Type: field.ErrorTypeInvalid, Field: "ca.name", BadValue: "ca&config-map", Detail: "a DNS-1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*')"},
134+
{Type: field.ErrorTypeInvalid, Field: "ca", BadValue: configv1.ConfigMapNameReference{Name: "ca&config-map"}, Detail: "cannot be specified when hostname is empty"},
135+
},
136+
},
137+
{
138+
name: "valid ca and hostname",
139+
args: args{
140+
provider: &configv1.GitHubIdentityProvider{
141+
ClientID: "client",
142+
ClientSecret: configv1.SecretNameReference{Name: "secret"},
143+
Organizations: []string{"org1"},
144+
Teams: nil,
145+
Hostname: "mo.co",
146+
CA: configv1.ConfigMapNameReference{Name: "ca-config-map"},
147+
},
148+
mappingMethod: "",
149+
},
150+
},
151+
{
152+
name: "GitHub requires client ID and secret",
153+
args: args{
154+
provider: &configv1.GitHubIdentityProvider{
155+
ClientID: "",
156+
ClientSecret: configv1.SecretNameReference{},
157+
Organizations: []string{"org1"},
158+
Teams: nil,
159+
Hostname: "",
160+
CA: configv1.ConfigMapNameReference{},
161+
},
162+
mappingMethod: "",
163+
},
164+
errors: field.ErrorList{
165+
{Type: field.ErrorTypeRequired, Field: "provider.clientID", BadValue: "", Detail: ""},
166+
{Type: field.ErrorTypeRequired, Field: "provider.clientSecret.name", BadValue: "", Detail: ""},
167+
},
168+
},
169+
{
170+
name: "GitHub warns when not constrained to organizations or teams without lookup",
171+
args: args{
172+
provider: &configv1.GitHubIdentityProvider{
173+
ClientID: "client",
174+
ClientSecret: configv1.SecretNameReference{Name: "secret"},
175+
Organizations: nil,
176+
Teams: nil,
177+
Hostname: "",
178+
CA: configv1.ConfigMapNameReference{},
179+
},
180+
mappingMethod: "",
181+
},
182+
errors: field.ErrorList{
183+
{Type: field.ErrorTypeInvalid, Field: "", BadValue: nil, Detail: "one of organizations or teams must be specified unless hostname is set or lookup is used"},
184+
},
185+
},
186+
{
187+
name: "GitHub does not warn when not constrained to organizations or teams with lookup",
188+
args: args{
189+
provider: &configv1.GitHubIdentityProvider{
190+
ClientID: "client",
191+
ClientSecret: configv1.SecretNameReference{Name: "secret"},
192+
Organizations: nil,
193+
Teams: nil,
194+
Hostname: "",
195+
CA: configv1.ConfigMapNameReference{},
196+
},
197+
mappingMethod: "lookup",
198+
},
199+
},
200+
{
201+
name: "invalid cannot specific both organizations and teams",
202+
args: args{
203+
provider: &configv1.GitHubIdentityProvider{
204+
ClientID: "client",
205+
ClientSecret: configv1.SecretNameReference{Name: "secret"},
206+
Organizations: []string{"org1"},
207+
Teams: []string{"org1/team1"},
208+
Hostname: "",
209+
CA: configv1.ConfigMapNameReference{},
210+
},
211+
mappingMethod: "",
212+
},
213+
errors: field.ErrorList{
214+
{Type: field.ErrorTypeInvalid, Field: "organizations", BadValue: []string{"org1"}, Detail: "specify organizations or teams, not both"},
215+
{Type: field.ErrorTypeInvalid, Field: "teams", BadValue: []string{"org1/team1"}, Detail: "specify organizations or teams, not both"},
216+
},
217+
},
218+
{
219+
name: "invalid team format",
220+
args: args{
221+
provider: &configv1.GitHubIdentityProvider{
222+
ClientID: "client",
223+
ClientSecret: configv1.SecretNameReference{Name: "secret"},
224+
Organizations: nil,
225+
Teams: []string{"org1/team1", "org2/not/team2", "org3//team3", "", "org4/team4"},
226+
Hostname: "",
227+
CA: configv1.ConfigMapNameReference{},
228+
},
229+
mappingMethod: "",
230+
},
231+
errors: field.ErrorList{
232+
{Type: field.ErrorTypeInvalid, Field: "teams[1]", BadValue: "org2/not/team2", Detail: "must be in the format <org>/<team>"},
233+
{Type: field.ErrorTypeInvalid, Field: "teams[2]", BadValue: "org3//team3", Detail: "must be in the format <org>/<team>"},
234+
{Type: field.ErrorTypeInvalid, Field: "teams[3]", BadValue: "", Detail: "must be in the format <org>/<team>"},
235+
},
236+
},
237+
}
238+
for _, tt := range tests {
239+
t.Run(tt.name, func(t *testing.T) {
240+
got := ValidateGitHubIdentityProvider(tt.args.provider, tt.args.mappingMethod, tt.args.fieldPath)
241+
if tt.errors == nil && len(got) == 0 {
242+
return
243+
}
244+
if !reflect.DeepEqual(got, tt.errors) {
245+
t.Errorf("ValidateGitHubIdentityProvider() = %v, want %v", got, tt.errors)
246+
}
247+
})
248+
}
249+
}

‎pkg/admission/customresourcevalidation/oauth/validate_gitlab.go‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
package oauth
2+
3+
import (
4+
"k8s.io/apimachinery/pkg/util/validation/field"
5+
6+
configv1 "github.com/openshift/api/config/v1"
7+
crvalidation "github.com/openshift/origin/pkg/admission/customresourcevalidation"
8+
"github.com/openshift/origin/pkg/cmd/server/apis/config/validation/common"
9+
)
10+
11+
func ValidateGitLabIdentityProvider(provider *configv1.GitLabIdentityProvider, fieldPath *field.Path) field.ErrorList {
12+
allErrs := field.ErrorList{}
13+
if provider == nil {
14+
allErrs = append(allErrs, field.Required(fieldPath, ""))
15+
return allErrs
16+
}
17+
18+
allErrs = append(allErrs, ValidateOAuthIdentityProvider(provider.ClientID, provider.ClientSecret, fieldPath)...)
19+
20+
_, urlErrs := common.ValidateSecureURL(provider.URL, fieldPath.Child("url"))
21+
allErrs = append(allErrs, urlErrs...)
22+
23+
allErrs = append(allErrs, crvalidation.ValidateConfigMapReference(fieldPath.Child("ca"), provider.CA, false)...)
24+
25+
return allErrs
26+
}

0 commit comments

Comments
 (0)