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

Add config option to disable HTTP printable char path check #4442

Merged
merged 1 commit into from
Jul 26, 2018

Conversation

banks
Copy link
Member

@banks banks commented Jul 25, 2018

The documentation change pretty much sums up this PR:

image

Consul 1.0.3 fixed a potential security vulnerability where
malicious users could craft KV keys with unprintable chars that would confuse
operators using the CLI or UI into taking wrong actions. Users who had data
written in older versions of Consul that did not have this restriction will be
unable to delete those values by default in 1.0.3 or later. This setting
enables those users to temporarily disable the filter such that delete
operations can work on those keys again to get back to a healthy state. It is
strongly recommended that this filter is not disabled permanently as it
exposes the original security vulnerability.

Copy link
Contributor

@pearkes pearkes left a comment

Choose a reason for hiding this comment

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

Nice, both the language in the docs and implementation look good to me.

I was going to suggest maybe logging at [WARN] on startup about the security risk but the configuration key is so clear that I think it is good as-is.

@banks banks merged commit 8dd50d5 into master Jul 26, 2018
@banks banks deleted the disable-printable branch July 26, 2018 12:53
@slackpad
Copy link
Contributor

An alternative way to remove bad keys like this is https://www.consul.io/api/txn.html, which doesn't carry the key name in the URL (it's in the JSON payload).

@banks
Copy link
Member Author

banks commented Aug 13, 2018 via email

@pearkes
Copy link
Contributor

pearkes commented Aug 13, 2018

Yeah that is a great point. In the scenario we've seen it was a time sensitive issue, so the idea that you could change the config and re-run some consul kv command to get out of a downtime scenario was helpful. My only concern with the transaction API is you'd have to educate someone about that live and it may require a bit more finagling...guess we could deprecate this option in 1.3 though and write a quick guide.

@slackpad
Copy link
Contributor

Yeah the txn API would require a bit of JSON to use. You could change the consul kv delete CLI to always use the kv.Txn() API under the hood (it's fine even if it's just deleting a single key) so it always uses the form that doesn't depend on any URL weirdness. This isn't a huge deal in any case :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants