Skip to content

Address deprecation TODOs for v13 #23679

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.

Sign up for GitHub

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

Merged
Joerger merged 5 commits into from
Mar 28, 2023
Merged

Address deprecation TODOs for v13 #23679

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a3ae32a
Address deprecation TODOs.
Joerger Mar 28, 2023
0f7642a
Add reserved lines.
Joerger Mar 28, 2023
d9f1d23
Remove unused mock servers.
Joerger Mar 28, 2023
0c06599
Merge branch 'master' into joerger/v13-deprecation-todos
Joerger Mar 28, 2023
cabef71
Merge branch 'master' into joerger/v13-deprecation-todos
Joerger Mar 28, 2023
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
19 changes: 0 additions & 19 deletions api/client/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -716,9 +716,6 @@ func (c *Client) GetCurrentUserRoles(ctx context.Context) ([]types.Role, error)
if err != nil {
return nil, trail.FromGRPC(err)
}
// An old server would send RequireSessionMFA instead of RequireMFAType
// DELETE IN 13.0.0
role.CheckSetRequireSessionMFA()
roles = append(roles, role)
}
return roles, nil
Expand Down Expand Up @@ -1439,9 +1436,6 @@ func (c *Client) GetRole(ctx context.Context, name string) (types.Role, error) {
if err != nil {
return nil, trail.FromGRPC(err)
}
// An old server would send RequireSessionMFA instead of RequireMFAType
// DELETE IN 13.0.0
role.CheckSetRequireSessionMFA()
return role, nil
}

Expand All @@ -1453,9 +1447,6 @@ func (c *Client) GetRoles(ctx context.Context) ([]types.Role, error) {
}
roles := make([]types.Role, 0, len(resp.GetRoles()))
for _, role := range resp.GetRoles() {
// An old server would send RequireSessionMFA instead of RequireMFAType
// DELETE IN 13.0.0
role.CheckSetRequireSessionMFA()
roles = append(roles, role)
}
return roles, nil
Expand All @@ -1468,10 +1459,6 @@ func (c *Client) UpsertRole(ctx context.Context, role types.Role) error {
return trace.BadParameter("invalid type %T", role)
}

// An old server would expect RequireSessionMFA instead of RequireMFAType
// DELETE IN 13.0.0
r.CheckSetRequireSessionMFA()

_, err := c.grpc.UpsertRole(ctx, r, c.callOpts...)
return trail.FromGRPC(err)
}
Expand Down Expand Up @@ -2106,9 +2093,6 @@ func (c *Client) GetAuthPreference(ctx context.Context) (types.AuthPreference, e
if err != nil {
return nil, trail.FromGRPC(err)
}
// An old server would send RequireSessionMFA instead of RequireMFAType
// DELETE IN 13.0.0
pref.CheckSetRequireSessionMFA()
return pref, nil
}

Expand All @@ -2118,9 +2102,6 @@ func (c *Client) SetAuthPreference(ctx context.Context, authPref types.AuthPrefe
if !ok {
return trace.BadParameter("invalid type %T", authPref)
}
// An old server would send RequireSessionMFA instead of RequireMFAType
// DELETE IN 13.0.0
authPrefV2.CheckSetRequireSessionMFA()
_, err := c.grpc.SetAuthPreference(ctx, authPrefV2, c.callOpts...)
return trail.FromGRPC(err)
}
Expand Down
201 changes: 0 additions & 201 deletions api/client/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ import (
"google.golang.org/grpc/codes"
"google.golang.org/grpc/credentials/insecure"
"google.golang.org/grpc/status"
"google.golang.org/protobuf/types/known/emptypb"

"github.com/gravitational/teleport/api/client/proto"
"github.com/gravitational/teleport/api/defaults"
Expand Down Expand Up @@ -623,203 +622,3 @@ func TestGetResources(t *testing.T) {
})
}
}

type mockRoleServer struct {
*mockServer
roles map[string]*types.RoleV6
}

func newMockRoleServer() *mockRoleServer {
m := &mockRoleServer{
&mockServer{
grpc: grpc.NewServer(),
UnimplementedAuthServiceServer: &proto.UnimplementedAuthServiceServer{},
},
make(map[string]*types.RoleV6),
}
proto.RegisterAuthServiceServer(m.grpc, m)
return m
}

func startMockRoleServer(t *testing.T) string {
l, err := net.Listen("tcp", "")
require.NoError(t, err)
t.Cleanup(func() { require.NoError(t, l.Close()) })
go newMockRoleServer().grpc.Serve(l)
return l.Addr().String()
}

func (m *mockRoleServer) GetRole(ctx context.Context, req *proto.GetRoleRequest) (*types.RoleV6, error) {
conn, ok := m.roles[req.Name]
if !ok {
return nil, trace.NotFound("not found")
}
return conn, nil
}

func (m *mockRoleServer) GetRoles(ctx context.Context, _ *emptypb.Empty) (*proto.GetRolesResponse, error) {
var connectors []*types.RoleV6
for _, conn := range m.roles {
connectors = append(connectors, conn)
}
return &proto.GetRolesResponse{
Roles: connectors,
}, nil
}

