Sanitize password from couchbase metric #1680 #3033
Conversation
|
I looked at the example output on https://developer.couchbase.com/documentation/server/3.x/admin/REST/rest-node-get-info.html, and it looks like |
|
You are right, I was more about to replace |
|
What about rebuilding the cluster url from the servers option list without the userinfo section? |
danielnelson
changed the title
|
Good idea. Just trim/cut string to remove those sensitive data. I'll work on it. Thanks! |
Add simple sanitization method to cut from `servers` configuration variable information about username and password from URI. This variable later is used as cluster tag.
plugins/inputs/couchbase/README.md
Outdated
| @@ -22,7 +22,7 @@ | |||
| ### couchbase_node | |||
|
|
|||
| Tags: | |||
| - cluster: whatever you called it in `servers` in the configuration, e.g.: `http://couchbase-0.example.com/` | |||
| - cluster: sanitized and scheme less string from `servers` configuration field e.g.: `http://user:password@couchbase-0.example.com:8091/endpoint` -> `couchbase-0.example.com:8091/endpoint` | |||
There was a problem hiding this comment.
Why remove the scheme?
There was a problem hiding this comment.
I think it is better to use scheme less name for displaying and creating dashboard. In other hand, it may cause problems in case when there will be 2+ elements in servers array that will be differ from each other only by used protocol (scheme). If you wish I can also add scheme into this string, just let me know.
Thanks!
There was a problem hiding this comment.
I agree about the concern on multiple servers differing by scheme only, but sympathize with respect to your style concerns. That said, we should keep the scheme if only to preserve backwards compatibility.
| // it also removes schema name from URI | ||
| func sanitizeURI(uri string) (result string, err error) { | ||
|
|
||
| re, err := regexp.Compile("(\\S+:\\/\\/)?(\\S+\\:\\S+@)") |
There was a problem hiding this comment.
Move to package scope and use MustCompile, this way it only will be compiled once. Personally I would use backtick strings so you don't need to double escape, also you don't need to escape / or :. This would leave you with
`(\S+://)?(\S+\:\S+@)`
This is a nitpick, but it would be nicer if there was less vertical whitespace in this function.
There was a problem hiding this comment.
Good point. In this case I think I can simplify those whole thing by removing function (MustCompile is gonna panic anyway).
| } | ||
|
|
||
| if len(sanitizedAddress) <= 1 { | ||
| log.Printf("I! WARNING: Cluster address tag \"'%s'\" is too short.", sanitizedAddress) |
There was a problem hiding this comment.
I'm not sure it's possible to hit this line, if it is can you add a testcase?
You can use "W!" to prefix the log message to indicate a warning.
There was a problem hiding this comment.
This line should be hit when server variable/record is empty, but you are right, it should be filtered by configuration parser.
Based on advise from @danielnelson.
Fix issue #1680
clustertag, use information provided by CouchDB API instead of getting it from telegraf server configuration where user is able to provide credentials.Required for all PRs: