Skip to content
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.

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

Windows user creation #24780

Merged
merged 27 commits into from
Apr 28, 2023
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
2273cff
Windows auto user creation
probakowski Mar 29, 2023
6594888
Merge branch 'master' into probakowski/windows_user_creation
probakowski Apr 4, 2023
840ade2
changes in role
probakowski Apr 5, 2023
89d89a8
Merge remote-tracking branch 'origin/master' into probakowski/windows…
probakowski Apr 13, 2023
41c7243
fix roles
probakowski Apr 13, 2023
229f2a6
Merge remote-tracking branch 'origin/master' into probakowski/windows…
probakowski Apr 18, 2023
3df2874
make grpc
probakowski Apr 18, 2023
27cb87f
fix imports
probakowski Apr 19, 2023
89056a6
fix test
probakowski Apr 19, 2023
76eb7b9
fix test
probakowski Apr 19, 2023
299ac3a
fix test
probakowski Apr 19, 2023
7c5f950
fix test
probakowski Apr 19, 2023
235c6e2
fix test
probakowski Apr 19, 2023
cd07741
Merge branch 'master' into probakowski/windows_user_creation
probakowski Apr 19, 2023
b351b24
windows labels
probakowski Apr 26, 2023
7166f28
rename OID, add json tags
probakowski Apr 26, 2023
07e0e11
params to struct
probakowski Apr 26, 2023
1ee9f5a
Merge remote-tracking branch 'origin/master' into probakowski/windows…
probakowski Apr 26, 2023
4d1cf76
grpc
probakowski Apr 26, 2023
1b802f8
Update lib/services/role.go
probakowski Apr 27, 2023
225204a
Update lib/services/access_checker.go
probakowski Apr 27, 2023
71a3551
Merge remote-tracking branch 'origin/master' into probakowski/windows…
probakowski Apr 27, 2023
15bfcf2
grpc
probakowski Apr 27, 2023
e76f95c
bump e
probakowski Apr 27, 2023
e486435
Merge remote-tracking branch 'origin/probakowski/windows_user_creatio…
probakowski Apr 27, 2023
bf8c640
Merge branch 'master' into probakowski/windows_user_creation
probakowski Apr 27, 2023
fb353bb
only add extension when we create user
probakowski Apr 28, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 10 additions & 0 deletions api/proto/teleport/legacy/types/types.proto
Original file line number Diff line number Diff line change
Expand Up @@ -2368,6 +2368,13 @@ message RoleOptions {
// IDP is a set of options related to accessing IdPs within Teleport.
// Requires Teleport Enterprise.
IdPOptions IDP = 25 [(gogoproto.jsontag) = "idp,omitempty"];

// CreateDesktopUser allows users to be automatically created on a Windows desktop
BoolValue CreateDesktopUser = 26 [
(gogoproto.nullable) = true,
(gogoproto.jsontag) = "create_desktop_user",
(gogoproto.customtype) = "BoolOption"
];
}

message RecordSession {
Expand Down Expand Up @@ -2536,6 +2543,9 @@ message RoleConditions {
(gogoproto.jsontag) = "db_service_labels,omitempty",
(gogoproto.customtype) = "Labels"
];

// DesktopGroups is a list of groups for created desktop users to be added to
repeated string DesktopGroups = 27 [(gogoproto.jsontag) = "desktop_groups,omitempty"];
}

// KubernetesResource is the Kubernetes resource identifier.
Expand Down
30 changes: 28 additions & 2 deletions api/types/role.go
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,11 @@ type Role interface {
// SetHostGroups sets the list of groups this role is put in when users are provisioned
SetHostGroups(RoleConditionType, []string)

// GetDesktopGroups gets the list of groups this role is put in when desktop users are provisioned
GetDesktopGroups(RoleConditionType) []string
// SetDesktopGroups sets the list of groups this role is put in when desktop users are provisioned
SetDesktopGroups(RoleConditionType, []string)

// GetHostSudoers gets the list of sudoers entries for the role
GetHostSudoers(RoleConditionType) []string
// SetHostSudoers sets the list of sudoers entries for the role
Expand All @@ -215,7 +220,7 @@ type Role interface {

// GetDatabaseServiceLabels gets the map of db service labels this role is allowed or denied access to.
GetDatabaseServiceLabels(RoleConditionType) Labels
// SetDatabaseLabels sets the map of db service labels this role is allowed or denied access to.
// SetDatabaseServiceLabels sets the map of db service labels this role is allowed or denied access to.
SetDatabaseServiceLabels(RoleConditionType, Labels)
}

Expand Down Expand Up @@ -714,7 +719,7 @@ func (r *RoleV6) SetRules(rct RoleConditionType, in []Rule) {
}
}

