Repository: Azure/aad-pod-identity
Pull request #500 (2020-02-10 09:51:57)

Deny requests without metadata header to avoid SSRF
<!-- Thank you for helping AAD Pod Identity with a pull request! -->

**Reason for Change**:
<!-- What does this PR improve or fix in AAD Pod Identity? Why is it needed? -->
This PR denies the requests without `metadata` header to avoid SSRF, per [Get a token using HTTP](https://docs.microsoft.com/en-us/azure/active-directory/managed-identities-azure-resources/how-to-use-vm-token#get-a-token-using-http).

**Issue Fixed**:
<!-- If this PR fixes GitHub issue 1234, add "Fixes #1234" to the next line. -->
[#514](https://github.com/Azure/aad-pod-identity/issues/514)


hellokangning: > Thank you for the PR. Looked at the changes and they look good to me. Would you be able to add a new unit test to the PR? If I remember correctly, you should be able to create a new fake server with the app handler that you want to test, send a GET request and see the handler return the error. You can create a new mux server and then pass that mux to `httptest.NewServer` ("net/http/httptest"). This should help you test the handler with a fake server.

Thanks Anish, Updated with UT
aramase: /azp run
azure-pipelines[bot]: <samp>
Azure Pipelines successfully started running 1 pipeline(s).<br>

</samp>
aramase: /azp run
azure-pipelines[bot]: <samp>
Azure Pipelines could not run because the pipeline triggers exclude this branch/path.<br>

</samp>
aramase: /azp run
azure-pipelines[bot]: <samp>
Azure Pipelines successfully started running 1 pipeline(s).<br>

</samp>
hellokangning: > 
> 
> @hellokangning - A small request to change the return header to default. Other wise /lgtm. Thank you !

Done
hellokangning: /azp run
azure-pipelines[bot]: <samp>
Commenter does not have sufficient privileges for PR 500 in repo Azure/aad-pod-identity<br>

</samp>
aramase: /azp run
azure-pipelines[bot]: <samp>
Azure Pipelines successfully started running 1 pipeline(s).<br>

</samp>

Patch:
diff --git a/charts/aad-pod-identity/templates/nmi-daemonset.yaml b/charts/aad-pod-identity/templates/nmi-daemonset.yaml
index 66ede3ff..874ec163 100644
--- a/charts/aad-pod-identity/templates/nmi-daemonset.yaml
+++ b/charts/aad-pod-identity/templates/nmi-daemonset.yaml
@@ -69,7 +69,10 @@ spec:
           {{- end }}
           {{- if .Values.nmi.blockInstanceMetadata }}
           - --block-instance-metadata={{ .Values.nmi.blockInstanceMetadata }}
-          {{- end }}
+          {{- end }}  
+          {{- if .Values.nmi.metadataHeaderRequired}}
+          - --metadata-header-required={{ .Values.nmi.metadataHeaderRequired }}
+          {{- end }} 
         env:
           - name: HOST_IP
             valueFrom:
diff --git a/charts/aad-pod-identity/values.yaml b/charts/aad-pod-identity/values.yaml
index eb04f5ac..4d986fab 100644
--- a/charts/aad-pod-identity/values.yaml
+++ b/charts/aad-pod-identity/values.yaml
@@ -142,6 +142,10 @@ nmi:
   # default is false
   blockInstanceMetadata: ""
 
+  # https://github.com/Azure/aad-pod-identity/blob/master/docs/readmes/README.featureflags.md#metadata-header-required-flag
+  # default is false
+  metadataHeaderRequired: ""
+
 rbac:
   enabled: true
   # NMI requires permissions to get secrets when service principal (type: 1) is used in AzureIdentity.
diff --git a/cmd/nmi/main.go b/cmd/nmi/main.go
index ef460894..c14d4061 100644
--- a/cmd/nmi/main.go
+++ b/cmd/nmi/main.go
@@ -45,6 +45,7 @@ var (
 	enableProfile                      = pflag.Bool("enableProfile", false, "Enable/Disable pprof profiling")
 	enableScaleFeatures                = pflag.Bool("enableScaleFeatures", false, "Enable/Disable features for scale clusters")
 	blockInstanceMetadata              = pflag.Bool("block-instance-metadata", false, "Block instance metadata endpoints")
+	metadataHeaderRequired             = pflag.Bool("metadata-header-required", false, "Metadata header required for querying Azure Instance Metadata service")
 	prometheusPort                     = pflag.String("prometheus-port", "9090", "Prometheus port for metrics")
 	operationMode                      = pflag.String("operation-mode", "standard", "NMI operation mode")
 )
@@ -82,7 +83,7 @@ func main() {
 	exit := make(<-chan struct{})
 	client.Start(exit)
 	*forceNamespaced = *forceNamespaced || "true" == os.Getenv("FORCENAMESPACED")
-	s := server.NewServer(*micNamespace, *blockInstanceMetadata)
+	s := server.NewServer(*micNamespace, *blockInstanceMetadata, *metadataHeaderRequired)
 	s.KubeClient = client
 	s.MetadataIP = *metadataIP
 	s.MetadataPort = *metadataPort
diff --git a/docs/readmes/README.featureflags.md b/docs/readmes/README.featureflags.md
index d449d2a4..78155b6e 100644
--- a/docs/readmes/README.featureflags.md
+++ b/docs/readmes/README.featureflags.md
@@ -48,3 +48,12 @@ their use cases.
 
 Aad-pod-identity has a new flag `immutable-user-msis` which can be used to prevent deletion of specified identities from VM/VMSS.
 The list is comma separated. Example: 00000000-0000-0000-0000-000000000000,11111111-1111-1111-1111-111111111111
+
+## Metadata header required flag
+> Available from 1.6 release
+
+When you query the Instance Metadata Service, you must provide the header `Metadata: true` to ensure the request was not unintentionally redirected. You can see examples of this header in [the Azure documentation](https://docs.microsoft.com/en-us/azure/virtual-machines/linux/instance-metadata-service#using-headers).
+
+This is critical especially when you [acquire an access token](https://docs.microsoft.com/en-us/azure/active-directory/managed-identities-azure-resources/how-to-use-vm-token#get-a-token-using-http) as a mitigation against Server Side Request Forgery (SSRF) attack. 
+
+The `metadataHeaderRequired` flag for NMI will block all requests without Metadata header and return an HTTP 400 response. This flag is disabled by default for compatibility, but recommended for users to enable this feature.
\ No newline at end of file
diff --git a/pkg/nmi/server/server.go b/pkg/nmi/server/server.go
index cb3ab7a9..9d565e52 100644
--- a/pkg/nmi/server/server.go
+++ b/pkg/nmi/server/server.go
@@ -44,10 +44,10 @@ type Server struct {
 	MICNamespace                       string
 	Initialized                        bool
 	BlockInstanceMetadata              bool
+	MetadataHeaderRequired             bool
 	// TokenClient is client that fetches identities and tokens
 	TokenClient nmi.TokenClient
-
-	Reporter *metrics.Reporter
+	Reporter    *metrics.Reporter
 }
 
 // NMIResponse is the response returned to caller
@@ -56,8 +56,13 @@ type NMIResponse struct {
 	ClientID string      `json:"clientid"`
 }
 
+type MetadataResponse struct {
+	Error            string `json:"error"`
+	ErrorDescription string `json:"error_description"`
+}
+
 // NewServer will create a new Server with default values.
-func NewServer(micNamespace string, blockInstanceMetadata bool) *Server {
+func NewServer(micNamespace string, blockInstanceMetadata bool, metadataHeaderRequired bool) *Server {
 	reporter, err := metrics.NewReporter()
 	if err != nil {
 		klog.Errorf("Error creating new reporter to emit metrics: %v", err)
@@ -67,9 +72,10 @@ func NewServer(micNamespace string, blockInstanceMetadata bool) *Server {
 		auth.InitReporter(reporter)
 	}
 	return &Server{
-		MICNamespace:          micNamespace,
-		BlockInstanceMetadata: blockInstanceMetadata,
-		Reporter:              reporter,
+		MICNamespace:           micNamespace,
+		BlockInstanceMetadata:  blockInstanceMetadata,
+		MetadataHeaderRequired: metadataHeaderRequired,
+		Reporter:               reporter,
 	}
 }
 
@@ -199,7 +205,7 @@ func (s *Server) hostHandler(w http.ResponseWriter, r *http.Request) (ns string)
 	hostIP := parseRemoteAddr(r.RemoteAddr)
 	rqClientID, rqResource := parseRequestClientIDAndResource(r)
 
-	podns, podname := parseRequestHeader(r)
+	podns, podname := parsePodInfo(r)
 	if podns == "" || podname == "" {
 		klog.Error("missing podname and podns from request")
 		http.Error(w, "missing 'podname' and 'podns' from request header", http.StatusBadRequest)
@@ -308,6 +314,12 @@ func (s *Server) getTokenForExceptedPod(rqClientID, rqResource string) ([]byte,
 // if the requests contains client id it validates it against the admin
 // configured id.
 func (s *Server) msiHandler(w http.ResponseWriter, r *http.Request) (ns string) {
+	if s.MetadataHeaderRequired && parseMetadata(r) != "true" {
+		klog.Errorf("metadata header is not specified, req.method=%s reg.path=%s req.remote=%s", r.Method, r.URL.Path, parseRemoteAddr(r.RemoteAddr))
+		metadataNotSpecifiedError(w)
+		return
+	}
+
 	podIP := parseRemoteAddr(r.RemoteAddr)
 	rqClientID, rqResource := parseRequestClientIDAndResource(r)
 
@@ -372,7 +384,32 @@ func (s *Server) msiHandler(w http.ResponseWriter, r *http.Request) (ns string)
 	return
 }
 
-func parseRequestHeader(r *http.Request) (podns string, podname string) {
+// Error replies to the request without the specified metadata header.
+// It does not otherwise end the request; the caller should ensure no further
+// writes are done to w.
+func metadataNotSpecifiedError(w http.ResponseWriter) {
+	metadataResp := MetadataResponse{
+		Error:            "invalid_request",
+		ErrorDescription: "Required metadata header not specified",
+	}
+	response, err := json.Marshal(metadataResp)
+	if err != nil {
+		klog.Errorf("failed to marshal metadata response, %+v", err)
+		http.Error(w, err.Error(), http.StatusInternalServerError)
+		return
+	}
+
+	w.Header().Set("Content-Type", "application/json; charset=utf-8")
+	w.WriteHeader(http.StatusBadRequest)
+	fmt.Fprintln(w, string(response))
+	return
+}
+
+func parseMetadata(r *http.Request) (metadata string) {
+	return r.Header.Get("metadata")
+}
+
+func parsePodInfo(r *http.Request) (podns string, podname string) {
 	podns = r.Header.Get("podns")
 	podname = r.Header.Get("podname")
 
@@ -402,6 +439,12 @@ func parseRequestClientIDAndResource(r *http.Request) (clientID string, resource
 
 // defaultPathHandler creates a new request and returns the response body and code
 func (s *Server) defaultPathHandler(w http.ResponseWriter, r *http.Request) (ns string) {
+	if s.MetadataHeaderRequired && parseMetadata(r) != "true" {
+		klog.Errorf("metadata header is not specified, req.method=%s reg.path=%s req.remote=%s", r.Method, r.URL.Path, parseRemoteAddr(r.RemoteAddr))
+		metadataNotSpecifiedError(w)
+		return
+	}
+
 	client := &http.Client{}
 	req, err := http.NewRequest(r.Method, r.URL.String(), r.Body)
 	if err != nil || req == nil {


Test patch:
diff --git a/pkg/nmi/server/server_test.go b/pkg/nmi/server/server_test.go
new file mode 100644
index 00000000..b9ddceb8
--- /dev/null
+++ b/pkg/nmi/server/server_test.go
@@ -0,0 +1,86 @@
+package server
+
+import (
+	"encoding/json"
+	"net/http"
+	"net/http/httptest"
+	"strings"
+	"testing"
+)
+
+var (
+	mux       *http.ServeMux
+	server    *httptest.Server
+	tokenPath = "/metadata/identity/oauth2/token"
+)
+
+func setup() {
+	mux = http.NewServeMux()
+	server = httptest.NewServer(mux)
+}
+
+func teardown() {
+	server.Close()
+}
+
+func TestMsiHandler_NoMetadataHeader(t *testing.T) {
+	setup()
+	defer teardown()
+
+	s := &Server{
+		MetadataHeaderRequired: true,
+	}
+	mux.Handle(tokenPath, appHandler(s.msiHandler))
+
+	req, err := http.NewRequest(http.MethodGet, tokenPath, nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	recorder := httptest.NewRecorder()
+	mux.ServeHTTP(recorder, req)
+
+	if recorder.Code != http.StatusBadRequest {
+		t.Errorf("Unexpected status code %d", recorder.Code)
+	}
+
+	resp := &MetadataResponse{
+		Error:            "invalid_request",
+		ErrorDescription: "Required metadata header not specified",
+	}
+	expected, err := json.Marshal(resp)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if string(expected) != strings.TrimSpace(recorder.Body.String()) {
+		t.Errorf("Unexpected response body %s", recorder.Body.String())
+	}
+}
+
+func TestMsiHandler_NoRemoteAddress(t *testing.T) {
+	setup()
+	defer teardown()
+
+	s := &Server{
+		MetadataHeaderRequired: false,
+	}
+	mux.Handle(tokenPath, appHandler(s.msiHandler))
+
+	req, err := http.NewRequest(http.MethodGet, tokenPath, nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	recorder := httptest.NewRecorder()
+	mux.ServeHTTP(recorder, req)
+
+	if recorder.Code != http.StatusInternalServerError {
+		t.Errorf("Unexpected status code %d", recorder.Code)
+	}
+
+	expected := "request remote address is empty"
+	if expected != strings.TrimSpace(recorder.Body.String()) {
+		t.Errorf("Unexpected response body %s", recorder.Body.String())
+	}
+}