Fix #382: Extract headers from IPFS API requests and apply them to hijacked ones. #623

hsanjuan · 2018-12-18T14:56:13Z

This commit makes the proxy extract useful fixed headers (like CORS) from
the IPFS daemon API responses and then apply them to the responses
from hijacked endpoints like /add or /repo/stat.

It does this by caching a list of headers from the first IPFS API
response which has them. If we have not performed any proxied request or
managed to obtain the headers we're interested in, this will try triggering a
request to "/api/v0/version" to obtain them first.

This should fix the issues with using Cluster proxy with IPFS Companion and
Chrome.

License: MIT
Signed-off-by: Hector Sanjuan code@hector.link

…cked ones. This commit makes the proxy extract useful fixed headers (like CORS) from the IPFS daemon API responses and then apply them to the responses from hijacked endpoints like /add or /repo/stat. It does this by caching a list of headers from the first IPFS API response which has them. If we have not performed any proxied request or managed to obtain the headers we're interested in, this will try triggering a request to "/api/v0/version" to obtain them first. This should fix the issues with using Cluster proxy with IPFS Companion and Chrome. License: MIT Signed-off-by: Hector Sanjuan <code@hector.link>

coveralls · 2018-12-18T15:13:49Z

Coverage increased (+0.09%) to 65.181% when pulling 159243d on fix/proxy-headers into 9f6a173 on master.

kishansagathiya

The PR looks good except a minor concerns about header checking

kishansagathiya · 2018-12-19T04:08:15Z

version/version.go

@@ -1,4 +1,4 @@
-package ipfscluster
+package version


It would have been nice to have a separate commit for creating the version package. But let it be as it is, it would be overkill to go back and spit the commit.

kishansagathiya · 2018-12-19T04:51:29Z

api/ipfsproxy/ipfsproxy.go

+func (proxy *Server) isIPFSHeadersKnown() bool {
+	_, ok := proxy.ipfsHeaders.Load(ipfsHeaderList[0])
+	return ok
+}


(From looking at this and references of isIPFSHeadersKnown) It assumes that if Server header is present, remaining headers are present as well. Shouldn't we check other headers? Depending we need to check for other headers or not the name can be changed to areIPFSHeadersKnown or isServerHeaderPresent.

Suggested change

}

func (proxy *Server) areIPFSHeadersKnown() bool {

for _, header := range ipfsHeaderList {

if _, ok := proxy.ipfsHeaders.Load(header); !ok {

return !ok

}

}

return true

}

is there a Go unwritten rule saying that boolean functions start with is? @lanzafame ?

Shouldn't we check other headers?

We can assume that if we learned about Server (or any one of them) we have learned about the rest. But maybe worth checking the whole list.

is there a Go unwritten rule saying that boolean functions start with is?

There is this:

"The Go Programming Language" by Donovan and Kernighan recommends in Chapter 6 that

Functions that merely access or modify internal values of a type..are called getters and setters. > However, when naming a getter method we usually omit the Get prefix. This Preference for brevity extends to all methods not just field accessors, and to other reductant prefixes as well, such as Fetch, Find and Lookup

But I wouldn't apply that to is. Though in this particular case, Known is enough of an indicator that this is a boolean predicate.

kishansagathiya

On trying it out, I don't see response headers with proxy endpoints, however I do see expected response headers( as in ipfsHeaderList) with ipfs endpoints. Can you check and confirm if you see the same thing?

