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

Refuse to store dotted keys to prevent cyclic reference in our configuration. #6077

Merged
merged 1 commit into from
Jan 16, 2018

Conversation

ph
Copy link
Contributor

@ph ph commented Jan 15, 2018

The following reference can make the configuration parse go into an
infinite loop and panic.

filebeat_name: ${filebeat_name}

@ph ph added the review label Jan 15, 2018
@ph
Copy link
Contributor Author

ph commented Jan 15, 2018

@tsg this PR block usage of "dot" in keys for the keystore until cyclic reference from go-ucfg is correctly supported, the changes are bigger to do in the library than I have originally though.


func (k *FileKeystore) validateKey(key string) error {
if strings.IndexAny(key, ".") != -1 {
return fmt.Errorf("invalid key format, key should not have '.', key: %s", key)
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should change this message to something like is not supported yet. I'm ok with not supporting it in the first iteration but I think moving forward to definitively have to (if possible) as it's kind of awkward having the dot nation in all our config files but not allow it for the key store. I think it's one of the first issues people will run into.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am ok with the "is not supported yet"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ruflin its possible just it require a bit more work on ucfg :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ruflin Maybe "Nested key is not supported yet, key: .."

Copy link
Contributor

Choose a reason for hiding this comment

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

TBH I think nested will not make it very clear on what it's about. Already now I often have confusion around nested fields and nested documents etc in ES so I would stear away from that word.

+		return fmt.Errorf("invalid key format. '.' in keys are not supported yet. key: %s", key)

Copy link
Contributor

@tsg tsg left a comment

Choose a reason for hiding this comment

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

LGTM.

@ph ph closed this Jan 16, 2018
@ph ph reopened this Jan 16, 2018
configuration.

The following reference can make the configuration parse go into an
infinite loop and panic.
```
filebeat_name: ${filebeat_name}
```
@ph ph force-pushed the fix/refuse-dotted-key-for-keystore branch from 596b449 to f7a4bc1 Compare January 16, 2018 17:45
@ph
Copy link
Contributor Author

ph commented Jan 16, 2018

@tsg travis-ci was looking stuck on something and was not running any jobs, I've rebased the PR and repush the changes. Hopefully travis will pick it up.

@ruflin ruflin added the libbeat label Jan 16, 2018
@ruflin ruflin merged commit 0cdcf4f into elastic:master Jan 16, 2018
@ruflin
Copy link
Contributor

ruflin commented Jan 16, 2018

Merged as travis and appveyor are heavily lagging behind. Jenkins was green.

ph added a commit to ph/beats that referenced this pull request Jan 23, 2018
urso pushed a commit that referenced this pull request Jan 23, 2018
…reference test case (#6098)

* Revert "Refuse to store dotted keys to prevent cyclic reference in our configuration. (#6077)"

This reverts commit 0cdcf4f.

* Keystore adding a system env test

Adding a specific system env test to target a cyclic reference in the
configuration.

* Update go-ucfg

This update allow us to support top level key reference when the top
level doesn't already exist in the YAML document and it allow us to use
cyclic reference in a yaml document if the key exist in other resolvers.

Ref: elastic/go-ucfg#97

* Update changelog
ph added a commit to ph/beats that referenced this pull request Jan 23, 2018
…reference test case (elastic#6098)

* Revert "Refuse to store dotted keys to prevent cyclic reference in our configuration. (elastic#6077)"

This reverts commit 0cdcf4f.

* Keystore adding a system env test

Adding a specific system env test to target a cyclic reference in the
configuration.

* Update go-ucfg

This update allow us to support top level key reference when the top
level doesn't already exist in the YAML document and it allow us to use
cyclic reference in a yaml document if the key exist in other resolvers.

Ref: elastic/go-ucfg#97

* Update changelog

(cherry picked from commit 34b6a46)
tsg pushed a commit that referenced this pull request Jan 24, 2018
…reference test case (#6098) (#6154)

* Revert "Refuse to store dotted keys to prevent cyclic reference in our configuration. (#6077)"

This reverts commit 0cdcf4f.

* Keystore adding a system env test

Adding a specific system env test to target a cyclic reference in the
configuration.

* Update go-ucfg

This update allow us to support top level key reference when the top
level doesn't already exist in the YAML document and it allow us to use
cyclic reference in a yaml document if the key exist in other resolvers.

Ref: elastic/go-ucfg#97

* Update changelog

(cherry picked from commit 34b6a46)
leweafan pushed a commit to leweafan/beats that referenced this pull request Apr 28, 2023
…reference test case (elastic#6098) (elastic#6154)

* Revert "Refuse to store dotted keys to prevent cyclic reference in our configuration. (elastic#6077)"

This reverts commit f531ac0.

* Keystore adding a system env test

Adding a specific system env test to target a cyclic reference in the
configuration.

* Update go-ucfg

This update allow us to support top level key reference when the top
level doesn't already exist in the YAML document and it allow us to use
cyclic reference in a yaml document if the key exist in other resolvers.

Ref: elastic/go-ucfg#97

* Update changelog

(cherry picked from commit 375191e)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants