Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

clientv3: allow setting JWT directly #16803

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 19 additions & 1 deletion client/v3/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,10 @@ type Client struct {
// Username is a user name for authentication.
Username string
// Password is a password for authentication.
Password string
Password string
// Token is a JWT used for authentication instead of a password.
Token string
mcrute marked this conversation as resolved.
Show resolved Hide resolved

authTokenBundle credentials.PerRPCCredentialsBundle

callOpts []grpc.CallOption
Expand Down Expand Up @@ -288,6 +291,11 @@ func (c *Client) Dial(ep string) (*grpc.ClientConn, error) {
func (c *Client) getToken(ctx context.Context) error {
var err error // return last error in a case of fail

if c.Token != "" {
c.authTokenBundle.UpdateAuthToken(c.Token)
return nil
}

if c.Username == "" || c.Password == "" {
return nil
}
Expand Down Expand Up @@ -376,6 +384,10 @@ func newClient(cfg *Config) (*Client, error) {
creds = credentials.NewTransportCredential(cfg.TLS)
}

if cfg.Token != "" && (cfg.Username != "" || cfg.Password != "") {
return nil, errors.New("Username/Password and Token configurations are mutually exclusive")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor observation/question: thinking about the work we have been doing with errors, does setting this error as an exported variable make sense? Would it be valuable? WDYT @ahrtr?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like a good suggestion.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given the number of follow-ups we have for this pull request, and the time it has been open, I think it's okay to do this in a follow-up PR.

}

// use a temporary skeleton client to bootstrap first connection
baseCtx := context.TODO()
if cfg.Context != nil {
Expand Down Expand Up @@ -414,6 +426,12 @@ func newClient(cfg *Config) (*Client, error) {
client.Password = cfg.Password
client.authTokenBundle = credentials.NewPerRPCCredentialBundle()
}

if cfg.Token != "" {
client.Token = cfg.Token
client.authTokenBundle = credentials.NewPerRPCCredentialBundle()
}

if cfg.MaxCallSendMsgSize > 0 || cfg.MaxCallRecvMsgSize > 0 {
if cfg.MaxCallRecvMsgSize > 0 && cfg.MaxCallSendMsgSize > cfg.MaxCallRecvMsgSize {
return nil, fmt.Errorf("gRPC message recv limit (%d bytes) must be greater than send limit (%d bytes)", cfg.MaxCallRecvMsgSize, cfg.MaxCallSendMsgSize)
Expand Down
77 changes: 77 additions & 0 deletions client/v3/client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -337,6 +337,75 @@ func TestAuthTokenBundleNoOverwrite(t *testing.T) {
}
}

func TestNewWithOnlyJWT(t *testing.T) {
// This call in particular changes working directory to the tmp dir of
// the test. The `etcd-auth-test:1` can be created in local directory,
// not exceeding the longest allowed path on OsX.
testutil.BeforeTest(t)

// Create a mock AuthServer to handle Authenticate RPCs.
lis, err := net.Listen("unix", "etcd-auth-test:1")
if err != nil {
t.Fatal(err)
}
defer lis.Close()
addr := "unix://" + lis.Addr().String()
srv := grpc.NewServer()
// Having a token removes the need to ever call Authenticate on the
// server. If that happens then this will cause a connection failure.
etcdserverpb.RegisterAuthServer(srv, mockFailingAuthServer{})
go srv.Serve(lis)
defer srv.Stop()

c, err := NewClient(t, Config{
DialTimeout: 5 * time.Second,
Endpoints: []string{addr},
Token: "foo",
})
if err != nil {
t.Fatal(err)
}
defer c.Close()

meta, err := c.authTokenBundle.PerRPCCredentials().GetRequestMetadata(context.Background(), "")
if err != nil {
t.Errorf("Error building request metadata: %s", err)
}

if tok, ok := meta[rpctypes.TokenFieldNameGRPC]; !ok {
t.Error("Token was not successfuly set in the auth bundle")
} else if tok != "foo" {
t.Errorf("Incorrect token set in auth bundle, got '%s', expected 'foo'", tok)
}
}

func TestNewOnlyJWTExclusivity(t *testing.T) {
testutil.BeforeTest(t)

// Create a mock AuthServer to handle Authenticate RPCs.
lis, err := net.Listen("unix", "etcd-auth-test:1")
if err != nil {
t.Fatal(err)
}
defer lis.Close()
addr := "unix://" + lis.Addr().String()
srv := grpc.NewServer()
// Having a token removes the need to ever call Authenticate on the
// server. If that happens then this will cause a connection failure.
etcdserverpb.RegisterAuthServer(srv, mockFailingAuthServer{})
go srv.Serve(lis)
defer srv.Stop()

_, err = NewClient(t, Config{
DialTimeout: 5 * time.Second,
Endpoints: []string{addr},
Token: "foo",
Username: "user",
Password: "pass",
})
require.Error(t, err, "Username/Password and Token configurations are mutually exclusive")
}

func TestSyncFiltersMembers(t *testing.T) {
c, _ := NewClient(t, Config{Endpoints: []string{"http://254.0.0.1:12345"}})
defer c.Close()
Expand Down Expand Up @@ -498,6 +567,14 @@ func (mm mockMaintenance) Downgrade(ctx context.Context, action DowngradeAction,
return nil, nil
}

type mockFailingAuthServer struct {
*etcdserverpb.UnimplementedAuthServer
}

func (mockFailingAuthServer) Authenticate(context.Context, *etcdserverpb.AuthenticateRequest) (*etcdserverpb.AuthenticateResponse, error) {
return nil, errors.New("this auth server always fails")
}

type mockAuthServer struct {
*etcdserverpb.UnimplementedAuthServer
}
Expand Down
7 changes: 6 additions & 1 deletion client/v3/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,9 @@ type Config struct {
// Password is a password for authentication.
Password string `json:"password"`

// Token is a JWT used for authentication instead of a password.
Token string `json:"token"`
Comment on lines +69 to +70
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that this will be exported and it may be difficult to change the name later on, I feel like "Token" may be too generic. Would "JWT" be a better name (more implementation specific)?

Copy link
Member

@ahrtr ahrtr Sep 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Theoretically, it can be any token types that are supported by etcd. The etcdserver just delegates to the related token provider to parse the token based on the token type configured in --auth-token.

But in practice, it's JWT for now. So suggest to keep using Token


// RejectOldCluster when set will refuse to create a client against an outdated cluster.
RejectOldCluster bool `json:"reject-old-cluster"`

Expand Down Expand Up @@ -128,10 +131,11 @@ type SecureConfig struct {
type AuthConfig struct {
Username string `json:"username"`
Password string `json:"password"`
Token string `json:"token"`
}

func (cfg AuthConfig) Empty() bool {
return cfg.Username == "" && cfg.Password == ""
return cfg.Username == "" && cfg.Password == "" && cfg.Token == ""
}

// NewClientConfig creates a Config based on the provided ConfigSpec.
Expand All @@ -152,6 +156,7 @@ func NewClientConfig(confSpec *ConfigSpec, lg *zap.Logger) (*Config, error) {
if confSpec.Auth != nil {
cfg.Username = confSpec.Auth.Username
cfg.Password = confSpec.Auth.Password
cfg.Token = confSpec.Auth.Token
}

return cfg, nil
Expand Down
19 changes: 19 additions & 0 deletions client/v3/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,25 @@ func TestNewClientConfig(t *testing.T) {
Password: "changeme",
},
},
{
name: "JWT specified",
spec: ConfigSpec{
Endpoints: []string{"http://192.168.0.12:2379"},
DialTimeout: 1 * time.Second,
KeepAliveTime: 4 * time.Second,
KeepAliveTimeout: 6 * time.Second,
Auth: &AuthConfig{
Token: "test",
},
},
expectedConf: Config{
Endpoints: []string{"http://192.168.0.12:2379"},
DialTimeout: 1 * time.Second,
DialKeepAliveTime: 4 * time.Second,
DialKeepAliveTimeout: 6 * time.Second,
Token: "test",
},
},
{
name: "default secure transport",
spec: ConfigSpec{
Expand Down
12 changes: 11 additions & 1 deletion etcdctl/ctlv3/command/global.go
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ type GlobalFlags struct {

User string
Password string
Token string

Debug bool
}
Expand Down Expand Up @@ -270,13 +271,22 @@ func authCfgFromCmd(cmd *cobra.Command) *clientv3.AuthConfig {
if err != nil {
cobrautl.ExitWithError(cobrautl.ExitBadArgs, err)
}
tokenFlag, err := cmd.Flags().GetString("auth-jwt-token")
if err != nil {
cobrautl.ExitWithError(cobrautl.ExitBadArgs, err)
}

if userFlag == "" {
if userFlag == "" && tokenFlag == "" {
return nil
}

var cfg clientv3.AuthConfig

if tokenFlag != "" {
cfg.Token = tokenFlag
return &cfg
}

if passwordFlag == "" {
splitted := strings.SplitN(userFlag, ":", 2)
if len(splitted) < 2 {
Expand Down
1 change: 1 addition & 0 deletions etcdctl/ctlv3/ctl.go
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ func init() {
rootCmd.PersistentFlags().StringVar(&globalFlags.TLS.CertFile, "cert", "", "identify secure client using this TLS certificate file")
rootCmd.PersistentFlags().StringVar(&globalFlags.TLS.KeyFile, "key", "", "identify secure client using this TLS key file")
rootCmd.PersistentFlags().StringVar(&globalFlags.TLS.TrustedCAFile, "cacert", "", "verify certificates of TLS-enabled secure servers using this CA bundle")
rootCmd.PersistentFlags().StringVar(&globalFlags.Token, "auth-jwt-token", "", "JWT token used for authentication (if this option is used, --user and --password should not be set)")
mcrute marked this conversation as resolved.
Show resolved Hide resolved
rootCmd.PersistentFlags().StringVar(&globalFlags.User, "user", "", "username[:password] for authentication (prompt if password is not supplied)")
rootCmd.PersistentFlags().StringVar(&globalFlags.Password, "password", "", "password for authentication (if this option is used, --user option shouldn't include password)")
rootCmd.PersistentFlags().StringVarP(&globalFlags.TLS.ServerName, "discovery-srv", "d", "", "domain name to query for SRV records describing cluster endpoints")
Expand Down
24 changes: 23 additions & 1 deletion tests/common/auth_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,11 @@ import (
)

var tokenTTL = time.Second
var defaultKeyPath = mustAbsPath("../fixtures/server.key.insecure")
var defaultAuthToken = fmt.Sprintf("jwt,pub-key=%s,priv-key=%s,sign-method=RS256,ttl=%s",
mustAbsPath("../fixtures/server.crt"), mustAbsPath("../fixtures/server.key.insecure"), tokenTTL)
mustAbsPath("../fixtures/server.crt"), defaultKeyPath, tokenTTL)
var verifyJWTOnlyAuth = fmt.Sprintf("jwt,pub-key=%s,sign-method=RS256,ttl=%s",
mustAbsPath("../fixtures/server.crt"), tokenTTL)

const (
PermissionDenied = "etcdserver: permission denied"
Expand Down Expand Up @@ -758,6 +761,25 @@ func TestAuthJWTExpire(t *testing.T) {
})
}

func TestAuthJWTOnly(t *testing.T) {
testRunner.BeforeTest(t)
ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
defer cancel()
clus := testRunner.NewCluster(ctx, t, config.WithClusterConfig(config.ClusterConfig{ClusterSize: 1, AuthToken: verifyJWTOnlyAuth}))
defer clus.Close()
cc := testutils.MustClient(clus.Client())
testutils.ExecuteUntil(ctx, t, func() {
authRev, err := setupAuthAndGetRevision(cc, []authRole{testRole}, []authUser{rootUser, testUser})
require.NoErrorf(t, err, "failed to enable auth")

token, err := createSignedJWT(defaultKeyPath, "RS256", testUserName, authRev)
require.NoErrorf(t, err, "failed to create test user JWT")

testUserAuthClient := testutils.MustClient(clus.Client(WithAuthToken(token)))
require.NoError(t, testUserAuthClient.Put(ctx, "foo", "bar", config.PutOptions{}))
})
}

// TestAuthRevisionConsistency ensures auth revision is the same after member restarts
func TestAuthRevisionConsistency(t *testing.T) {
testRunner.BeforeTest(t)
Expand Down
49 changes: 49 additions & 0 deletions tests/common/auth_util.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,11 @@ package common
import (
"context"
"fmt"
"os"
"testing"
"time"

"github.com/golang-jwt/jwt/v4"
"github.com/stretchr/testify/require"

"go.etcd.io/etcd/api/v3/authpb"
Expand Down Expand Up @@ -93,6 +96,29 @@ func createUsers(c interfaces.Client, users []authUser) error {
return nil
}

func createSignedJWT(keyPath, alg, username string, authRevision uint64) (string, error) {
signMethod := jwt.GetSigningMethod(alg)

keyBytes, err := os.ReadFile(keyPath)
if err != nil {
return "", err
}

key, err := jwt.ParseRSAPrivateKeyFromPEM(keyBytes)
if err != nil {
return "", err
}

tk := jwt.NewWithClaims(signMethod,
jwt.MapClaims{
"username": username,
"revision": authRevision,
"exp": time.Now().Add(time.Minute).Unix(),
})

return tk.SignedString(key)
}

func setupAuth(c interfaces.Client, roles []authRole, users []authUser) error {
// create roles
if err := createRoles(c, roles); err != nil {
Expand All @@ -107,6 +133,29 @@ func setupAuth(c interfaces.Client, roles []authRole, users []authUser) error {
return c.AuthEnable(context.TODO())
}

func setupAuthAndGetRevision(c interfaces.Client, roles []authRole, users []authUser) (uint64, error) {
// create roles
if err := createRoles(c, roles); err != nil {
return 0, err
}

if err := createUsers(c, users); err != nil {
return 0, err
}

// This needs to happen before enabling auth for the TestAuthJWTOnly
// test case because once auth is enabled we can no longer mint a valid
// auth token without the revision, which we won't be able to obtain
// without a valid auth token.
authrev, err := c.AuthStatus(context.TODO())
if err != nil {
return 0, err
}

// enable auth
return authrev.AuthRevision, c.AuthEnable(context.TODO())
}

func requireRolePermissionEqual(t *testing.T, expectRole authRole, actual []*authpb.Permission) {
require.Equal(t, 1, len(actual))
require.Equal(t, expectRole.permission, clientv3.PermissionType(actual[0].PermType))
Expand Down
4 changes: 4 additions & 0 deletions tests/common/e2e_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,10 @@ func WithAuth(userName, password string) config.ClientOption {
return e2e.WithAuth(userName, password)
}

func WithAuthToken(token string) config.ClientOption {
return e2e.WithAuthToken(token)
}

func WithEndpoints(endpoints []string) config.ClientOption {
return e2e.WithEndpoints(endpoints)
}
4 changes: 4 additions & 0 deletions tests/common/integration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,10 @@ func WithAuth(userName, password string) config.ClientOption {
return integration.WithAuth(userName, password)
}

func WithAuthToken(token string) config.ClientOption {
return integration.WithAuthToken(token)
}

func WithEndpoints(endpoints []string) config.ClientOption {
return integration.WithEndpoints(endpoints)
}
4 changes: 4 additions & 0 deletions tests/common/unit_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,10 @@ func WithAuth(userName, password string) config.ClientOption {
return func(any) {}
}

func WithAuthToken(token string) config.ClientOption {
return func(any) {}
}

func WithEndpoints(endpoints []string) config.ClientOption {
return func(any) {}
}
Loading
Loading