kishan@kishansagathiya:~/$ curl -X GET http://localhost:9096/api/v0/pin/ls -v
Note: Unnecessary use of -X or --request, GET is already inferred.
*   Trying 127.0.0.1...
* TCP_NODELAY set
* Connected to localhost (127.0.0.1) port 9096 (#0)
> GET /api/v0/pin/ls HTTP/1.1
> Host: localhost:9096
> User-Agent: curl/7.58.0
> Accept: */*
> 
* Connection #0 to host localhost left intact
��f#[L��{m-�P�p9��Ѫ��#3���B�%���)��S"�~��kishan@kishansagathiya:~/$

lanzafame · 2018-12-19T06:37:59Z

@kishansagathiya that is the wrong port number. The proxy api is on 9095. 9096 is the libp2p swarm/cluster-api port.

lanzafame · 2018-12-19T06:39:28Z

$ curl http://localhost:9095/api/v0/pin/ls -v                          16:38
*   Trying ::1...
* TCP_NODELAY set
* connect to ::1 port 9095 failed: Connection refused
*   Trying 127.0.0.1...
* TCP_NODELAY set
* Connected to localhost (127.0.0.1) port 9095 (#0)
> GET /api/v0/pin/ls HTTP/1.1
> Host: localhost:9095
> User-Agent: curl/7.63.0
> Accept: */*
> 
< HTTP/1.1 200 OK
< Access-Control-Allow-Headers: X-Stream-Output, X-Chunked-Output, X-Content-Length
< Access-Control-Expose-Headers: X-Stream-Output, X-Chunked-Output, X-Content-Length
< Content-Type: application/json
< Server: ipfs-cluster/ipfsproxy/0.7.0+git862c1eb3eaaa3b6103e65b4804df0faf56b07882
< Trailer: X-Stream-Error
< Vary: Origin
< Date: Wed, 19 Dec 2018 06:38:11 GMT
< Transfer-Encoding: chunked
< 
* Connection #0 to host localhost left intact
{"Keys":{}}

kishansagathiya · 2018-12-19T07:14:30Z

@lanzafame

that is the wrong port number. The proxy api is on 9095. 9096 is the libp2p swarm/cluster-api port.

My bad, this is what confused me. Will have to modify that docker-compose file 😆

He asked me to.

License: MIT Signed-off-by: Hector Sanjuan <code@hector.link>

And do it by only loading them from the sync.Map once. License: MIT Signed-off-by: Hector Sanjuan <code@hector.link>

hsanjuan · 2018-12-20T13:47:33Z

Ok thanks for the feedback. I made it in a way that we only load the headers once from the sync.Map for every request. And if we don't have all the headers we want we will try to call /version to get them. Ready on my side.

kishansagathiya

LGTM. Trick used in last commit to get headers from sync.Map.

I will leave merging for you

hsanjuan self-assigned this Dec 18, 2018

hsanjuan requested review from lanzafame and kishansagathiya December 18, 2018 14:56

ghost added the status/in-progress In progress label Dec 18, 2018

hsanjuan force-pushed the fix/proxy-headers branch from 889c1f8 to df98ffe Compare December 18, 2018 14:57

ipfs-cluster deleted a comment from GitCop Dec 18, 2018

hsanjuan force-pushed the fix/proxy-headers branch from df98ffe to 862c1eb Compare December 18, 2018 15:05

kishansagathiya reviewed Dec 19, 2018

View reviewed changes

kishansagathiya suggested changes Dec 19, 2018

View reviewed changes

kishansagathiya previously approved these changes Dec 19, 2018

View reviewed changes

hsanjuan added 2 commits December 20, 2018 14:24

proxy: Rename helper to ipfsHeadersKnown()

3896858

License: MIT Signed-off-by: Hector Sanjuan <code@hector.link>

proxy: Check all headers to decide if we need to request

159243d

And do it by only loading them from the sync.Map once. License: MIT Signed-off-by: Hector Sanjuan <code@hector.link>

kishansagathiya approved these changes Dec 20, 2018

View reviewed changes

hsanjuan mentioned this pull request Dec 20, 2018

Release 0.8.0 #620

Closed

hsanjuan merged commit 17599da into master Dec 20, 2018

hsanjuan deleted the fix/proxy-headers branch December 20, 2018 14:36

ghost removed the status/in-progress In progress label Dec 20, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #382: Extract headers from IPFS API requests and apply them to hijacked ones. #623

Fix #382: Extract headers from IPFS API requests and apply them to hijacked ones. #623

hsanjuan commented Dec 18, 2018

coveralls commented Dec 18, 2018 •

edited

kishansagathiya left a comment •

edited

kishansagathiya Dec 19, 2018

kishansagathiya Dec 19, 2018

hsanjuan Dec 19, 2018

lanzafame Dec 20, 2018

kishansagathiya left a comment •

edited

lanzafame commented Dec 19, 2018

lanzafame commented Dec 19, 2018

kishansagathiya commented Dec 19, 2018 •

edited

hsanjuan commented Dec 20, 2018

kishansagathiya left a comment

-}
+func (proxy *Server) areIPFSHeadersKnown() bool {
+	for _, header := range ipfsHeaderList {
+		if _, ok := proxy.ipfsHeaders.Load(header); !ok {
+			return !ok
+		}
+	}
+	return true
+}

Fix #382: Extract headers from IPFS API requests and apply them to hijacked ones. #623

Fix #382: Extract headers from IPFS API requests and apply them to hijacked ones. #623

Conversation

hsanjuan commented Dec 18, 2018

coveralls commented Dec 18, 2018 • edited

kishansagathiya left a comment • edited

Choose a reason for hiding this comment

kishansagathiya Dec 19, 2018

Choose a reason for hiding this comment

kishansagathiya Dec 19, 2018

Choose a reason for hiding this comment

hsanjuan Dec 19, 2018

Choose a reason for hiding this comment

lanzafame Dec 20, 2018

Choose a reason for hiding this comment

kishansagathiya left a comment • edited

Choose a reason for hiding this comment

lanzafame commented Dec 19, 2018

lanzafame commented Dec 19, 2018

kishansagathiya commented Dec 19, 2018 • edited

hsanjuan commented Dec 20, 2018

kishansagathiya left a comment

Choose a reason for hiding this comment

coveralls commented Dec 18, 2018 •

edited

kishansagathiya left a comment •

edited

kishansagathiya left a comment •

edited

kishansagathiya commented Dec 19, 2018 •

edited