Improve label format enforcement #4102
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fspmarshall
requested review from
awly,
klizhentas,
russjones and
webvictim
as code owners
|
retest this please |
webvictim
reviewed
Jul 23, 2020
awly
reviewed
Jul 23, 2020
russjones
approved these changes
Jul 23, 2020
fspmarshall
force-pushed
the
fspmarshall/label-validation
branch
2 times, most recently
from
0babc2a to
ca29d6a
Compare
awly
approved these changes
Jul 23, 2020
lib/srv/regular/sshserver.go
Outdated
Comment on lines
347
to
354
| for key := range cmdLabels { | ||
| if !services.IsValidLabelKey(key) { | ||
| return trace.BadParameter("invalid label key: %q", key) | ||
| } | ||
| } | ||
| // make sure to clone labels to avoid | ||
| // concurrent writes to the map during reloads | ||
| cmdLabels = cmdLabels.Clone() |
There was a problem hiding this comment.
Combine this with the loop below.
Also, should labels be cloned to prevent concurrent writes?
There was a problem hiding this comment.
As far as I can tell, only cmdLabels is mutated. The labels value should be calculated once at program start based on the config file and/or command-line inputs.
edit: Decided to clone it anyway. I choose to live in fear.
fspmarshall
force-pushed
the
fspmarshall/label-validation
branch
from
ca29d6a to
a8af8c1
Compare
fspmarshall
force-pushed
the
fspmarshall/label-validation
branch
from
a8af8c1 to
5df6ab0
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Label key format requirements were only being enforced by json schema, which is only run on a subset of deserializing operations, and (unsurprisingly) only when the the format of origin is json. As a result, invalid labels were not detected when passing through the GRPC API, being inserted into the backend, or when propagating through the event system. This resulted in teleport working mostly correctly up until the point where it is restarted, at which point initialization of the auth server cache would fail, causing startup to fail.
This PR addresses the specific issue of labels, but I'm working on drafting an issue about the broader problem of inconsistent validation. I'm generally of the opinion that json schema validation in teleport does more harm than good, and I think problems like this are a prime example of why validation logic should not be tied to a specific serialization format.
Partially addresses #4034