From 438c96035870dd7f3f96298d016f532f2d9c4b14 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roger=20Torrentsgener=C3=B3s?= Date: Wed, 31 Jan 2024 17:48:34 +0100 Subject: [PATCH 1/5] chore: handle empty or invalid secrets nicely MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Roger Torrentsgenerós --- flyteadmin/auth/authzserver/provider.go | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/flyteadmin/auth/authzserver/provider.go b/flyteadmin/auth/authzserver/provider.go index be33ac28f4..7246dcaa4c 100644 --- a/flyteadmin/auth/authzserver/provider.go +++ b/flyteadmin/auth/authzserver/provider.go @@ -147,6 +147,9 @@ func NewProvider(ctx context.Context, cfg config.AuthorizationServer, sm core.Se if err != nil { return Provider{}, fmt.Errorf("failed to read secretTokenHash file. Error: %w", err) } + if tokenHashBase64 == "" { + return Provider{}, fmt.Errorf("failed to read secretTokenHash. Error: empty value") + } secret, err := base64.RawStdEncoding.DecodeString(tokenHashBase64) if err != nil { @@ -158,8 +161,14 @@ func NewProvider(ctx context.Context, cfg config.AuthorizationServer, sm core.Se if err != nil { return Provider{}, fmt.Errorf("failed to read token signing RSA Key. Error: %w", err) } + if privateKeyPEM == "" { + return Provider{}, fmt.Errorf("failed to read token signing RSA Key. Error: empty value") + } block, _ := pem.Decode([]byte(privateKeyPEM)) + if block == nil { + return Provider{}, fmt.Errorf("failed to decode token signing RSA Key. Error: no PEM data found") + } privateKey, err := x509.ParsePKCS1PrivateKey(block.Bytes) if err != nil { return Provider{}, fmt.Errorf("failed to parse PKCS1PrivateKey. Error: %w", err) @@ -196,8 +205,14 @@ func NewProvider(ctx context.Context, cfg config.AuthorizationServer, sm core.Se // Try to load old key to validate tokens using it to support key rotation. privateKeyPEM, err = sm.Get(ctx, cfg.OldTokenSigningRSAKeySecretName) + if privateKeyPEM == "" { + return Provider{}, fmt.Errorf("failed to read PKCS1PrivateKey. Error: empty value") + } if err == nil { block, _ = pem.Decode([]byte(privateKeyPEM)) + if block == nil { + return Provider{}, fmt.Errorf("failed to decode PKCS1PrivateKey. Error: no PEM data found") + } oldPrivateKey, err := x509.ParsePKCS1PrivateKey(block.Bytes) if err != nil { return Provider{}, fmt.Errorf("failed to parse PKCS1PrivateKey. Error: %w", err) From 4b96a5aac671071a49186f9417233b4d8b33e39e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roger=20Torrentsgener=C3=B3s?= Date: Mon, 5 Feb 2024 14:12:58 +0100 Subject: [PATCH 2/5] chore: this belongs inside the if clause MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Roger Torrentsgenerós --- flyteadmin/auth/authzserver/provider.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/flyteadmin/auth/authzserver/provider.go b/flyteadmin/auth/authzserver/provider.go index 7246dcaa4c..b2948331fb 100644 --- a/flyteadmin/auth/authzserver/provider.go +++ b/flyteadmin/auth/authzserver/provider.go @@ -205,10 +205,10 @@ func NewProvider(ctx context.Context, cfg config.AuthorizationServer, sm core.Se // Try to load old key to validate tokens using it to support key rotation. privateKeyPEM, err = sm.Get(ctx, cfg.OldTokenSigningRSAKeySecretName) - if privateKeyPEM == "" { - return Provider{}, fmt.Errorf("failed to read PKCS1PrivateKey. Error: empty value") - } if err == nil { + if privateKeyPEM == "" { + return Provider{}, fmt.Errorf("failed to read PKCS1PrivateKey. Error: empty value") + } block, _ = pem.Decode([]byte(privateKeyPEM)) if block == nil { return Provider{}, fmt.Errorf("failed to decode PKCS1PrivateKey. Error: no PEM data found") From 78df65adf374662ac12b2324787bba56e7fc7d70 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roger=20Torrentsgener=C3=B3s?= Date: Mon, 5 Feb 2024 14:13:14 +0100 Subject: [PATCH 3/5] test: use old key too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Roger Torrentsgenerós --- flyteadmin/auth/authzserver/provider_test.go | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/flyteadmin/auth/authzserver/provider_test.go b/flyteadmin/auth/authzserver/provider_test.go index 4659e603a4..2eab258069 100644 --- a/flyteadmin/auth/authzserver/provider_test.go +++ b/flyteadmin/auth/authzserver/provider_test.go @@ -7,7 +7,6 @@ import ( "crypto/x509" "encoding/base64" "encoding/pem" - "fmt" "testing" "time" @@ -34,7 +33,7 @@ func newMockProvider(t testing.TB) (Provider, auth.SecretsSet) { var buf bytes.Buffer assert.NoError(t, pem.Encode(&buf, &pem.Block{Type: "RSA PRIVATE KEY", Bytes: privBytes})) sm.OnGet(ctx, config.SecretNameTokenSigningRSAKey).Return(buf.String(), nil) - sm.OnGet(ctx, config.SecretNameOldTokenSigningRSAKey).Return("", fmt.Errorf("not found")) + sm.OnGet(ctx, config.SecretNameOldTokenSigningRSAKey).Return(buf.String(), nil) p, err := NewProvider(ctx, config.DefaultConfig.AppAuth.SelfAuthServer, sm) assert.NoError(t, err) @@ -47,7 +46,7 @@ func TestNewProvider(t *testing.T) { func TestProvider_KeySet(t *testing.T) { p, _ := newMockProvider(t) - assert.Equal(t, 1, p.KeySet().Len()) + assert.Equal(t, 2, p.KeySet().Len()) } func TestProvider_NewJWTSessionToken(t *testing.T) { @@ -64,7 +63,7 @@ func TestProvider_NewJWTSessionToken(t *testing.T) { func TestProvider_PublicKeys(t *testing.T) { p, _ := newMockProvider(t) - assert.Len(t, p.PublicKeys(), 1) + assert.Len(t, p.PublicKeys(), 2) } type CustomClaimsExample struct { @@ -175,7 +174,7 @@ func TestProvider_ValidateAccessToken(t *testing.T) { var buf bytes.Buffer assert.NoError(t, pem.Encode(&buf, &pem.Block{Type: "RSA PRIVATE KEY", Bytes: privBytes})) sm.OnGet(ctx, config.SecretNameTokenSigningRSAKey).Return(buf.String(), nil) - sm.OnGet(ctx, config.SecretNameOldTokenSigningRSAKey).Return("", fmt.Errorf("not found")) + sm.OnGet(ctx, config.SecretNameOldTokenSigningRSAKey).Return(buf.String(), nil) p, err := NewProvider(ctx, config.DefaultConfig.AppAuth.SelfAuthServer, sm) assert.NoError(t, err) From e2490330439c7231a3ac130a4a0d2a6f15903438 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roger=20Torrentsgener=C3=B3s?= Date: Thu, 15 Feb 2024 10:28:03 +0100 Subject: [PATCH 4/5] test: cover newly added code MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Roger Torrentsgenerós --- flyteadmin/auth/authzserver/provider_test.go | 118 +++++++++++++++++++ 1 file changed, 118 insertions(+) diff --git a/flyteadmin/auth/authzserver/provider_test.go b/flyteadmin/auth/authzserver/provider_test.go index 2eab258069..cdfae8fd5f 100644 --- a/flyteadmin/auth/authzserver/provider_test.go +++ b/flyteadmin/auth/authzserver/provider_test.go @@ -7,6 +7,7 @@ import ( "crypto/x509" "encoding/base64" "encoding/pem" + "fmt" "testing" "time" @@ -44,6 +45,123 @@ func TestNewProvider(t *testing.T) { newMockProvider(t) } +func newInvalidMockProvider(t *testing.T, ctx context.Context, secrets auth.SecretsSet, sm *mocks.SecretManager, invalidFunc func() *mocks.SecretManager_Get, errorContains string) { + + sm.OnGet(ctx, config.SecretNameClaimSymmetricKey).Return(base64.RawStdEncoding.EncodeToString(secrets.TokenHashKey), nil) + sm.OnGet(ctx, config.SecretNameCookieBlockKey).Return(base64.RawStdEncoding.EncodeToString(secrets.CookieBlockKey), nil) + sm.OnGet(ctx, config.SecretNameCookieHashKey).Return(base64.RawStdEncoding.EncodeToString(secrets.CookieHashKey), nil) + + privBytes := x509.MarshalPKCS1PrivateKey(secrets.TokenSigningRSAPrivateKey) + var buf bytes.Buffer + assert.NoError(t, pem.Encode(&buf, &pem.Block{Type: "RSA PRIVATE KEY", Bytes: privBytes})) + sm.OnGet(ctx, config.SecretNameTokenSigningRSAKey).Return(buf.String(), nil) + sm.OnGet(ctx, config.SecretNameOldTokenSigningRSAKey).Return(buf.String(), nil) + + invalidFunc() + p, err := NewProvider(ctx, config.DefaultConfig.AppAuth.SelfAuthServer, sm) + assert.Error(t, err) + assert.ErrorContains(t, err, errorContains) + assert.Equal(t, Provider{}, p) +} + +func TestNewInvalidProviderSecretTokenHashBad(t *testing.T) { + secrets, err := auth.NewSecrets() + assert.NoError(t, err) + + ctx := context.Background() + sm := &mocks.SecretManager{} + + invalidFunc := func() *mocks.SecretManager_Get { + sm.OnGet(ctx, config.SecretNameClaimSymmetricKey).Unset() + return sm.OnGet(ctx, config.SecretNameClaimSymmetricKey).Return("", fmt.Errorf("test error")) + } + newInvalidMockProvider(t, ctx, secrets, sm, invalidFunc, "failed to read secretTokenHash file. Error: test error") +} + +func TestNewInvalidProviderSecretTokenHashEmpty(t *testing.T) { + secrets, err := auth.NewSecrets() + assert.NoError(t, err) + + ctx := context.Background() + sm := &mocks.SecretManager{} + + invalidFunc := func() *mocks.SecretManager_Get { + sm.OnGet(ctx, config.SecretNameClaimSymmetricKey).Unset() + return sm.OnGet(ctx, config.SecretNameClaimSymmetricKey).Return("", nil) + } + newInvalidMockProvider(t, ctx, secrets, sm, invalidFunc, "failed to read secretTokenHash. Error: empty value") +} + +func TestNewInvalidProviderTokenSigningRSAKeyBad(t *testing.T) { + secrets, err := auth.NewSecrets() + assert.NoError(t, err) + + ctx := context.Background() + sm := &mocks.SecretManager{} + + invalidFunc := func() *mocks.SecretManager_Get { + sm.OnGet(ctx, config.SecretNameTokenSigningRSAKey).Unset() + return sm.OnGet(ctx, config.SecretNameTokenSigningRSAKey).Return("", fmt.Errorf("test error")) + } + newInvalidMockProvider(t, ctx, secrets, sm, invalidFunc, "failed to read token signing RSA Key. Error: test error") +} + +func TestNewInvalidProviderTokenSigningRSAKeyEmpty(t *testing.T) { + secrets, err := auth.NewSecrets() + assert.NoError(t, err) + + ctx := context.Background() + sm := &mocks.SecretManager{} + + invalidFunc := func() *mocks.SecretManager_Get { + sm.OnGet(ctx, config.SecretNameTokenSigningRSAKey).Unset() + return sm.OnGet(ctx, config.SecretNameTokenSigningRSAKey).Return("", nil) + } + newInvalidMockProvider(t, ctx, secrets, sm, invalidFunc, "failed to read token signing RSA Key. Error: empty value") +} + +func TestNewInvalidProviderTokenSigningRSAKeyNoPEMData(t *testing.T) { + secrets, err := auth.NewSecrets() + assert.NoError(t, err) + + ctx := context.Background() + sm := &mocks.SecretManager{} + + invalidFunc := func() *mocks.SecretManager_Get { + sm.OnGet(ctx, config.SecretNameTokenSigningRSAKey).Unset() + return sm.OnGet(ctx, config.SecretNameTokenSigningRSAKey).Return("this is no PEM data", nil) + } + newInvalidMockProvider(t, ctx, secrets, sm, invalidFunc, "failed to decode token signing RSA Key. Error: no PEM data found") +} + +func TestNewInvalidProviderOldTokenSigningRSAKeyEmpty(t *testing.T) { + secrets, err := auth.NewSecrets() + assert.NoError(t, err) + + ctx := context.Background() + sm := &mocks.SecretManager{} + + invalidFunc := func() *mocks.SecretManager_Get { + sm.OnGet(ctx, config.SecretNameOldTokenSigningRSAKey).Unset() + return sm.OnGet(ctx, config.SecretNameOldTokenSigningRSAKey).Return("", nil) + } + newInvalidMockProvider(t, ctx, secrets, sm, invalidFunc, "failed to read PKCS1PrivateKey. Error: empty value") +} + +func TestNewInvalidProviderOldTokenSigningRSAKeyNoPEMData(t *testing.T) { + secrets, err := auth.NewSecrets() + assert.NoError(t, err) + + ctx := context.Background() + sm := &mocks.SecretManager{} + + invalidFunc := func() *mocks.SecretManager_Get { + sm.OnGet(ctx, config.SecretNameOldTokenSigningRSAKey).Unset() + return sm.OnGet(ctx, config.SecretNameOldTokenSigningRSAKey).Return("this is no PEM data", nil) + } + newInvalidMockProvider(t, ctx, secrets, sm, invalidFunc, "failed to decode PKCS1PrivateKey. Error: no PEM data found") +} + func TestProvider_KeySet(t *testing.T) { p, _ := newMockProvider(t) assert.Equal(t, 2, p.KeySet().Len()) From 0238be86a25854b209e395c453b7a523a4e163fe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roger=20Torrentsgener=C3=B3s?= Date: Mon, 6 May 2024 17:03:59 +0200 Subject: [PATCH 5/5] chore: make linter happy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Roger Torrentsgenerós --- flyteadmin/auth/authzserver/provider_test.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/flyteadmin/auth/authzserver/provider_test.go b/flyteadmin/auth/authzserver/provider_test.go index cdfae8fd5f..45f0778b51 100644 --- a/flyteadmin/auth/authzserver/provider_test.go +++ b/flyteadmin/auth/authzserver/provider_test.go @@ -45,7 +45,7 @@ func TestNewProvider(t *testing.T) { newMockProvider(t) } -func newInvalidMockProvider(t *testing.T, ctx context.Context, secrets auth.SecretsSet, sm *mocks.SecretManager, invalidFunc func() *mocks.SecretManager_Get, errorContains string) { +func newInvalidMockProvider(ctx context.Context, t *testing.T, secrets auth.SecretsSet, sm *mocks.SecretManager, invalidFunc func() *mocks.SecretManager_Get, errorContains string) { sm.OnGet(ctx, config.SecretNameClaimSymmetricKey).Return(base64.RawStdEncoding.EncodeToString(secrets.TokenHashKey), nil) sm.OnGet(ctx, config.SecretNameCookieBlockKey).Return(base64.RawStdEncoding.EncodeToString(secrets.CookieBlockKey), nil) @@ -75,7 +75,7 @@ func TestNewInvalidProviderSecretTokenHashBad(t *testing.T) { sm.OnGet(ctx, config.SecretNameClaimSymmetricKey).Unset() return sm.OnGet(ctx, config.SecretNameClaimSymmetricKey).Return("", fmt.Errorf("test error")) } - newInvalidMockProvider(t, ctx, secrets, sm, invalidFunc, "failed to read secretTokenHash file. Error: test error") + newInvalidMockProvider(ctx, t, secrets, sm, invalidFunc, "failed to read secretTokenHash file. Error: test error") } func TestNewInvalidProviderSecretTokenHashEmpty(t *testing.T) { @@ -89,7 +89,7 @@ func TestNewInvalidProviderSecretTokenHashEmpty(t *testing.T) { sm.OnGet(ctx, config.SecretNameClaimSymmetricKey).Unset() return sm.OnGet(ctx, config.SecretNameClaimSymmetricKey).Return("", nil) } - newInvalidMockProvider(t, ctx, secrets, sm, invalidFunc, "failed to read secretTokenHash. Error: empty value") + newInvalidMockProvider(ctx, t, secrets, sm, invalidFunc, "failed to read secretTokenHash. Error: empty value") } func TestNewInvalidProviderTokenSigningRSAKeyBad(t *testing.T) { @@ -103,7 +103,7 @@ func TestNewInvalidProviderTokenSigningRSAKeyBad(t *testing.T) { sm.OnGet(ctx, config.SecretNameTokenSigningRSAKey).Unset() return sm.OnGet(ctx, config.SecretNameTokenSigningRSAKey).Return("", fmt.Errorf("test error")) } - newInvalidMockProvider(t, ctx, secrets, sm, invalidFunc, "failed to read token signing RSA Key. Error: test error") + newInvalidMockProvider(ctx, t, secrets, sm, invalidFunc, "failed to read token signing RSA Key. Error: test error") } func TestNewInvalidProviderTokenSigningRSAKeyEmpty(t *testing.T) { @@ -117,7 +117,7 @@ func TestNewInvalidProviderTokenSigningRSAKeyEmpty(t *testing.T) { sm.OnGet(ctx, config.SecretNameTokenSigningRSAKey).Unset() return sm.OnGet(ctx, config.SecretNameTokenSigningRSAKey).Return("", nil) } - newInvalidMockProvider(t, ctx, secrets, sm, invalidFunc, "failed to read token signing RSA Key. Error: empty value") + newInvalidMockProvider(ctx, t, secrets, sm, invalidFunc, "failed to read token signing RSA Key. Error: empty value") } func TestNewInvalidProviderTokenSigningRSAKeyNoPEMData(t *testing.T) { @@ -131,7 +131,7 @@ func TestNewInvalidProviderTokenSigningRSAKeyNoPEMData(t *testing.T) { sm.OnGet(ctx, config.SecretNameTokenSigningRSAKey).Unset() return sm.OnGet(ctx, config.SecretNameTokenSigningRSAKey).Return("this is no PEM data", nil) } - newInvalidMockProvider(t, ctx, secrets, sm, invalidFunc, "failed to decode token signing RSA Key. Error: no PEM data found") + newInvalidMockProvider(ctx, t, secrets, sm, invalidFunc, "failed to decode token signing RSA Key. Error: no PEM data found") } func TestNewInvalidProviderOldTokenSigningRSAKeyEmpty(t *testing.T) { @@ -145,7 +145,7 @@ func TestNewInvalidProviderOldTokenSigningRSAKeyEmpty(t *testing.T) { sm.OnGet(ctx, config.SecretNameOldTokenSigningRSAKey).Unset() return sm.OnGet(ctx, config.SecretNameOldTokenSigningRSAKey).Return("", nil) } - newInvalidMockProvider(t, ctx, secrets, sm, invalidFunc, "failed to read PKCS1PrivateKey. Error: empty value") + newInvalidMockProvider(ctx, t, secrets, sm, invalidFunc, "failed to read PKCS1PrivateKey. Error: empty value") } func TestNewInvalidProviderOldTokenSigningRSAKeyNoPEMData(t *testing.T) { @@ -159,7 +159,7 @@ func TestNewInvalidProviderOldTokenSigningRSAKeyNoPEMData(t *testing.T) { sm.OnGet(ctx, config.SecretNameOldTokenSigningRSAKey).Unset() return sm.OnGet(ctx, config.SecretNameOldTokenSigningRSAKey).Return("this is no PEM data", nil) } - newInvalidMockProvider(t, ctx, secrets, sm, invalidFunc, "failed to decode PKCS1PrivateKey. Error: no PEM data found") + newInvalidMockProvider(ctx, t, secrets, sm, invalidFunc, "failed to decode PKCS1PrivateKey. Error: no PEM data found") } func TestProvider_KeySet(t *testing.T) {