Repository: osbuild/osbuild-composer
Pull request #2433 (2022-03-14 10:38:47)

Client creds grant worker client


Patch:
diff --git a/.github/workflows/trigger-gitlab.yml b/.github/workflows/trigger-gitlab.yml
index 696d4962d..e05c6bce8 100644
--- a/.github/workflows/trigger-gitlab.yml
+++ b/.github/workflows/trigger-gitlab.yml
@@ -76,6 +76,7 @@ jobs:
 
       - name: Push to gitlab
         run: |
+          set -x
           mkdir -p ~/.ssh
           echo "${SCHUTZBOT_SSH_KEY}" > ~/.ssh/id_rsa
           chmod 400 ~/.ssh/id_rsa
diff --git a/cmd/osbuild-worker/jobimpl-depsolve.go b/cmd/osbuild-worker/jobimpl-depsolve.go
index f5cdce415..04056ed88 100644
--- a/cmd/osbuild-worker/jobimpl-depsolve.go
+++ b/cmd/osbuild-worker/jobimpl-depsolve.go
@@ -3,6 +3,8 @@ package main
 import (
 	"fmt"
 
+	"github.com/sirupsen/logrus"
+
 	"github.com/osbuild/osbuild-composer/internal/rpmmd"
 	"github.com/osbuild/osbuild-composer/internal/worker"
 	"github.com/osbuild/osbuild-composer/internal/worker/clienterrors"
@@ -36,6 +38,7 @@ func (impl *DepsolveJobImpl) depsolve(packageSets map[string]rpmmd.PackageSet, r
 }
 
 func (impl *DepsolveJobImpl) Run(job worker.Job) error {
+	logWithId := logrus.WithField("jobId", job.Id())
 	var args worker.DepsolveJob
 	err := job.Args(&args)
 	if err != nil {
@@ -57,10 +60,12 @@ func (impl *DepsolveJobImpl) Run(job worker.Job) error {
 				// This still has the kind/reason format but a kind that's returned
 				// by dnf-json and not explicitly handled here.
 				result.JobError = clienterrors.WorkerClientError(clienterrors.ErrorDNFOtherError, err.Error())
+				logWithId.Errorf("Unhandled dnf-json error in depsolve job: %v", err)
 			}
 		case error:
 			// Error originates from internal/rpmmd, not from dnf-json
 			result.JobError = clienterrors.WorkerClientError(clienterrors.ErrorRPMMDError, err.Error())
+			logWithId.Errorf("rpmmd error in depsolve job: %v", err)
 		}
 	}
 
diff --git a/cmd/osbuild-worker/main.go b/cmd/osbuild-worker/main.go
index 11ce8fa27..e6cacb7d6 100644
--- a/cmd/osbuild-worker/main.go
+++ b/cmd/osbuild-worker/main.go
@@ -142,6 +142,8 @@ func main() {
 		Authentication *struct {
 			OAuthURL         string `toml:"oauth_url"`
 			OfflineTokenPath string `toml:"offline_token"`
+			ClientId         string `toml:"client_id"`
+			ClientSecretPath string `toml:"client_secret"`
 		} `toml:"authentication"`
 		RelaxTimeoutFactor uint   `toml:"RelaxTimeoutFactor"`
 		BasePath           string `toml:"base_path"`
@@ -201,8 +203,11 @@ func main() {
 
 	var client *worker.Client
 	if unix {
-		client = worker.NewClientUnix(address, config.BasePath)
-	} else if config.Authentication != nil && config.Authentication.OfflineTokenPath != "" {
+		client = worker.NewClientUnix(worker.ClientConfig{
+			BaseURL:  address,
+			BasePath: config.BasePath,
+		})
+	} else if config.Authentication != nil {
 		var conf *tls.Config
 		conConf := &connectionConfig{
 			CACertFile: "/etc/osbuild-composer/ca-crt.pem",
@@ -214,21 +219,33 @@ func main() {
 			}
 		}
 
-		var token *string
-		var oAuthURL *string
-		t, err := ioutil.ReadFile(config.Authentication.OfflineTokenPath)
-		if err != nil {
-			logrus.Fatalf("Could not read offline token: %v", err)
+		token := ""
+		if config.Authentication.OfflineTokenPath != "" {
+			t, err := ioutil.ReadFile(config.Authentication.OfflineTokenPath)
+			if err != nil {
+				logrus.Fatalf("Could not read offline token: %v", err)
+			}
+			token = strings.TrimSpace(string(t))
 		}
-		t2 := strings.TrimSpace(string(t))
-		token = &t2
 
-		if config.Authentication.OAuthURL == "" {
-			logrus.Fatal("OAuth URL should be specified together with the offline token")
+		clientSecret := ""
+		if config.Authentication.ClientSecretPath != "" {
+			cs, err := ioutil.ReadFile(config.Authentication.ClientSecretPath)
+			if err != nil {
+				logrus.Fatalf("Could not read client secret: %v", err)
+			}
+			clientSecret = strings.TrimSpace(string(cs))
 		}
-		oAuthURL = &config.Authentication.OAuthURL
 
-		client, err = worker.NewClient(fmt.Sprintf("https://%s", address), conf, token, oAuthURL, config.BasePath)
+		client, err = worker.NewClient(worker.ClientConfig{
+			BaseURL:      fmt.Sprintf("https://%s", address),
+			TlsConfig:    conf,
+			OfflineToken: token,
+			OAuthURL:     config.Authentication.OAuthURL,
+			ClientId:     config.Authentication.ClientId,
+			ClientSecret: clientSecret,
+			BasePath:     config.BasePath,
+		})
 		if err != nil {
 			logrus.Fatalf("Error creating worker client: %v", err)
 		}
@@ -246,7 +263,11 @@ func main() {
 			}
 		}
 
-		client, err = worker.NewClient(fmt.Sprintf("https://%s", address), conf, nil, nil, config.BasePath)
+		client, err = worker.NewClient(worker.ClientConfig{
+			BaseURL:   fmt.Sprintf("https://%s", address),
+			TlsConfig: conf,
+			BasePath:  config.BasePath,
+		})
 		if err != nil {
 			logrus.Fatalf("Error creating worker client: %v", err)
 		}
diff --git a/internal/worker/client.go b/internal/worker/client.go
index 87d2574e3..3e1384784 100644
--- a/internal/worker/client.go
+++ b/internal/worker/client.go
@@ -12,27 +12,32 @@ import (
 	"net/http"
 	"net/url"
 	"sync"
-	"time"
 
 	"github.com/google/uuid"
 
 	"github.com/osbuild/osbuild-composer/internal/worker/api"
 )
 
-type bearerToken struct {
-	AccessToken     string `json:"access_token"`
-	ValidForSeconds int    `json:"expires_in"`
+type Client struct {
+	server       *url.URL
+	requester    *http.Client
+	offlineToken string
+	oAuthURL     string
+	accessToken  string
+	clientId     string
+	clientSecret string
+
+	tokenMu sync.RWMutex
 }
 
-type Client struct {
-	server           *url.URL
-	requester        *http.Client
-	offlineToken     *string
-	oAuthURL         *string
-	lastTokenRefresh *time.Time
-	bearerToken      *bearerToken
-
-	tokenMu *sync.Mutex
+type ClientConfig struct {
+	BaseURL      string
+	TlsConfig    *tls.Config
+	OfflineToken string
+	OAuthURL     string
+	ClientId     string
+	ClientSecret string
+	BasePath     string
 }
 
 type Job interface {
@@ -58,36 +63,56 @@ type job struct {
 	dynamicArgs      []json.RawMessage
 }
 
-func NewClient(baseURL string, conf *tls.Config, offlineToken, oAuthURL *string, basePath string) (*Client, error) {
-	server, err := url.Parse(baseURL)
+type tokenResponse struct {
+	AccessToken string `json:"access_token"`
+}
+
+func NewClient(conf ClientConfig) (*Client, error) {
+	server, err := url.Parse(conf.BaseURL)
 	if err != nil {
 		return nil, err
 	}
 
-	api.BasePath = basePath
+	api.BasePath = conf.BasePath
 
 	server, err = server.Parse(api.BasePath + "/")
 	if err != nil {
 		panic(err)
 	}
 
+	if conf.OAuthURL != "" {
+		if conf.ClientId == "" {
+			return nil, fmt.Errorf("OAuthURL token url specified but no client id")
+		}
+		if conf.OfflineToken == "" && conf.ClientSecret == "" {
+			return nil, fmt.Errorf("OAuthURL token url specified but no client secret or offline token")
+		}
+	}
+
 	requester := &http.Client{}
 	transport := http.DefaultTransport.(*http.Transport).Clone()
-	if conf != nil {
-		transport.TLSClientConfig = conf
+	if conf.TlsConfig != nil {
+		transport.TLSClientConfig = conf.TlsConfig
 	}
 	requester.Transport = transport
 
-	return &Client{server, requester, offlineToken, oAuthURL, nil, nil, &sync.Mutex{}}, nil
+	return &Client{
+		server:       server,
+		requester:    requester,
+		offlineToken: conf.OfflineToken,
+		oAuthURL:     conf.OAuthURL,
+		clientId:     conf.ClientId,
+		clientSecret: conf.ClientSecret,
+	}, nil
 }
 
-func NewClientUnix(path string, basePath string) *Client {
+func NewClientUnix(conf ClientConfig) *Client {
 	server, err := url.Parse("http://localhost/")
 	if err != nil {
 		panic(err)
 	}
 
-	api.BasePath = basePath
+	api.BasePath = conf.BasePath
 
 	server, err = server.Parse(api.BasePath + "/")
 	if err != nil {
@@ -97,67 +122,83 @@ func NewClientUnix(path string, basePath string) *Client {
 	requester := &http.Client{
 		Transport: &http.Transport{
 			DialContext: func(context context.Context, network, addr string) (net.Conn, error) {
-				return net.Dial("unix", path)
+				return net.Dial("unix", conf.BaseURL)
 			},
 		},
 	}
 
-	return &Client{server, requester, nil, nil, nil, nil, nil}
+	return &Client{
+		server:    server,
+		requester: requester,
+	}
 }
 
-// Note: Only call this function with Client.tokenMu locked!
-func (c *Client) refreshBearerToken() error {
-	if c.offlineToken == nil || c.oAuthURL == nil {
-		return fmt.Errorf("No offline token or oauth url available")
-	}
+func (c *Client) refreshAccessToken() error {
+	c.tokenMu.Lock()
+	defer c.tokenMu.Unlock()
 
 	data := url.Values{}
-	data.Set("grant_type", "refresh_token")
-	data.Set("client_id", "rhsm-api")
-	data.Set("refresh_token", *c.offlineToken)
+	if c.offlineToken != "" {
+		data.Set("grant_type", "refresh_token")
+		data.Set("client_id", c.clientId)
+		data.Set("refresh_token", c.offlineToken)
+	}
+	if c.clientSecret != "" {
+		data.Set("grant_type", "client_credentials")
+		data.Set("client_id", c.clientId)
+		data.Set("client_secret", c.clientSecret)
+	}
 
-	t := time.Now()
-	resp, err := http.PostForm(*c.oAuthURL, data)
+	resp, err := http.PostForm(c.oAuthURL, data)
 	if err != nil {
 		return err
 	}
 
-	var bt bearerToken
-	err = json.NewDecoder(resp.Body).Decode(&bt)
+	var tr tokenResponse
+	err = json.NewDecoder(resp.Body).Decode(&tr)
 	if err != nil {
 		return err
 	}
 
-	c.bearerToken = &bt
-	c.lastTokenRefresh = &t
+	c.accessToken = tr.AccessToken
 	return nil
 }
 
-func (c *Client) NewRequest(method, url string, body io.Reader) (*http.Request, error) {
+func (c *Client) NewRequest(method, url string, headers map[string]string, body io.Reader) (*http.Response, error) {
+	token := func() string {
+		c.tokenMu.RLock()
+		defer c.tokenMu.RUnlock()
+		return c.accessToken
+	}
+
 	req, err := http.NewRequest(method, url, body)
 	if err != nil {
 		return nil, err
 	}
 
-	// If we're using OAUTH, add the Bearer token
-	if c.offlineToken != nil {
-		// make sure we have a valid token
-		var d time.Duration
-		c.tokenMu.Lock()
-		defer c.tokenMu.Unlock()
-		if c.lastTokenRefresh != nil {
-			d = time.Since(*c.lastTokenRefresh)
-		}
-		if c.bearerToken == nil || d.Seconds() >= (float64(c.bearerToken.ValidForSeconds)*0.8) {
-			err = c.refreshBearerToken()
-			if err != nil {
-				return nil, err
-			}
+	if c.oAuthURL != "" {
+		req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", token()))
+	}
+
+	for k, v := range headers {
+		req.Header.Add(k, v)
+	}
+
+	resp, err := c.requester.Do(req)
+	if err != nil {
+		return nil, err
+	}
+
+	if resp.StatusCode == http.StatusUnauthorized && c.oAuthURL != "" {
+		err = c.refreshAccessToken()
+		if err != nil {
+			return nil, err
 		}
 
-		req.Header.Add("Authorization", fmt.Sprintf("Bearer %s", c.bearerToken.AccessToken))
+		req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", token()))
+		resp, err = c.requester.Do(req)
 	}
-	return req, nil
+	return resp, err
 }
 
 func (c *Client) RequestJob(types []string, arch string) (Job, error) {
@@ -176,16 +217,10 @@ func (c *Client) RequestJob(types []string, arch string) (Job, error) {
 		panic(err)
 	}
 
-	req, err := c.NewRequest("POST", url.String(), &buf)
+	response, err := c.NewRequest("POST", url.String(), map[string]string{"Content-Type": "application/json"}, &buf)
 	if err != nil {
 		return nil, err
 	}
-	req.Header.Add("Content-Type", "application/json")
-
-	response, err := c.requester.Do(req)
-	if err != nil {
-		return nil, fmt.Errorf("error requesting job: %v", err)
-	}
 	defer response.Body.Close()
 
 	if response.StatusCode == http.StatusNoContent {
@@ -273,14 +308,7 @@ func (j *job) Update(result interface{}) error {
 		panic(err)
 	}
 
-	req, err := j.client.NewRequest("PATCH", j.location, &buf)
-	if err != nil {
-		panic(err)
-	}
-
-	req.Header.Add("Content-Type", "application/json")
-
-	response, err := j.client.requester.Do(req)
+	response, err := j.client.NewRequest("PATCH", j.location, map[string]string{"Content-Type": "application/json"}, &buf)
 	if err != nil {
 		return fmt.Errorf("error fetching job info: %v", err)
 	}
@@ -294,12 +322,7 @@ func (j *job) Update(result interface{}) error {
 }
 
 func (j *job) Canceled() (bool, error) {
-	req, err := j.client.NewRequest("GET", j.location, nil)
-	if err != nil {
-		return false, err
-	}
-
-	response, err := j.client.requester.Do(req)
+	response, err := j.client.NewRequest("GET", j.location, map[string]string{}, nil)
 	if err != nil {
 		return false, fmt.Errorf("error fetching job info: %v", err)
 	}
@@ -333,14 +356,7 @@ func (j *job) UploadArtifact(name string, reader io.Reader) error {
 		panic(err)
 	}
 
-	req, err := j.client.NewRequest("PUT", loc.String(), reader)
-	if err != nil {
-		return fmt.Errorf("cannot create request: %v", err)
-	}
-
-	req.Header.Add("Content-Type", "application/octet-stream")
-
-	response, err := j.client.requester.Do(req)
+	response, err := j.client.NewRequest("PUT", loc.String(), map[string]string{"Content-Type": "application/octet-stream"}, reader)
 	if err != nil {
 		return fmt.Errorf("error uploading artifact: %v", err)
 	}
diff --git a/templates/packer/ansible/roles/common/files/worker-initialization-scripts/client_credentials.sh b/templates/packer/ansible/roles/common/files/worker-initialization-scripts/client_credentials.sh
new file mode 100755
index 000000000..10cbb6cda
--- /dev/null
+++ b/templates/packer/ansible/roles/common/files/worker-initialization-scripts/client_credentials.sh
@@ -0,0 +1,26 @@
+#!/bin/bash
+set -eo pipefail
+source /tmp/cloud_init_vars
+
+echo "Writing client credentials."
+
+if [[ -z "$CLIENT_CREDENTIALS_ARN" ]]; then
+  echo "CLIENT_CREDENTIALS_ARN not defined, skipping."
+  exit 0
+fi
+
+# get client credentials
+/usr/local/bin/aws secretsmanager get-secret-value \
+  --endpoint-url "${SECRETS_MANAGER_ENDPOINT_URL}" \
+  --secret-id "${CLIENT_CREDENTIALS_ARN}" | jq -r ".SecretString" > /tmp/client-credentials.json
+
+CLIENT_ID=$(jq -r ".client_id" /tmp/client-credentials.json)
+jq -r ".client_secret" /tmp/client-credentials.json > /etc/osbuild-worker/client-secret
+rm -f /tmp/client-credentials.json
+
+sudo tee -a /etc/osbuild-worker/osbuild-worker.toml > /dev/null << EOF
+[authentication]
+oauth_url = "https://identity.api.openshift.com/auth/realms/rhoas/protocol/openid-connect/token"
+client_id = "${CLIENT_ID}"
+client_secret = "/etc/osbuild-worker/client-secret"
+EOF
diff --git a/templates/packer/ansible/roles/common/files/worker-initialization-scripts/offline_token.sh b/templates/packer/ansible/roles/common/files/worker-initialization-scripts/offline_token.sh
index ba6e5a470..c47f1b563 100755
--- a/templates/packer/ansible/roles/common/files/worker-initialization-scripts/offline_token.sh
+++ b/templates/packer/ansible/roles/common/files/worker-initialization-scripts/offline_token.sh
@@ -1,9 +1,14 @@
 #!/bin/bash
-set -euo pipefail
+set -eo pipefail
 source /tmp/cloud_init_vars
 
 echo "Writing offline token."
 
+if [[ -z "$OFFLINE_TOKEN_ARN" ]]; then
+  echo "OFFLINE_TOKEN_ARN not defined, skipping."
+  exit 0
+fi
+
 # get offline token
 /usr/local/bin/aws secretsmanager get-secret-value \
   --endpoint-url "${SECRETS_MANAGER_ENDPOINT_URL}" \
@@ -15,5 +20,6 @@ rm -f /tmp/offline-token.json
 sudo tee -a /etc/osbuild-worker/osbuild-worker.toml > /dev/null << EOF
 [authentication]
 oauth_url = "https://sso.redhat.com/auth/realms/redhat-external/protocol/openid-connect/token"
+client_id = "rhsm-api"
 offline_token = "/etc/osbuild-worker/offline-token"
 EOF
diff --git a/templates/packer/ansible/roles/common/files/worker-initialization.service b/templates/packer/ansible/roles/common/files/worker-initialization.service
index 7bc04f481..cbb6f7558 100644
--- a/templates/packer/ansible/roles/common/files/worker-initialization.service
+++ b/templates/packer/ansible/roles/common/files/worker-initialization.service
@@ -10,6 +10,7 @@ ExecStart=touch /etc/worker-first-boot
 ExecStart=/usr/local/libexec/worker-initialization-scripts/set_hostname.sh
 ExecStart=/usr/local/libexec/worker-initialization-scripts/vector.sh
 ExecStart=/usr/local/libexec/worker-initialization-scripts/offline_token.sh
+ExecStart=/usr/local/libexec/worker-initialization-scripts/client_credentials.sh
 ExecStart=/usr/local/libexec/worker-initialization-scripts/subscription_manager.sh
 ExecStart=/usr/local/libexec/worker-initialization-scripts/get_aws_creds.sh
 ExecStart=/usr/local/libexec/worker-initialization-scripts/get_azure_creds.sh


Test patch:
diff --git a/internal/worker/server_test.go b/internal/worker/server_test.go
index 1de2391a2..36bf653ba 100644
--- a/internal/worker/server_test.go
+++ b/internal/worker/server_test.go
@@ -342,7 +342,13 @@ func TestOAuth(t *testing.T) {
 	defer workSrv.Close()
 
 	/* Check that the worker supplies the access token  */
+	calls := 0
 	proxySrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		if calls == 0 {
+			require.Equal(t, "Bearer", r.Header.Get("Authorization"))
+			w.WriteHeader(http.StatusUnauthorized)
+			return
+		}
 		require.Equal(t, "Bearer accessToken!", r.Header.Get("Authorization"))
 		handler.ServeHTTP(w, r)
 	}))
@@ -351,6 +357,7 @@ func TestOAuth(t *testing.T) {
 	offlineToken := "someOfflineToken"
 	/* Start oauth srv supplying the bearer token */
 	oauthSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		calls += 1
 		require.Equal(t, "POST", r.Method)
 		err = r.ParseForm()
 		require.NoError(t, err)
@@ -360,11 +367,9 @@ func TestOAuth(t *testing.T) {
 		require.Equal(t, offlineToken, r.FormValue("refresh_token"))
 
 		bt := struct {
-			AccessToken     string `json:"access_token"`
-			ValidForSeconds int    `json:"expires_in"`
+			AccessToken string `json:"access_token"`
 		}{
 			"accessToken!",
-			900,
 		}
 
 		w.WriteHeader(http.StatusOK)
@@ -391,7 +396,14 @@ func TestOAuth(t *testing.T) {
 	_, err = workerServer.EnqueueOSBuild(arch.Name(), &worker.OSBuildJob{Manifest: manifest}, "")
 	require.NoError(t, err)
 
-	client, err := worker.NewClient(proxySrv.URL, nil, &offlineToken, &oauthSrv.URL, "/api/image-builder-worker/v1")
+	client, err := worker.NewClient(worker.ClientConfig{
+		BaseURL:      proxySrv.URL,
+		TlsConfig:    nil,
+		ClientId:     "rhsm-api",
+		OfflineToken: offlineToken,
+		OAuthURL:     oauthSrv.URL,
+		BasePath:     "/api/image-builder-worker/v1",
+	})
 	require.NoError(t, err)
 	job, err := client.RequestJob([]string{"osbuild"}, arch.Name())
 	require.NoError(t, err)
diff --git a/test/cases/api.sh b/test/cases/api.sh
index 852e6c053..bb7ddd209 100755
--- a/test/cases/api.sh
+++ b/test/cases/api.sh
@@ -1294,6 +1294,7 @@ EOF
 cat <<EOF | sudo tee "/etc/osbuild-worker/osbuild-worker.toml"
 [authentication]
 oauth_url = http://localhost:8081/token
+client_id = "rhsm-api"
 offline_token = "/etc/osbuild-worker/token"
 EOF
 
diff --git a/test/cases/multi-tenancy.sh b/test/cases/multi-tenancy.sh
index 8a76fe211..00455e2db 100755
--- a/test/cases/multi-tenancy.sh
+++ b/test/cases/multi-tenancy.sh
@@ -90,6 +90,7 @@ EOF
 sudo tee "/etc/osbuild-worker/osbuild-worker.toml" >/dev/null <<EOF
 [authentication]
 oauth_url = "http://localhost:8081/token"
+client_id = "rhsm-api"
 offline_token = "/etc/osbuild-worker/token"
 
 [koji.localhost.kerberos]