Merge pull request #446 from sapcc/fix-creation-of-managed-replicas

fix sublease token propagation during creation of managed replica accounts

internal/api/keppel/accounts.go

@@ -36,6 +36,8 @@ import (
 	"github.com/sapcc/keppel/internal/models"
 )
 
+const SubleaseHeader = "X-Keppel-Sublease-Token"
+
 func (a *API) handleGetAccounts(w http.ResponseWriter, r *http.Request) {
 	httpapi.IdentifyEndpoint(r, "/keppel/v1/accounts")
 	var accounts []models.Account
@@ -119,12 +121,12 @@ func (a *API) handlePutAccount(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	getSubleaseTokenCallback := func(_ models.Peer) (string, *keppel.RegistryV2Error) {
-		subleaseToken, err := SubleaseTokenFromRequest(r)
+	getSubleaseTokenCallback := func(_ models.Peer) (keppel.SubleaseToken, error) {
+		t, err := keppel.ParseSubleaseToken(r.Header.Get(SubleaseHeader))
 		if err != nil {
-			return "", keppel.AsRegistryV2Error(err)
+			return keppel.SubleaseToken{}, fmt.Errorf("malformed %s header: %w", SubleaseHeader, err)
 		}
-		return subleaseToken.Secret, nil
+		return t, nil
 	}
 	finalizeAccountCallback := func(account *models.Account) *keppel.RegistryV2Error {
 		if account.IsManaged {
@@ -186,7 +188,7 @@ func (a *API) handlePostAccountSublease(w http.ResponseWriter, r *http.Request)
 		return
 	}
 
-	st := SubleaseToken{
+	st := keppel.SubleaseToken{
 		AccountName:     account.Name,
 		PrimaryHostname: a.cfg.APIPublicHostname,
 	}
@@ -196,16 +198,7 @@ func (a *API) handlePostAccountSublease(w http.ResponseWriter, r *http.Request)
 	if respondwith.ErrorText(w, err) {
 		return
 	}
-
-	// only serialize SubleaseToken if it contains a secret at all
-	var serialized string
-	if st.Secret == "" {
-		serialized = ""
-	} else {
-		serialized = st.Serialize()
-	}
-
-	respondwith.JSON(w, http.StatusOK, map[string]any{"sublease_token": serialized})
+	respondwith.JSON(w, http.StatusOK, map[string]any{"sublease_token": st.Serialize()})
 }
 
 func (a *API) handleGetSecurityScanPolicies(w http.ResponseWriter, r *http.Request) {

internal/auth/uid_peer.go

@@ -50,7 +50,8 @@ func (uid *PeerUserIdentity) PluginTypeID() string {
 // HasPermission implements the keppel.UserIdentity interface.
 func (uid *PeerUserIdentity) HasPermission(perm keppel.Permission, tenantID string) bool {
 	// allow universal pull access for replication purposes
-	return perm == keppel.CanViewAccount || perm == keppel.CanPullFromAccount
+	// (CanChangeAccount is required to issue sublease tokens as part of managed account creation)
+	return perm == keppel.CanViewAccount || perm == keppel.CanChangeAccount || perm == keppel.CanPullFromAccount
 }
 
 // UserType implements the keppel.UserIdentity interface.

internal/client/peer/requests.go

@@ -84,15 +84,15 @@ func (c Client) GetForeignAccountConfigurationInto(ctx context.Context, target a
 
 // GetSubleaseToken asks the peer for a sublease token for this account to replicate it on another Keppel instance.
 // Only the primary instance of an account can be asked for a sublease token.
-func (c Client) GetSubleaseToken(ctx context.Context, accountName models.AccountName) (string, error) {
+func (c Client) GetSubleaseToken(ctx context.Context, accountName models.AccountName) (keppel.SubleaseToken, error) {
 	reqURL := c.buildRequestURL("keppel/v1/accounts/" + string(accountName) + "/sublease")
 
 	respBodyBytes, respStatusCode, _, err := c.doRequest(ctx, http.MethodPost, reqURL, http.NoBody, nil)
 	if err != nil {
-		return "", err
+		return keppel.SubleaseToken{}, err
 	}
 	if respStatusCode != http.StatusOK {
-		return "", fmt.Errorf("while obtaining sublease token with POST %s: expected 200, got %d with response: %s",
+		return keppel.SubleaseToken{}, fmt.Errorf("while obtaining sublease token with POST %s: expected 200, got %d with response: %s",
 			reqURL, respStatusCode, string(respBodyBytes))
 	}
 
@@ -101,9 +101,9 @@ func (c Client) GetSubleaseToken(ctx context.Context, accountName models.Account
 	}{}
 	err = jsonUnmarshalStrict(respBodyBytes, &data)
 	if err != nil {
-		return "", fmt.Errorf("while parsing sublease token response from POST %s: %w", reqURL, err)
+		return keppel.SubleaseToken{}, fmt.Errorf("while parsing sublease token response from POST %s: %w", reqURL, err)
 	}
-	return data.SubleaseToken, nil
+	return keppel.ParseSubleaseToken(data.SubleaseToken)
 }
 
 // PerformReplicaSync uses the replica-sync API to perform an optimized

internal/keppel/sublease_token.go

@@ -0,0 +1,69 @@
+/*******************************************************************************
+*
+* Copyright 2020-2024 SAP SE
+*
+* Licensed under the Apache License, Version 2.0 (the "License");
+* you may not use this file except in compliance with the License.
+* You should have received a copy of the License along with this
+* program. If not, you may obtain a copy of the License at
+*
+*     http://www.apache.org/licenses/LICENSE-2.0
+*
+* Unless required by applicable law or agreed to in writing, software
+* distributed under the License is distributed on an "AS IS" BASIS,
+* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+* See the License for the specific language governing permissions and
+* limitations under the License.
+*
+*******************************************************************************/
+
+package keppel
+
+import (
+	"encoding/base64"
+	"encoding/json"
+
+	"github.com/sapcc/keppel/internal/models"
+)
+
+// SubleaseToken is the internal structure of a sublease token. Only the secret
+// is passed on to the federation driver. The other attributes are only
+// informational. GUIs/CLIs can display these data to the user for confirmation
+// when the token is entered.
+type SubleaseToken struct {
+	AccountName     models.AccountName `json:"account"`
+	PrimaryHostname string             `json:"primary"`
+	Secret          string             `json:"secret"`
+}
+
+// Serialize returns the Base64-encoded JSON of this token. This is the format
+// that gets passed to the user.
+func (t SubleaseToken) Serialize() string {
+	// only serialize SubleaseToken if it contains a secret at all
+	if t.Secret == "" {
+		return ""
+	}
+
+	buf, _ := json.Marshal(t)
+	return base64.StdEncoding.EncodeToString(buf)
+}
+
+// ParseSubleaseToken constructs a SubleaseToken from its serialized form.
+func ParseSubleaseToken(in string) (SubleaseToken, error) {
+	if in == "" {
+		// empty sublease token is acceptable for federation drivers that don't need one
+		return SubleaseToken{}, nil
+	}
+
+	buf, err := base64.StdEncoding.DecodeString(in)
+	if err != nil {
+		return SubleaseToken{}, err
+	}
+
+	var t SubleaseToken
+	err = json.Unmarshal(buf, &t)
+	if err != nil {
+		return SubleaseToken{}, err
+	}
+	return t, nil
+}

internal/processor/accounts.go

@@ -64,7 +64,7 @@ var looksLikeAPIVersionRx = regexp.MustCompile(`^v[0-9][1-9]*$`)
 var ErrAccountNameEmpty = errors.New("account name cannot be empty string")
 
 // CreateOrUpdate can be used on an API account and returns the database representation of it.
-func (p *Processor) CreateOrUpdateAccount(ctx context.Context, account keppel.Account, userInfo audittools.UserInfo, r *http.Request, getSubleaseToken func(models.Peer) (string, *keppel.RegistryV2Error), setCustomFields func(*models.Account) *keppel.RegistryV2Error) (models.Account, *keppel.RegistryV2Error) {
+func (p *Processor) CreateOrUpdateAccount(ctx context.Context, account keppel.Account, userInfo audittools.UserInfo, r *http.Request, getSubleaseToken func(models.Peer) (keppel.SubleaseToken, error), setCustomFields func(*models.Account) *keppel.RegistryV2Error) (models.Account, *keppel.RegistryV2Error) {
 	if account.Name == "" {
 		return models.Account{}, keppel.AsRegistryV2Error(ErrAccountNameEmpty)
 	}
@@ -239,11 +239,11 @@ func (p *Processor) CreateOrUpdateAccount(ctx context.Context, account keppel.Ac
 		// sublease tokens are only relevant when creating replica accounts
 		subleaseTokenSecret := ""
 		if targetAccount.UpstreamPeerHostName != "" {
-			var rerr *keppel.RegistryV2Error
-			subleaseTokenSecret, rerr = getSubleaseToken(peer)
-			if rerr != nil {
-				return models.Account{}, rerr.WithStatus(http.StatusBadRequest)
+			subleaseToken, err := getSubleaseToken(peer)
+			if err != nil {
+				return models.Account{}, keppel.AsRegistryV2Error(err).WithStatus(http.StatusBadRequest)
 			}
+			subleaseTokenSecret = subleaseToken.Secret
 		}
 
 		// check permission to claim account name (this only happens here because

internal/tasks/account_management.go

@@ -198,7 +198,7 @@ func (j *Janitor) createOrUpdateManagedAccount(ctx context.Context, account kepp
 	userIdentity := janitorUserIdentity{TaskName: "account-management"}
 
 	// if the managed account is an internal replica, the processor needs to ask the primary account for a sublease token
-	getSubleaseToken := func(peer models.Peer) (string, *keppel.RegistryV2Error) {
+	getSubleaseToken := func(peer models.Peer) (keppel.SubleaseToken, error) {
 		viewScope := auth.Scope{
 			ResourceType: "keppel_account",
 			ResourceName: string(account.Name),
@@ -207,14 +207,10 @@ func (j *Janitor) createOrUpdateManagedAccount(ctx context.Context, account kepp
 
 		client, err := peerclient.New(ctx, j.cfg, peer, viewScope)
 		if err != nil {
-			return "", keppel.AsRegistryV2Error(err)
+			return keppel.SubleaseToken{}, err
 		}
 
-		subleaseToken, err := client.GetSubleaseToken(ctx, account.Name)
-		if err != nil {
-			return "", keppel.AsRegistryV2Error(err)
-		}
-		return subleaseToken, nil
+		return client.GetSubleaseToken(ctx, account.Name)
 	}
 
 	// some fields are not contained in `keppel.Account` and must be handled through a custom callback

internal/tasks/account_management_test.go

@@ -73,6 +73,35 @@ func TestAccountManagementBasic(t *testing.T) {
 	`)
 }
 
+func TestAccountManagementWithReplicaCreation(t *testing.T) {
+	test.WithRoundTripper(func(_ *test.RoundTripper) {
+		_, s1 := setup(t)
+		j2, s2 := setupReplica(t, s1, "on_first_use")
+
+		tr, tr0 := easypg.NewTracker(t, s2.DB.DbMap.Db)
+		tr0.Ignore()
+
+		// The setup already includes an account "test1" set up on both ends, but we
+		// want to test the setup of a managed replica account, so we will use a
+		// fresh account called "managed" instead.
+		mustDo(t, s1.DB.Insert(&models.Account{Name: "managed", AuthTenantID: "managedauthtenant"}))
+		s1.FD.NextSubleaseTokenSecretToIssue = "thisisasecret"
+		s2.FD.ValidSubleaseTokenSecrets["managed"] = "thisisasecret"
+
+		// test seeding the replica on the secondary side
+		s2.AMD.ConfigPath = "./fixtures/account_management_replica.json"
+		job := j2.EnforceManagedAccountsJob(s2.Registry)
+		expectSuccess(t, job.ProcessOne(s2.Ctx))
+
+		// check that the replica was created
+		tr.DBChanges().AssertEqualf(`
+				INSERT INTO accounts (name, auth_tenant_id, upstream_peer_hostname, security_scan_policies_json, is_managed, next_enforcement_at) VALUES ('managed', 'managedauthtenant', 'registry.example.org', 'null', TRUE, %[1]d);
+			`,
+			s2.Clock.Now().Add(1*time.Hour).Unix(),
+		)
+	})
+}
+
 func TestAccountManagementWithComplexDeletion(t *testing.T) {
 	j, s := setup(t)
 	job := j.EnforceManagedAccountsJob(s.Registry)

internal/tasks/fixtures/account_management_replica.json

@@ -0,0 +1,12 @@
+{
+  "accounts": [
+    {
+      "name": "managed",
+      "auth_tenant_id": "managedauthtenant",
+      "replication": {
+        "strategy": "on_first_use",
+        "upstream": "registry.example.org"
+      }
+    }
+  ]
+}

internal/tasks/shared_test.go

@@ -34,6 +34,7 @@ var (
 
 func setup(t *testing.T, opts ...test.SetupOption) (*Janitor, test.Setup) {
 	params := []test.SetupOption{
+		test.WithKeppelAPI, // for issuing sublease tokens to registry-secondary (if any)
 		test.WithPeerAPI,
 		test.WithAccount(models.Account{Name: "test1", AuthTenantID: "test1authtenant"}),
 		test.WithRepo(models.Repository{AccountName: "test1", Name: "foo"}),