func (m *mockRoleServer) UpsertRole(ctx context.Context, role *types.RoleV6) (*emptypb.Empty, error) {
m.roles[role.Metadata.Name] = role
return &emptypb.Empty{}, nil
}

func (m *mockRoleServer) GetCurrentUserRoles(_ *emptypb.Empty, stream proto.AuthService_GetCurrentUserRolesServer) error {
for _, role := range m.roles {
if err := stream.Send(role); err != nil {
return trace.Wrap(err)
}
}

return nil
}

// Test that client will perform properly with an old server
// DELETE IN 13.0.0
func TestSetRoleRequireSessionMFABackwardsCompatibility(t *testing.T) {
ctx := context.Background()
addr := startMockRoleServer(t)

// Create client
clt, err := New(ctx, Config{
Addrs: []string{addr},
Credentials: []Credentials{
&mockInsecureTLSCredentials{}, // TODO(Joerger) replace insecure credentials
},
DialOpts: []grpc.DialOption{
grpc.WithTransportCredentials(insecure.NewCredentials()), // TODO(Joerger) remove insecure dial option
},
})
require.NoError(t, err)

role := &types.RoleV6{
Metadata: types.Metadata{
Name: "one",
},
}

t.Run("UpsertRole", func(t *testing.T) {
// UpsertRole should set "RequireSessionMFA" on the provided role if "RequireMFAType" is set
role.Spec.Options.RequireMFAType = types.RequireMFAType_SESSION
role.Spec.Options.RequireSessionMFA = false
err = clt.UpsertRole(ctx, role)
require.NoError(t, err)
require.True(t, role.GetOptions().RequireSessionMFA)
})

t.Run("GetRole", func(t *testing.T) {
// GetRole should set "RequireMFAType" on the received role if empty
role.Spec.Options.RequireMFAType = 0
role.Spec.Options.RequireSessionMFA = true
roleResp, err := clt.GetRole(ctx, role.GetName())
require.NoError(t, err)
require.Equal(t, types.RequireMFAType_SESSION, roleResp.GetOptions().RequireMFAType)
})

t.Run("GetRoles", func(t *testing.T) {
// GetRoles should set "RequireMFAType" on the received roles if empty
role.Spec.Options.RequireMFAType = 0
role.Spec.Options.RequireSessionMFA = true
rolesResp, err := clt.GetRoles(ctx)
require.NoError(t, err)
require.Len(t, rolesResp, 1)
require.Equal(t, types.RequireMFAType_SESSION, rolesResp[0].GetOptions().RequireMFAType)
})

t.Run("GetCurrentUserRoles", func(t *testing.T) {
// GetCurrentUserRoles should set "RequireMFAType" on the received roles if empty
role.Spec.Options.RequireMFAType = 0
role.Spec.Options.RequireSessionMFA = true
rolesResp, err := clt.GetCurrentUserRoles(ctx)
require.NoError(t, err)
require.Len(t, rolesResp, 1)
require.Equal(t, types.RequireMFAType_SESSION, rolesResp[0].GetOptions().RequireMFAType)
})
}

type mockAuthPreferenceServer struct {
*mockServer
pref *types.AuthPreferenceV2
}

func newMockAuthPreferenceServer() *mockAuthPreferenceServer {
m := &mockAuthPreferenceServer{
mockServer: &mockServer{
grpc: grpc.NewServer(),
UnimplementedAuthServiceServer: &proto.UnimplementedAuthServiceServer{},
},
}
proto.RegisterAuthServiceServer(m.grpc, m)
return m
}

func startMockAuthPreferenceServer(t *testing.T) string {
l, err := net.Listen("tcp", "")
require.NoError(t, err)
t.Cleanup(func() { require.NoError(t, l.Close()) })
go newMockAuthPreferenceServer().grpc.Serve(l)
return l.Addr().String()
}

func (m *mockAuthPreferenceServer) GetAuthPreference(ctx context.Context, _ *emptypb.Empty) (*types.AuthPreferenceV2, error) {
if m.pref == nil {
return nil, trace.NotFound("not found")
}
return m.pref, nil
}

func (m *mockAuthPreferenceServer) SetAuthPreference(ctx context.Context, pref *types.AuthPreferenceV2) (*emptypb.Empty, error) {
m.pref = pref
return &emptypb.Empty{}, nil
}

