Skip to content

Role configuration for default host user shell#46539

Merged
eriktate merged 1 commit intomasterfrom
eriktate/14252/host-user-shell-config
Sep 13, 2024
Merged

Role configuration for default host user shell#46539
eriktate merged 1 commit intomasterfrom
eriktate/14252/host-user-shell-config

Conversation

@eriktate
Copy link
Copy Markdown
Contributor

@eriktate eriktate commented Sep 12, 2024

Relates to #14252

This PR adds the host_user_shell configuration to the role spec. Once defined, any newly created host users should have their default shell assigned to reflect the value in host_user_shell. If a path cannot be resolved, then we log a warning reflecting that and fallback to the default behavior of the host. One thing I'd like some feedback on is how best to handle multiple roles defining host_user_shell. Right now this PR does the simplest thing and returns the first value for host_user_shell encountered. I'm not entirely sure how often this will happen in real world scenarios, so I'm not sure if we need to figure out something more robust or if this is good enough for now.

changelog: Added a new create_host_user_default_shell configuration under role options that changes the default shell of auto provisioned host users. create_host_user_default_shell can be set to a shell's absolute path or the name of a shell reachable through the system PATH. This only applies to users created after create_host_user_default_shell has been assigned. An example can be found in the roles reference: https://goteleport.com/docs/reference/access-controls/roles/#example-role-specification

@github-actions
Copy link
Copy Markdown
Contributor

The PR changelog entry failed validation: The changelog entry must not contain a Markdown link or image.

@github-actions
Copy link
Copy Markdown
Contributor

The PR changelog entry failed validation: The changelog entry must not contain a Markdown link or image.

@eriktate eriktate force-pushed the eriktate/14252/host-user-shell-config branch from e14dcc3 to ff3f41a Compare September 12, 2024 14:30
@github-actions
Copy link
Copy Markdown
Contributor

The PR changelog entry failed validation: The changelog entry must not contain a Markdown link or image.

Comment thread lib/services/access_checker.go Outdated
@eriktate eriktate force-pushed the eriktate/14252/host-user-shell-config branch from ff3f41a to 5c73f4c Compare September 12, 2024 14:34
@github-actions
Copy link
Copy Markdown
Contributor

The PR changelog entry failed validation: The changelog entry must not contain a Markdown link or image.

// Verify users have the correct shell assigned
userShells, err := getUserShells("/etc/passwd")
require.NoError(t, err)
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.

Alternatively, we could run echo $SHELL as each user and validate the output matches our expectations.

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.

Yeah I considered this too 🤔 Would there be any benefits to doing one versus the other? Parsing the file seemed simpler to me than spinning up commands, but I'd be happy to swap for this if it's a more robust test

Copy link
Copy Markdown
Contributor

@rosstimothy rosstimothy Sep 12, 2024

Choose a reason for hiding this comment

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

I suppose it would also validate that a user with a custom shell provisioned is able to access the target host. I don't think there are any other tests that both set a custom shell and start an SSH session.

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.

I don't think there are any other tests that both set a custom shell and start an SSH session.

There aren't, but I can take a look at adding one if you think we need the extra coverage. Although that seems like testing the behavior of the host moreso than the behavior of Teleport since how things are handled after updating /etc/passwd is up to the host

Comment thread lib/utils/host/hostusers.go Outdated
Comment thread lib/utils/host/hostusers.go Outdated
@eriktate eriktate force-pushed the eriktate/14252/host-user-shell-config branch from e328e5a to 9f73f6b Compare September 12, 2024 17:46
Comment thread integration/hostuser_test.go Outdated
Comment thread integration/hostuser_test.go
@eriktate eriktate force-pushed the eriktate/14252/host-user-shell-config branch 3 times, most recently from 30d614d to 4433501 Compare September 12, 2024 20:19
@github-actions
Copy link
Copy Markdown
Contributor

🤖 Vercel preview here: https://docs-35694zgq7-goteleport.vercel.app/docs/ver/preview

Comment thread integration/hostuser_test.go Outdated
@eriktate eriktate force-pushed the eriktate/14252/host-user-shell-config branch 2 times, most recently from d5d7db5 to c46466c Compare September 13, 2024 14:31
Comment thread api/proto/teleport/legacy/types/types.proto Outdated
@github-actions
Copy link
Copy Markdown
Contributor

🤖 Vercel preview here: https://docs-1gvcwjcy9-goteleport.vercel.app/docs/ver/preview

@eriktate eriktate force-pushed the eriktate/14252/host-user-shell-config branch from c46466c to 78a2290 Compare September 13, 2024 14:50
@github-actions
Copy link
Copy Markdown
Contributor

🤖 Vercel preview here: https://docs-9ndiys2qg-goteleport.vercel.app/docs/ver/preview

@eriktate eriktate force-pushed the eriktate/14252/host-user-shell-config branch from 78a2290 to 1f0ba8a Compare September 13, 2024 15:23
@github-actions
Copy link
Copy Markdown
Contributor

🤖 Vercel preview here: https://docs-p7f795keu-goteleport.vercel.app/docs/ver/preview

Comment thread lib/services/access_checker.go
@eriktate eriktate force-pushed the eriktate/14252/host-user-shell-config branch from 1f0ba8a to 6d3a54d Compare September 13, 2024 17:34
@github-actions
Copy link
Copy Markdown
Contributor

🤖 Vercel preview here: https://docs-65nzbwuxz-goteleport.vercel.app/docs/ver/preview

@eriktate eriktate added this pull request to the merge queue Sep 13, 2024
Merged via the queue into master with commit c592dbc Sep 13, 2024
@eriktate eriktate deleted the eriktate/14252/host-user-shell-config branch September 13, 2024 20:24
@public-teleport-github-review-bot
Copy link
Copy Markdown

@eriktate See the table below for backport results.

Branch Result
branch/v14 Failed
branch/v15 Failed
branch/v16 Failed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants