Merge pull request #14770 from ahrtr/auth_20221116

clientv3: do not refresh token when users use CommonName based authentication

client/v3/retry_interceptor.go

@@ -77,13 +77,7 @@ func (c *Client) unaryClientInterceptor(optFuncs ...retryOption) grpc.UnaryClien
 				continue
 			}
 			if c.shouldRefreshToken(lastErr, callOpts) {
-				// clear auth token before refreshing it.
+				gterr := c.refreshToken(ctx)
-				// call c.Auth.Authenticate with an invalid token will always fail the auth check on the server-side,
-				// if the server has not apply the patch of pr #12165 (https://github.com/etcd-io/etcd/pull/12165)
-				// and a rpctypes.ErrInvalidAuthToken will recursively call c.getToken until system run out of resource.
-				c.authTokenBundle.UpdateAuthToken("")
-
-				gterr := c.getToken(ctx)
 				if gterr != nil {
 					c.GetLogger().Warn(
 						"retrying of unary invoker failed to fetch new auth token",
@@ -164,6 +158,24 @@ func (c *Client) shouldRefreshToken(err error, callOpts *options) bool {
 		(rpctypes.Error(err) == rpctypes.ErrInvalidAuthToken || rpctypes.Error(err) == rpctypes.ErrAuthOldRevision)
 }
 
+func (c *Client) refreshToken(ctx context.Context) error {
+	if c.authTokenBundle == nil {
+		// c.authTokenBundle will be initialized only when
+		// c.Username != "" && c.Password != "".
+		//
+		// When users use the TLS CommonName based authentication, the
+		// authTokenBundle is always nil. But it's possible for the clients
+		// to get `rpctypes.ErrAuthOldRevision` response when the clients
+		// concurrently modify auth data (e.g, addUser, deleteUser etc.).
+		// In this case, there is no need to refresh the token; instead the
+		// clients just need to retry the operations (e.g. Put, Delete etc).
+		return nil
+	}
+	// clear auth token before refreshing it.
+	c.authTokenBundle.UpdateAuthToken("")
+	return c.getToken(ctx)
+}
+
 // type serverStreamingRetryingStream is the implementation of grpc.ClientStream that acts as a
 // proxy to the underlying call. If any of the RecvMsg() calls fail, it will try to reestablish
 // a new ClientStream according to the retry policy.
@@ -262,10 +274,7 @@ func (s *serverStreamingRetryingStream) receiveMsgAndIndicateRetry(m interface{}
 		return true, err
 	}
 	if s.client.shouldRefreshToken(err, s.callOpts) {
-		// clear auth token to avoid failure when call getToken
+		gterr := s.client.refreshToken(s.ctx)
-		s.client.authTokenBundle.UpdateAuthToken("")
-
-		gterr := s.client.getToken(s.ctx)
 		if gterr != nil {
 			s.client.lg.Warn("retry failed to fetch new auth token", zap.Error(gterr))
 			return false, err // return the original error for simplicity

tests/e2e/ctl_v3_auth_cluster_test.go

@@ -17,6 +17,7 @@ package e2e
 import (
 	"context"
 	"fmt"
+	"path/filepath"
 	"testing"
 	"time"
 
@@ -97,6 +98,28 @@ func TestAuthCluster(t *testing.T) {
 
 }
 
+func applyTLSWithRootCommonName() func() {
+	var (
+		oldCertPath       = e2e.CertPath
+		oldPrivateKeyPath = e2e.PrivateKeyPath
+		oldCaPath         = e2e.CaPath
+
+		newCertPath       = filepath.Join(e2e.FixturesDir, "CommonName-root.crt")
+		newPrivateKeyPath = filepath.Join(e2e.FixturesDir, "CommonName-root.key")
+		newCaPath         = filepath.Join(e2e.FixturesDir, "CommonName-root.crt")
+	)
+
+	e2e.CertPath = newCertPath
+	e2e.PrivateKeyPath = newPrivateKeyPath
+	e2e.CaPath = newCaPath
+
+	return func() {
+		e2e.CertPath = oldCertPath
+		e2e.PrivateKeyPath = oldPrivateKeyPath
+		e2e.CaPath = oldCaPath
+	}
+}
+
 func createUsers(ctx context.Context, t *testing.T, client *e2e.EtcdctlV3) {
 	if _, err := client.UserAdd(ctx, "root", "rootPassword", config.UserAddOptions{}); err != nil {
 		t.Fatalf("could not add root user (%v)", err)

tests/e2e/ctl_v3_auth_no_proxy_test.go

@@ -12,7 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-// These tests depends on certificate-based authentication that is NOT supported
+// These tests depend on certificate-based authentication that is NOT supported
 // by gRPC proxy.
 //go:build !cluster_proxy
 // +build !cluster_proxy
@@ -20,8 +20,13 @@
 package e2e
 
 import (
+	"context"
+	"fmt"
+	"sync"
 	"testing"
+	"time"
 
+	"go.etcd.io/etcd/tests/v3/framework/config"
 	"go.etcd.io/etcd/tests/v3/framework/e2e"
 )
 
@@ -34,3 +39,105 @@ func TestCtlV3AuthCertCNAndUsername(t *testing.T) {
 func TestCtlV3AuthCertCNAndUsernameNoPassword(t *testing.T) {
 	testCtl(t, authTestCertCNAndUsernameNoPassword, withCfg(*e2e.NewConfigClientTLSCertAuth()))
 }
+
+func TestCtlV3AuthCertCNWithWithConcurrentOperation(t *testing.T) {
+	e2e.BeforeTest(t)
+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
+
+	// apply the certificate which has `root` CommonName,
+	// and reset the setting when the test case finishes.
+	// TODO(ahrtr): enhance the e2e test framework to support
+	// certificates with CommonName.
+	t.Log("Apply certificate with root CommonName")
+	resetCert := applyTLSWithRootCommonName()
+	defer resetCert()
+
+	t.Log("Create etcd cluster")
+	epc, err := e2e.NewEtcdProcessCluster(ctx, t,
+		e2e.WithClusterSize(1),
+		e2e.WithClientTLS(e2e.ClientTLS),
+		e2e.WithClientCertAuthEnabled(true),
+	)
+	if err != nil {
+		t.Fatalf("could not start etcd process cluster (%v)", err)
+	}
+	defer func() {
+		if err := epc.Close(); err != nil {
+			t.Fatalf("could not close test cluster (%v)", err)
+		}
+	}()
+
+	epcClient := epc.Client()
+	t.Log("Create users")
+	createUsers(ctx, t, epcClient)
+
+	t.Log("Enable auth")
+	if err := epcClient.AuthEnable(ctx); err != nil {
+		t.Fatalf("could not enable Auth: (%v)", err)
+	}
+
+	// Create two goroutines, one goroutine keeps creating & deleting users,
+	// and the other goroutine keeps writing & deleting K/V entries.
+	var wg sync.WaitGroup
+	wg.Add(2)
+	errs := make(chan error, 2)
+	donec := make(chan struct{})
+
+	// Create the first goroutine to create & delete users
+	t.Log("Create the first goroutine to create & delete users")
+	go func() {
+		defer wg.Done()
+		for i := 0; i < 100; i++ {
+			user := fmt.Sprintf("testuser-%d", i)
+			pass := fmt.Sprintf("testpass-%d", i)
+			if _, err := epcClient.UserAdd(ctx, user, pass, config.UserAddOptions{}); err != nil {
+				errs <- fmt.Errorf("failed to create user %q: %w", user, err)
+				break
+			}
+
+			if _, err := epcClient.UserDelete(ctx, user); err != nil {
+				errs <- fmt.Errorf("failed to delete user %q: %w", user, err)
+				break
+			}
+		}
+		t.Log("The first goroutine finished")
+	}()
+
+	// Create the second goroutine to write & delete K/V entries
+	t.Log("Create the second goroutine to write & delete K/V entries")
+	go func() {
+		defer wg.Done()
+		for i := 0; i < 100; i++ {
+			key := fmt.Sprintf("key-%d", i)
+			value := fmt.Sprintf("value-%d", i)
+
+			if err := epcClient.Put(ctx, key, value, config.PutOptions{}); err != nil {
+				errs <- fmt.Errorf("failed to put key %q: %w", key, err)
+				break
+			}
+
+			if _, err := epcClient.Delete(ctx, key, config.DeleteOptions{}); err != nil {
+				errs <- fmt.Errorf("failed to delete key %q: %w", key, err)
+				break
+			}
+		}
+		t.Log("The second goroutine finished")
+	}()
+
+	t.Log("Waiting for the two goroutines to complete")
+	go func() {
+		wg.Wait()
+		close(donec)
+	}()
+
+	t.Log("Waiting for test result")
+	select {
+	case err := <-errs:
+		t.Fatalf("Unexpected error: %v", err)
+	case <-donec:
+		t.Log("All done!")
+	case <-time.After(30 * time.Second):
+		t.Fatal("Test case timeout after 20 seconds")
+	}
+}

tests/fixtures/CommonName-root.crt

@@ -0,0 +1,29 @@
+-----BEGIN CERTIFICATE-----
+MIIE5zCCA8+gAwIBAgIJAKooGDZuR2mMMA0GCSqGSIb3DQEBCwUAMH8xCzAJBgNV
+BAYTAkNOMRAwDgYDVQQIDAdCZWlqaW5nMRAwDgYDVQQHDAdCZWlqaW5nMQ0wCwYD
+VQQKDAREZW1vMQ0wCwYDVQQLDAREZW1vMQ0wCwYDVQQDDARyb290MR8wHQYJKoZI
+hvcNAQkBFhB0ZXN0QGV4YW1wbGUuY29tMB4XDTIyMTExNjA2NTI1M1oXDTMyMTEx
+MzA2NTI1M1owfzELMAkGA1UEBhMCQ04xEDAOBgNVBAgMB0JlaWppbmcxEDAOBgNV
+BAcMB0JlaWppbmcxDTALBgNVBAoMBERlbW8xDTALBgNVBAsMBERlbW8xDTALBgNV
+BAMMBHJvb3QxHzAdBgkqhkiG9w0BCQEWEHRlc3RAZXhhbXBsZS5jb20wggEiMA0G
+CSqGSIb3DQEBAQUAA4IBDwAwggEKAoIBAQDEAKcjzhtOG3hWbAUCbudE1gPOeteT
+0INk2ngN2uCMYjYSZmaGhW/GZk3EvV7wKVuhdTyrh36E5Iajng9d2t1iOU/8jROU
++uAyrS3C/S5P/urq8VBUrt3VG/44bhwTEdafNnAWQ6ojYfmK0tRqoQn1Ftm30l8I
+nWof5Jm3loNA2WdNdvAp/D+6OpjUdqGdMkFd0NhkuQODMnycBMw6btUTj5SnmrMk
+q7V1aasx4BqN5C4DciZF0pyyR/TT8MoQ5Vcit8rHvQUyz42Lj8+28RkDoi4prJ1i
+tLaCt2egDp58vXlYQZTd50inMhnBIapKNdGpg3flW/8AFul1tCTqd8NfAgMBAAGj
+ggFkMIIBYDAdBgNVHQ4EFgQUpwwvEqXjA/ArJu1Jnpw7+/sttOAwgbMGA1UdIwSB
+qzCBqIAUpwwvEqXjA/ArJu1Jnpw7+/sttOChgYSkgYEwfzELMAkGA1UEBhMCQ04x
+EDAOBgNVBAgMB0JlaWppbmcxEDAOBgNVBAcMB0JlaWppbmcxDTALBgNVBAoMBERl
+bW8xDTALBgNVBAsMBERlbW8xDTALBgNVBAMMBHJvb3QxHzAdBgkqhkiG9w0BCQEW
+EHRlc3RAZXhhbXBsZS5jb22CCQCqKBg2bkdpjDAMBgNVHRMEBTADAQH/MAsGA1Ud
+DwQEAwIC/DA2BgNVHREELzAtggtleGFtcGxlLmNvbYINKi5leGFtcGxlLmNvbYIJ
+bG9jYWxob3N0hwR/AAABMDYGA1UdEgQvMC2CC2V4YW1wbGUuY29tgg0qLmV4YW1w
+bGUuY29tgglsb2NhbGhvc3SHBH8AAAEwDQYJKoZIhvcNAQELBQADggEBAGi48ntm
+8cn08FrsCDWapsck7a56/dyFyzLg10c0blu396tzC3ZDCAwQYzHjeXVdeWHyGO+f
+KSFlmR6IA0jq6pFhUyJtgaAUJ91jW6s68GTVhlLoFhtYjy6EvhQ0lo+7GWh4qB2s
+LI0mJPjaLZY1teAC4TswzwMDVD8QsB06/aFBlA65VjgZiVH+aMwWJ88gKfVGp0Pv
+AApsy5MvwQn8WZ2L6foSY04OzXtmAg2gCl0PyDNgieqFDcM1g7mklHNgWl2Gvtte
+G6+TiB3gGUUlTsdy0+LS2psL71RS5Jv7g/7XGmSKBPqRmYyQ2t7m2kLPwWKtL5tE
+63c0FPtpV0FzKdU=
+-----END CERTIFICATE-----

tests/fixtures/CommonName-root.key

@@ -0,0 +1,27 @@
+-----BEGIN RSA PRIVATE KEY-----
+MIIEowIBAAKCAQEAxACnI84bTht4VmwFAm7nRNYDznrXk9CDZNp4DdrgjGI2EmZm
+hoVvxmZNxL1e8ClboXU8q4d+hOSGo54PXdrdYjlP/I0TlPrgMq0twv0uT/7q6vFQ
+VK7d1Rv+OG4cExHWnzZwFkOqI2H5itLUaqEJ9RbZt9JfCJ1qH+SZt5aDQNlnTXbw
+Kfw/ujqY1HahnTJBXdDYZLkDgzJ8nATMOm7VE4+Up5qzJKu1dWmrMeAajeQuA3Im
+RdKcskf00/DKEOVXIrfKx70FMs+Ni4/PtvEZA6IuKaydYrS2grdnoA6efL15WEGU
+3edIpzIZwSGqSjXRqYN35Vv/ABbpdbQk6nfDXwIDAQABAoIBAA5AMebTjH6wVp6J
++g9EOwJxQROZMOVparRBgisXt+3dEitiUKAFQaw+MfdVAXsatrPVj1S1ZEiLSRLK
+YjmjuSb0HdGx/DN/zh9BIiukNuLQGQp+AyY1FKHzCBfYQahNSrqGvb2Qq+UosXkb
+fSBHly6/u5K28/vvXhD1kQudIOvtAc9tOg8LZnM6N3J4E0GtLqWimRZ4jNK4APu1
+YsLIg87Eam+7x25+phz9xc22tZ1H4WY9FnOGprPnievqiV7mgcNGAklTB93C6yX1
+EI+QxQnPg0P732C4EJZFDPqhVRA4E7BUb5uTIXCJBA/FFuRIx9ppyLZKt9vjTchM
+8YWIEsECgYEA/5DRR9FkIWJZb0Pv3SCc53PMPT/xpYB6lH2lGtG+u+L71dJNDiPt
+da3dPXSBy+aF7BbmRDawRvyOLGArlWiSsoEUVlES8BYzQ1MmfDf+MJooJoBE6/g6
+2OyyNnPde1GqyxsxgNTITvJCTjYH64lxKVRYfMgMAASK49SjYiEgGn8CgYEAxFXs
+Oe0sUcc3P1cQ9pJfSVKpSczZq/OGAxqlniqRHvoWgFfKOWB6F9PN0rd8G2aMlfGS
+BjyiPe770gtpX8Z4G4lrtkJD8NvGoVC8yX78HbrXL2RA4lPjQfrveUnwXIRbRKWa
+6D/GAYPOuNvJmwF4hY/orWyIqvpNczIjTjs1JyECgYEAvhuNAn6JnKfbXYBM+tIa
+xbWHFXzula2IAdOhMN0bpApKSZmBxmYFa0elTuTO9M2Li77RFacU5AlU/T+gzCiZ
+D34jkb4Hd18cTRWaiEbiqGbUPSennVzu8ZTJUOZJuEVc5m9ZGLuwMcHWfvWEWLrJ
+2fOrS09IVe8LHkV8MC/yAKMCgYBmDUdhgK9Fvqgv60Cs+b4/rZDDBJCsOUOSP3qQ
+sQ2HrXSet4MsucIcuoJEog0HbRFsKwm85i1qxdrs/fOCzfXGUnLDZMRN4N7pIL9Q
+eQnxJhoNzy2Otw3sUNPDFrSyUjXig7X2PJfeV7XPDqdHQ8dynS/TXRPY04wIcao6
+Uro5IQKBgFUz2GjAxI6uc7ihmRv/GYTuXYOlO0IN7MFwQDd0pVnWHkLNZscO9L9/
+ALV4g1p/75CewlQfyC8ynOJJWcDeHHFNsSMsOzAxUOVtVenaF/dgwk95wpXj6Rx6
+4kvQqnJg97fRBbyzvQcdL36kL8+pbmHNoqHPwxbuigYShB74d6/h
+-----END RSA PRIVATE KEY-----