// Test that client will perform properly with an old server
// DELETE IN 13.0.0
func TestSetAuthPreferenceRequireSessionMFABackwardsCompatibility(t *testing.T) {
ctx := context.Background()
addr := startMockAuthPreferenceServer(t)

// Create client
clt, err := New(ctx, Config{
Addrs: []string{addr},
Credentials: []Credentials{
&mockInsecureTLSCredentials{}, // TODO(Joerger) replace insecure credentials
},
DialOpts: []grpc.DialOption{
grpc.WithTransportCredentials(insecure.NewCredentials()), // TODO(Joerger) remove insecure dial option
},
})
require.NoError(t, err)

pref := &types.AuthPreferenceV2{
Metadata: types.Metadata{
Name: "one",
},
}

t.Run("SetAuthPreference", func(t *testing.T) {
// SetAuthPreference should set "RequireSessionMFA" on the provided auth pref if "RequireMFAType" is set
pref.Spec.RequireMFAType = types.RequireMFAType_SESSION
pref.Spec.RequireSessionMFA = false
err = clt.SetAuthPreference(ctx, pref)
require.NoError(t, err)
require.True(t, pref.Spec.RequireSessionMFA)
})

t.Run("GetAuthPreference", func(t *testing.T) {
// GetAuthPreference should set "RequireMFAType" on the received auth pref if empty
pref.Spec.RequireMFAType = 0
pref.Spec.RequireSessionMFA = true
prefResp, err := clt.GetAuthPreference(ctx)
require.NoError(t, err)
require.Equal(t, types.RequireMFAType_SESSION, prefResp.GetRequireMFAType())
})
}
12 changes: 2 additions & 10 deletions api/proto/teleport/legacy/types/types.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1613,11 +1613,7 @@ message AuthPreferenceSpecV2 {
(gogoproto.jsontag) = "u2f,omitempty"
];

// RequireSessionMFA causes all sessions in this cluster to require MFA
// checks.
//
// DELETE IN 13.0.0 in favor of RequireMFAType
bool RequireSessionMFA = 5 [(gogoproto.jsontag) = "-"];
Comment thread
Joerger marked this conversation as resolved.
reserved 5; // RequireSessionMFA replaced by RequireMFAType

// DisconnectExpiredCert provides disconnect expired certificate setting -
// if true, connections with expired client certificates will get disconnected
Expand Down Expand Up @@ -2301,11 +2297,7 @@ message RoleOptions {
// RequestPrompt is an optional message which tells users what they aught to
string RequestPrompt = 12 [(gogoproto.jsontag) = "request_prompt,omitempty"];

// RequireSessionMFA specifies whether a user is required to do an MFA
// check for every session.
//
// DELETE IN 13.0.0 in favor of RequireMFAType
bool RequireSessionMFA = 13 [(gogoproto.jsontag) = "-"];
reserved 13; // RequireSessionMFA replaced by RequireMFAType

// Lock specifies the locking mode (strict|best_effort) to be applied with
// the role.
Expand Down
13 changes: 0 additions & 13 deletions api/types/authentication.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -449,9 +449,6 @@ func (c *AuthPreferenceV2) CheckAndSetDefaults() error {
return trace.Wrap(err)
}

// DELETE IN 13.0.0
c.CheckSetRequireSessionMFA()

if c.Spec.Type == "" {
c.Spec.Type = constants.Local
}
Expand Down Expand Up @@ -609,16 +606,6 @@ func (c *AuthPreferenceV2) CheckAndSetDefaults() error {
return nil
}

// RequireSessionMFA must be checked/set when communicating with an old server or client.
// DELETE IN 13.0.0
func (c *AuthPreferenceV2) CheckSetRequireSessionMFA() {
if c.Spec.RequireMFAType != RequireMFAType_OFF {
c.Spec.RequireSessionMFA = c.Spec.RequireMFAType.IsSessionMFARequired()
} else if c.Spec.RequireSessionMFA {
c.Spec.RequireMFAType = RequireMFAType_SESSION
}
}

// String represents a human readable version of authentication settings.
func (c *AuthPreferenceV2) String() string {
return fmt.Sprintf("AuthPreference(Type=%q,SecondFactor=%q)", c.Spec.Type, c.Spec.SecondFactor)
Expand Down
13 changes: 0 additions & 13 deletions api/types/role.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -777,9 +777,6 @@ func (r *RoleV6) CheckAndSetDefaults() error {
return trace.Wrap(err)
}

// DELETE IN 13.0.0
r.CheckSetRequireSessionMFA()

// Make sure all fields have defaults.
if r.Spec.Options.CertificateFormat == "" {
r.Spec.Options.CertificateFormat = constants.CertificateFormatStandard
Expand Down Expand Up @@ -981,16 +978,6 @@ func (r *RoleV6) CheckAndSetDefaults() error {
return nil
}

// RequireSessionMFA must be checked/set when communicating with an old server or client.
// DELETE IN 13.0.0
func (r *RoleV6) CheckSetRequireSessionMFA() {
if r.Spec.Options.RequireMFAType != RequireMFAType_OFF {
r.Spec.Options.RequireSessionMFA = r.Spec.Options.RequireMFAType.IsSessionMFARequired()
} else if r.Spec.Options.RequireSessionMFA {
r.Spec.Options.RequireMFAType = RequireMFAType_SESSION
}
}

// String returns the human readable representation of a role.
func (r *RoleV6) String() string {
options, _ := json.Marshal(r.Spec.Options)
Expand Down
Loading