Skip to content

Commit b282163

Browse files
authored
Merge pull request #14790 from ahrtr/auth_3.5_20221117
[3.5] clientv3: do not refresh token when users use CommonName based authentication
2 parents 5f387e6 + 4097c24 commit b282163

File tree

5 files changed

+206
-12
lines changed

5 files changed

+206
-12
lines changed

client/v3/retry_interceptor.go

Lines changed: 20 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -74,13 +74,7 @@ func (c *Client) unaryClientInterceptor(optFuncs ...retryOption) grpc.UnaryClien
7474
continue
7575
}
7676
if c.shouldRefreshToken(lastErr, callOpts) {
77-
// clear auth token before refreshing it.
78-
// call c.Auth.Authenticate with an invalid token will always fail the auth check on the server-side,
79-
// if the server has not apply the patch of pr #12165 (https://github.com/etcd-io/etcd/pull/12165)
80-
// and a rpctypes.ErrInvalidAuthToken will recursively call c.getToken until system run out of resource.
81-
c.authTokenBundle.UpdateAuthToken("")
82-
83-
gterr := c.getToken(ctx)
77+
gterr := c.refreshToken(ctx)
8478
if gterr != nil {
8579
c.GetLogger().Warn(
8680
"retrying of unary invoker failed to fetch new auth token",
@@ -161,6 +155,24 @@ func (c *Client) shouldRefreshToken(err error, callOpts *options) bool {
161155
(rpctypes.Error(err) == rpctypes.ErrInvalidAuthToken || rpctypes.Error(err) == rpctypes.ErrAuthOldRevision)
162156
}
163157

158+
func (c *Client) refreshToken(ctx context.Context) error {
159+
if c.authTokenBundle == nil {
160+
// c.authTokenBundle will be initialized only when
161+
// c.Username != "" && c.Password != "".
162+
//
163+
// When users use the TLS CommonName based authentication, the
164+
// authTokenBundle is always nil. But it's possible for the clients
165+
// to get `rpctypes.ErrAuthOldRevision` response when the clients
166+
// concurrently modify auth data (e.g, addUser, deleteUser etc.).
167+
// In this case, there is no need to refresh the token; instead the
168+
// clients just need to retry the operations (e.g. Put, Delete etc).
169+
return nil
170+
}
171+
// clear auth token before refreshing it.
172+
c.authTokenBundle.UpdateAuthToken("")
173+
return c.getToken(ctx)
174+
}
175+
164176
// type serverStreamingRetryingStream is the implementation of grpc.ClientStream that acts as a
165177
// proxy to the underlying call. If any of the RecvMsg() calls fail, it will try to reestablish
166178
// a new ClientStream according to the retry policy.
@@ -259,10 +271,7 @@ func (s *serverStreamingRetryingStream) receiveMsgAndIndicateRetry(m interface{}
259271
return true, err
260272
}
261273
if s.client.shouldRefreshToken(err, s.callOpts) {
262-
// clear auth token to avoid failure when call getToken
263-
s.client.authTokenBundle.UpdateAuthToken("")
264-
265-
gterr := s.client.getToken(s.ctx)
274+
gterr := s.client.refreshToken(s.ctx)
266275
if gterr != nil {
267276
s.client.lg.Warn("retry failed to fetch new auth token", zap.Error(gterr))
268277
return false, err // return the original error for simplicity

tests/e2e/ctl_v3_auth_no_proxy_test.go

Lines changed: 107 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,15 +12,18 @@
1212
// See the License for the specific language governing permissions and
1313
// limitations under the License.
1414

15-
// These tests depends on certificate-based authentication that is NOT supported
15+
// These tests depend on certificate-based authentication that is NOT supported
1616
// by gRPC proxy.
1717
//go:build !cluster_proxy
1818
// +build !cluster_proxy
1919

2020
package e2e
2121

2222
import (
23+
"fmt"
24+
"sync"
2325
"testing"
26+
"time"
2427
)
2528

2629
func TestCtlV3AuthCertCN(t *testing.T) {
@@ -32,3 +35,106 @@ func TestCtlV3AuthCertCNAndUsername(t *testing.T) {
3235
func TestCtlV3AuthCertCNAndUsernameNoPassword(t *testing.T) {
3336
testCtl(t, authTestCertCNAndUsernameNoPassword, withCfg(*newConfigClientTLSCertAuth()))
3437
}
38+
39+
func TestCtlV3AuthCertCNWithWithConcurrentOperation(t *testing.T) {
40+
BeforeTest(t)
41+
42+
// apply the certificate which has `root` CommonName,
43+
// and reset the setting when the test case finishes.
44+
// TODO(ahrtr): enhance the e2e test framework to support
45+
// certificates with CommonName.
46+
t.Log("Apply certificate with root CommonName")
47+
resetCert := applyTLSWithRootCommonName()
48+
defer resetCert()
49+
50+
t.Log("Create an etcd cluster")
51+
cx := getDefaultCtlCtx(t)
52+
cx.cfg = etcdProcessClusterConfig{
53+
clusterSize: 1,
54+
clientTLS: clientTLS,
55+
clientCertAuthEnabled: true,
56+
initialToken: "new",
57+
}
58+
59+
epc, err := newEtcdProcessCluster(t, &cx.cfg)
60+
if err != nil {
61+
t.Fatalf("Failed to start etcd cluster: %v", err)
62+
}
63+
cx.epc = epc
64+
cx.dataDir = epc.procs[0].Config().dataDirPath
65+
66+
defer func() {
67+
if err := epc.Close(); err != nil {
68+
t.Fatalf("could not close test cluster (%v)", err)
69+
}
70+
}()
71+
72+
t.Log("Enable auth")
73+
authEnableTest(cx)
74+
75+
// Create two goroutines, one goroutine keeps creating & deleting users,
76+
// and the other goroutine keeps writing & deleting K/V entries.
77+
var wg sync.WaitGroup
78+
wg.Add(2)
79+
errs := make(chan error, 2)
80+
donec := make(chan struct{})
81+
82+
// Create the first goroutine to create & delete users
83+
t.Log("Create the first goroutine to create & delete users")
84+
go func() {
85+
defer wg.Done()
86+
for i := 0; i < 100; i++ {
87+
user := fmt.Sprintf("testuser-%d", i)
88+
pass := fmt.Sprintf("testpass-%d", i)
89+
90+
if err := ctlV3User(cx, []string{"add", user, "--interactive=false"}, fmt.Sprintf("User %s created", user), []string{pass}); err != nil {
91+
errs <- fmt.Errorf("failed to create user %q: %w", user, err)
92+
break
93+
}
94+
95+
err := ctlV3User(cx, []string{"delete", user}, fmt.Sprintf("User %s deleted", user), []string{})
96+
if err != nil {
97+
errs <- fmt.Errorf("failed to delete user %q: %w", user, err)
98+
break
99+
}
100+
}
101+
t.Log("The first goroutine finished")
102+
}()
103+
104+
// Create the second goroutine to write & delete K/V entries
105+
t.Log("Create the second goroutine to write & delete K/V entries")
106+
go func() {
107+
defer wg.Done()
108+
for i := 0; i < 100; i++ {
109+
key := fmt.Sprintf("key-%d", i)
110+
value := fmt.Sprintf("value-%d", i)
111+
112+
if err := ctlV3Put(cx, key, value, ""); err != nil {
113+
errs <- fmt.Errorf("failed to put key %q: %w", key, err)
114+
break
115+
}
116+
117+
if err := ctlV3Del(cx, []string{key}, 1); err != nil {
118+
errs <- fmt.Errorf("failed to delete key %q: %w", key, err)
119+
break
120+
}
121+
}
122+
t.Log("The second goroutine finished")
123+
}()
124+
125+
t.Log("Waiting for the two goroutines to complete")
126+
go func() {
127+
wg.Wait()
128+
close(donec)
129+
}()
130+
131+
t.Log("Waiting for test result")
132+
select {
133+
case err := <-errs:
134+
t.Fatalf("Unexpected error: %v", err)
135+
case <-donec:
136+
t.Log("All done!")
137+
case <-time.After(60 * time.Second):
138+
t.Fatal("Test case timeout after 60 seconds")
139+
}
140+
}

tests/e2e/ctl_v3_auth_test.go

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ import (
1919
"fmt"
2020
"net/url"
2121
"os"
22+
"path/filepath"
2223
"syscall"
2324
"testing"
2425
"time"
@@ -100,6 +101,28 @@ func authEnable(cx ctlCtx) error {
100101
return nil
101102
}
102103

104+
func applyTLSWithRootCommonName() func() {
105+
var (
106+
oldCertPath = certPath
107+
oldPrivateKeyPath = privateKeyPath
108+
oldCaPath = caPath
109+
110+
newCertPath = filepath.Join(fixturesDir, "CommonName-root.crt")
111+
newPrivateKeyPath = filepath.Join(fixturesDir, "CommonName-root.key")
112+
newCaPath = filepath.Join(fixturesDir, "CommonName-root.crt")
113+
)
114+
115+
certPath = newCertPath
116+
privateKeyPath = newPrivateKeyPath
117+
caPath = newCaPath
118+
119+
return func() {
120+
certPath = oldCertPath
121+
privateKeyPath = oldPrivateKeyPath
122+
caPath = oldCaPath
123+
}
124+
}
125+
103126
func ctlV3AuthEnable(cx ctlCtx) error {
104127
cmdArgs := append(cx.PrefixArgs(), "auth", "enable")
105128
return spawnWithExpectWithEnv(cmdArgs, cx.envMap, "Authentication Enabled")

tests/fixtures/CommonName-root.crt

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
-----BEGIN CERTIFICATE-----
2+
MIIE5zCCA8+gAwIBAgIJAKooGDZuR2mMMA0GCSqGSIb3DQEBCwUAMH8xCzAJBgNV
3+
BAYTAkNOMRAwDgYDVQQIDAdCZWlqaW5nMRAwDgYDVQQHDAdCZWlqaW5nMQ0wCwYD
4+
VQQKDAREZW1vMQ0wCwYDVQQLDAREZW1vMQ0wCwYDVQQDDARyb290MR8wHQYJKoZI
5+
hvcNAQkBFhB0ZXN0QGV4YW1wbGUuY29tMB4XDTIyMTExNjA2NTI1M1oXDTMyMTEx
6+
MzA2NTI1M1owfzELMAkGA1UEBhMCQ04xEDAOBgNVBAgMB0JlaWppbmcxEDAOBgNV
7+
BAcMB0JlaWppbmcxDTALBgNVBAoMBERlbW8xDTALBgNVBAsMBERlbW8xDTALBgNV
8+
BAMMBHJvb3QxHzAdBgkqhkiG9w0BCQEWEHRlc3RAZXhhbXBsZS5jb20wggEiMA0G
9+
CSqGSIb3DQEBAQUAA4IBDwAwggEKAoIBAQDEAKcjzhtOG3hWbAUCbudE1gPOeteT
10+
0INk2ngN2uCMYjYSZmaGhW/GZk3EvV7wKVuhdTyrh36E5Iajng9d2t1iOU/8jROU
11+
+uAyrS3C/S5P/urq8VBUrt3VG/44bhwTEdafNnAWQ6ojYfmK0tRqoQn1Ftm30l8I
12+
nWof5Jm3loNA2WdNdvAp/D+6OpjUdqGdMkFd0NhkuQODMnycBMw6btUTj5SnmrMk
13+
q7V1aasx4BqN5C4DciZF0pyyR/TT8MoQ5Vcit8rHvQUyz42Lj8+28RkDoi4prJ1i
14+
tLaCt2egDp58vXlYQZTd50inMhnBIapKNdGpg3flW/8AFul1tCTqd8NfAgMBAAGj
15+
ggFkMIIBYDAdBgNVHQ4EFgQUpwwvEqXjA/ArJu1Jnpw7+/sttOAwgbMGA1UdIwSB
16+
qzCBqIAUpwwvEqXjA/ArJu1Jnpw7+/sttOChgYSkgYEwfzELMAkGA1UEBhMCQ04x
17+
EDAOBgNVBAgMB0JlaWppbmcxEDAOBgNVBAcMB0JlaWppbmcxDTALBgNVBAoMBERl
18+
bW8xDTALBgNVBAsMBERlbW8xDTALBgNVBAMMBHJvb3QxHzAdBgkqhkiG9w0BCQEW
19+
EHRlc3RAZXhhbXBsZS5jb22CCQCqKBg2bkdpjDAMBgNVHRMEBTADAQH/MAsGA1Ud
20+
DwQEAwIC/DA2BgNVHREELzAtggtleGFtcGxlLmNvbYINKi5leGFtcGxlLmNvbYIJ
21+
bG9jYWxob3N0hwR/AAABMDYGA1UdEgQvMC2CC2V4YW1wbGUuY29tgg0qLmV4YW1w
22+
bGUuY29tgglsb2NhbGhvc3SHBH8AAAEwDQYJKoZIhvcNAQELBQADggEBAGi48ntm
23+
8cn08FrsCDWapsck7a56/dyFyzLg10c0blu396tzC3ZDCAwQYzHjeXVdeWHyGO+f
24+
KSFlmR6IA0jq6pFhUyJtgaAUJ91jW6s68GTVhlLoFhtYjy6EvhQ0lo+7GWh4qB2s
25+
LI0mJPjaLZY1teAC4TswzwMDVD8QsB06/aFBlA65VjgZiVH+aMwWJ88gKfVGp0Pv
26+
AApsy5MvwQn8WZ2L6foSY04OzXtmAg2gCl0PyDNgieqFDcM1g7mklHNgWl2Gvtte
27+
G6+TiB3gGUUlTsdy0+LS2psL71RS5Jv7g/7XGmSKBPqRmYyQ2t7m2kLPwWKtL5tE
28+
63c0FPtpV0FzKdU=
29+
-----END CERTIFICATE-----

tests/fixtures/CommonName-root.key

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
-----BEGIN RSA PRIVATE KEY-----
2+
MIIEowIBAAKCAQEAxACnI84bTht4VmwFAm7nRNYDznrXk9CDZNp4DdrgjGI2EmZm
3+
hoVvxmZNxL1e8ClboXU8q4d+hOSGo54PXdrdYjlP/I0TlPrgMq0twv0uT/7q6vFQ
4+
VK7d1Rv+OG4cExHWnzZwFkOqI2H5itLUaqEJ9RbZt9JfCJ1qH+SZt5aDQNlnTXbw
5+
Kfw/ujqY1HahnTJBXdDYZLkDgzJ8nATMOm7VE4+Up5qzJKu1dWmrMeAajeQuA3Im
6+
RdKcskf00/DKEOVXIrfKx70FMs+Ni4/PtvEZA6IuKaydYrS2grdnoA6efL15WEGU
7+
3edIpzIZwSGqSjXRqYN35Vv/ABbpdbQk6nfDXwIDAQABAoIBAA5AMebTjH6wVp6J
8+
+g9EOwJxQROZMOVparRBgisXt+3dEitiUKAFQaw+MfdVAXsatrPVj1S1ZEiLSRLK
9+
YjmjuSb0HdGx/DN/zh9BIiukNuLQGQp+AyY1FKHzCBfYQahNSrqGvb2Qq+UosXkb
10+
fSBHly6/u5K28/vvXhD1kQudIOvtAc9tOg8LZnM6N3J4E0GtLqWimRZ4jNK4APu1
11+
YsLIg87Eam+7x25+phz9xc22tZ1H4WY9FnOGprPnievqiV7mgcNGAklTB93C6yX1
12+
EI+QxQnPg0P732C4EJZFDPqhVRA4E7BUb5uTIXCJBA/FFuRIx9ppyLZKt9vjTchM
13+
8YWIEsECgYEA/5DRR9FkIWJZb0Pv3SCc53PMPT/xpYB6lH2lGtG+u+L71dJNDiPt
14+
da3dPXSBy+aF7BbmRDawRvyOLGArlWiSsoEUVlES8BYzQ1MmfDf+MJooJoBE6/g6
15+
2OyyNnPde1GqyxsxgNTITvJCTjYH64lxKVRYfMgMAASK49SjYiEgGn8CgYEAxFXs
16+
Oe0sUcc3P1cQ9pJfSVKpSczZq/OGAxqlniqRHvoWgFfKOWB6F9PN0rd8G2aMlfGS
17+
BjyiPe770gtpX8Z4G4lrtkJD8NvGoVC8yX78HbrXL2RA4lPjQfrveUnwXIRbRKWa
18+
6D/GAYPOuNvJmwF4hY/orWyIqvpNczIjTjs1JyECgYEAvhuNAn6JnKfbXYBM+tIa
19+
xbWHFXzula2IAdOhMN0bpApKSZmBxmYFa0elTuTO9M2Li77RFacU5AlU/T+gzCiZ
20+
D34jkb4Hd18cTRWaiEbiqGbUPSennVzu8ZTJUOZJuEVc5m9ZGLuwMcHWfvWEWLrJ
21+
2fOrS09IVe8LHkV8MC/yAKMCgYBmDUdhgK9Fvqgv60Cs+b4/rZDDBJCsOUOSP3qQ
22+
sQ2HrXSet4MsucIcuoJEog0HbRFsKwm85i1qxdrs/fOCzfXGUnLDZMRN4N7pIL9Q
23+
eQnxJhoNzy2Otw3sUNPDFrSyUjXig7X2PJfeV7XPDqdHQ8dynS/TXRPY04wIcao6
24+
Uro5IQKBgFUz2GjAxI6uc7ihmRv/GYTuXYOlO0IN7MFwQDd0pVnWHkLNZscO9L9/
25+
ALV4g1p/75CewlQfyC8ynOJJWcDeHHFNsSMsOzAxUOVtVenaF/dgwk95wpXj6Rx6
26+
4kvQqnJg97fRBbyzvQcdL36kL8+pbmHNoqHPwxbuigYShB74d6/h
27+
-----END RSA PRIVATE KEY-----

0 commit comments

Comments
 (0)