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

SQL: improve URL parsing error messages for JDBC connection #56474

Closed
astefan opened this issue May 8, 2020 · 8 comments · Fixed by #58650
Closed

SQL: improve URL parsing error messages for JDBC connection #56474

astefan opened this issue May 8, 2020 · 8 comments · Fixed by #58650
Assignees
Labels
:Analytics/SQL SQL querying >enhancement Team:QL (Deprecated) Meta label for query languages team

Comments

@astefan
Copy link
Contributor

astefan commented May 8, 2020

If the url the jdbc driver is using to connect to Elasticsearch is failing to parse, an error message of the form Invalid URL [jdbc:es://localhost:9200/?user=xyz&password=abc], format should be [jdbc:es://[http|https]?[host[:port]]*/[prefix]*[?[option=value]&]*] is generated. The sensitive data needs to be obfuscated.

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-ql (:Query Languages/SQL)

@elasticmachine elasticmachine added the Team:QL (Deprecated) Meta label for query languages team label May 8, 2020
@nikoncode
Copy link
Contributor

I can handle this, please assign it to me to avoid double work.

@bpintea
Copy link
Contributor

bpintea commented May 26, 2020

Redacting the passwords out of the exception messages can be tricky. The above provided example is raised in two cases:

  • the scheme isn't an HTTP one;
  • the URI can't be parsed (by java.net.URI).

In the former case - and other few cases, like invalid property name or value - we could safely redact out the credentials.

But in the latter - and in general, as there are 3-4 hot spots where we dump the URI - this can't be done reliably since we can't know what's broken, w/o reimplementing a best-effort URI parser.

In easiest case we can redact the string between first (unescaped) @ and first (unescaped) / before it (in user info URI part) and after first password= (in query part), but depending on how malformed is is, even that could lead to altering the original input in a way that's confusing for the user and in some cases it might still leak the credentials (ex.: missing or misplaced @, pasword=foo etc.).

The safest alternative I see would be to never log the original URI, but just the URISyntaxException stripped hints.

And a compromise mix might be using a different schema to get the desired behavior: using jdbc:esv keeps the currently implemented behavior (verbose) and jdbc:es would just hint.

@bpintea
Copy link
Contributor

bpintea commented May 26, 2020

I can handle this, please assign it to me to avoid double work.

@nikoncode, many thanks for the offer. I will take over this issue.

@astefan
Copy link
Contributor Author

astefan commented May 29, 2020

What's a valid scenario for pasword=foo? This one should be caught by Unknown parameter "password"; did you mean password. I can see, though, a situation where validate.properties = false and that URL would pass... and later fail to be parsed by the URI class.

For debugging purposes, I still want the URL to be logged somehow and not hidden in its entirety. To be able to control the obfuscation, maybe we should consider creating our own URL parser. This would:

@costin thoughts?

@bpintea
Copy link
Contributor

bpintea commented May 29, 2020

What's a valid scenario for pasword=foo?

I meant it as a typo, a misconfiguration.

This one should be caught by Unknown parameter "password"; did you mean password. I can see, though, a situation where validate.properties = false and that URL would pass... and later fail to be parsed by the URI class.

Correct. Or something "easy" like user=foopassword=bar would log a Invalid parameter [user=foopassword=bar], format needs to be key=value.

maybe we should consider creating our own URL parser.

We could even use it as a fallback or as a front-end for java.net.URI and engage it only if that one fails.

@costin
Copy link
Member

costin commented May 29, 2020

We can take baby steps in improving the improving the existing situation. That is have a bit more understanding about the error code and use that inside the message.

  1. If the connection has failed all together, return only the URL and prefix without parameters (potentially)
  2. If the auth has failed (and we have a way to determine that) or something is wrong, try and look for the password and hide it (in the error message). If there's a typo or something along those lines, let the URL as is.

At the end of the day, this is about improving the user experience and cleaning security a bit.

@bpintea
Copy link
Contributor

bpintea commented May 30, 2020

return only the URL and prefix without parameters

The URL can still contain the credentials, since jdbc:es://http(s)://user:pass@host:port is another way to provide them.

If the auth has failed (and we have a way to determine that) or something is wrong, try and look for the password and hide it (in the error message)

The logging of parts or the entire URL only happens on URL parsing failure. In case the parsing succeeds we only log if authentication failed. Which simplifies a bit the situation, since we only need to consider parsing failure.

Thanks for the ideas, though, I'll try to come up with a simple extension to at least hint where the parsing jams and take it from there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/SQL SQL querying >enhancement Team:QL (Deprecated) Meta label for query languages team
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants