Skip to content

NodeJoin script: fix when no labels are provided#15709

Merged
marcoandredinis merged 1 commit into
masterfrom
marco/fix_node_script_labels
Aug 23, 2022
Merged

NodeJoin script: fix when no labels are provided#15709
marcoandredinis merged 1 commit into
masterfrom
marco/fix_node_script_labels

Conversation

@marcoandredinis
Copy link
Copy Markdown
Contributor

@marcoandredinis marcoandredinis commented Aug 22, 2022

Recently we added a way to add labels on newly added nodes based on the
token.
#15114
Each token now has a list of SuggestedLabels, which are used to feed
that list.

However, if that list is empty, the generated script would trigger the
following error when running the teleport node configure ... command:
teleport: error: unexpected

This happens because the command is generating an empty argument ""
when running the teleport node configure ... command.
So it looks like this:

${TELEPORT_BINARY_DIR}/teleport node configure \
      --token token \
      joinmethod \
      --ca-pin pin \
      --auth-server host:port \
      "" \
      --output someport

That empty argument breaks things.

So, in order to fix it, we are going to change the default value when no
labels are provided.
Instead of an empty string, we'll use an empty array.

Demo (teleport node configure message removed for brev
No label

$ LABELS_FLAG=(); f=$(mktemp -d)/node.yaml; teleport node configure --auth-server w:1 "${LABELS_FLAG[@]}" --output $f && yq .s
sh_service.labels $f

enabled: "yes"
commands:
  - name: hostname
    command: [hostname]
    period: 1m0s

Single label

$ LABELS_FLAG=(--labels x=y); f=$(mktemp -d)/node.yaml; teleport node configure --auth-server w:1 "${LABELS_FLAG[@]}" --output $f && yq .ssh_service $f

enabled: "yes"
labels:
  x: "y"
commands:
  - name: hostname
    command: [hostname]
    period: 1m0s

Multiple labels

$ LABELS_FLAG=(--labels x=y,dev=prod); f=$(mktemp -d)/node.yaml; teleport node configure --auth-server w:1 "${LABELS_FLAG[@]}" --output $f && yq .ssh_service $f

enabled: "yes"
labels:
  dev: prod
  x: "y"
commands:
  - name: hostname
    command: [hostname]
    period: 1m0s

@zmb3
Copy link
Copy Markdown
Collaborator

zmb3 commented Aug 22, 2022

Are these labels user-controlled? If so, we need to protect against code injection.

What happens if someone specifies labels of x=y && rm -rf /?

Seems it might be safer for us to generate a file with the desired labels, and update teleport node configure to point it at a file rather than put user-controlled input in the script itself.

@marcoandredinis
Copy link
Copy Markdown
Contributor Author

SuggestedLabels is a new field and not exposed anywhere client side.
The only place that writes to it is here

// If using the automatic method to add a Node, the `install.sh` will add the token's suggested labels
// as part of the initial Labels configuration for that Node
// Script install-node.sh:
// ...
// $ teleport configure ... --labels <suggested_label=value>,<suggested_label=value> ...
// ...
//
// We create an ID and return it as part of the Token, so the UI can use this ID to query the Node that joined using this token
// WebUI can then query the resources by this id and answer the question:
// - Which Node joined the cluster from this token Y?
req.SuggestedLabels = types.Labels{
types.InternalResourceIDLabel: apiutils.Strings{uuid.NewString()},
}

Should not be an issue:

  • we don't receive those from the (web) client
  • we don't set them based on some other input
  • the only injected label is a static key and a random uuid value

@github-actions github-actions Bot removed the request for review from nklaassen August 23, 2022 00:28
Recently we added a way to add labels on newly added nodes based on the
token.
Each token now has a list of SuggestedLabels, which are used to feed
that list.

However, if that list is empty, the generated script would trigger the
following error:
`teleport: error: unexpected`

This happens when running the `teleport node configure ...` command.

This happens because the command is generating an empty argument `""`
when running the `teleport node configure ...` command.
So it looks like this:
```bash
${TELEPORT_BINARY_DIR}/teleport node configure \
      --token token \
      joinmethod \
      --ca-pin pin \
      --auth-server host:port \
      "" \
      --output someport
```
That empty argument breaks things.

So, in order to fix it, we are going to change the default value when no
labels are provided.
Instead of an empty string, we'll use an empty array.

Demo (teleport node configure message removed for brev
No label
```bash
$ LABELS_FLAG=(); f=$(mktemp -d)/node.yaml; teleport node configure --auth-server w:1 "${LABELS_FLAG[@]}" --output $f && yq .s
sh_service.labels $f

enabled: "yes"
commands:
  - name: hostname
    command: [hostname]
    period: 1m0s

```

Single label
```bash
$ LABELS_FLAG=(--labels x=y); f=$(mktemp -d)/node.yaml; teleport node configure --auth-server w:1 "${LABELS_FLAG[@]}" --output $f && yq .ssh_service $f

enabled: "yes"
labels:
  x: "y"
commands:
  - name: hostname
    command: [hostname]
    period: 1m0s

```

Multiple labels
```bash
$ LABELS_FLAG=(--labels x=y,dev=prod); f=$(mktemp -d)/node.yaml; teleport node configure --auth-server w:1 "${LABELS_FLAG[@]}" --output $f && yq .ssh_service $f

enabled: "yes"
labels:
  dev: prod
  x: "y"
commands:
  - name: hostname
    command: [hostname]
    period: 1m0s
```
@marcoandredinis marcoandredinis force-pushed the marco/fix_node_script_labels branch from 8ae8845 to 8705d1b Compare August 23, 2022 06:43
@marcoandredinis marcoandredinis merged commit 3c74cee into master Aug 23, 2022
@github-actions
Copy link
Copy Markdown
Contributor

@marcoandredinis See the table below for backport results.

Branch Result
branch/v10 Create PR

@marcoandredinis marcoandredinis deleted the marco/fix_node_script_labels branch August 23, 2022 07:10
@marcoandredinis marcoandredinis added the discover Issues related to Teleport Discover label Aug 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

discover Issues related to Teleport Discover

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants