GCE metadata proxy blocks instance identity & recursive calls, & excludes port from redirects #51302

ikehz · 2017-08-25T01:32:16Z

What this PR does / why we need it: Metadata proxy blocks instance identity & recursive calls, and no longer includes port in redirects (it was serving redirects to http://metadata.google.internal:988, which doesn't resolve. Ref #8867.

Special notes for your reviewer: Container is defined https://github.com/kubernetes/contrib/tree/master/metadata-proxy; I plan to send a separate PR to remove the nginx.conf directly in the container to reduce confusion.

Release note:

Fix security holes in GCE metadata proxy.

ahmetb · 2017-08-25T21:28:17Z

cluster/addons/metadata-proxy/gce/metadata-proxy-configmap.yaml

        }

        # Allow for REST discovery.
        location = / {
+            if ($args ~ "recursive=true") {


have you tried if recursive=1 or recursive=True or recursive=y etc also effective on GCE metadata server. If it's more permissive than we blacklisted here, it's still a problem.

ahmetb · 2017-08-28T17:15:31Z

cluster/addons/metadata-proxy/gce/metadata-proxy-configmap.yaml

                    return 403 "?recursive calls are not allowed by the metadata proxy.";
            }
            proxy_pass http://169.254.169.254;
        }
        location = /computeMetadata/ {
-            if ($args ~ "recursive=true") {
+            if ($args ~* "recursive") {


I'm now afraid this will prevent ?recursive=false, too. @ihmccreery if you've figured out the values that count as true, I think we should add them here. (But then I'll probably worry about if this $args~* check actually normalizes the URLencoding.)

I feel okay about blocking ?recursive=false (and ?recursive=0, etc.), since, as far as I can tell, there's no conventions on a strict set for what to accept as booleans. Params are just a bunch of strings that can be interpreted however the server wants.

(From some messing around, true, 1, yes, and y are all accepted.)

(on is also accepted, tried because I found spring-framework's StringToBooleanConverter.java.)

On a second thought maybe blocking ?recursive is fine. (We can always relax it later if we get a user request. What I'm actually worried is Google rolling out a software/daemon that runs inside the VM and that makes a ?recursive= query to the metadata server, that sounds apocalyptic.)

I'm not familiar with the nginx.conf language enough to see if it warrants a pattern like ^recursive= (maybe at least leave the = in for more precise matching).

ikehz · 2017-08-30T18:30:36Z

@ahmetb I made the regex's a bit more explicit, at your suggestion. If this looks good to you, can I get an lgtm?

Ping @mikedanese @Q-Lee for review?

ahmetb · 2017-08-30T20:01:56Z

/lgtm

ikehz · 2017-08-30T23:22:43Z

/assign @mikedanese

mikedanese · 2017-08-30T23:48:11Z

/approve

k8s-github-robot · 2017-08-30T23:48:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahmetb, ihmccreery, mikedanese

Associated issue: 8867

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~cluster/addons/metadata-proxy/OWNERS~~ [mikedanese]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

destijl · 2017-08-30T23:28:19Z

cluster/addons/metadata-proxy/gce/metadata-proxy-configmap.yaml

        }

        # Allow for REST discovery.
        location = / {
+            if ($args ~* "^(.+&)?recursive=") {
+                    return 403 "?recursive calls are not allowed by the metadata proxy.";


you should mention the SECURE setting in all of these errors so people realise they are getting this error from the proxy rather than the metadata server itself, and know what to change if they need it to work.

This isn't GKE-specific, so we can't mention the GKE feature. The error message needs to also apply to GCE (hence 'metadata proxy').

k8s-github-robot · 2017-09-01T07:11:21Z

Automatic merge from submit-queue (batch tested with PRs 51628, 51637, 51490, 51279, 51302)

…1302-upstream-release-1.7 Automatic merge from submit-queue Automated cherry pick of #51302 Cherry pick of #51302 on release-1.7. #51302: Block instance identity, block recursive=true

k8s-cherrypick-bot · 2017-09-12T13:44:39Z

Commit found in the "release-1.7" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this is an error find help to get your PR picked.

@spxtr

Automatic merge from submit-queue Add a container & job spec to check metadata concealment correctness This checks for some of the fixes made in kubernetes/kubernetes#51302. Ref #8867. I'm unsure of where this should live, but this seems like as good a place as any? @spxtr We discussed, and I'd rather just check it in here now.

Block instance identity, block recursive=true

98a7311

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 25, 2017

k8s-github-robot assigned mikedanese and Q-Lee Aug 25, 2017

k8s-github-robot added the release-note-none Denotes a PR that doesn't merit a release note. label Aug 25, 2017

ahmetb reviewed Aug 25, 2017

View reviewed changes

Made blacklist stricter to deal with alternate versions of true

86c0579

ahmetb reviewed Aug 28, 2017

View reviewed changes

Fix regex's and redirect port

6f4ee0d

ikehz changed the title ~~GCE metadata proxy block instance identity & recursive calls~~ GCE metadata proxy blocks instance identity & recursive calls Aug 30, 2017

ikehz changed the title ~~GCE metadata proxy blocks instance identity & recursive calls~~ GCE metadata proxy blocks instance identity & recursive calls, & excludes port from redirects Aug 30, 2017

k8s-ci-robot assigned ahmetb Aug 30, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 30, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 30, 2017

destijl reviewed Aug 30, 2017

View reviewed changes

This was referenced Aug 31, 2017

Add a container & job spec to check metadata concealment correctness kubernetes-retired/contrib#2738

Merged

Fix nginx.conf to block recursive calls and identity endpoints kubernetes-retired/contrib#2739

Closed

k8s-github-robot merged commit 61bc3aa into kubernetes:master Sep 1, 2017

ikehz mentioned this pull request Sep 1, 2017

Automated cherry pick of #51302 #51810

Merged

wojtek-t added this to the v1.7 milestone Sep 12, 2017

wojtek-t added cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. cherrypick-candidate release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Sep 12, 2017

wojtek-t removed the release-note-none Denotes a PR that doesn't merit a release note. label Sep 12, 2017

k8s-cherrypick-bot removed the cherrypick-candidate label Sep 12, 2017

ikehz mentioned this pull request Oct 12, 2017

Convert proxy to go, update Makefiles and Dockerfiles, and update test GoogleCloudPlatform/k8s-metadata-proxy#1

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GCE metadata proxy blocks instance identity & recursive calls, & excludes port from redirects #51302

GCE metadata proxy blocks instance identity & recursive calls, & excludes port from redirects #51302

ikehz commented Aug 25, 2017 •

edited

Loading

ahmetb Aug 25, 2017

ahmetb Aug 28, 2017

ikehz Aug 29, 2017

ikehz Aug 29, 2017

ikehz Aug 29, 2017 •

edited

Loading

ahmetb Aug 29, 2017 •

edited

Loading

ikehz commented Aug 30, 2017

ahmetb commented Aug 30, 2017

ikehz commented Aug 30, 2017

mikedanese commented Aug 30, 2017

k8s-github-robot commented Aug 30, 2017

destijl Aug 30, 2017

ikehz Aug 31, 2017

k8s-github-robot commented Sep 1, 2017

k8s-cherrypick-bot commented Sep 12, 2017

GCE metadata proxy blocks instance identity & recursive calls, & excludes port from redirects #51302

GCE metadata proxy blocks instance identity & recursive calls, & excludes port from redirects #51302

Conversation

ikehz commented Aug 25, 2017 • edited Loading

ahmetb Aug 25, 2017

Choose a reason for hiding this comment

ahmetb Aug 28, 2017

Choose a reason for hiding this comment

ikehz Aug 29, 2017

Choose a reason for hiding this comment

ikehz Aug 29, 2017

Choose a reason for hiding this comment

ikehz Aug 29, 2017 • edited Loading

Choose a reason for hiding this comment

ahmetb Aug 29, 2017 • edited Loading

Choose a reason for hiding this comment

ikehz commented Aug 30, 2017

ahmetb commented Aug 30, 2017

ikehz commented Aug 30, 2017

mikedanese commented Aug 30, 2017

k8s-github-robot commented Aug 30, 2017

destijl Aug 30, 2017

Choose a reason for hiding this comment

ikehz Aug 31, 2017

Choose a reason for hiding this comment

k8s-github-robot commented Sep 1, 2017

k8s-cherrypick-bot commented Sep 12, 2017

ikehz commented Aug 25, 2017 •

edited

Loading

ikehz Aug 29, 2017 •

edited

Loading

ahmetb Aug 29, 2017 •

edited

Loading