Skip to content
This repository has been archived by the owner on Jan 3, 2023. It is now read-only.

Clean & lowercase filter #13

Merged
merged 3 commits into from
Dec 22, 2017
Merged

Clean & lowercase filter #13

merged 3 commits into from
Dec 22, 2017

Conversation

ikehz
Copy link
Contributor

@ikehz ikehz commented Dec 19, 2017

... plus some fixups:

  • Move filter business logic to a separate package to make it easier to test, and
  • Build docker images from scratch instead of larger base images.

}

// Check against lowercase.
lowerPath := strings.ToLower(req.URL.Path)
Copy link
Contributor

Choose a reason for hiding this comment

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

the metadata server is case sensitive. what is this used for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

HTTP doesn't specify case sensitivity in its spec, so in case the metadata server decides to stop being case sensitive, we should be aggressive about filtering.

// "service-accounts/<valid-service-account-or-alias>/identity".
// In particular, note that '.' may only come after '@' in the valid
// service account or alias.
concealedPattern = regexp.MustCompile("^\\/(0\\.1\\/meta\\-data|computemetadata\\/v1beta1\\/instance|computemetadata\\/v1\\/instance)/(attributes/kube-env|service-accounts/[0-9a-z_\\-]+(@[0-9a-z_\\-\\.]*)?/identity)$")
Copy link
Contributor

Choose a reason for hiding this comment

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

these are pretty painful. would it simplify things to clean the path first?

https://golang.org/pkg/path/#Clean

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor Author

@ikehz ikehz left a comment

Choose a reason for hiding this comment

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

Ptal.

// "service-accounts/<valid-service-account-or-alias>/identity".
// In particular, note that '.' may only come after '@' in the valid
// service account or alias.
concealedPattern = regexp.MustCompile("^\\/(0\\.1\\/meta\\-data|computemetadata\\/v1beta1\\/instance|computemetadata\\/v1\\/instance)/(attributes/kube-env|service-accounts/[0-9a-z_\\-]+(@[0-9a-z_\\-\\.]*)?/identity)$")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

}

// Check against lowercase.
lowerPath := strings.ToLower(req.URL.Path)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

HTTP doesn't specify case sensitivity in its spec, so in case the metadata server decides to stop being case sensitive, we should be aggressive about filtering.

@ikehz ikehz changed the title Convert filter to regexp instead of lists, maps, etc. Clean & lowercase filter Dec 20, 2017
@mikedanese
Copy link
Contributor

/lgtm

@ikehz ikehz merged commit 94e6b93 into GoogleCloudPlatform:master Dec 22, 2017
k8s-github-robot pushed a commit to kubernetes/kubernetes that referenced this pull request Jan 6, 2018
Automatic merge from submit-queue (batch tested with PRs 57906, 57425, 56939, 57317, 57762). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Bump metadata proxy and test versions

**What this PR does / why we need it**: Bump metadata proxy version to v0.1.7 (to pick up GoogleCloudPlatform/k8s-metadata-proxy#13).

**Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*:
Fixes #

**Special notes for your reviewer**:

**Release note**:

```release-note
Bump metadata proxy version to v0.1.7 to pick up security fix.
```
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants