Skip to content

Windows user creation#24780

Merged
probakowski merged 27 commits intomasterfrom
probakowski/windows_user_creation
Apr 28, 2023
Merged

Windows user creation#24780
probakowski merged 27 commits intomasterfrom
probakowski/windows_user_creation

Conversation

@probakowski
Copy link
Copy Markdown
Contributor

@probakowski probakowski commented Apr 18, 2023

This change adds automatic user creation for non-AD Windows desktops.
It also bumps e reference

@probakowski probakowski requested a review from zmb3 April 20, 2023 19:08
Comment thread lib/auth/windows/windows.go
Comment thread lib/auth/windows/windows.go
Comment thread lib/services/role.go Outdated
Comment thread lib/srv/desktop/windows_server.go Outdated
Comment thread lib/srv/desktop/windows_server.go Outdated
Comment thread lib/tlsca/ca.go Outdated
@probakowski probakowski requested a review from zmb3 April 26, 2023 00:36
@probakowski
Copy link
Copy Markdown
Contributor Author

@zmb3 @timothyb89 @gabrielcorado friendly ping

Comment thread lib/services/role.go Outdated
Comment thread lib/services/access_checker.go Outdated
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

probakowski and others added 2 commits April 28, 2023 00:14
Co-authored-by: Isaiah Becker-Mayer <isaiah@goteleport.com>
Co-authored-by: Isaiah Becker-Mayer <isaiah@goteleport.com>
Comment thread lib/auth/windows/windows.go Outdated
deny: {}
options:
cert_format: ""
create_desktop_user: null
Copy link
Copy Markdown
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
Copy Markdown
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

@public-teleport-github-review-bot public-teleport-github-review-bot Bot removed the request for review from timothyb89 April 28, 2023 18:05
@probakowski probakowski enabled auto-merge April 28, 2023 18:10
@probakowski probakowski added this pull request to the merge queue Apr 28, 2023
Merged via the queue into master with commit f775f96 Apr 28, 2023
@probakowski probakowski deleted the probakowski/windows_user_creation branch April 28, 2023 18:27
@public-teleport-github-review-bot
Copy link
Copy Markdown

@probakowski See the table below for backport results.

Branch Result
branch/v12 Failed
branch/v13 Failed

probakowski added a commit that referenced this pull request Apr 28, 2023
* Windows auto user creation

* changes in role

* fix roles

* make grpc

* fix imports

* fix test

* fix test

* fix test

* fix test

* fix test

* windows labels

* rename OID, add json tags

* params to struct

* grpc

* Update lib/services/role.go

Co-authored-by: Isaiah Becker-Mayer <isaiah@goteleport.com>

* Update lib/services/access_checker.go

Co-authored-by: Isaiah Becker-Mayer <isaiah@goteleport.com>

* grpc

* bump e

* only add extension when we create user

---------

Co-authored-by: Isaiah Becker-Mayer <isaiah@goteleport.com>
probakowski added a commit that referenced this pull request Apr 28, 2023
* Windows auto user creation

* changes in role

* fix roles

* make grpc

* fix imports

* fix test

* fix test

* fix test

* fix test

* fix test

* windows labels

* rename OID, add json tags

* params to struct

* grpc

* Update lib/services/role.go

Co-authored-by: Isaiah Becker-Mayer <isaiah@goteleport.com>

* Update lib/services/access_checker.go

Co-authored-by: Isaiah Becker-Mayer <isaiah@goteleport.com>

* grpc

* bump e

* only add extension when we create user

---------

Co-authored-by: Isaiah Becker-Mayer <isaiah@goteleport.com>
probakowski added a commit that referenced this pull request Apr 28, 2023
* Windows user creation (#24780)

* Windows auto user creation

* changes in role

* fix roles

* make grpc

* fix imports

* fix test

* fix test

* fix test

* fix test

* fix test

* windows labels

* rename OID, add json tags

* params to struct

* grpc

* Update lib/services/role.go

Co-authored-by: Isaiah Becker-Mayer <isaiah@goteleport.com>

* Update lib/services/access_checker.go

Co-authored-by: Isaiah Becker-Mayer <isaiah@goteleport.com>

* grpc

* bump e

* only add extension when we create user

---------

Co-authored-by: Isaiah Becker-Mayer <isaiah@goteleport.com>

* grpc

---------

Co-authored-by: Isaiah Becker-Mayer <isaiah@goteleport.com>
probakowski added a commit that referenced this pull request Apr 28, 2023
* Windows auto user creation

* changes in role

* fix roles

* make grpc

* fix imports

* fix test

* fix test

* fix test

* fix test

* fix test

* windows labels

* rename OID, add json tags

* params to struct

* grpc

* Update lib/services/role.go



* Update lib/services/access_checker.go



* grpc

* bump e

* only add extension when we create user

---------

Co-authored-by: Isaiah Becker-Mayer <isaiah@goteleport.com>
@r0mant r0mant mentioned this pull request Jul 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants