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

Trailing slash in host causes issues #1777

Open
BackEndTea opened this issue May 14, 2020 · 16 comments
Open

Trailing slash in host causes issues #1777

BackEndTea opened this issue May 14, 2020 · 16 comments

Comments

@BackEndTea
Copy link

When i use elastica, and configure my host to be elastic.dev everything is fine, but if i use elastic.dev/ i'm unable to connect.

Maybe this normal for elastic, but it wasn't something i expected. My best guess would be that an rtrim on the host could fix this.

@ruflin
Copy link
Owner

ruflin commented May 19, 2020

I remember some historical discussion around and there was a reason we did not trim. Unfortunately couldn't find it.

In general I'm in favor of no magic code but good errors. So I rather show an error the first time the config is not correct instead of fixing a potentially invalid config which might brake at a later stage.

@fabianaromagnoli
Copy link

I experienced the opposite issue: when host is configured with trailing slash, everything works fine, while if the trailing slash is missing, I get a "Malformed URL" exception.

@ruflin What about trimmig and re-adding a slash, in order to be compliant with both host with and without trailing slash? Something like $baseUri = rtrim($baseUri, '/') . '/'; in \Elastica\Transport\Http::exec

Sometimes you cannot simply change the host in your configuration, for instance when you use symfony local webserver and you have a docker integration:

When using the Symfony binary with php bin/console (symfony console ...), the binary will always use environment variables detected via Docker and will ignore local environment variables.[...]

In this case you have no power to change the value of the env variable.

@ruflin
Copy link
Owner

ruflin commented Nov 30, 2020

@fabianaromagnoli Possible, but sounds like even more magic :-)

In the Docker case, who sets these environment variables? Who can change it?

@jhuebner79
Copy link
Contributor

Trailing dots in hostnames, very well a valid choice, also cause 403 from the remote end, when using signed requests towards AWS ElasticSearch domains.

Why is it a valid choice? To reduce DNS load in busy clusters as described here: https://medium.com/cloutive/production-ready-eks-coredns-configuration-6fea830606f8 - Solution 3

Should I create a separate issue? @ruflin

@ruflin
Copy link
Owner

ruflin commented Jun 15, 2022

@jhuebner79 What is your preferred solution here?

@jhuebner79
Copy link
Contributor

I'd be the happiest if Elastica stripped the trailing dot during building the signed request, hoping it'll fix the issue. We strongly suspect this to be the reason for the 403s we see when using the trailing dot in the hostname given to Elastica.

@jhuebner79
Copy link
Contributor

It would also be good to just have this confirmed. When the only thing we change in our setup is the trailing dot in the hostname, we receive "The request signature we calculated does not match the signature you provided. Check your AWS Secret Access Key and signing method. Consult the service documentation for details.".

@ruflin
Copy link
Owner

ruflin commented Jun 20, 2022

@jhuebner79 Trying to get my head around all this. Initially above we talk about the / but you talk about trailing dot. Is this the same issue or only the potential solution to remove it magically would be the same?

@jhuebner79 Is this error specific to AWS? Where in the Elastica code does this happen? I wonder if the magic could only happen around the signing and not on the top level?

@jhuebner79
Copy link
Contributor

jhuebner79 commented Jun 20, 2022

@ruflin I think these are two separate issues with a similar approach for a solution, yes. That's why I asked if I maybe better open another issue for what I see in the work environment I'm in. Both cases (slash and dot) would be solved if Elastica at some point would strip (maybe we call it sanitize instead of magic? :)) a trailing character in a string (uri, hostname).

@ruflin I think the error I face at work is specific to AWS. And a bit tricky as well. Because we want Elastica to use a hostname with a trailing dot in it to find out where to connect to (during DNS resolution of the hostname), to have less lookups (the PHP application is running in a container on EKS). But we do most probably not want to have Elastica use the trailing dot in the hostname during calculating the signature it uses for making signed requests towards an AWS ES domain.

So in my case it would be enough to sanitize the string around the signing. As we receive a negative response from the domain the connection is still working despite the dot, but the signing is not.

Edit: I hope this somehow makes sense, otherwise I can try to explain in German in a PM.

@ruflin
Copy link
Owner

ruflin commented Jun 22, 2022

As you described, different problems potentially similar solutions. You brought up sanitise vs magic. What if it would be a config flag that could be set (one for each case)? sanitise might be false by default but can be enabled. Like this, it would not be magic anymore. @jhuebner79 Would that work for your case?

@jhuebner79
Copy link
Contributor

jhuebner79 commented Jun 23, 2022

I think so, @ruflin - sounds quite good. But: in the case of request signing it seems not sanitising is always wrong.

Remember, currently we have three cases we're talking about:

  • slash in URI (what @BackEndTea reported), probably good to have sanitising flag here, maybe he can tell us more
  • trailing dot in hostname during connecting to ElasticSearch domain (DNS lookup): never sanitise, can and always should keep the dot (reduce DNS load), flag makes no sense here imho, but if I was able to set it (I wouldn't, because false is what I want here), I'm fine with it.
  • trailing dot in hostname during request signing: should always be sanitised, makes no sense to not sanitise (otherwise signing fails/gives wrong result), but is okay if it's a flag; but false would be a wrong default here, imho.

@ruflin
Copy link
Owner

ruflin commented Jun 24, 2022

For the dots, any chance you could open a PR with the proposed change? I'm ok to skip the config in a first iteration, I agree we likely don't need it.

@BackEndTea Let me know if the config flag for your scenario would work? Maybe also a PR would help here for the setting?

@BackEndTea
Copy link
Author

Oof its been a while since i made this issue. Currently i'm not using Elastica (or Elasticsearch for that matter) anymore.

@jhuebner79
Copy link
Contributor

@ruflin I have no idea if I'm able to do that, would be my first look into the code altogether, and I'm more of a ops person. But I'll try, maybe with the help of a colleague.

@jhuebner79
Copy link
Contributor

@ruflin we prepared a pull request #2090 - maybe you can have a look at that.

While we were at it I noticed that FROM php:7.2-fpm-alpine in docker/php/Dockerfile prevents make docker-run-phpcs as described in CONRIBUTING.md from working. I tested with 7.4, phive did work afterwards. That's a new issue, I suppose?

Thanks for your time and attention! :)

@ruflin
Copy link
Owner

ruflin commented Jun 28, 2022

Thanks for taking a stab at it @jhuebner79 ! Left a comment / question there.

The Dockerfile seems indeed to be an other issue. Happy to discuss further in a separate issue or PR ;-)

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

No branches or pull requests

4 participants