Skip to content
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.

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

Tighten teleport.yaml validation #5056

Closed
awly opened this issue Dec 4, 2020 · 2 comments · Fixed by #6520
Closed

Tighten teleport.yaml validation #5056

awly opened this issue Dec 4, 2020 · 2 comments · Fixed by #6520
Assignees
Labels
Milestone

Comments

@awly
Copy link
Contributor

awly commented Dec 4, 2020

Feature Request

Use strict YAML validation when reading teleport.yaml.
Any fields that don't map to a config value should be treated as errors.

To prevent breaking customers on upgrade, we should spread this over 2 releases:

  • 6.0: run strict validation in warning more and fall back to old validation on failure; the warning should be very visible at startup, perhaps with a short sleep to prompt users to look at the logs
  • 7.0: always run strict validation

Motivation

The current validation of teleport.yaml checks that keys are one of a hardcoded valid set:

// now check for unknown (misspelled) config keys:
var validateKeys func(m YAMLMap) error
validateKeys = func(m YAMLMap) error {
var recursive, ok bool
var key string
for k, v := range m {
if key, ok = k.(string); ok {
if recursive, ok = validKeys[key]; !ok {
return trace.BadParameter("unrecognized configuration key: '%v'", key)
}
if recursive {
if m2, ok := v.(YAMLMap); ok {
if err := validateKeys(m2); err != nil {
return err
}
}
}
}
}
return nil
}
// validate configuration keys:
var tmp YAMLMap
if err = yaml.Unmarshal(bytes, &tmp); err != nil {
return nil, trace.BadParameter("error parsing YAML config")
}
if err = validateKeys(tmp); err != nil {
return nil, trace.Wrap(err)
}

But it doesn't validate that it's in the right place in the config.

Some examples of invalid configs that pass validation:

auth_service:
  data_dir: /custom/path # this must be under "teleport:", not "auth_service:"
proxy_service:
  kubernetes_service: # this must be "kubernetes"
    enabled: yes
    listen_addr: 0.0.0.0:3026

kubernetes: # this must be "kubernetes_service"
  enabled: yes
  listen_addr: 0.0.0.0:3027

Who's it for?

OSS User, Pro, Enterprise

@awly awly added the ux label Dec 4, 2020
@awly awly added this to the 5.1 "San Diego" milestone Dec 4, 2020
@awly awly self-assigned this Dec 4, 2020
@awly
Copy link
Contributor Author

awly commented Dec 18, 2020

6.0 will start warning about malformed configs.
7.0 should start enforcing it, leaving this bug open to track that.

@webvictim
Copy link
Contributor

Should be done at the same time as #3248

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 a pull request may close this issue.

2 participants