From 30b87739277df9d5b5e4862dc1e3813f1f4b33f9 Mon Sep 17 00:00:00 2001 From: Grzegorz Zdunek Date: Tue, 27 Feb 2024 13:34:22 +0100 Subject: [PATCH 1/2] Handle "expired certs" errors that come from the server --- lib/auth/auth_with_roles.go | 2 +- lib/client/api.go | 12 +++++++++++- lib/utils/utils.go | 5 +++++ 3 files changed, 17 insertions(+), 2 deletions(-) diff --git a/lib/auth/auth_with_roles.go b/lib/auth/auth_with_roles.go index a873d1dbd9013..8bdd89cd10334 100644 --- a/lib/auth/auth_with_roles.go +++ b/lib/auth/auth_with_roles.go @@ -2905,7 +2905,7 @@ func (a *ServerWithRoles) generateUserCerts(ctx context.Context, req proto.UserC return nil, trace.AccessDenied("access denied") } if req.Expires.Before(a.authServer.GetClock().Now()) { - return nil, trace.AccessDenied("access denied: client credentials have expired, please relogin.") + return nil, utils.ErrClientCredentialsHaveExpired } if identity.Renewable || isRoleImpersonation(req) { diff --git a/lib/client/api.go b/lib/client/api.go index 5efb601be81e1..c9b9a53b4f813 100644 --- a/lib/client/api.go +++ b/lib/client/api.go @@ -679,7 +679,17 @@ func IsErrorResolvableWithRelogin(err error) bool { // https://github.com/gravitational/teleport/pull/30578. var remoteErr *interceptors.RemoteError if errors.As(err, &remoteErr) { - return false + // Exception for the two "retryable" errors that come from RPCs. + // + // Since Connect no longer checks the user cert before making an RPC, + // it has to be able to properly recognize "expired certs" errors + // that come from the server (to show a re-login dialog). + // + // TODO(gzdunek): These manual checks should be replaced with retryable + // errors returned explicitly, as described below by codingllama. + isClientCredentialsHaveExpired := errors.Is(err, utils.ErrClientCredentialsHaveExpired) + isTLSExpiredCertificate := strings.Contains(err.Error(), "tls: expired certificate") + return isClientCredentialsHaveExpired || isTLSExpiredCertificate } return keys.IsPrivateKeyPolicyError(err) || diff --git a/lib/utils/utils.go b/lib/utils/utils.go index 042a126fd3262..9c89d1edd1ddf 100644 --- a/lib/utils/utils.go +++ b/lib/utils/utils.go @@ -697,6 +697,11 @@ func ByteCount(b int64) string { // ErrLimitReached means that the read limit is reached. var ErrLimitReached = &trace.LimitExceededError{Message: "the read limit is reached"} +// ErrClientCredentialsHaveExpired means +// that the credentials expired on the server-side, +// and the user should relogin. +var ErrClientCredentialsHaveExpired = &trace.AccessDeniedError{Message: "access denied: client credentials have expired, please relogin."} + const ( // CertTeleportUser specifies teleport user CertTeleportUser = "x-teleport-user" From 6e5579f2ede668b8d183df91aa917df32480a932 Mon Sep 17 00:00:00 2001 From: Grzegorz Zdunek Date: Tue, 27 Feb 2024 15:47:21 +0100 Subject: [PATCH 2/2] Review comments --- api/client/errors.go | 21 +++++++++++++++++++++ lib/auth/auth_with_roles.go | 2 +- lib/client/api.go | 2 +- lib/utils/utils.go | 5 ----- 4 files changed, 23 insertions(+), 7 deletions(-) create mode 100644 api/client/errors.go diff --git a/api/client/errors.go b/api/client/errors.go new file mode 100644 index 0000000000000..69ee906467885 --- /dev/null +++ b/api/client/errors.go @@ -0,0 +1,21 @@ +// Copyright 2024 Gravitational, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package client + +import "github.com/gravitational/trace" + +// ErrClientCredentialsHaveExpired means that the credentials expired on +// the server-side and the user should relogin. +var ErrClientCredentialsHaveExpired = &trace.AccessDeniedError{Message: "access denied: client credentials have expired, please relogin."} diff --git a/lib/auth/auth_with_roles.go b/lib/auth/auth_with_roles.go index 8bdd89cd10334..1e7eb30467fe5 100644 --- a/lib/auth/auth_with_roles.go +++ b/lib/auth/auth_with_roles.go @@ -2905,7 +2905,7 @@ func (a *ServerWithRoles) generateUserCerts(ctx context.Context, req proto.UserC return nil, trace.AccessDenied("access denied") } if req.Expires.Before(a.authServer.GetClock().Now()) { - return nil, utils.ErrClientCredentialsHaveExpired + return nil, trace.Wrap(client.ErrClientCredentialsHaveExpired) } if identity.Renewable || isRoleImpersonation(req) { diff --git a/lib/client/api.go b/lib/client/api.go index c9b9a53b4f813..b24b5149652de 100644 --- a/lib/client/api.go +++ b/lib/client/api.go @@ -687,7 +687,7 @@ func IsErrorResolvableWithRelogin(err error) bool { // // TODO(gzdunek): These manual checks should be replaced with retryable // errors returned explicitly, as described below by codingllama. - isClientCredentialsHaveExpired := errors.Is(err, utils.ErrClientCredentialsHaveExpired) + isClientCredentialsHaveExpired := errors.Is(err, client.ErrClientCredentialsHaveExpired) isTLSExpiredCertificate := strings.Contains(err.Error(), "tls: expired certificate") return isClientCredentialsHaveExpired || isTLSExpiredCertificate } diff --git a/lib/utils/utils.go b/lib/utils/utils.go index 9c89d1edd1ddf..042a126fd3262 100644 --- a/lib/utils/utils.go +++ b/lib/utils/utils.go @@ -697,11 +697,6 @@ func ByteCount(b int64) string { // ErrLimitReached means that the read limit is reached. var ErrLimitReached = &trace.LimitExceededError{Message: "the read limit is reached"} -// ErrClientCredentialsHaveExpired means -// that the credentials expired on the server-side, -// and the user should relogin. -var ErrClientCredentialsHaveExpired = &trace.AccessDeniedError{Message: "access denied: client credentials have expired, please relogin."} - const ( // CertTeleportUser specifies teleport user CertTeleportUser = "x-teleport-user"