diff --git a/src/common/config/keyprovider.go b/src/common/config/keyprovider.go index eac95612d..163ced369 100644 --- a/src/common/config/keyprovider.go +++ b/src/common/config/keyprovider.go @@ -46,3 +46,13 @@ func (f *FileKeyProvider) Get(params map[string]interface{}) (string, error) { } return string(b), nil } + +// PresetKeyProvider returns the preset key disregarding the parm, this is for testing only +type PresetKeyProvider struct { + Key string +} + +// Get ... +func (p *PresetKeyProvider) Get(params map[string]interface{}) (string, error) { + return p.Key, nil +} diff --git a/src/common/config/keyprovider_test.go b/src/common/config/keyprovider_test.go index 48127d903..c3a025ff2 100644 --- a/src/common/config/keyprovider_test.go +++ b/src/common/config/keyprovider_test.go @@ -15,6 +15,7 @@ package config import ( + "github.com/stretchr/testify/assert" "io/ioutil" "os" "testing" @@ -42,3 +43,12 @@ func TestGetOfFileKeyProvider(t *testing.T) { return } } + +func TestPresetKeyProvider(t *testing.T) { + kp := &PresetKeyProvider{ + Key: "mykey", + } + k, err := kp.Get(nil) + assert.Nil(t, err) + assert.Equal(t, "mykey", k) +} diff --git a/src/common/utils/oidc/helper_test.go b/src/common/utils/oidc/helper_test.go index 6e4f357e2..7ad3266d3 100644 --- a/src/common/utils/oidc/helper_test.go +++ b/src/common/utils/oidc/helper_test.go @@ -16,6 +16,7 @@ package oidc import ( "github.com/goharbor/harbor/src/common" + config2 "github.com/goharbor/harbor/src/common/config" "github.com/goharbor/harbor/src/common/models" "github.com/goharbor/harbor/src/core/config" "github.com/stretchr/testify/assert" @@ -36,8 +37,9 @@ func TestMain(m *testing.M) { common.OIDCClientSecret: "secret", common.ExtEndpoint: "https://harbor.test", } + kp := &config2.PresetKeyProvider{Key: "naa4JtarA1Zsc3uY"} - config.InitWithSettings(conf) + config.InitWithSettings(conf, kp) result := m.Run() if result != 0 { diff --git a/src/common/utils/oidc/secret_test.go b/src/common/utils/oidc/secret_test.go new file mode 100644 index 000000000..1a376f54f --- /dev/null +++ b/src/common/utils/oidc/secret_test.go @@ -0,0 +1,32 @@ +package oidc + +import ( + "context" + "fmt" + "github.com/stretchr/testify/assert" + "testing" +) + +func TestSecretVerifyError(t *testing.T) { + sve := &SecretVerifyError{cause: fmt.Errorf("myerror")} + assert.Equal(t, "failed to verify the secret: myerror", sve.Error()) + err := verifyError(fmt.Errorf("myerror")) + assert.Equal(t, sve, err) +} + +func TestDefaultManagerGetEncryptKey(t *testing.T) { + d := &defaultManager{} + k, err := d.getEncryptKey() + assert.Nil(t, err) + assert.Equal(t, "naa4JtarA1Zsc3uY", k) + d2 := &defaultManager{key: "oldkey"} + k2, err := d2.getEncryptKey() + assert.Nil(t, err) + assert.Equal(t, "oldkey", k2) +} + +func TestPkgVerifySecret(t *testing.T) { + SetHardcodeVerifierForTest("secret") + assert.Nil(t, VerifySecret(context.Background(), 1, "secret")) + assert.NotNil(t, VerifySecret(context.Background(), 1, "not-the-secret")) +} diff --git a/src/common/utils/oidc/testutils.go b/src/common/utils/oidc/testutils.go new file mode 100644 index 000000000..b8ceb624b --- /dev/null +++ b/src/common/utils/oidc/testutils.go @@ -0,0 +1,26 @@ +package oidc + +import "context" +import "errors" + +// This is for testing only +type fakeVerifier struct { + secret string +} + +func (fv *fakeVerifier) SetSecret(uid int, s string, t *Token) error { + return nil +} + +func (fv *fakeVerifier) VerifySecret(ctx context.Context, userID int, secret string) error { + if secret != fv.secret { + return verifyError(errors.New("mismatch")) + } + return nil +} + +// SetHardcodeVerifierForTest overwrite the default secret manager for testing. +// Be reminded this is for testing only. +func SetHardcodeVerifierForTest(s string) { + m = &fakeVerifier{s} +} diff --git a/src/core/api/user.go b/src/core/api/user.go index c9a76f09d..5f1a1c806 100644 --- a/src/core/api/user.go +++ b/src/core/api/user.go @@ -444,7 +444,7 @@ func (ua *UserAPI) getOIDCUserInfo() (*models.OIDCUser, error) { return nil, err } o, err := dao.GetOIDCUserByUserID(ua.userID) - if err != nil { + if err != nil || o == nil { return nil, err } if len(o.Secret) > 0 { diff --git a/src/core/config/config.go b/src/core/config/config.go index 0b6049091..d5259e209 100644 --- a/src/core/config/config.go +++ b/src/core/config/config.go @@ -80,11 +80,14 @@ func Init() error { return nil } -// InitWithSettings init config with predefined configs -func InitWithSettings(cfgs map[string]interface{}) { +// InitWithSettings init config with predefined configs, and optionally overwrite the keyprovider +func InitWithSettings(cfgs map[string]interface{}, kp ...comcfg.KeyProvider) { Init() cfgMgr = comcfg.NewInMemoryManager() cfgMgr.UpdateConfig(cfgs) + if len(kp) > 0 { + keyProvider = kp[0] + } } func initKeyProvider() { diff --git a/src/core/filter/security_test.go b/src/core/filter/security_test.go index 91e2d31a7..bb18f901f 100644 --- a/src/core/filter/security_test.go +++ b/src/core/filter/security_test.go @@ -16,6 +16,8 @@ package filter import ( "context" + "github.com/goharbor/harbor/src/common/utils/oidc" + "github.com/stretchr/testify/require" "log" "net/http" "net/http/httptest" @@ -28,6 +30,7 @@ import ( "github.com/astaxie/beego" beegoctx "github.com/astaxie/beego/context" "github.com/astaxie/beego/session" + config2 "github.com/goharbor/harbor/src/common/config" "github.com/goharbor/harbor/src/common/dao" "github.com/goharbor/harbor/src/common/models" commonsecret "github.com/goharbor/harbor/src/common/secret" @@ -118,6 +121,53 @@ func TestSecretReqCtxModifier(t *testing.T) { assert.NotNil(t, projectManager(ctx)) } +func TestOIDCCliReqCtxModifier(t *testing.T) { + conf := map[string]interface{}{ + common.AUTHMode: common.OIDCAuth, + common.OIDCName: "test", + common.OIDCEndpoint: "https://accounts.google.com", + common.OIDCSkipCertVerify: "false", + common.OIDCScope: "openid, profile, offline_access", + common.OIDCCLientID: "client", + common.OIDCClientSecret: "secret", + common.ExtEndpoint: "https://harbor.test", + } + + kp := &config2.PresetKeyProvider{Key: "naa4JtarA1Zsc3uY"} + config.InitWithSettings(conf, kp) + + modifier := &oidcCliReqCtxModifier{} + req1, err := http.NewRequest(http.MethodGet, + "http://127.0.0.1/api/projects/", nil) + require.Nil(t, err) + ctx1, err := newContext(req1) + require.Nil(t, err) + assert.False(t, modifier.Modify(ctx1)) + req2, err := http.NewRequest(http.MethodGet, "http://127.0.0.1/service/token", nil) + require.Nil(t, err) + ctx2, err := newContext(req2) + require.Nil(t, err) + assert.False(t, modifier.Modify(ctx2)) + username := "oidcModiferTester" + password := "oidcSecret" + u := &models.User{ + Username: username, + Email: "testtest@test.org", + Password: "12345678", + } + id, err := dao.Register(*u) + require.Nil(t, err) + oidc.SetHardcodeVerifierForTest(password) + req3, err := http.NewRequest(http.MethodGet, "http://127.0.0.1/service/token", nil) + require.Nil(t, err) + req3.SetBasicAuth(username, password) + ctx3, err := newContext(req3) + assert.True(t, modifier.Modify(ctx3)) + o := dao.GetOrmer() + _, err = o.Delete(&models.User{UserID: int(id)}) + assert.Nil(t, err) +} + func TestRobotReqCtxModifier(t *testing.T) { req, err := http.NewRequest(http.MethodGet, "http://127.0.0.1/api/projects/", nil) @@ -135,7 +185,7 @@ func TestRobotReqCtxModifier(t *testing.T) { assert.False(t, modified) } -func TestAutoProxyReqCtxModifier(t *testing.T) { +func TestAuthProxyReqCtxModifier(t *testing.T) { server, err := fiter_test.NewAuthProxyTestServer() assert.Nil(t, err)