Repository: prometheus/common
Pull request #272 (2021-02-14 22:09:49)

Support authorization_credentials
This backward-compatible patch enables authorization header type to be
set.

For consistency, bearer_token is renamed to authorization_credentials
and a new authorization_type is introduced.

The terminology is taken from
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Authorization


refs:
https://github.com/prometheus/common/issues/271
https://github.com/prometheus/prometheus/issues/5107

Signed-off-by: Julien Pivotto <roidelapluie@inuits.eu>

roidelapluie: Questions:
Do we want this?


Do we want the form:


```
authorization_credentials: superlongbearer
authorization_type: APIKEY
```
or
```
authorization:
   credentials: superlongbearer
   type: APIKEY
```

Note: the latter would mean always 2 lines (in pretty yaml) and therefore you can't easily `sed` your existing configuration file.

```
authorization_credentials: superlongbearer
```
or
```
authorization:
   credentials: superlongbearer
```
dazinator: My vote would be for the latter format

```
authorization:
   credentials: superlongbearer
```
roidelapluie: I have implemented the latter. cc @beorn7 
beorn7: I'll review properly ASAP.

In the meantime, you can fix the CI failures. (o:

Patch:
diff --git a/config/http_config.go b/config/http_config.go
index 4dd8875..6060a09 100644
--- a/config/http_config.go
+++ b/config/http_config.go
@@ -52,6 +52,21 @@ func (a *BasicAuth) SetDirectory(dir string) {
 	a.PasswordFile = JoinDir(dir, a.PasswordFile)
 }
 
+// Authorization contains HTTP authorization credentials.
+type Authorization struct {
+	Type            string `yaml:"type,omitempty"`
+	Credentials     Secret `yaml:"credentials,omitempty"`
+	CredentialsFile string `yaml:"credentials_file,omitempty"`
+}
+
+// SetDirectory joins any relative file paths with dir.
+func (a *Authorization) SetDirectory(dir string) {
+	if a == nil {
+		return
+	}
+	a.CredentialsFile = JoinDir(dir, a.CredentialsFile)
+}
+
 // URL is a custom URL type that allows validation at configuration load time.
 type URL struct {
 	*url.URL
@@ -84,14 +99,21 @@ func (u URL) MarshalYAML() (interface{}, error) {
 type HTTPClientConfig struct {
 	// The HTTP basic authentication credentials for the targets.
 	BasicAuth *BasicAuth `yaml:"basic_auth,omitempty"`
-	// The bearer token for the targets.
+	// The HTTP authorization credentials for the targets.
+	Authorization *Authorization `yaml:"authorization,omitempty"`
+	// The bearer token for the targets. Deprecated in favour of
+	// Authorization.Credentials.
 	BearerToken Secret `yaml:"bearer_token,omitempty"`
-	// The bearer token file for the targets.
+	// The bearer token file for the targets. Deprecated in favour of
+	// Authorization.CredentialsFile.
 	BearerTokenFile string `yaml:"bearer_token_file,omitempty"`
 	// HTTP proxy server to use to connect to the targets.
 	ProxyURL URL `yaml:"proxy_url,omitempty"`
 	// TLSConfig to use to connect to the targets.
 	TLSConfig TLSConfig `yaml:"tls_config,omitempty"`
+	// Used to make sure that the configuration is valid and that BearerToken to
+	// Authorization.Credentials change has been handled.
+	valid bool
 }
 
 // SetDirectory joins any relative file paths with dir.
@@ -101,12 +123,14 @@ func (c *HTTPClientConfig) SetDirectory(dir string) {
 	}
 	c.TLSConfig.SetDirectory(dir)
 	c.BasicAuth.SetDirectory(dir)
+	c.Authorization.SetDirectory(dir)
 	c.BearerTokenFile = JoinDir(dir, c.BearerTokenFile)
 }
 
 // Validate validates the HTTPClientConfig to check only one of BearerToken,
 // BasicAuth and BearerTokenFile is configured.
 func (c *HTTPClientConfig) Validate() error {
+	// Backwards compatibility with the bearer_token field.
 	if len(c.BearerToken) > 0 && len(c.BearerTokenFile) > 0 {
 		return fmt.Errorf("at most one of bearer_token & bearer_token_file must be configured")
 	}
@@ -116,6 +140,37 @@ func (c *HTTPClientConfig) Validate() error {
 	if c.BasicAuth != nil && (string(c.BasicAuth.Password) != "" && c.BasicAuth.PasswordFile != "") {
 		return fmt.Errorf("at most one of basic_auth password & password_file must be configured")
 	}
+	if c.Authorization != nil {
+		if len(c.BearerToken) > 0 || len(c.BearerTokenFile) > 0 {
+			return fmt.Errorf("authorization is not compatible with bearer_token & bearer_token_file")
+		}
+		if string(c.Authorization.Credentials) != "" && c.Authorization.CredentialsFile != "" {
+			return fmt.Errorf("at most one of authorization credentials & credentials_file must be configured")
+		}
+		c.Authorization.Type = strings.TrimSpace(c.Authorization.Type)
+		if len(c.Authorization.Type) == 0 {
+			c.Authorization.Type = "Bearer"
+		}
+		if strings.ToLower(c.Authorization.Type) == "basic" {
+			return fmt.Errorf(`authorization type cannot be set to "basic", use "basic_auth" instead`)
+		}
+		if c.BasicAuth != nil {
+			return fmt.Errorf("at most one of basic_auth & authorization must be configured")
+		}
+	} else {
+		if len(c.BearerToken) > 0 {
+			c.Authorization = &Authorization{Credentials: c.BearerToken}
+			c.Authorization.Type = "Bearer"
+			c.BearerToken = ""
+		}
+		if len(c.BearerTokenFile) > 0 {
+			c.Authorization = &Authorization{CredentialsFile: c.BearerTokenFile}
+			c.Authorization.Type = "Bearer"
+			c.BearerTokenFile = ""
+		}
+	}
+
+	c.valid = true
 	return nil
 }
 
@@ -152,6 +207,12 @@ func NewClientFromConfig(cfg HTTPClientConfig, name string, disableKeepAlives, e
 // NewRoundTripperFromConfig returns a new HTTP RoundTripper configured for the
 // given config.HTTPClientConfig. The name is used as go-conntrack metric label.
 func NewRoundTripperFromConfig(cfg HTTPClientConfig, name string, disableKeepAlives, enableHTTP2 bool) (http.RoundTripper, error) {
+	// Make sure that the configuration is valid.
+	if !cfg.valid {
+		if err := cfg.Validate(); err != nil {
+			return nil, err
+		}
+	}
 	newRT := func(tlsConfig *tls.Config) (http.RoundTripper, error) {
 		// The only timeout we care about is the configured scrape timeout.
 		// It is applied on request. So we leave out any timings here.
@@ -186,12 +247,12 @@ func NewRoundTripperFromConfig(cfg HTTPClientConfig, name string, disableKeepAli
 			}
 		}
 
-		// If a bearer token is provided, create a round tripper that will set the
+		// If a authorization_credentials is provided, create a round tripper that will set the
 		// Authorization header correctly on each request.
-		if len(cfg.BearerToken) > 0 {
-			rt = NewBearerAuthRoundTripper(cfg.BearerToken, rt)
-		} else if len(cfg.BearerTokenFile) > 0 {
-			rt = NewBearerAuthFileRoundTripper(cfg.BearerTokenFile, rt)
+		if cfg.Authorization != nil && len(cfg.Authorization.Credentials) > 0 {
+			rt = NewAuthorizationCredentialsRoundTripper(cfg.Authorization.Type, cfg.Authorization.Credentials, rt)
+		} else if cfg.Authorization != nil && len(cfg.Authorization.CredentialsFile) > 0 {
+			rt = NewAuthorizationCredentialsFileRoundTripper(cfg.Authorization.Type, cfg.Authorization.CredentialsFile, rt)
 		}
 
 		if cfg.BasicAuth != nil {
@@ -214,58 +275,61 @@ func NewRoundTripperFromConfig(cfg HTTPClientConfig, name string, disableKeepAli
 	return newTLSRoundTripper(tlsConfig, cfg.TLSConfig.CAFile, newRT)
 }
 
-type bearerAuthRoundTripper struct {
-	bearerToken Secret
-	rt          http.RoundTripper
+type authorizationCredentialsRoundTripper struct {
+	authType        string
+	authCredentials Secret
+	rt              http.RoundTripper
 }
 
-// NewBearerAuthRoundTripper adds the provided bearer token to a request unless the authorization
-// header has already been set.
-func NewBearerAuthRoundTripper(token Secret, rt http.RoundTripper) http.RoundTripper {
-	return &bearerAuthRoundTripper{token, rt}
+// NewAuthorizationCredentialsRoundTripper adds the provided credentials to a
+// request unless the authorization header has already been set.
+func NewAuthorizationCredentialsRoundTripper(authType string, authCredentials Secret, rt http.RoundTripper) http.RoundTripper {
+	return &authorizationCredentialsRoundTripper{authType, authCredentials, rt}
 }
 
-func (rt *bearerAuthRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) {
+func (rt *authorizationCredentialsRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) {
 	if len(req.Header.Get("Authorization")) == 0 {
 		req = cloneRequest(req)
-		req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", string(rt.bearerToken)))
+		req.Header.Set("Authorization", fmt.Sprintf("%s %s", rt.authType, string(rt.authCredentials)))
 	}
 	return rt.rt.RoundTrip(req)
 }
 
-func (rt *bearerAuthRoundTripper) CloseIdleConnections() {
+func (rt *authorizationCredentialsRoundTripper) CloseIdleConnections() {
 	if ci, ok := rt.rt.(closeIdler); ok {
 		ci.CloseIdleConnections()
 	}
 }
 
-type bearerAuthFileRoundTripper struct {
-	bearerFile string
-	rt         http.RoundTripper
+type authorizationCredentialsFileRoundTripper struct {
+	authType            string
+	authCredentialsFile string
+	rt                  http.RoundTripper
 }
 
-// NewBearerAuthFileRoundTripper adds the bearer token read from the provided file to a request unless
-// the authorization header has already been set. This file is read for every request.
-func NewBearerAuthFileRoundTripper(bearerFile string, rt http.RoundTripper) http.RoundTripper {
-	return &bearerAuthFileRoundTripper{bearerFile, rt}
+// NewAuthorizationCredentialsFileRoundTripper adds the authorization
+// credentials read from the provided file to a request unless the authorization
+// header has already been set. This file is read for every request.
+func NewAuthorizationCredentialsFileRoundTripper(authType, authCredentialsFile string, rt http.RoundTripper) http.RoundTripper {
+	return &authorizationCredentialsFileRoundTripper{authType, authCredentialsFile, rt}
 }
 
-func (rt *bearerAuthFileRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) {
+func (rt *authorizationCredentialsFileRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) {
 	if len(req.Header.Get("Authorization")) == 0 {
-		b, err := ioutil.ReadFile(rt.bearerFile)
+		b, err := ioutil.ReadFile(rt.authCredentialsFile)
 		if err != nil {
-			return nil, fmt.Errorf("unable to read bearer token file %s: %s", rt.bearerFile, err)
+			return nil, fmt.Errorf("unable to read authorization credentials file %s: %s", rt.authCredentialsFile, err)
 		}
-		bearerToken := strings.TrimSpace(string(b))
+		authCredentials := strings.TrimSpace(string(b))
 
 		req = cloneRequest(req)
-		req.Header.Set("Authorization", "Bearer "+bearerToken)
+		req.Header.Set("Authorization", fmt.Sprintf("%s %s", rt.authType, authCredentials))
 	}
 
 	return rt.rt.RoundTrip(req)
 }
 
-func (rt *bearerAuthFileRoundTripper) CloseIdleConnections() {
+func (rt *authorizationCredentialsFileRoundTripper) CloseIdleConnections() {
 	if ci, ok := rt.rt.(closeIdler); ok {
 		ci.CloseIdleConnections()
 	}


Test patch:
diff --git a/config/http_config_test.go b/config/http_config_test.go
index 8596e80..d0cdee8 100644
--- a/config/http_config_test.go
+++ b/config/http_config_test.go
@@ -49,13 +49,17 @@ const (
 	MissingCert           = "missing/cert.crt"
 	MissingKey            = "missing/secret.key"
 
-	ExpectedMessage        = "I'm here to serve you!!!"
-	BearerToken            = "theanswertothegreatquestionoflifetheuniverseandeverythingisfortytwo"
-	BearerTokenFile        = "testdata/bearer.token"
-	MissingBearerTokenFile = "missing/bearer.token"
-	ExpectedBearer         = "Bearer " + BearerToken
-	ExpectedUsername       = "arthurdent"
-	ExpectedPassword       = "42"
+	ExpectedMessage                   = "I'm here to serve you!!!"
+	AuthorizationCredentials          = "theanswertothegreatquestionoflifetheuniverseandeverythingisfortytwo"
+	AuthorizationCredentialsFile      = "testdata/bearer.token"
+	AuthorizationType                 = "APIKEY"
+	BearerToken                       = AuthorizationCredentials
+	BearerTokenFile                   = AuthorizationCredentialsFile
+	MissingBearerTokenFile            = "missing/bearer.token"
+	ExpectedBearer                    = "Bearer " + BearerToken
+	ExpectedAuthenticationCredentials = AuthorizationType + " " + BearerToken
+	ExpectedUsername                  = "arthurdent"
+	ExpectedPassword                  = "42"
 )
 
 var invalidHTTPClientConfigs = []struct {
@@ -74,6 +78,22 @@ var invalidHTTPClientConfigs = []struct {
 		httpClientConfigFile: "testdata/http.conf.basic-auth.too-much.bad.yaml",
 		errMsg:               "at most one of basic_auth password & password_file must be configured",
 	},
+	{
+		httpClientConfigFile: "testdata/http.conf.mix-bearer-and-creds.bad.yaml",
+		errMsg:               "authorization is not compatible with bearer_token & bearer_token_file",
+	},
+	{
+		httpClientConfigFile: "testdata/http.conf.auth-creds-and-file-set.too-much.bad.yaml",
+		errMsg:               "at most one of authorization credentials & credentials_file must be configured",
+	},
+	{
+		httpClientConfigFile: "testdata/http.conf.basic-auth-and-auth-creds.too-much.bad.yaml",
+		errMsg:               "at most one of basic_auth & authorization must be configured",
+	},
+	{
+		httpClientConfigFile: "testdata/http.conf.auth-creds-no-basic.bad.yaml",
+		errMsg:               `authorization type cannot be set to "basic", use "basic_auth" instead`,
+	},
 }
 
 func newTestServer(handler func(w http.ResponseWriter, r *http.Request)) (*httptest.Server, error) {
@@ -170,6 +190,87 @@ func TestNewClientFromConfig(t *testing.T) {
 					fmt.Fprint(w, ExpectedMessage)
 				}
 			},
+		}, {
+			clientConfig: HTTPClientConfig{
+				Authorization: &Authorization{Credentials: BearerToken},
+				TLSConfig: TLSConfig{
+					CAFile:             TLSCAChainPath,
+					CertFile:           ClientCertificatePath,
+					KeyFile:            ClientKeyNoPassPath,
+					ServerName:         "",
+					InsecureSkipVerify: false},
+			},
+			handler: func(w http.ResponseWriter, r *http.Request) {
+				bearer := r.Header.Get("Authorization")
+				if bearer != ExpectedBearer {
+					fmt.Fprintf(w, "The expected Bearer Authorization (%s) differs from the obtained Bearer Authorization (%s)",
+						ExpectedBearer, bearer)
+				} else {
+					fmt.Fprint(w, ExpectedMessage)
+				}
+			},
+		}, {
+			clientConfig: HTTPClientConfig{
+				Authorization: &Authorization{CredentialsFile: AuthorizationCredentialsFile, Type: AuthorizationType},
+				TLSConfig: TLSConfig{
+					CAFile:             TLSCAChainPath,
+					CertFile:           ClientCertificatePath,
+					KeyFile:            ClientKeyNoPassPath,
+					ServerName:         "",
+					InsecureSkipVerify: false},
+			},
+			handler: func(w http.ResponseWriter, r *http.Request) {
+				bearer := r.Header.Get("Authorization")
+				if bearer != ExpectedAuthenticationCredentials {
+					fmt.Fprintf(w, "The expected Bearer Authorization (%s) differs from the obtained Bearer Authorization (%s)",
+						ExpectedAuthenticationCredentials, bearer)
+				} else {
+					fmt.Fprint(w, ExpectedMessage)
+				}
+			},
+		}, {
+			clientConfig: HTTPClientConfig{
+				Authorization: &Authorization{
+					Credentials: AuthorizationCredentials,
+					Type:        AuthorizationType,
+				},
+				TLSConfig: TLSConfig{
+					CAFile:             TLSCAChainPath,
+					CertFile:           ClientCertificatePath,
+					KeyFile:            ClientKeyNoPassPath,
+					ServerName:         "",
+					InsecureSkipVerify: false},
+			},
+			handler: func(w http.ResponseWriter, r *http.Request) {
+				bearer := r.Header.Get("Authorization")
+				if bearer != ExpectedAuthenticationCredentials {
+					fmt.Fprintf(w, "The expected Bearer Authorization (%s) differs from the obtained Bearer Authorization (%s)",
+						ExpectedAuthenticationCredentials, bearer)
+				} else {
+					fmt.Fprint(w, ExpectedMessage)
+				}
+			},
+		}, {
+			clientConfig: HTTPClientConfig{
+				Authorization: &Authorization{
+					CredentialsFile: BearerTokenFile,
+				},
+				TLSConfig: TLSConfig{
+					CAFile:             TLSCAChainPath,
+					CertFile:           ClientCertificatePath,
+					KeyFile:            ClientKeyNoPassPath,
+					ServerName:         "",
+					InsecureSkipVerify: false},
+			},
+			handler: func(w http.ResponseWriter, r *http.Request) {
+				bearer := r.Header.Get("Authorization")
+				if bearer != ExpectedBearer {
+					fmt.Fprintf(w, "The expected Bearer Authorization (%s) differs from the obtained Bearer Authorization (%s)",
+						ExpectedBearer, bearer)
+				} else {
+					fmt.Fprint(w, ExpectedMessage)
+				}
+			},
 		}, {
 			clientConfig: HTTPClientConfig{
 				BasicAuth: &BasicAuth{
@@ -304,7 +405,7 @@ func TestMissingBearerAuthFile(t *testing.T) {
 		t.Fatal("No error is returned here")
 	}
 
-	if !strings.Contains(err.Error(), "unable to read bearer token file missing/bearer.token: open missing/bearer.token: no such file or directory") {
+	if !strings.Contains(err.Error(), "unable to read authorization credentials file missing/bearer.token: open missing/bearer.token: no such file or directory") {
 		t.Fatal("wrong error message being returned")
 	}
 }
@@ -323,7 +424,7 @@ func TestBearerAuthRoundTripper(t *testing.T) {
 	}, nil, nil)
 
 	// Normal flow.
-	bearerAuthRoundTripper := NewBearerAuthRoundTripper(BearerToken, fakeRoundTripper)
+	bearerAuthRoundTripper := NewAuthorizationCredentialsRoundTripper("Bearer", BearerToken, fakeRoundTripper)
 	request, _ := http.NewRequest("GET", "/hitchhiker", nil)
 	request.Header.Set("User-Agent", "Douglas Adams mind")
 	_, err := bearerAuthRoundTripper.RoundTrip(request)
@@ -332,7 +433,7 @@ func TestBearerAuthRoundTripper(t *testing.T) {
 	}
 
 	// Should honor already Authorization header set.
-	bearerAuthRoundTripperShouldNotModifyExistingAuthorization := NewBearerAuthRoundTripper(newBearerToken, fakeRoundTripper)
+	bearerAuthRoundTripperShouldNotModifyExistingAuthorization := NewAuthorizationCredentialsRoundTripper("Bearer", newBearerToken, fakeRoundTripper)
 	request, _ = http.NewRequest("GET", "/hitchhiker", nil)
 	request.Header.Set("Authorization", ExpectedBearer)
 	_, err = bearerAuthRoundTripperShouldNotModifyExistingAuthorization.RoundTrip(request)
@@ -351,7 +452,7 @@ func TestBearerAuthFileRoundTripper(t *testing.T) {
 	}, nil, nil)
 
 	// Normal flow.
-	bearerAuthRoundTripper := NewBearerAuthFileRoundTripper(BearerTokenFile, fakeRoundTripper)
+	bearerAuthRoundTripper := NewAuthorizationCredentialsFileRoundTripper("Bearer", BearerTokenFile, fakeRoundTripper)
 	request, _ := http.NewRequest("GET", "/hitchhiker", nil)
 	request.Header.Set("User-Agent", "Douglas Adams mind")
 	_, err := bearerAuthRoundTripper.RoundTrip(request)
@@ -360,7 +461,7 @@ func TestBearerAuthFileRoundTripper(t *testing.T) {
 	}
 
 	// Should honor already Authorization header set.
-	bearerAuthRoundTripperShouldNotModifyExistingAuthorization := NewBearerAuthFileRoundTripper(MissingBearerTokenFile, fakeRoundTripper)
+	bearerAuthRoundTripperShouldNotModifyExistingAuthorization := NewAuthorizationCredentialsFileRoundTripper("Bearer", MissingBearerTokenFile, fakeRoundTripper)
 	request, _ = http.NewRequest("GET", "/hitchhiker", nil)
 	request.Header.Set("Authorization", ExpectedBearer)
 	_, err = bearerAuthRoundTripperShouldNotModifyExistingAuthorization.RoundTrip(request)
diff --git a/config/testdata/http.conf.auth-creds-and-file-set.too-much.bad.yaml b/config/testdata/http.conf.auth-creds-and-file-set.too-much.bad.yaml
new file mode 100644
index 0000000..b2da2b8
--- /dev/null
+++ b/config/testdata/http.conf.auth-creds-and-file-set.too-much.bad.yaml
@@ -0,0 +1,3 @@
+authorization:
+  credentials: bearertoken
+  credentials_file: key.txt
diff --git a/config/testdata/http.conf.auth-creds-no-basic.bad.yaml b/config/testdata/http.conf.auth-creds-no-basic.bad.yaml
new file mode 100644
index 0000000..1f1f326
--- /dev/null
+++ b/config/testdata/http.conf.auth-creds-no-basic.bad.yaml
@@ -0,0 +1,2 @@
+authorization:
+  type: Basic
diff --git a/config/testdata/http.conf.basic-auth-and-auth-creds.too-much.bad.yaml b/config/testdata/http.conf.basic-auth-and-auth-creds.too-much.bad.yaml
new file mode 100644
index 0000000..89b3c0d
--- /dev/null
+++ b/config/testdata/http.conf.basic-auth-and-auth-creds.too-much.bad.yaml
@@ -0,0 +1,5 @@
+basic_auth:
+  username: user
+  password: foo
+authorization:
+  credentials: foo
diff --git a/config/testdata/http.conf.mix-bearer-and-creds.bad.yaml b/config/testdata/http.conf.mix-bearer-and-creds.bad.yaml
new file mode 100644
index 0000000..905fea6
--- /dev/null
+++ b/config/testdata/http.conf.mix-bearer-and-creds.bad.yaml
@@ -0,0 +1,3 @@
+authorization:
+  type: APIKEY
+bearer_token: foo