// GetGroups gets all groups for provisioned user
// GetHostGroups gets all groups for provisioned user
func (r *RoleV6) GetHostGroups(rct RoleConditionType) []string {
if rct == Allow {
return r.Spec.Allow.HostGroups
Expand All @@ -732,6 +737,24 @@ func (r *RoleV6) SetHostGroups(rct RoleConditionType, groups []string) {
}
}

// GetDesktopGroups gets all groups for provisioned user
func (r *RoleV6) GetDesktopGroups(rct RoleConditionType) []string {
if rct == Allow {
return r.Spec.Allow.DesktopGroups
}
return r.Spec.Deny.DesktopGroups
}

// SetDesktopGroups sets all groups for provisioned user
func (r *RoleV6) SetDesktopGroups(rct RoleConditionType, groups []string) {
ncopy := utils.CopyStrings(groups)
if rct == Allow {
r.Spec.Allow.DesktopGroups = ncopy
} else {
r.Spec.Deny.DesktopGroups = ncopy
}
}

// GetHostSudoers gets the list of sudoers entries for the role
func (r *RoleV6) GetHostSudoers(rct RoleConditionType) []string {
if rct == Allow {
Expand Down Expand Up @@ -808,6 +831,9 @@ func (r *RoleV6) CheckAndSetDefaults() error {
if r.Spec.Options.CreateHostUser == nil {
r.Spec.Options.CreateHostUser = NewBoolOption(false)
}
if r.Spec.Options.CreateDesktopUser == nil {
r.Spec.Options.CreateDesktopUser = NewBoolOption(false)
}
if r.Spec.Options.SSHFileCopy == nil {
r.Spec.Options.SSHFileCopy = NewBoolOption(true)
}
Expand Down
2,906 changes: 1,508 additions & 1,398 deletions api/types/types.pb.go

Large diffs are not rendered by default.

25 changes: 25 additions & 0 deletions lib/auth/windows/windows.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"crypto/x509"
"crypto/x509/pkix"
"encoding/asn1"
"encoding/json"
"encoding/pem"
"fmt"
"time"
Expand All @@ -30,6 +31,7 @@ import (
"github.com/gravitational/teleport/api/client/proto"
"github.com/gravitational/teleport/api/types"
"github.com/gravitational/teleport/lib/services"
"github.com/gravitational/teleport/lib/tlsca"
)

const (
Expand All @@ -46,6 +48,20 @@ type certRequest struct {
keyDER []byte
}

func createUsersExtension(createUser bool, groups []string) (pkix.Extension, error) {
value, err := json.Marshal(struct {
CreateUser bool
Groups []string
}{createUser, groups})
if err != nil {
return pkix.Extension{}, trace.Wrap(err)
}
return pkix.Extension{
Id: tlsca.CreateUserOID,
Value: value,
}, nil
}

func getCertRequest(req *GenerateCredentialsRequest) (*certRequest, error) {
// Important: rdpclient currently only supports 2048-bit RSA keys.
// If you switch the key type here, update handle_general_authentication in
Expand All @@ -64,6 +80,10 @@ func getCertRequest(req *GenerateCredentialsRequest) (*certRequest, error) {
if err != nil {
return nil, trace.Wrap(err)
}
createUser, err := createUsersExtension(req.CreateUser, req.Groups)
gabrielcorado marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return nil, trace.Wrap(err)
}
csr := &x509.CertificateRequest{
Subject: pkix.Name{CommonName: req.Username},
// We have to pass SAN and ExtKeyUsage as raw extensions because
Expand All @@ -75,6 +95,7 @@ func getCertRequest(req *GenerateCredentialsRequest) (*certRequest, error) {
ExtraExtensions: []pkix.Extension{
EnhancedKeyUsageExtension,
san,
createUser,
},
}

Expand Down Expand Up @@ -145,6 +166,10 @@ type GenerateCredentialsRequest struct {
// CAType is the certificate authority type used to generate the certificate.
// This is used to proper generate the CRL LDAP path.
CAType types.CertAuthType
// CreateUser specifies if Windows user should be created if missing
CreateUser bool
zmb3 marked this conversation as resolved.
Show resolved Hide resolved
// Groups are groups that user should be member of
Groups []string
zmb3 marked this conversation as resolved.
Show resolved Hide resolved
}

// GenerateWindowsDesktopCredentials generates a private key / certificate pair for the given
Expand Down
3 changes: 3 additions & 0 deletions lib/services/access_checker.go
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,9 @@ type AccessChecker interface {
// a role disallows host user creation
HostUsers(types.Server) (*HostUsersInfo, error)

// DesktopGroups returns desktop groups matching a desktop or nil if a role disallows desktop user creation
probakowski marked this conversation as resolved.
Show resolved Hide resolved
DesktopGroups(types.WindowsDesktop) ([]string, error)

// PinSourceIP forces the same client IP for certificate generation and SSH usage
PinSourceIP() bool

Expand Down
42 changes: 42 additions & 0 deletions lib/services/role.go
Original file line number Diff line number Diff line change
Expand Up @@ -411,6 +411,9 @@ func ApplyTraits(r types.Role, traits map[string][]string) types.Role {
r.SetHostSudoers(condition,
applyValueTraitsSlice(r.GetHostSudoers(condition), traits, "host_sudoers"))

r.SetDesktopGroups(condition,
applyValueTraitsSlice(r.GetDesktopGroups(condition), traits, "desktop_groups"))

options := r.GetOptions()
for i, ext := range options.CertExtensions {
vals, err := ApplyValueTraits(ext.Value, traits)
Expand Down Expand Up @@ -2703,6 +2706,45 @@ func (set RoleSet) EnhancedRecordingSet() map[string]bool {
return m
}

// DesktopGroups returns desktop groups matching a desktop or nil if a role disallows desktop user creation
probakowski marked this conversation as resolved.
Show resolved Hide resolved
func (set RoleSet) DesktopGroups(s types.WindowsDesktop) ([]string, error) {
groups := make(map[string]struct{})
desktop := s.GetAllLabels()
probakowski marked this conversation as resolved.
Show resolved Hide resolved
for _, role := range set {
result, _, err := MatchLabels(role.GetNodeLabels(types.Allow), desktop)
if err != nil {
return nil, trace.Wrap(err)
}
// skip nodes that dont have matching labels
if !result {
continue
}
createDesktopUser := role.GetOptions().CreateDesktopUser
// if any of the matching roles do not enable create host
// user, the user should not be allowed on
if createDesktopUser == nil || !createDesktopUser.Value {
return nil, trace.AccessDenied("user is not allowed to create host users")
}
for _, group := range role.GetDesktopGroups(types.Allow) {
groups[group] = struct{}{}
}
}
for _, role := range set {
result, _, err := MatchLabels(role.GetNodeLabels(types.Deny), desktop)
if err != nil {
return nil, trace.Wrap(err)
}
if !result {
continue
}
for _, group := range role.GetDesktopGroups(types.Deny) {
delete(groups, group)
}
}

return utils.StringsSliceFromSet(groups), nil
}

// HostUsers returns host user information matching a server or nil if
// a role disallows host user creation
func (set RoleSet) HostUsers(s types.Server) (*HostUsersInfo, error) {
Expand Down
5 changes: 5 additions & 0 deletions lib/services/role_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -234,6 +234,7 @@ func TestRoleParse(t *testing.T) {
BPF: apidefaults.EnhancedEvents(),
DesktopClipboard: types.NewBoolOption(true),
DesktopDirectorySharing: types.NewBoolOption(true),
CreateDesktopUser: types.NewBoolOption(false),
CreateHostUser: types.NewBoolOption(false),
SSHFileCopy: types.NewBoolOption(true),
IDP: &types.IdPOptions{
Expand Down Expand Up @@ -285,6 +286,7 @@ func TestRoleParse(t *testing.T) {
BPF: apidefaults.EnhancedEvents(),
DesktopClipboard: types.NewBoolOption(true),
DesktopDirectorySharing: types.NewBoolOption(true),
CreateDesktopUser: types.NewBoolOption(false),
CreateHostUser: types.NewBoolOption(false),
SSHFileCopy: types.NewBoolOption(true),
IDP: &types.IdPOptions{
Expand Down Expand Up @@ -375,6 +377,7 @@ func TestRoleParse(t *testing.T) {
BPF: apidefaults.EnhancedEvents(),
DesktopClipboard: types.NewBoolOption(true),
DesktopDirectorySharing: types.NewBoolOption(true),
CreateDesktopUser: types.NewBoolOption(false),
CreateHostUser: types.NewBoolOption(false),
SSHFileCopy: types.NewBoolOption(false),
IDP: &types.IdPOptions{
Expand Down Expand Up @@ -480,6 +483,7 @@ func TestRoleParse(t *testing.T) {
BPF: apidefaults.EnhancedEvents(),
DesktopClipboard: types.NewBoolOption(true),
DesktopDirectorySharing: types.NewBoolOption(true),
CreateDesktopUser: types.NewBoolOption(false),
CreateHostUser: types.NewBoolOption(false),
SSHFileCopy: types.NewBoolOption(true),
IDP: &types.IdPOptions{
Expand Down Expand Up @@ -572,6 +576,7 @@ func TestRoleParse(t *testing.T) {
BPF: apidefaults.EnhancedEvents(),
DesktopClipboard: types.NewBoolOption(true),
DesktopDirectorySharing: types.NewBoolOption(true),
CreateDesktopUser: types.NewBoolOption(false),
CreateHostUser: types.NewBoolOption(false),
SSHFileCopy: types.NewBoolOption(true),
IDP: &types.IdPOptions{
Expand Down
21 changes: 16 additions & 5 deletions lib/srv/desktop/windows_server.go
Original file line number Diff line number Diff line change
Expand Up @@ -455,7 +455,7 @@ func (s *WindowsService) tlsConfigForLDAP() (*tls.Config, error) {
using to sign in. This is set to become a strict requirement by May 2023,
please update your configuration file before then.`)
}
certDER, keyDER, err := s.generateCredentials(s.closeCtx, user, s.cfg.Domain, windowsDesktopServiceCertTTL, s.cfg.SID)
certDER, keyDER, err := s.generateCredentials(s.closeCtx, user, s.cfg.Domain, windowsDesktopServiceCertTTL, s.cfg.SID, false, nil)
zmb3 marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return nil, trace.Wrap(err)
}
Expand Down Expand Up @@ -857,10 +857,19 @@ func (s *WindowsService) connectRDP(ctx context.Context, log logrus.FieldLogger,
tdpConn.OnRecv = s.makeTDPReceiveHandler(ctx, sw, delay, &identity, string(sessionID), desktop.GetAddr(), tdpConn)

sessionStartTime := s.cfg.Clock.Now().UTC().Round(time.Millisecond)
groups, err := authCtx.Checker.DesktopGroups(desktop)
if err != nil && !trace.IsAccessDenied(err) {
s.onSessionStart(ctx, sw, &identity, sessionStartTime, windowsUser, string(sessionID), desktop, err)
return trace.Wrap(err)
}
var createUsers bool
if err == nil {
createUsers = true
}
probakowski marked this conversation as resolved.
Show resolved Hide resolved
rdpc, err := rdpclient.New(rdpclient.Config{
Log: log,
GenerateUserCert: func(ctx context.Context, username string, ttl time.Duration) (certDER, keyDER []byte, err error) {
return s.generateUserCert(ctx, username, ttl, desktop)
return s.generateUserCert(ctx, username, ttl, desktop, createUsers, groups)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could be mistaken, but my understanding is that a role that allows for creating users looks like

kind: "role"
version: "v5"
metadata:
  name: "example"
spec:
  options:
    create_desktop_user: true
  allow:
    desktop_groups: [ "reader", "writer", "{{external.desktop_groups}}" ]
    windows_desktop_logins: ['DBAdmin']
    windows_desktop_labels:
      'env': ['staging', 'test']

and that such a role would only allow a user to create the DBAdmin user with the given desktop_groups on nodes with the labels env: staging or env: test. However, afaict, the certificate created here won't restrict the system to only creating the DBAdmin user in those groups -- for example, a user might have another role like

kind: "role"
version: "v5"
metadata:
  name: "another-example"
spec:
  options:
    create_desktop_user: true
  allow:
    desktop_groups: [ "reader" ]
    windows_desktop_logins: ['SystemAdmin']
    windows_desktop_labels:
      'env': ['staging', 'test']

In that case, the user's intention would be to only allow SystemAdmin to be created and given the reader group on env: staging/test nodes, however groups, err := authCtx.Checker.DesktopGroups(desktop) would result in a groups = ["reader", "writer", "{{external.desktop_groups}}"] and createUsers would be true. In which case if the user were logging in as SystemAdmin, that user would be created and then added to all of ["reader", "writer", "{{external.desktop_groups}}"].

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is how it would work, this behavior matches what we have in server access, login is not considered there when gathering groups, only node labels, host_groups and create_host_user

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha. I see that as an error prone API design and think we should reconsider making it "role-bound", but beyond the scope here.

},
CertTTL: windows.CertTTL,
Addr: desktop.GetAddr(),
Expand Down Expand Up @@ -1097,7 +1106,7 @@ func timer() func() int64 {

// generateUserCert generates a keypair for the given Windows username,
// optionally querying LDAP for the user's Security Identifier.
func (s *WindowsService) generateUserCert(ctx context.Context, username string, ttl time.Duration, desktop types.WindowsDesktop) (certDER, keyDER []byte, err error) {
func (s *WindowsService) generateUserCert(ctx context.Context, username string, ttl time.Duration, desktop types.WindowsDesktop, createUsers bool, groups []string) (certDER, keyDER []byte, err error) {
var activeDirectorySID string
if !desktop.NonAD() {
// Find the user's SID
Expand Down Expand Up @@ -1130,15 +1139,15 @@ func (s *WindowsService) generateUserCert(ctx context.Context, username string,
}
s.cfg.Log.Debugf("Found objectSid %v for Windows username %v", activeDirectorySID, username)
}
return s.generateCredentials(ctx, username, desktop.GetDomain(), ttl, activeDirectorySID)
return s.generateCredentials(ctx, username, desktop.GetDomain(), ttl, activeDirectorySID, createUsers, groups)
}

// generateCredentials generates a private key / certificate pair for the given
// Windows username. The certificate has certain special fields different from
// the regular Teleport user certificate, to meet the requirements of Active
// Directory. See:
// https://docs.microsoft.com/en-us/windows/security/identity-protection/smart-cards/smart-card-certificate-requirements-and-enumeration
func (s *WindowsService) generateCredentials(ctx context.Context, username, domain string, ttl time.Duration, activeDirectorySID string) (certDER, keyDER []byte, err error) {
func (s *WindowsService) generateCredentials(ctx context.Context, username, domain string, ttl time.Duration, activeDirectorySID string, createUser bool, groups []string) (certDER, keyDER []byte, err error) {
return windows.GenerateWindowsDesktopCredentials(ctx, &windows.GenerateCredentialsRequest{
CAType: types.UserCA,
Username: username,
Expand All @@ -1148,6 +1157,8 @@ func (s *WindowsService) generateCredentials(ctx context.Context, username, doma
ActiveDirectorySID: activeDirectorySID,
LDAPConfig: s.cfg.LDAPConfig,
AuthClient: s.cfg.AuthClient,
CreateUser: createUser,
Groups: groups,
})
}

Expand Down
2 changes: 1 addition & 1 deletion lib/srv/desktop/windows_server_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ func TestGenerateCredentials(t *testing.T) {
activeDirectorySID: testSid,
},
} {
certb, keyb, err := w.generateCredentials(ctx, user, domain, windows.CertTTL, test.activeDirectorySID)
certb, keyb, err := w.generateCredentials(ctx, user, domain, windows.CertTTL, test.activeDirectorySID, false, nil)
require.NoError(t, err)
require.NotNil(t, certb)
require.NotNil(t, keyb)
Expand Down
3 changes: 3 additions & 0 deletions lib/tlsca/ca.go
Original file line number Diff line number Diff line change
Expand Up @@ -483,6 +483,9 @@ var (
// PinnedIPASN1ExtensionOID is an extension ID used when encoding/decoding
// the IP the certificate is pinned to.
PinnedIPASN1ExtensionOID = asn1.ObjectIdentifier{1, 3, 9999, 2, 15}

// CreateUserOID
probakowski marked this conversation as resolved.
Show resolved Hide resolved
CreateUserOID = asn1.ObjectIdentifier{1, 3, 9999, 2, 16}
)

// Device Trust OIDs.
Expand Down
1 change: 1 addition & 0 deletions lib/web/resources_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,7 @@ spec:
deny: {}
options:
cert_format: standard
create_desktop_user: false
create_host_user: false
desktop_clipboard: true
desktop_directory_sharing: true
Expand Down
4 changes: 3 additions & 1 deletion tool/tctl/sso/tester/format_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ allow:
deny: {}
options:
cert_format: ""
create_desktop_user: null
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add an omitempty so this doesn't happen? It's odd for a boolean to also be able to be null.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All our booleans are nullable so it follows the convention here

create_host_user: null
desktop_clipboard: null
desktop_directory_sharing: null
Expand Down Expand Up @@ -137,7 +138,8 @@ func Test_formatJSON(t *testing.T) {
"desktop_directory_sharing": null,
"create_host_user": null,
"pin_source_ip": false,
"ssh_file_copy": null
"ssh_file_copy": null,
"create_desktop_user": null
},
"allow": {
"logins": [
Expand Down