From 422894a0f523c1f5fe6e4ff47b82ff5b3af6e747 Mon Sep 17 00:00:00 2001 From: Daniel Jiang Date: Sun, 11 Jul 2021 21:49:42 +0800 Subject: [PATCH] Remediate the empty subiss problem This commit include 2 changes to mitigate and remediate the problem described in #15241 1. When the token is to be updated in the "oidc_user" table, make sure only the column "token" will be udpated. 2. Restore the subiss column for the record that has this column cleared by mistake, by decoding the persisted token. Signed-off-by: Daniel Jiang --- src/core/main.go | 4 +++ src/pkg/oidc/fix.go | 70 ++++++++++++++++++++++++++++++++++++++++ src/pkg/oidc/fix_test.go | 39 ++++++++++++++++++++++ src/pkg/oidc/secret.go | 3 +- 4 files changed, 115 insertions(+), 1 deletion(-) create mode 100644 src/pkg/oidc/fix.go create mode 100644 src/pkg/oidc/fix_test.go diff --git a/src/core/main.go b/src/core/main.go index 0233811d1..e4bcb7331 100755 --- a/src/core/main.go +++ b/src/core/main.go @@ -27,6 +27,7 @@ import ( "time" configCtl "github.com/goharbor/harbor/src/controller/config" + "github.com/goharbor/harbor/src/pkg/oidc" "github.com/astaxie/beego" _ "github.com/astaxie/beego/session/redis" @@ -234,6 +235,9 @@ func main() { } log.Infof("Version: %s, Git commit: %s", version.ReleaseVersion, version.GitCommit) + + log.Info("Fix empty subiss for meta info data.") + oidc.FixEmptySubIss(orm.Context()) beego.RunWithMiddleWares("", middlewares.MiddleWares()...) } diff --git a/src/pkg/oidc/fix.go b/src/pkg/oidc/fix.go new file mode 100644 index 000000000..c667bed7a --- /dev/null +++ b/src/pkg/oidc/fix.go @@ -0,0 +1,70 @@ +package oidc + +import ( + "context" + "encoding/base64" + "encoding/json" + "fmt" + "strings" + + "github.com/goharbor/harbor/src/common/models" + "github.com/goharbor/harbor/src/common/utils" + "github.com/goharbor/harbor/src/lib/errors" + "github.com/goharbor/harbor/src/lib/log" + "github.com/goharbor/harbor/src/pkg/oidc/dao" +) + +// FixEmptySubIss remediates the issue https://github.com/goharbor/harbor/issues/15241 +// by restoring the subiss via the persisted token +func FixEmptySubIss(ctx context.Context) (bool, error) { + metaMgr := NewMetaMgr() + meta, err := metaMgr.GetBySubIss(ctx, "", "") + if errors.IsNotFoundErr(err) { + log.Info("Not found any records with empty subiss, good to go.") + return false, nil + } else if err != nil { + return false, fmt.Errorf("failed to query for OIDC info with empty subiss: %v", err) + } + log.Infof("Found record with empty subiss, user ID: %d, trying to restore...", meta.UserID) + key, err := keyLoader.encryptKey() + if err != nil { + return false, fmt.Errorf("failed to load the key for encryption/decryption： %v", err) + } + tokenStr, err := utils.ReversibleDecrypt(meta.Token, key) + if err != nil { + return false, fmt.Errorf("failed to decrypt token： %v", err) + } + // Extract the subject issuer from persisted ID token + tok := &Token{} + err = json.Unmarshal(([]byte)(tokenStr), tok) + if err != nil { + return false, fmt.Errorf("failed to decode token: %v", err) + } + rawIDToken := tok.RawIDToken + parts := strings.Split(rawIDToken, ".") + if len(parts) < 2 { + return false, fmt.Errorf("malformed jwt, got %d parts", len(parts)) + } + data, err := base64.RawURLEncoding.DecodeString(parts[1]) + if err != nil { + return false, fmt.Errorf("malformed jwt: %v", err) + } + p := &struct { + Issuer string `json:"iss"` + Subject string `json:"sub"` + }{} + err = json.Unmarshal(data, p) + if err != nil { + return false, fmt.Errorf("failed to extract subject and issuer from ID token: %v", err) + } + newRec := &models.OIDCUser{ + ID: meta.ID, + SubIss: p.Subject + p.Issuer, + } + metaDao := dao.NewMetaDao() + if err := metaDao.Update(ctx, newRec, "subiss"); err != nil { + return false, fmt.Errorf("failed to update meta info in DB: %v", err) + } + log.Infof("Restored subiss for user, id: %d", meta.UserID) + return true, nil +} diff --git a/src/pkg/oidc/fix_test.go b/src/pkg/oidc/fix_test.go new file mode 100644 index 000000000..78373092e --- /dev/null +++ b/src/pkg/oidc/fix_test.go @@ -0,0 +1,39 @@ +package oidc + +import ( + "testing" + + "github.com/goharbor/harbor/src/common/models" + "github.com/goharbor/harbor/src/lib/orm" + "github.com/goharbor/harbor/src/pkg/oidc/dao" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestFixEmptySubIss(t *testing.T) { + // --- no empty subiss + ctx := orm.Context() + res, err := FixEmptySubIss(ctx) + assert.False(t, res) + assert.Nil(t, err) + // --- create a record with empty subiss + dao := dao.NewMetaDao() + tok := `t9tmJEKwGq+jUcdPksRVQGJdI1BdtBB3PuPGzoOKzM0OJeQlXUXcI4Nlh40cFbWvGOuwi5AWpivSzTLq9l+A8swyPC4MGCUCd7cWWQSD2NMibVwJv5/d6YB9AQ5smdCr5G+0n/zMkWAII7UPfFZQQjeErxsug9bNsRV02Ti6IE979e3eEYZqaarDzxOo3wmHEOhigDvuFFSlCr3KF4+xXMh9o6hmQciq4Lwyh9dBfnh5hHB8bMuFW79rUul9mDODcQrlXuL+OzFqM3XB3Al2zVtswFN+gznQemqPHfmteT6dIOx39W72nQe3aOzHZp0nzLmTlpMkBPmpWKeWgX4KLlUbcbDzDAfIrPLrj+wvZ43zyMWSjwnAWmIiBtN2FuoQHiC6vyZfAP7Jix+9ZeJAzEV/Ha2r1g6wmHc7qxyv+SngbsuVWED/4k2ggjFm6ma6W+jqBSoJIy+CEM2zrQaAPyOAZlVuAN4lPDoeehrZMQnQf/zNYrCyWuVdeoeekmiin0hpDoClFox96Eq+nECUT5HwCGuSuNhnT6UFzst96JONChYr5F1CuGg2zY16yS9i40Fl8+lHPzjC3/2Ap5ysWGsr5GTnUEJp+vgimRmPkbIhCVgeTybnhNbsImBsYG+ykc7zi7Dx+7MeMtGbLjPv7Xglc62n40LSXJV/UPBYt+A0DbeWLWw3wm/D7ZejNNKdyEV8mG/f5wMr1PshTxUG0AuJe8YR4GeMYYPH/QNpAlD/8PfY8kfXb/+EUqZqOzbH07iFyDqr7mlwXJ7lYiOwHn4GQEhVCH4PH5M7w02eGZvHcmHI19Ki7UxzU14hbpN+4IpBrMLKmNOQdVozMQBmEiC7vER9Rsgc87hL6foQ0oMB3JvJ3t1vKTHH+8YmMSaPzag352rqVhkbQWOXhP2XBvYeISwQaFx2McqsqLv1+WhI6x8c7cVtP7cTuPJEi6e0KUEqShog9/fPZBQ1jZHUYVHRjld3lx4aiQVwbo0Znl1uDW2lM9T0ClDqlp0NME4FaLZPvXd5NsrLmeXVALMKGJtSj4+OBRmGXhOWULJTH6FtC072PXUwhEqIzkrFSwA8vEvbF67mpSLDvf8L3W8hgknMQn7Coa6Pf5+UwpRcU/eG9eCddDTmXCEgsCKqLG/Lr4QeFjlprAxSHYbtcBLhDvCZJj4116KYlZyJuJrh6C2tJiOUjYlIGwScSoHdiKXIL4Di/Rl+g9VtYfIXIfgZMnHPcD89OXolnYqZt1WA3iRlSEE5f3Cfo2Y16LVMpJYEXzNdvr6eb1oAk0nr24pttGOsQSmL4VGtzKr0MIRPBqBIR/siMhufw8kCRIhMuqi0/orf4R+6ZXvBA0jYMpVzrCPTBNWM6P3qFmtL0cYPwlRRbcY8JlLRhAO84Be4gvHty1NXBzaVLsxaLvId/PJvso/fIcBrhZawNBunNSun70USN2DXLwsjUx7HsArZqjZ02uGgteTEuXkKWEDQ79gA2qatRL4uy69tJc6OLpziKQbYNdoECeOoNTnxw98jihwEu4+6LpO3mC0AsdBitEn8ND798TLVuSc1WFe0nX8aOoDLfKbn03UO4iWhlgQjxKfUYopqbRFE1ueCyPeZlBTUaQsvRQ4xQi12uxBFuQ11o5Bukxmf4ThOffInHaeXpeXZDI6R9Ap1qmYrA2ZF6j17hJlYtySt1KfojApw/WXQJOZsWj0cdEnJ8dHsxJiteOrLOMkvEXXFORJDkdNjKAlraozvMM5WdqqUfa/huy0LH33oCi49PRgSyWcS+Yn1ouU6MkuYkbCKClztjKiTdQKnIbTqg4LapRX9RCqkUbOyRE4jk2j07MPHxwF45xQcQ2Glz3DZc7cWD+XzDsPkaknTRisAUIPKC2PeCdwlyHOjfuCeutklzO+VeQUpgMvtauPVfxezwiEbDReD8NGCS8gkGMwRt9QkskIHsnvGa5bI9FXZKI88YQPn5zmQ9OFmnyOKWQ1fM4xfNzqF5G8x4/InhX6WwbCPqj0ildhofEeCaay/fdhdJdCUW3PSlkXG5E3xQf6+pwRR/iGWgNtgRH46QSJrlg5fpeAXVr+q/2K/x+YAEQpCyN82l7VHgx8baYiW4l+nd48ypNnCwyayvVhrq3S3F1CozaEhm/Q7JbBIscApOV25517gpX00zSeo0CFfRpctGUj1NQXC+qRESF4Z93PZmUKknAyd` + rec := &models.OIDCUser{ + UserID: 1, + Secret: "C328t5lfBJBgBOysSUVWO4GiAM9yOFEkoX5P15Fqz1pQzKdZVGPzYN9qWcHfvt4q", + Token: tok, + SubIss: "", + } + _, err2 := dao.Create(ctx, rec) + require.Nil(t, err2) + updated, err := FixEmptySubIss(ctx) + assert.True(t, updated) + assert.Nil(t, err) + mgr := NewMetaMgr() + data, err := mgr.GetByUserID(ctx, 1) + require.Nil(t, err) + assert.Equal(t, "CgV0ZXN0MxIFbG9jYWwhttps://10.202.250.18:5554/dex", data.SubIss) + err3 := dao.DeleteByUserID(ctx, 1) + require.Nil(t, err3) +} diff --git a/src/pkg/oidc/secret.go b/src/pkg/oidc/secret.go index f476b4c6a..5a52646d7 100644 --- a/src/pkg/oidc/secret.go +++ b/src/pkg/oidc/secret.go @@ -112,7 +112,8 @@ func (dm *defaultManager) VerifySecret(ctx context.Context, username string, sec } encToken, _ := utils.ReversibleEncrypt(string(tb), key) oidcUser.Token = encToken - err = dm.metaDao.Update(ctx, oidcUser) + // only updates the token column of the record + err = dm.metaDao.Update(ctx, oidcUser, "token") if err != nil { log.Errorf("Failed to persist token, user id: %d, error: %v", oidcUser.UserID, err) }