Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Ensure HPA has valid resource/name/subresource, validate path segments #16717

Merged
merged 1 commit into from
Nov 3, 2015
Merged

Ensure HPA has valid resource/name/subresource, validate path segments #16717

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
34 changes: 26 additions & 8 deletions pkg/api/validation/name.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,17 +27,26 @@ var NameMayNotBe = []string{".", ".."}
// NameMayNotContain specifies substrings that cannot be used in names specified as path segments (like the REST API or etcd store)
var NameMayNotContain = []string{"/", "%"}

// ValidatePathSegmentName validates the name can be used as a path segment
func ValidatePathSegmentName(name string, prefix bool) (bool, string) {
// Only check for exact matches if this is the full name (not a prefix)
if prefix == false {
for _, illegalName := range NameMayNotBe {
if name == illegalName {
return false, fmt.Sprintf(`name may not be %q`, illegalName)
}
// IsValidPathSegmentName validates the name can be safely encoded as a path segment
func IsValidPathSegmentName(name string) (bool, string) {
for _, illegalName := range NameMayNotBe {
if name == illegalName {
return false, fmt.Sprintf(`name may not be %q`, illegalName)
}
}

for _, illegalContent := range NameMayNotContain {
if strings.Contains(name, illegalContent) {
return false, fmt.Sprintf(`name may not contain %q`, illegalContent)
}
}

return true, ""
}

// IsValidPathSegmentPrefix validates the name can be used as a prefix for a name which will be encoded as a path segment
// It does not check for exact matches with disallowed names, since an arbitrary suffix might make the name valid
func IsValidPathSegmentPrefix(name string) (bool, string) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add an example for why you would call this function and not IsValidPathSegmentName?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to comply with the name validation required by ValidateObjectMeta, implemented here: https://github.com/kubernetes/kubernetes/pull/16717/files#diff-b72277cd2bba86ca73e54d9589bdf5f9R62

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, sorry, the "prefix" was confusing me.

for _, illegalContent := range NameMayNotContain {
if strings.Contains(name, illegalContent) {
return false, fmt.Sprintf(`name may not contain %q`, illegalContent)
Expand All @@ -46,3 +55,12 @@ func ValidatePathSegmentName(name string, prefix bool) (bool, string) {

return true, ""
}

// ValidatePathSegmentName validates the name can be safely encoded as a path segment
func ValidatePathSegmentName(name string, prefix bool) (bool, string) {
if prefix {
return IsValidPathSegmentPrefix(name)
} else {
return IsValidPathSegmentName(name)
}
}
27 changes: 27 additions & 0 deletions pkg/apis/extensions/validation/validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,33 @@ func validateHorizontalPodAutoscalerSpec(autoscaler extensions.HorizontalPodAuto
if autoscaler.CPUUtilization != nil && autoscaler.CPUUtilization.TargetPercentage < 1 {
allErrs = append(allErrs, errs.NewFieldInvalid("cpuUtilization.targetPercentage", autoscaler.CPUUtilization.TargetPercentage, `must be bigger or equal to 1`))
}
if refErrs := ValidateSubresourceReference(autoscaler.ScaleRef); len(refErrs) > 0 {
allErrs = append(allErrs, refErrs.Prefix("scaleRef")...)
} else if autoscaler.ScaleRef.Subresource != "scale" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we have this lever?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's a good question, but as long as we do, we need to make sure we're not accepting bad data, so when we start making use of it, the world doesn't fall apart. @jszczepkowski, any feedback on why we have this field, and why we're not using it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Subresource is a generic reference, which may point to other sub-resources than scale. In HPA, we need to ensure that it points to scale.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see why it has to be named "scale", as long as the referenced subresource accepts and returns API objects of the correct type

allErrs = append(allErrs, errs.NewFieldValueNotSupported("scaleRef.subresource", autoscaler.ScaleRef.Subresource, []string{"scale"}))
}
return allErrs
}

func ValidateSubresourceReference(ref extensions.SubresourceReference) errs.ValidationErrorList {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we have an APIVersion too?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we do, it's unused, and it's unclear (to me) what the validation should be

allErrs := errs.ValidationErrorList{}
if len(ref.Kind) == 0 {
allErrs = append(allErrs, errs.NewFieldRequired("kind"))
} else if ok, msg := apivalidation.IsValidPathSegmentName(ref.Kind); !ok {
allErrs = append(allErrs, errs.NewFieldInvalid("kind", ref.Kind, msg))
}

if len(ref.Name) == 0 {
allErrs = append(allErrs, errs.NewFieldRequired("name"))
} else if ok, msg := apivalidation.IsValidPathSegmentName(ref.Name); !ok {
allErrs = append(allErrs, errs.NewFieldInvalid("name", ref.Name, msg))
}

if len(ref.Subresource) == 0 {
allErrs = append(allErrs, errs.NewFieldRequired("subresource"))
} else if ok, msg := apivalidation.IsValidPathSegmentName(ref.Subresource); !ok {
allErrs = append(allErrs, errs.NewFieldInvalid("subresource", ref.Subresource, msg))
}
return allErrs
}

Expand Down
88 changes: 88 additions & 0 deletions pkg/apis/extensions/validation/validation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@ func TestValidateHorizontalPodAutoscaler(t *testing.T) {
},
Spec: extensions.HorizontalPodAutoscalerSpec{
ScaleRef: extensions.SubresourceReference{
Kind: "ReplicationController",
Name: "myrc",
Subresource: "scale",
},
MinReplicas: newInt(1),
Expand All @@ -50,6 +52,8 @@ func TestValidateHorizontalPodAutoscaler(t *testing.T) {
},
Spec: extensions.HorizontalPodAutoscalerSpec{
ScaleRef: extensions.SubresourceReference{
Kind: "ReplicationController",
Name: "myrc",
Subresource: "scale",
},
MinReplicas: newInt(1),
Expand All @@ -67,6 +71,90 @@ func TestValidateHorizontalPodAutoscaler(t *testing.T) {
horizontalPodAutoscaler extensions.HorizontalPodAutoscaler
msg string
}{
{
horizontalPodAutoscaler: extensions.HorizontalPodAutoscaler{
ObjectMeta: api.ObjectMeta{Name: "myautoscaler", Namespace: api.NamespaceDefault},
Spec: extensions.HorizontalPodAutoscalerSpec{
ScaleRef: extensions.SubresourceReference{Name: "myrc", Subresource: "scale"},
MinReplicas: newInt(1),
MaxReplicas: 5,
CPUUtilization: &extensions.CPUTargetUtilization{TargetPercentage: 70},
},
},
msg: "scaleRef.kind: required",
},
{
horizontalPodAutoscaler: extensions.HorizontalPodAutoscaler{
ObjectMeta: api.ObjectMeta{Name: "myautoscaler", Namespace: api.NamespaceDefault},
Spec: extensions.HorizontalPodAutoscalerSpec{
ScaleRef: extensions.SubresourceReference{Kind: "..", Name: "myrc", Subresource: "scale"},
MinReplicas: newInt(1),
MaxReplicas: 5,
CPUUtilization: &extensions.CPUTargetUtilization{TargetPercentage: 70},
},
},
msg: "scaleRef.kind: invalid",
},
{
horizontalPodAutoscaler: extensions.HorizontalPodAutoscaler{
ObjectMeta: api.ObjectMeta{Name: "myautoscaler", Namespace: api.NamespaceDefault},
Spec: extensions.HorizontalPodAutoscalerSpec{
ScaleRef: extensions.SubresourceReference{Kind: "ReplicationController", Subresource: "scale"},
MinReplicas: newInt(1),
MaxReplicas: 5,
CPUUtilization: &extensions.CPUTargetUtilization{TargetPercentage: 70},
},
},
msg: "scaleRef.name: required",
},
{
horizontalPodAutoscaler: extensions.HorizontalPodAutoscaler{
ObjectMeta: api.ObjectMeta{Name: "myautoscaler", Namespace: api.NamespaceDefault},
Spec: extensions.HorizontalPodAutoscalerSpec{
ScaleRef: extensions.SubresourceReference{Kind: "ReplicationController", Name: "..", Subresource: "scale"},
MinReplicas: newInt(1),
MaxReplicas: 5,
CPUUtilization: &extensions.CPUTargetUtilization{TargetPercentage: 70},
},
},
msg: "scaleRef.name: invalid",
},
{
horizontalPodAutoscaler: extensions.HorizontalPodAutoscaler{
ObjectMeta: api.ObjectMeta{Name: "myautoscaler", Namespace: api.NamespaceDefault},
Spec: extensions.HorizontalPodAutoscalerSpec{
ScaleRef: extensions.SubresourceReference{Kind: "ReplicationController", Name: "myrc", Subresource: ""},
MinReplicas: newInt(1),
MaxReplicas: 5,
CPUUtilization: &extensions.CPUTargetUtilization{TargetPercentage: 70},
},
},
msg: "scaleRef.subresource: required",
},
{
horizontalPodAutoscaler: extensions.HorizontalPodAutoscaler{
ObjectMeta: api.ObjectMeta{Name: "myautoscaler", Namespace: api.NamespaceDefault},
Spec: extensions.HorizontalPodAutoscalerSpec{
ScaleRef: extensions.SubresourceReference{Kind: "ReplicationController", Name: "myrc", Subresource: ".."},
MinReplicas: newInt(1),
MaxReplicas: 5,
CPUUtilization: &extensions.CPUTargetUtilization{TargetPercentage: 70},
},
},
msg: "scaleRef.subresource: invalid",
},
{
horizontalPodAutoscaler: extensions.HorizontalPodAutoscaler{
ObjectMeta: api.ObjectMeta{Name: "myautoscaler", Namespace: api.NamespaceDefault},
Spec: extensions.HorizontalPodAutoscalerSpec{
ScaleRef: extensions.SubresourceReference{Kind: "ReplicationController", Name: "myrc", Subresource: "randomsubresource"},
MinReplicas: newInt(1),
MaxReplicas: 5,
CPUUtilization: &extensions.CPUTargetUtilization{TargetPercentage: 70},
},
},
msg: "scaleRef.subresource: unsupported",
},
{
horizontalPodAutoscaler: extensions.HorizontalPodAutoscaler{
ObjectMeta: api.ObjectMeta{
Expand Down
19 changes: 19 additions & 0 deletions pkg/client/unversioned/request.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import (
"github.com/golang/glog"
"k8s.io/kubernetes/pkg/api/errors"
"k8s.io/kubernetes/pkg/api/unversioned"
"k8s.io/kubernetes/pkg/api/validation"
"k8s.io/kubernetes/pkg/client/metrics"
"k8s.io/kubernetes/pkg/conversion/queryparams"
"k8s.io/kubernetes/pkg/fields"
Expand Down Expand Up @@ -149,6 +150,10 @@ func (r *Request) Resource(resource string) *Request {
r.err = fmt.Errorf("resource already set to %q, cannot change to %q", r.resource, resource)
return r
}
if ok, msg := validation.IsValidPathSegmentName(resource); !ok {
r.err = fmt.Errorf("invalid resource %q: %s", resource, msg)
return r
}
r.resource = resource
return r
}
Expand All @@ -164,6 +169,12 @@ func (r *Request) SubResource(subresources ...string) *Request {
r.err = fmt.Errorf("subresource already set to %q, cannot change to %q", r.resource, subresource)
return r
}
for _, s := range subresources {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same everywhere in file.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we either have to guard everywhere every one of these methods is called, or guard here. the primary problem is with names that will get collapsed or traverse client-side

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see examples called out in #16717 (comment)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixing this in the client is at best an optimization for non-adversarial clients, and I worry it will lead to us leaving a vulnerability in the server.

If that doesn't persuade you, then I would at least like to not import the entire api/validation package, and I think it might be simpler to check the path once in the function that builds the final URL.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixing this in the client is at best an optimization for non-adversarial clients, and I worry it will lead to us leaving a vulnerability in the server.

You need both client-side and server-side checks. Server-side URL normalization we get for free(ish) from ServeMux, and we obviously want to defer as much validation to the server as possible, but it only makes sense to prevent the client from building requests that are invalid, yet will appear valid to the server (like path segments that will collapse or traverse).

I would at least like to not import the entire api/validation package

I can copy the check into this package if you like.

I think it might be simpler to check the path once in the function that builds the final URL.

The checks need to remain on the individual segments. Once you combine segments into the final URL, you lose to ability to detect invalid resource names like "foo/bar" (not to mention path.Join(...) does a Clean, which also masks traversal)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would at least like to not import the entire api/validation package

I can copy the check into this package if you like.

Although we're already using these packages:

    "k8s.io/kubernetes/pkg/api/errors"
    "k8s.io/kubernetes/pkg/api/unversioned"

Why don't you want to use a shared function for minimal safe path segment validation?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have a huge import tree right now, which makes the client inconvenient for people to use, I'd like to not make that problem worse. But I guess we can fix that problem later.

I guess I will not object as long as we're all on the same page that we do not get any additional security from these checks in the client.

if ok, msg := validation.IsValidPathSegmentName(s); !ok {
r.err = fmt.Errorf("invalid subresource %q: %s", s, msg)
return r
}
}
r.subresource = subresource
return r
}
Expand All @@ -181,6 +192,10 @@ func (r *Request) Name(resourceName string) *Request {
r.err = fmt.Errorf("resource name already set to %q, cannot change to %q", r.resourceName, resourceName)
return r
}
if ok, msg := validation.IsValidPathSegmentName(resourceName); !ok {
r.err = fmt.Errorf("invalid resource name %q: %s", resourceName, msg)
return r
}
r.resourceName = resourceName
return r
}
Expand All @@ -194,6 +209,10 @@ func (r *Request) Namespace(namespace string) *Request {
r.err = fmt.Errorf("namespace already set to %q, cannot change to %q", r.namespace, namespace)
return r
}
if ok, msg := validation.IsValidPathSegmentName(namespace); !ok {
r.err = fmt.Errorf("invalid namespace %q: %s", namespace, msg)
return r
}
r.namespaceSet = true
r.namespace = namespace
return r
Expand Down
19 changes: 19 additions & 0 deletions pkg/client/unversioned/request_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,25 @@ func TestRequestSetTwiceError(t *testing.T) {
}
}

func TestInvalidSegments(t *testing.T) {
invalidSegments := []string{".", "..", "test/segment", "test%2bsegment"}
setters := map[string]func(string, *Request){
"namespace": func(s string, r *Request) { r.Namespace(s) },
"resource": func(s string, r *Request) { r.Resource(s) },
"name": func(s string, r *Request) { r.Name(s) },
"subresource": func(s string, r *Request) { r.SubResource(s) },
}
for _, invalidSegment := range invalidSegments {
for setterName, setter := range setters {
r := &Request{}
setter(invalidSegment, r)
if r.err == nil {
t.Errorf("%s: %s: expected error, got none", setterName, invalidSegment)
}
}
}
}

func TestRequestParam(t *testing.T) {
r := (&Request{}).Param("foo", "a")
if !reflect.DeepEqual(r.params, url.Values{"foo": []string{"a"}}) {
Expand Down
4 changes: 2 additions & 2 deletions pkg/registry/generic/etcd/etcd.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ func NamespaceKeyFunc(ctx api.Context, prefix string, name string) (string, erro
if len(name) == 0 {
return "", kubeerr.NewBadRequest("Name parameter required.")
}
if ok, msg := validation.ValidatePathSegmentName(name, false); !ok {
if ok, msg := validation.IsValidPathSegmentName(name); !ok {
return "", kubeerr.NewBadRequest(fmt.Sprintf("Name parameter invalid: %v.", msg))
}
key = key + "/" + name
Expand All @@ -141,7 +141,7 @@ func NoNamespaceKeyFunc(ctx api.Context, prefix string, name string) (string, er
if len(name) == 0 {
return "", kubeerr.NewBadRequest("Name parameter required.")
}
if ok, msg := validation.ValidatePathSegmentName(name, false); !ok {
if ok, msg := validation.IsValidPathSegmentName(name); !ok {
return "", kubeerr.NewBadRequest(fmt.Sprintf("Name parameter invalid: %v.", msg))
}
key := prefix + "/" + name
Expand Down
2 changes: 2 additions & 0 deletions pkg/registry/horizontalpodautoscaler/etcd/etcd_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,8 @@ func validNewHorizontalPodAutoscaler(name string) *extensions.HorizontalPodAutos
},
Spec: extensions.HorizontalPodAutoscalerSpec{
ScaleRef: extensions.SubresourceReference{
Kind: "ReplicationController",
Name: "myrc",
Subresource: "scale",
},
MaxReplicas: 5,
Expand Down
4 changes: 2 additions & 2 deletions pkg/storage/util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ func NamespaceKeyFunc(prefix string, obj runtime.Object) (string, error) {
return "", err
}
name := meta.Name()
if ok, msg := validation.ValidatePathSegmentName(name, false); !ok {
if ok, msg := validation.IsValidPathSegmentName(name); !ok {
return "", fmt.Errorf("invalid name: %v", msg)
}
return prefix + "/" + meta.Namespace() + "/" + meta.Name(), nil
Expand All @@ -71,7 +71,7 @@ func NoNamespaceKeyFunc(prefix string, obj runtime.Object) (string, error) {
return "", err
}
name := meta.Name()
if ok, msg := validation.ValidatePathSegmentName(name, false); !ok {
if ok, msg := validation.IsValidPathSegmentName(name); !ok {
return "", fmt.Errorf("invalid name: %v", msg)
}
return prefix + "/" + meta.Name(), nil
Expand Down