Skip to content

feature: extract coder_workspace_tags from template, detect dangerous data source usage #15087

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.

Sign up for GitHub

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

Jump to bottom
Closed
Tracked by #15428
johnstcn opened this issue Oct 15, 2024 · 11 comments
Closed
Tracked by #15428

feature: extract coder_workspace_tags from template, detect dangerous data source usage #15087

johnstcn opened this issue Oct 15, 2024 · 11 comments
Assignees
@johnstcn
Labels
must-do Issues that must be completed by the end of the Sprint. Or else. Only humans may set this. s2 Broken use cases or features (with a workaround). Only humans may set this.

Comments

@johnstcn
Copy link
Member

johnstcn commented Oct 15, 2024 •
edited
Loading

Relates to #15047

Motivation

Provisioner daemons can expose a set of tags [1] that allow Coder administrators to control the context in which a workspace or template job are evaluated. When creating a template version import job or workspace build job, users can manually specify a set of tags to be applied to the job as part of the request.

These tags are normally evaluated in coderd/wsbuilder.

The coder_workspace_tags [2] data source allows users to specify provisioner tags for workspaces built from a given template.

Example:

data "coder_workspace_tags" "custom_workspace_tags" {
  tags = {
    "zone"        = "developers"
    "runtime"     = data.coder_parameter.runtime_selector.value
    "project_id"  = "PROJECT_${data.coder_parameter.project_name.value}"
    "cache"       = data.coder_parameter.feature_cache_enabled.value == "true" ? "with-cache" : "no-cache"
  }
}

While investigating #15047 we noticed an issue whereby a template import job would appear to 'hang'. This turned out to be due to the template specifying a set of tags in the HCL but not having any suitably tagged provisioners available. There is an inherent chicken and egg problem here, as Coder would need to run the job to determine the tag values, but needs the tag values to schedule the job.

Additionally, we also noted an issue where users could specify potentially mutable / variable data sources as a value source for coder_workspace_tags.
Example:

data "coder_workspace_tags" "custom_workspace_tags" {
  tags = {
    "runtime"     = data.env_var.runtime.value # using some provider that reads environment variables
  }
}

The value of that environment variable is not guaranteed to resolve to the same value, depending on what provisioner is running the job.

Proposed Solution

  1. Add a pre-flight check when importing a template. This will involve parsing the Terraform files provided, checking for any declarations of coder_workspace_tags, and validating that the values for each key resolve to either:
  • A static variable ("developers")
  • A template-level variable (var.az)
  • A coder parameter (data.coder_parameter.foo.value)

Any other data sources for workspace tags should cause a large warning on the potential ramifications of this block the template import job as the required data sources will not be available. We could also block a template version import but that runs the risk of blocking a critical path for a hypothetical problem.

As a side-effect, the pre-flight check can also automatically set the provisioner tags it discovers on the provisioner job.

[1] https://coder.com/docs/admin/templates/extending-templates/workspace-tags
[2] https://registry.terraform.io/providers/coder/coder/latest/docs/data-sources/workspace_tags
@coder-labeler coder-labeler bot added needs-rfc Issues that needs an RFC due to an expansive scope and unclear implementation path. s3 Bugs that confuse, annoy, or are purely cosmetic labels Oct 15, 2024
@johnstcn johnstcn removed the needs-rfc Issues that needs an RFC due to an expansive scope and unclear implementation path. label Oct 15, 2024
@johnstcn johnstcn self-assigned this Oct 15, 2024
@ammario
Copy link
Member

ammario commented Oct 15, 2024

Is it really important to support dynamic tags in the template? Does it really make sense for the tags to specified in the template when the evaluation of such tags depends on the provisioner? There may be some way we can jank it by first routing the request to the last known provisioner and then re-routing it if the tags suggest a different provisioner. That would be pretty hard to reason about and has surprising operations / security ramifications.

At first glance it makes more sense to put tags in coderd_template.

@johnstcn
Copy link
Member Author

Is it really important to support dynamic tags in the template?

@ammario from my read of the background motivation for #5979 it appears it was a customer-requested feature.

@bpmct
Copy link
Member

bpmct commented Oct 16, 2024

Yes, dynamic tags in the template is important

@ammario
Copy link
Member

ammario commented Oct 16, 2024

I see a single user requested for provisioning local workspaces. If it's just that it's pretty niche and potentially solvable outside dynamic tags.

What are the other supporting requests or use cases? #5979 is detailed on implementation but not user value.

Maybe @kylecarbs could shed some light here.

@spikecurtis
Copy link
Contributor

@ammario it's to allow a single template to target multiple clusters, where the provisionerd is specific to the cluster. Making provisionerds specific to a cluster is important to security isolation, so each can have credentials on just one cluster. Often you want to run the provisionerd within the cluster itself to take advantage of cluster-provided IAM (AWS role, Kubernetes service account, etc).

Without this, you'd need to have a different template per cluster, or have a single provisionerd have credentials on multiple clusters.

@spikecurtis
Copy link
Contributor

Any other data sources for workspace tags should cause a large warning on the potential ramifications of this. We could also block a template version import but that runs the risk of blocking a critical path for a hypothetical problem.

This needs to be a hard block, not a warning. Other data sources are not available at the time we evaluate the tags (before the provisionerd is even chosen), so using them will be broken, plain and simple.

@mtojek
Copy link
Member

mtojek commented Oct 16, 2024

Can we mention/document use case when the proposed solution may not work and could be exploited?

@johnstcn
Copy link
Member Author

Can we mention/document use case when the proposed solution may not work and could be exploited?

Yes, documentation will be an important component of this too. (Context: the workspace_tags data source could be defined outside of the scope of the template, such as a Terraform module.)

@johnstcn
Copy link
Member Author

Implementation note: there is existing logic in provisioner/terraform that should probably be re-used here for consistency.

This was referenced Oct 22, 2024
Closed
Merged
Merged
johnstcn added a commit that referenced this issue Oct 25, 2024
@johnstcn @DanielleMaywood
chore(provisioner/terraform): extract terraform parsing logic to pack…
5ad4747 
…age tfparse (#15230)

Related to #15087

Extracts the logic for extracting variables and workspace tags
to a separate package `tfparse`.

---------

Co-authored-by: Danielle Maywood <danielle@themaywoods.com>
johnstcn added a commit that referenced this issue Oct 25, 2024
@johnstcn
chore(coderd): extract fileszip to package archive for reuse (#15229)
df34858 
Related to #15087
As part of sniffing the workspace tags from an uploaded file, we need to
be able to handle both zip and tar files. Extracting the functions to
a separate `archive` package will be helpful here.
@johnstcn johnstcn mentioned this issue Oct 25, 2024
Merged
@johnstcn johnstcn mentioned this issue Nov 5, 2024
Closed
@johnstcn johnstcn added must-do Issues that must be completed by the end of the Sprint. Or else. Only humans may set this. s2 Broken use cases or features (with a workaround). Only humans may set this. and removed s3 Bugs that confuse, annoy, or are purely cosmetic labels Nov 7, 2024
This was referenced Nov 7, 2024
Open
Closed
johnstcn added a commit that referenced this issue Nov 14, 2024
@johnstcn
feat(provisioner/terraform/tfparse): implement WorkspaceTagDefaultsFr…
bebc38e 
…omFile (#15236)

Relates to #15087 and
#15427

Adds functionality to `provisioner/terraform/tfparse` to extract the
default values for a `coder_workspace_tags` data source from a given
file.
@johnstcn johnstcn mentioned this issue Nov 14, 2024
Merged
johnstcn added a commit that referenced this issue Nov 15, 2024
@johnstcn
feat(provisionersdk): allow variadic tags in provisionersdk.MutateTags (
aca5be5 
#15518)

Relates to #15087 and
#15427

Allows specifying multiple sets of provisioner tags into `MutateTags`.
These tags get additively merged.

This will simplify handling tags from multiple sources when sniffing
tags from the template.
@bpmct
Copy link
Member

bpmct commented Nov 18, 2024

@johnstcn can you be sure to work with @EdwardAngert to get this in our documentation for the release?

@johnstcn johnstcn mentioned this issue Nov 18, 2024
Merged
5 tasks
@johnstcn johnstcn mentioned this issue Nov 21, 2024
Merged
johnstcn added a commit that referenced this issue Nov 25, 2024
@johnstcn @mafredri
feat!: extract provisioner tags from coder_workspace_tags data source (…
1cdc3e8 
…#15578)

Relates to #15087 and
#15427

- Extracts provisioner job tags from `coder_workspace_tags` on template
version creation using `provisioner/terraform/tfparse` added in
#15236
- Drops a WARN log in coderd if no matching provisioners found.
- Also drops a warning message in the CLI if no provisioners are found.
- To support both CLI and UI warnings, added a
`codersdk.MatchedProvisioners` struct to the `TemplateVersion` response
containing details of how many provisioners were around at the time of
the insert.

Co-authored-by: Mathias Fredriksson <mafredri@gmail.com>
johnstcn added a commit that referenced this issue Nov 25, 2024
@johnstcn
chore(docs): update documentation for coder_workspace_tags (#15620)
e550365 
Adds documentation relating to
#15087 and
#15427

See also: #15578
@johnstcn
Copy link
Member Author

#15578 adds logic to extract and validate tags from uploaded templates.
Docs are updated in #15620.

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

@johnstcn johnstcn

Labels
must-do Issues that must be completed by the end of the Sprint. Or else. Only humans may set this. s2 Broken use cases or features (with a workaround). Only humans may set this.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@johnstcn @spikecurtis @ammario @mtojek @bpmct