gateway: clean up its surface, and remove BlockList

This patch is in preparation for the gateway's extraction.

It's interesting to trace technical debt back to its
origin, understanding the circumstances in which it
was introduced and built up, and then cutting it back
at exactly the right places.

- Clean up the gateway's surface
  The option builder GatewayOption() now takes only
  arguments which are relevant for HTTP handler muxing,
  i.e. the paths where the gateway should be mounted.
  All other configuration happens through the
  GatewayConfig object.

- Remove BlockList
  I know why this was introduced in the first place,
  but it never ended up fulfilling that purpose.
  Somehow it was only ever used by the API server,
  not the gateway, which really doesn't make sense.
  It was also never wired up with CLI nor fs-repo.
  Eventually @krl started punching holes into it
  to make the Web UI accessible.

- Remove --unrestricted-api
  This was holes being punched into BlockList too,
  for accessing /ipfs and /ipn on the API server.
  With BlockList removed and /ipfs and /ipns freely
  accessible, putting this option out of action
  is safe. With the next major release,
  the option can be removed for good.

License: MIT
Signed-off-by: Lars Gierth <[email protected]>

Author: Lars Gierth

cmd/ipfs/daemon.go

@@ -8,7 +8,6 @@ import (
 	_ "net/http/pprof"
 	"os"
 	"sort"
-	"strings"
 	"sync"
 
 	"gx/ipfs/QmPpRcbNUXauP3zWZ1NJMLWpe4QnmEHrd2ba2D3yqWznw7/go-multiaddr-net"
@@ -135,7 +134,7 @@ Headers.
 		cmds.BoolOption(writableKwd, "Enable writing objects (with POST, PUT and DELETE)").Default(false),
 		cmds.StringOption(ipfsMountKwd, "Path to the mountpoint for IPFS (if using --mount). Defaults to config setting."),
 		cmds.StringOption(ipnsMountKwd, "Path to the mountpoint for IPNS (if using --mount). Defaults to config setting."),
-		cmds.BoolOption(unrestrictedApiAccessKwd, "Allow API access to unlisted hashes").Default(false),
+		cmds.BoolOption(unrestrictedApiAccessKwd, "This option has no effect since v0.4.3").Default(false),
 		cmds.BoolOption(unencryptTransportKwd, "Disable transport encryption (for debugging protocols)").Default(false),
 		cmds.BoolOption(enableGCKwd, "Enable automatic periodic repo garbage collection").Default(false),
 		cmds.BoolOption(adjustFDLimitKwd, "Check and raise file descriptor limits if needed").Default(false),
@@ -364,33 +363,11 @@ func serveHTTPApi(req cmds.Request) (error, <-chan error) {
 	apiMaddr = apiLis.Multiaddr()
 	fmt.Printf("API server listening on %s\n", apiMaddr)
 
-	unrestricted, _, err := req.Option(unrestrictedApiAccessKwd).Bool()
-	if err != nil {
-		return fmt.Errorf("serveHTTPApi: Option(%s) failed: %s", unrestrictedApiAccessKwd, err), nil
-	}
-
-	apiGw := corehttp.NewGateway(corehttp.GatewayConfig{
-		Writable: true,
-		BlockList: &corehttp.BlockList{
-			Decider: func(s string) bool {
-				if unrestricted {
-					return true
-				}
-				// for now, only allow paths in the WebUI path
-				for _, webuipath := range corehttp.WebUIPaths {
-					if strings.HasPrefix(s, webuipath) {
-						return true
-					}
-				}
-				return false
-			},
-		},
-	})
 	var opts = []corehttp.ServeOption{
 		corehttp.MetricsCollectionOption("api"),
 		corehttp.CommandsOption(*req.InvocContext()),
 		corehttp.WebUIOption,
-		apiGw.ServeOption(),
+		corehttp.GatewayOption("/ipfs", "/ipns"),
 		corehttp.VersionOption(),
 		defaultMux("/debug/vars"),
 		defaultMux("/debug/pprof/"),
@@ -452,8 +429,8 @@ func serveHTTPGateway(req cmds.Request) (error, <-chan error) {
 	if err != nil {
 		return fmt.Errorf("serveHTTPGateway: req.Option(%s) failed: %s", writableKwd, err), nil
 	}
-	if !writableOptionFound {
-		writable = cfg.Gateway.Writable
+	if writableOptionFound {
+		cfg.Gateway.Writable = writable
 	}
 
 	gwLis, err := manet.Listen(gatewayMaddr)
@@ -474,7 +451,7 @@ func serveHTTPGateway(req cmds.Request) (error, <-chan error) {
 		corehttp.CommandsROOption(*req.InvocContext()),
 		corehttp.VersionOption(),
 		corehttp.IPNSHostnameOption(),
-		corehttp.GatewayOption(writable, cfg.Gateway.PathPrefixes),
+		corehttp.GatewayOption("/ipfs", "/ipns"),
 	}
 
 	if len(cfg.Gateway.RootRedirect) > 0 {

cmd/ipfswatch/main.go

@@ -81,10 +81,16 @@ func run(ipfsPath, watchPath string) error {
 	}
 	defer node.Close()
 
+	cfg, err := node.Repo.Config()
+	if err != nil {
+		return err
+	}
+	cfg.Gateway.Writable = true
+
 	if *http {
 		addr := "/ip4/127.0.0.1/tcp/5001"
 		var opts = []corehttp.ServeOption{
-			corehttp.GatewayOption(true, nil),
+			corehttp.GatewayOption("/ipfs", "/ipns"),
 			corehttp.WebUIOption,
 			corehttp.CommandsOption(cmdCtx(node, ipfsPath)),
 		}

core/corehttp/gateway.go

@@ -4,60 +4,38 @@ import (
 	"fmt"
 	"net"
 	"net/http"
-	"sync"
 
 	core "github.com/ipfs/go-ipfs/core"
 	config "github.com/ipfs/go-ipfs/repo/config"
 	id "gx/ipfs/QmdBpVuSYuTGDA8Kn66CbKvEThXqKUh2nTANZEhzSxqrmJ/go-libp2p/p2p/protocol/identify"
 )
 
-// Gateway should be instantiated using NewGateway
-type Gateway struct {
-	Config GatewayConfig
-}
-
 type GatewayConfig struct {
 	Headers      map[string][]string
-	BlockList    *BlockList
 	Writable     bool
 	PathPrefixes []string
 }
 
-func NewGateway(conf GatewayConfig) *Gateway {
-	return &Gateway{
-		Config: conf,
-	}
-}
-
-func (g *Gateway) ServeOption() ServeOption {
+func GatewayOption(paths ...string) ServeOption {
 	return func(n *core.IpfsNode, _ net.Listener, mux *http.ServeMux) (*http.ServeMux, error) {
-		// pass user's HTTP headers
 		cfg, err := n.Repo.Config()
 		if err != nil {
 			return nil, err
 		}
 
-		g.Config.Headers = cfg.Gateway.HTTPHeaders
+		gateway := newGatewayHandler(n, GatewayConfig{
+			Headers:      cfg.Gateway.HTTPHeaders,
+			Writable:     cfg.Gateway.Writable,
+			PathPrefixes: cfg.Gateway.PathPrefixes,
+		})
 
-		gateway, err := newGatewayHandler(n, g.Config)
-		if err != nil {
-			return nil, err
+		for _, p := range paths {
+			mux.Handle(p+"/", gateway)
 		}
-		mux.Handle("/ipfs/", gateway)
-		mux.Handle("/ipns/", gateway)
 		return mux, nil
 	}
 }
 
-func GatewayOption(writable bool, prefixes []string) ServeOption {
-	g := NewGateway(GatewayConfig{
-		Writable:     writable,
-		BlockList:    &BlockList{},
-		PathPrefixes: prefixes,
-	})
-	return g.ServeOption()
-}
-
 func VersionOption() ServeOption {
 	return func(n *core.IpfsNode, _ net.Listener, mux *http.ServeMux) (*http.ServeMux, error) {
 		mux.HandleFunc("/version", func(w http.ResponseWriter, r *http.Request) {
@@ -68,33 +46,3 @@ func VersionOption() ServeOption {
 		return mux, nil
 	}
 }
-
-// Decider decides whether to Allow string
-type Decider func(string) bool
-
-type BlockList struct {
-	mu      sync.RWMutex
-	Decider Decider
-}
-
-func (b *BlockList) ShouldAllow(s string) bool {
-	b.mu.RLock()
-	d := b.Decider
-	b.mu.RUnlock()
-	if d == nil {
-		return true
-	}
-	return d(s)
-}
-
-// SetDecider atomically swaps the blocklist's decider. This method is
-// thread-safe.
-func (b *BlockList) SetDecider(d Decider) {
-	b.mu.Lock()
-	b.Decider = d
-	b.mu.Unlock()
-}
-
-func (b *BlockList) ShouldBlock(s string) bool {
-	return !b.ShouldAllow(s)
-}

core/corehttp/gateway_handler.go

@@ -36,12 +36,12 @@ type gatewayHandler struct {
 	config GatewayConfig
 }
 
-func newGatewayHandler(node *core.IpfsNode, conf GatewayConfig) (*gatewayHandler, error) {
+func newGatewayHandler(node *core.IpfsNode, conf GatewayConfig) *gatewayHandler {
 	i := &gatewayHandler{
 		node:   node,
 		config: conf,
 	}
-	return i, nil
+	return i
 }
 
 // TODO(cryptix):  find these helpers somewhere else
@@ -152,12 +152,6 @@ func (i *gatewayHandler) getOrHeadHandler(w http.ResponseWriter, r *http.Request
 		ipnsHostname = true
 	}
 
-	if i.config.BlockList != nil && i.config.BlockList.ShouldBlock(urlPath) {
-		w.WriteHeader(http.StatusForbidden)
-		w.Write([]byte("403 - Forbidden"))
-		return
-	}
-
 	nd, err := core.Resolve(ctx, i.node, path.Path(urlPath))
 	// If node is in offline mode the error code and message should be different
 	if err == core.ErrNoNamesys && !i.node.OnlineMode() {

core/corehttp/gateway_test.go

@@ -89,6 +89,12 @@ func newTestServerAndNode(t *testing.T, ns mockNamesys) (*httptest.Server, *core
 		t.Fatal(err)
 	}
 
+	cfg, err := n.Repo.Config()
+	if err != nil {
+		t.Fatal(err)
+	}
+	cfg.Gateway.PathPrefixes = []string{"/good-prefix"}
+
 	// need this variable here since we need to construct handler with
 	// listener, and server with handler. yay cycles.
 	dh := &delegatedHandler{}
@@ -98,7 +104,7 @@ func newTestServerAndNode(t *testing.T, ns mockNamesys) (*httptest.Server, *core
 		ts.Listener,
 		VersionOption(),
 		IPNSHostnameOption(),
-		GatewayOption(false, []string{"/good-prefix"}),
+		GatewayOption("/ipfs", "/ipns"),
 	)
 	if err != nil {
 		t.Fatal(err)

misc/completion/ipfs-completion.bash

@@ -104,7 +104,7 @@ _ipfs_config_show()
 _ipfs_daemon()
 {
     _ipfs_comp "--init --routing= --mount --writable --mount-ipfs= \
-        --mount-ipns= --unrestricted-api --disable-transport-encryption \
+        --mount-ipns= --disable-transport-encryption \
         --help"
 }
 
@@ -314,7 +314,7 @@ _ipfs_resolve()
 
 _ipfs_stats()
 {
-    _ipfs_comp "bw --help" 
+    _ipfs_comp "bw --help"
 }
 
 _ipfs_stats_bw()
@@ -401,17 +401,17 @@ _ipfs()
 {
     COMPREPLY=()
     local word="${COMP_WORDS[COMP_CWORD]}"
-    
+
     case "${COMP_CWORD}" in
-        1)  
+        1)
             local opts="add bitswap block bootstrap cat commands config daemon dht \
                         diag dns file get id init log ls mount name object pin ping \
                         refs repo stats swarm tour update version"
             COMPREPLY=( $(compgen -W "${opts}" -- ${word}) );;
-        2)  
+        2)
             local command="${COMP_WORDS[1]}"
             eval "_ipfs_$command" 2> /dev/null ;;
-        *)  
+        *)
             local command="${COMP_WORDS[1]}"
             local subcommand="${COMP_WORDS[2]}"
             eval "_ipfs_${command}_${subcommand}" 2> /dev/null && return

test/sharness/t0061-daemon-opts.sh

@@ -11,20 +11,11 @@ test_description="Test daemon command"
 
 test_init_ipfs
 
-test_launch_ipfs_daemon --unrestricted-api --disable-transport-encryption
+test_launch_ipfs_daemon --disable-transport-encryption
 
 gwyaddr=$GWAY_ADDR
 apiaddr=$API_ADDR
 
-test_expect_success 'api gateway should be unrestricted' '
-  echo "hello mars :$gwyaddr :$apiaddr" >expected &&
-  HASH=$(ipfs add -q expected) &&
-  curl -sfo actual1 "http://$gwyaddr/ipfs/$HASH" &&
-  curl -sfo actual2 "http://$apiaddr/ipfs/$HASH" &&
-  test_cmp expected actual1 &&
-  test_cmp expected actual2
-'
-
 # Odd. this fails here, but the inverse works on t0060-daemon.
 test_expect_success 'transport should be unencrypted' '
   nc -w 1 localhost $SWARM_PORT > swarmnc < ../t0060-data/mss-ls &&

test/sharness/t0110-gateway.sh

@@ -32,10 +32,6 @@ test_expect_success "GET IPFS path output looks good" '
   rm actual
 '
 
-test_expect_success "GET IPFS path on API forbidden" '
-  test_curl_resp_http_code "http://127.0.0.1:$apiport/ipfs/$HASH" "HTTP/1.1 403 Forbidden"
-'
-
 test_expect_success "GET IPFS directory path succeeds" '
   mkdir dir &&
   echo "12345" >dir/test &&

test/supernode_client/main.go

@@ -109,7 +109,7 @@ func run() error {
 
 	opts := []corehttp.ServeOption{
 		corehttp.CommandsOption(cmdCtx(node, repoPath)),
-		corehttp.GatewayOption(false, nil),
+		corehttp.GatewayOption(),
 	}
 
 	if *cat {