Empty or invalid secrets should be handled nicely

[trutx]

Describe the bug

When secrets like the PrivateKeyPEM are either empty or invalid, flyteadmin blows up with a stack trace like the following:

time="2024-01-31T13:19:12Z" level=info msg="Using config file: [/etc/flyte/config/flyteadmin_config.yaml]"
{"data":{"src":"service.go:71"},"message":"setting metrics keys to [project domain wf task phase tasktype runtime_type runtime_version app_name]","severity":"INFO","timestamp":"2024-01-31T13:19:12Z"}
{"data":{"src":"service.go:302"},"message":"Serving Flyte Admin Insecure","severity":"INFO","timestamp":"2024-01-31T13:19:12Z"}
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x18 pc=0x205939c]

goroutine 1 [running]:
github.com/flyteorg/flyte/flyteadmin/auth/authzserver.NewProvider({_, _}, {{0x0, 0x0}, {0x1a3185c5000}, {0x34630b8a000}, {0x45d964b800}, {0xc000bfdd10, 0x13}, {0xc000bfdd28, ...}, ...}, ...)
	/go/src/github.com/flyteorg/flyteadmin/auth/authzserver/provider.go:163 +0x33c
github.com/flyteorg/flyte/flyteadmin/pkg/server.serveGatewayInsecure({0x31c9678?, 0xc00013a000}, 0x4871340?, 0xc000225680, 0xc0001d2600, 0xc000f55bc0?, 0xc000f55c28?, {0x31e5328, 0xc0013ee370})
	/go/src/github.com/flyteorg/flyteadmin/pkg/server/service.go:316 +0x1f2
github.com/flyteorg/flyte/flyteadmin/pkg/server.Serve({0x31c9678, 0xc00013a000}, 0x0?, 0x0?)
	/go/src/github.com/flyteorg/flyteadmin/pkg/server/service.go:62 +0x19f
github.com/flyteorg/flyte/flyteadmin/cmd/entrypoints.glob..func7(0x4882b60?, {0x2bbada5?, 0x2?, 0x2?})
	/go/src/github.com/flyteorg/flyteadmin/cmd/entrypoints/serve.go:44 +0x1f2
github.com/spf13/cobra.(*Command).execute(0x4882b60, {0xc0001a7c00, 0x2, 0x2})
	/go/pkg/mod/github.com/spf13/[email protected]/command.go:940 +0x862
github.com/spf13/cobra.(*Command).ExecuteC(0x4883f80)
	/go/pkg/mod/github.com/spf13/[email protected]/command.go:1068 +0x3bd
github.com/spf13/cobra.(*Command).Execute(...)
	/go/pkg/mod/github.com/spf13/[email protected]/command.go:992
github.com/flyteorg/flyte/flyteadmin/cmd/entrypoints.Execute(0x60?)
	/go/src/github.com/flyteorg/flyteadmin/cmd/entrypoints/root.go:49 +0x3a
main.main()
	/go/src/github.com/flyteorg/flyteadmin/cmd/main.go:12 +0x85

This is caused because the block variable has nil value and we're using it here.

The pem.Decode() function returns nil when the passed in []byte(string) is either an empty string or a string that doesn't contain a PEM block. In our case we had a wrongly formatted certificate because it had no newlines and this caused pem.Decode() to return nil and the subsequent nil pointer error. But if we dig into sm.Get() we see data can also be read from environment variables because it uses os.LookupEnv() here, which will return true if the environment variable is set but empty. This will make sm.Get() return "", nil which in turn will also result in the error above.

Expected behavior

Empty or invalid secrets should be handled nicely and an informative error should be emitted instead of a stack trace.

Additional context to reproduce

No response

Screenshots

No response

Are you sure this issue hasn't been raised already?

• [X] Yes

Have you read the Code of Conduct?

• [X] Yes

[welcome[bot]]

Thank you for opening your first issue here! 🛠

[dosubot[bot]]

🤖

Hey @trutx, great to see you back here! Hope you're doing well.

Thank you for bringing this to our attention. It seems like the issue arises when the PrivateKeyPEM is either empty or invalid, causing the pem.Decode() function to return nil and subsequently leading to a nil pointer error.

To address this, we could add a validation check right after the pem.Decode() function to ensure that the block variable is not nil before proceeding. Here's how you can do it:

block, _ := pem.Decode([]byte(privateKeyPEM))
if block == nil {
    return Provider{}, fmt.Errorf("failed to decode PEM block. Ensure the PrivateKeyPEM is not empty or invalid")
}
privateKey, err := x509.ParsePKCS1PrivateKey(block.Bytes)
if err != nil {
    return Provider{}, fmt.Errorf("failed to parse PKCS1PrivateKey. Error: %w", err)
}

This way, if the PrivateKeyPEM is empty or invalid, an informative error message will be returned instead of a stack trace.

As for the sm.Get() function, it's true that it uses os.LookupEnv() which will return true if the environment variable is set but empty. This could be addressed by adding a check in the sm.Get() function to ensure that the returned secret is not an empty string. If it is, an error could be returned. Here's an example:

secret, ok := os.LookupEnv(secretName)
if !ok || secret == "" {
    return "", fmt.Errorf("secret %s is not set or empty", secretName)
}

These changes should help improve the handling of empty or invalid secrets in Flyte. Please let me know if you have any other questions or need further clarification.

Sources

• flyteidl/gen/pb-go/flyteidl/core/security.pb.go
• flyteadmin/auth/authzserver/provider.go

---

This response is meant to be useful and save you time. It is not meant to be a precise solution, but rather a starting point for your own research.

Help me be more useful! Please leave a 👍 if this is helpful and 👎 if it is irrelevant.

If you want to continue the conversation, start your reply with @dosu-bot.