Skip to content

Tighten discovery service permissions#29717

Merged
greedy52 merged 3 commits intomasterfrom
STeve/p471_limit_discovery_service_permission
Aug 3, 2023
Merged

Tighten discovery service permissions#29717
greedy52 merged 3 commits intomasterfrom
STeve/p471_limit_discovery_service_permission

Conversation

@greedy52
Copy link
Copy Markdown
Contributor

@greedy52 greedy52 commented Jul 27, 2023

Related issue https://github.com/gravitational/teleport-private/issues/471

  • Permissions for Discovery Service allowed KubernetesLabels and DatabaseLabels are changed from wildcard to only cloud origin.
  • In addition, disallow Discovery Service to create databases/kubes with dynamic labels.

Will do changes on DB agent side (e.g. URL validation) in a separate change.

@greedy52 greedy52 self-assigned this Jul 27, 2023
@greedy52 greedy52 force-pushed the STeve/p471_limit_discovery_service_permission branch from 171d798 to 26255b6 Compare July 27, 2023 21:04
@greedy52 greedy52 force-pushed the STeve/p471_limit_discovery_service_permission branch from 26255b6 to 512a838 Compare July 27, 2023 21:17
@greedy52 greedy52 changed the title Limit Discovery Service permission by cloud origin label Tighten discovery service permissions Jul 28, 2023
@greedy52 greedy52 added security Security Issues changelog kubernetes-access database-access Database access related issues and PRs labels Jul 28, 2023
@greedy52 greedy52 marked this pull request as ready for review July 28, 2023 20:33
Copy link
Copy Markdown
Contributor

@GavinFrazar GavinFrazar left a comment

Choose a reason for hiding this comment

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

nice!

return trace.Wrap(err)
}
// Don't allow discovery service to create databases with dynamic labels.
if a.hasBuiltinRole(types.RoleDiscovery) && len(database.GetDynamicLabels()) > 0 {
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.

Can you please include the same check into kube endpoints?

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.

What is identity was generated for DBDiscovery and DBDatabse so the a.hasBuiltinRole(types.RoleDiscovery) && a.hasBuiltinRole(types.RoleDatabase) == true and CreateDatabase method was called from db agent process ?

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.

What is identity was generated for DBDiscovery and DBDatabse so the a.hasBuiltinRole(types.RoleDiscovery) && a.hasBuiltinRole(types.RoleDatabase) == true and CreateDatabase method was called from db agent process ?

DB service doesn't call CreateDatabase right? Worth testing this scenario but I don't think it should be a problem

Copy link
Copy Markdown
Contributor Author

@greedy52 greedy52 Jul 31, 2023

Choose a reason for hiding this comment

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

Can you please include the same check into kube endpoints?

Thanks! I missed as somehow i thought kube cluster doesn't have dynamic labels. Added now in 98d53a7

What is identity was generated for DBDiscovery and DBDatabse so the a.hasBuiltinRole(types.RoleDiscovery) && a.hasBuiltinRole(types.RoleDatabase) == true and CreateDatabase method was called from db agent process ?

DB service doesn't call CreateDatabase right? Worth testing this scenario but I don't think it should be a problem

Yes, database service doesn't call CreateDatabase. Also, even if it does it wouldn't be a problem:

func (process *TeleportProcess) initDatabases() {
process.RegisterWithAuthServer(types.RoleDatabase, DatabasesIdentityEvent)
process.RegisterCriticalFunc("db.init", process.initDatabaseService)
}

Discovery vs database services in the same process use its own auth connector with a single system role.

@greedy52 greedy52 force-pushed the STeve/p471_limit_discovery_service_permission branch from a43638d to 98d53a7 Compare July 31, 2023 15:47
@codingllama codingllama removed their request for review August 1, 2023 14:04
@public-teleport-github-review-bot public-teleport-github-review-bot Bot removed the request for review from flyinghermit August 3, 2023 07:34
@greedy52 greedy52 added this pull request to the merge queue Aug 3, 2023
Merged via the queue into master with commit f780fc7 Aug 3, 2023
@greedy52 greedy52 deleted the STeve/p471_limit_discovery_service_permission branch August 3, 2023 14:37
@public-teleport-github-review-bot
Copy link
Copy Markdown

@greedy52 See the table below for backport results.

Branch Result
branch/v11 Failed
branch/v12 Failed
branch/v13 Create PR

greedy52 added a commit that referenced this pull request Aug 3, 2023
* Limit Discovery Service permission by cloud origin label

* fix ut

* disallow dynamic labels for kube cluster
greedy52 added a commit that referenced this pull request Aug 3, 2023
* Limit Discovery Service permission by cloud origin label

* fix ut

* disallow dynamic labels for kube cluster
github-merge-queue Bot pushed a commit that referenced this pull request Aug 4, 2023
* Limit Discovery Service permission by cloud origin label

* fix ut

* disallow dynamic labels for kube cluster
github-merge-queue Bot pushed a commit that referenced this pull request Aug 5, 2023
* Limit Discovery Service permission by cloud origin label

* fix ut

* disallow dynamic labels for kube cluster
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

database-access Database access related issues and PRs kubernetes-access security Security Issues size/sm

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants