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

[QA] UI maps credentials to a malformed URL, when protocol is specified. #258

Closed
jnweiger opened this issue Feb 24, 2022 · 12 comments
Closed
Assignees
Labels
Priority:p2-high Escalation, on top of current planning, release blocker QA:blocker Issues with this label block the release, issue found by QA

Comments

@jnweiger
Copy link
Contributor

jnweiger commented Feb 24, 2022

New scope: HTTPS support is needed for Elastic.

Seen in search_elastic 2.1.0 RC1

  • admin enables app search_elastic
  • admin goes to the 'search' settings and enters host and port.
  • occ config:app:get search_elastic servers
http://elastic:[email protected]:9200
  • Switch the Authentiction dropdown from 'Select Authentication' to 'User and password'

  • enter correct username and password. Click save settings, the indicator remains red and the message says cannot connect.
    image

  • The server URL is now malformed: occ config:app:get search_elastic servers

elastic:ua9116rq7e@http://172.17.0.3:9200

Expected behaviour

  • Credentials follow after ://

Attempted Workaround

  • embed the credentials in the URL and do not select 'Username and Password'

image

This stores the URL correctly in the database (http://elastic:[email protected]:9200), but after a refresh the web page displays garbled fields:

image

Still an index cannot be created:
image

Actual workaround

  • must not specify a protocol
    • (we are not prepared to handle HTTPS then?)

image

This was referenced Feb 24, 2022
@hodyroff
Copy link
Contributor

not a regression, needs checking later

@hodyroff
Copy link
Contributor

Seems there is no workaround and this is mandatory for the authentication scheme. Blocker.

@hodyroff hodyroff added Priority:p2-high Escalation, on top of current planning, release blocker QA:blocker Issues with this label block the release, issue found by QA labels Mar 21, 2022
@xoxys
Copy link
Contributor

xoxys commented Mar 21, 2022

@C0rby had some objections as well... IIRC reading back the password to the UI (even if it is hidden) isn't recommended https://github.com/owncloud/search_elastic/blob/master/js/settings/admin.js#L12-L19

So please coordinate with @C0rby if required...

@xoxys
Copy link
Contributor

xoxys commented Mar 21, 2022

Summary: The URL parameter needs a proper handling for URL schemas (https/http/none)

@C0rby
Copy link

C0rby commented Mar 21, 2022

@C0rby had some objections as well... IIRC reading back the password to the UI (even if it is hidden) isn't recommended https://github.com/owncloud/search_elastic/blob/master/js/settings/admin.js#L12-L19

So please coordinate with @C0rby if required...

Yes, the password shouldn't be displayed in the UI nor should it be sent back as an API response.
Also the handling of the URL and credentials need to be improved. I.e. the given URL needs to be parsed before appending the path and the credentials shouldn't be added to the URL by concatenating strings.

@ahherrera
Copy link
Contributor

@jnweiger I have been working on this ticket and would like to know if the protocol could be a select with two options (HTTP, HTTPS)?
@C0rby I don't understand the behavior you want to have with the password field. Could you explain me a little more in detail how the password field should behave?
Regards!!

@xoxys
Copy link
Contributor

xoxys commented Mar 22, 2022

The protocol should IMO be part of the url and not a separate field. Php should provide a function to properly parse/split and build urls instead of the manual split and concatenation.

@C0rby
Copy link

C0rby commented Mar 22, 2022

@jnweiger I have been working on this ticket and would like to know if the protocol could be a select with two options (HTTP, HTTPS)? @C0rby I don't understand the behavior you want to have with the password field. Could you explain me a little more in detail how the password field should behave? Regards!!

Well the least that shoud be done is that the password field is URL encoded before sending the request.
Imagine if someone has '#' in their password, this would completely break the url when we send the credentials in the URL. Not so much a problem when sending the credentials properly via headers.

But generally the URL handling needs to be made more resilient.
In a normal case the admin sets for example the url: 10.10.10.161:9200
The request will be sent to user:[email protected]:9200/oc-randomchars
But if I now instead enter the url: 10.10.10.161:9200/somepath
The request will be sent to user:[email protected]:9200/somepath/oc-randomchars
Everything good so far...
But now I enter the url: 10.10.10.161:9200/somepath#
The request will be sent to user:[email protected]:9200/somepath (without our appended path /oc-randomchars)

That is why I say that we need to parse the URL and then correctly append the path.

@ahherrera
Copy link
Contributor

@jnweiger I have been working on this ticket and would like to know if the protocol could be a select with two options (HTTP, HTTPS)? @C0rby I don't understand the behavior you want to have with the password field. Could you explain me a little more in detail how the password field should behave? Regards!!

Well the least that shoud be done is that the password field is URL encoded before sending the request. Imagine if someone has '#' in their password, this would completely break the url when we send the credentials in the URL. Not so much a problem when sending the credentials properly via headers.

But generally the URL handling needs to be made more resilient. In a normal case the admin sets for example the url: 10.10.10.161:9200 The request will be sent to user:[email protected]:9200/oc-randomchars But if I now instead enter the url: 10.10.10.161:9200/somepath The request will be sent to user:[email protected]:9200/somepath/oc-randomchars Everything good so far... But now I enter the url: 10.10.10.161:9200/somepath# The request will be sent to user:[email protected]:9200/somepath (without our appended path /oc-randomchars)

That is why I say that we need to parse the URL and then correctly append the path.

@C0rby I think that after the connection port should not go anything else because in that field we are configuring the connection to the server, not requests to the elastic. Do you think there is any scenario where you have parts of the url after the port?

@C0rby
Copy link

C0rby commented Mar 22, 2022

It's not about what you intend a user to put there but what a (malicious) user COULD put there. This could be abused to scan the internal network for hosts/applications. Even if it would take a while with that method.

@C0rby
Copy link

C0rby commented Mar 22, 2022

And also yes, I think there could be setups where the elastic server is reachable at a subpath like 10.10.10.161/elastic.

@jnweiger
Copy link
Contributor Author

Confirmed fixed in 2.2.0-rc1

@jnweiger jnweiger mentioned this issue Jul 4, 2022
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority:p2-high Escalation, on top of current planning, release blocker QA:blocker Issues with this label block the release, issue found by QA
Projects
None yet
Development

No branches or pull requests

5 participants