Skip to content

Commit

Permalink
[v15] Require admin MFA for get/list CAs with secrets (#38777)
Browse files Browse the repository at this point in the history 
* Require admin MFA to list cert authority secrets.

* Allow reuse of Admin MFA for GetCertAuthorities.

* Add tctl tests.

* Reuse MFA for tctl auth export with keys and add tests.

* Fix lint; fix test panic.

* Check for admin MFA after RBAC check.
  • Loading branch information
@Joerger
Joerger committed Feb 29, 2024
1 parent 7a1f09c commit 9edeafa
Show file tree
Hide file tree
Showing 5 changed files with 76 additions and 18 deletions.
30 changes: 21 additions & 9 deletions lib/auth/trust/trustv1/service.go
View file
Expand Up @@ -91,6 +91,11 @@ func NewService(cfg *ServiceConfig) (*Service, error) {

// GetCertAuthority retrieves the matching certificate authority.
func (s *Service) GetCertAuthority(ctx context.Context, req *trustpb.GetCertAuthorityRequest) (*types.CertAuthorityV2, error) {
authCtx, err := s.authorizer.Authorize(ctx)
if err != nil {
return nil, trace.Wrap(err)
}

readVerb := types.VerbReadNoSecrets
if req.IncludeKey {
readVerb = types.VerbRead
Expand All @@ -108,13 +113,15 @@ func (s *Service) GetCertAuthority(ctx context.Context, req *trustpb.GetCertAuth
return nil, trace.Wrap(err)
}

authzCtx, err := s.authorizer.Authorize(ctx)
if err != nil {
if err = authCtx.CheckAccessToResource(contextCA, readVerb); err != nil {
return nil, trace.Wrap(err)
}

if err = authzCtx.CheckAccessToResource(contextCA, readVerb); err != nil {
return nil, trace.Wrap(err)
// Require admin MFA to read secrets.
if req.IncludeKey {
if err := authCtx.AuthorizeAdminActionAllowReusedMFA(); err != nil {
return nil, trace.Wrap(err)
}
}

// Retrieve the requested CA and perform RBAC on it to ensure that
Expand All @@ -124,7 +131,7 @@ func (s *Service) GetCertAuthority(ctx context.Context, req *trustpb.GetCertAuth
return nil, trace.Wrap(err)
}

if err = authzCtx.CheckAccessToResource(ca, readVerb); err != nil {
if err = authCtx.CheckAccessToResource(ca, readVerb); err != nil {
return nil, trace.Wrap(err)
}

Expand All @@ -138,15 +145,20 @@ func (s *Service) GetCertAuthority(ctx context.Context, req *trustpb.GetCertAuth

// GetCertAuthorities retrieves the cert authorities with the specified type.
func (s *Service) GetCertAuthorities(ctx context.Context, req *trustpb.GetCertAuthoritiesRequest) (*trustpb.GetCertAuthoritiesResponse, error) {
authCtx, err := s.authorizer.Authorize(ctx)
if err != nil {
return nil, trace.Wrap(err)
}

verbs := []string{types.VerbList, types.VerbReadNoSecrets}

if req.IncludeKey {
verbs = append(verbs, types.VerbRead)
}

authCtx, err := s.authorizer.Authorize(ctx)
if err != nil {
return nil, trace.Wrap(err)
// Require admin MFA to read secrets.
if err := authCtx.AuthorizeAdminActionAllowReusedMFA(); err != nil {
return nil, trace.Wrap(err)
}
}

if err := authCtx.CheckAccessToKind(types.KindCertAuthority, verbs[0], verbs[1:]...); err != nil {
Expand Down
11 changes: 11 additions & 0 deletions lib/client/ca_export.go
View file
Expand Up @@ -21,12 +21,14 @@ package client
import (
"context"
"encoding/pem"
"errors"
"strings"
"time"

"github.com/gravitational/trace"

apidefaults "github.com/gravitational/teleport/api/defaults"
"github.com/gravitational/teleport/api/mfa"
"github.com/gravitational/teleport/api/types"
"github.com/gravitational/teleport/lib/auth"
"github.com/gravitational/teleport/lib/services"
Expand Down Expand Up @@ -86,6 +88,15 @@ func ExportAuthoritiesSecrets(ctx context.Context, client auth.ClientI, req Expo
func exportAuth(ctx context.Context, client auth.ClientI, req ExportAuthoritiesRequest, exportSecrets bool) (string, error) {
var typesToExport []types.CertAuthType

if exportSecrets {
mfaResponse, err := mfa.PerformAdminActionMFACeremony(ctx, client.PerformMFACeremony, true /*allowReuse*/)
if err == nil {
ctx = mfa.ContextWithMFAResponse(ctx, mfaResponse)
} else if !errors.Is(err, &mfa.ErrMFANotRequired) && !errors.Is(err, &mfa.ErrMFANotSupported) {
return "", trace.Wrap(err)
}
}

// this means to export TLS authority
switch req.AuthType {
// "tls" is supported for backwards compatibility.
Expand Down
7 changes: 7 additions & 0 deletions lib/client/ca_export_test.go
View file
Expand Up @@ -28,6 +28,8 @@ import (
"github.com/gravitational/trace"
"github.com/stretchr/testify/require"

"github.com/gravitational/teleport/api/client/proto"
"github.com/gravitational/teleport/api/mfa"
"github.com/gravitational/teleport/api/types"
"github.com/gravitational/teleport/lib/auth"
)
Expand Down Expand Up @@ -61,6 +63,11 @@ func (m *mockAuthClient) GetCertAuthority(ctx context.Context, id types.CertAuth
return m.server.GetCertAuthority(ctx, id, loadKeys)
}

func (m *mockAuthClient) PerformMFACeremony(ctx context.Context, challengeRequest *proto.CreateAuthenticateChallengeRequest, promptOpts ...mfa.PromptOpt) (*proto.MFAAuthenticateResponse, error) {
// return MFA not required to gracefully skip the MFA prompt.
return nil, &mfa.ErrMFANotRequired
}

func TestExportAuthorities(t *testing.T) {
ctx := context.Background()
const localClusterName = "localcluster"
Expand Down
27 changes: 20 additions & 7 deletions tool/tctl/common/admin_action_test.go
View file
Expand Up @@ -63,7 +63,7 @@ func TestAdminActionMFA(t *testing.T) {

t.Run("Users", s.testUsers)
t.Run("Bots", s.testBots)
t.Run("AuthSign", s.testAuthSign)
t.Run("Auth", s.testAuth)
t.Run("Roles", s.testRoles)
t.Run("AccessRequests", s.testAccessRequests)
t.Run("Tokens", s.testTokens)
Expand Down Expand Up @@ -179,7 +179,7 @@ func (s *adminActionTestSuite) testBots(t *testing.T) {
})
}

func (s *adminActionTestSuite) testAuthSign(t *testing.T) {
func (s *adminActionTestSuite) testAuth(t *testing.T) {
ctx := context.Background()

user, err := types.NewUser("teleuser")
Expand All @@ -194,12 +194,24 @@ func (s *adminActionTestSuite) testAuthSign(t *testing.T) {
identityFilePath := filepath.Join(t.TempDir(), "identity")

t.Run("AuthCommands", func(t *testing.T) {
t.Run("Impersonation", func(t *testing.T) {
s.testCommand(t, ctx, adminActionTestCase{
for name, tc := range map[string]adminActionTestCase{
"auth sign --user=impersonate": {
command: fmt.Sprintf("auth sign --out=%v --user=%v --overwrite", identityFilePath, user.GetName()),
cliCommand: &tctl.AuthCommand{},
},
"auth export": {
command: "auth export --keys",
cliCommand: &tctl.AuthCommand{},
},
"auth export --type": {
command: "auth export --keys --type=user",
cliCommand: &tctl.AuthCommand{},
},
} {
t.Run(name, func(t *testing.T) {
s.testCommand(t, ctx, tc)
})
})
}

// Renewing certs for yourself should not require admin MFA.
t.Run("RenewCerts", func(t *testing.T) {
Expand Down Expand Up @@ -470,6 +482,7 @@ func (s *adminActionTestSuite) testCertAuthority(t *testing.T) {
resource: ca,
resourceCreate: createCertAuthority,
resourceCleanup: deleteCertAuthority,
testGetList: true,
})

s.testEditCommand(t, ctx, editCommandTestCase{
Expand Down Expand Up @@ -868,7 +881,7 @@ func (s *adminActionTestSuite) testResourceCommand(t *testing.T, ctx context.Con
if tc.testGetList {
t.Run("tctl get", func(t *testing.T) {
s.testCommand(t, ctx, adminActionTestCase{
command: fmt.Sprintf("get %v", getResourceRef(tc.resource)),
command: fmt.Sprintf("get --with-secrets %v", getResourceRef(tc.resource)),
cliCommand: &tctl.ResourceCommand{},
setup: tc.resourceCreate,
cleanup: tc.resourceCleanup,
Expand All @@ -877,7 +890,7 @@ func (s *adminActionTestSuite) testResourceCommand(t *testing.T, ctx context.Con

t.Run("tctl list", func(t *testing.T) {
s.testCommand(t, ctx, adminActionTestCase{
command: fmt.Sprintf("get %v", tc.resource.GetKind()),
command: fmt.Sprintf("get --with-secrets %v", tc.resource.GetKind()),
cliCommand: &tctl.ResourceCommand{},
setup: tc.resourceCreate,
cleanup: tc.resourceCleanup,
Expand Down
19 changes: 17 additions & 2 deletions tool/tctl/common/resource_command.go
View file
Expand Up @@ -47,6 +47,7 @@ import (
loginrulepb "github.com/gravitational/teleport/api/gen/proto/go/teleport/loginrule/v1"
machineidv1pb "github.com/gravitational/teleport/api/gen/proto/go/teleport/machineid/v1"
"github.com/gravitational/teleport/api/internalutils/stream"
"github.com/gravitational/teleport/api/mfa"
"github.com/gravitational/teleport/api/types"
"github.com/gravitational/teleport/api/types/accesslist"
"github.com/gravitational/teleport/api/types/discoveryconfig"
Expand Down Expand Up @@ -898,7 +899,6 @@ func (rc *ResourceCommand) createUIConfig(ctx context.Context, client auth.Clien
}
fmt.Printf("ui_config %q has been set\n", uic.GetName())
return nil

}

func (rc *ResourceCommand) createNode(ctx context.Context, client auth.ClientI, raw services.UnknownResource) error {
Expand Down Expand Up @@ -1819,7 +1819,21 @@ func (rc *ResourceCommand) getCollection(ctx context.Context, client auth.Client
}
return &reverseTunnelCollection{tunnels: []types.ReverseTunnel{tunnel}}, nil
case types.KindCertAuthority:
if rc.ref.SubKind == "" && rc.ref.Name == "" {
getAll := rc.ref.SubKind == "" && rc.ref.Name == ""

// Prompt for admin action MFA if secrets were requested.
if rc.withSecrets {
// allow reuse for multiple calls to GetCertAuthorities with different ca types.
allowReuse := getAll
mfaResponse, err := mfa.PerformAdminActionMFACeremony(ctx, client.PerformMFACeremony, allowReuse)
if err == nil {
ctx = mfa.ContextWithMFAResponse(ctx, mfaResponse)
} else if !errors.Is(err, &mfa.ErrMFANotRequired) && !errors.Is(err, &mfa.ErrMFANotSupported) {
return nil, trace.Wrap(err)
}
}

if getAll {
var allAuthorities []types.CertAuthority
for _, caType := range types.CertAuthTypes {
authorities, err := client.GetCertAuthorities(ctx, caType, rc.withSecrets)
Expand All @@ -1834,6 +1848,7 @@ func (rc *ResourceCommand) getCollection(ctx context.Context, client auth.Client
}
return &authorityCollection{cas: allAuthorities}, nil
}

id := types.CertAuthID{Type: types.CertAuthType(rc.ref.SubKind), DomainName: rc.ref.Name}
authority, err := client.GetCertAuthority(ctx, id, rc.withSecrets)
if err != nil {
Expand Down

0 comments on commit 9edeafa

Please sign in to comment.