From 1afebc21f324982141ca8a29710da0d6f83ca804 Mon Sep 17 00:00:00 2001 From: Lars Gierth Date: Fri, 17 Jun 2016 16:33:25 +0200 Subject: [PATCH] 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 --- cmd/ipfs/daemon.go | 33 ++------------ cmd/ipfswatch/main.go | 8 +++- core/corehttp/gateway.go | 68 ++++------------------------ core/corehttp/gateway_handler.go | 10 +--- core/corehttp/gateway_test.go | 8 +++- misc/completion/ipfs-completion.bash | 12 ++--- test/sharness/t0061-daemon-opts.sh | 11 +---- test/sharness/t0110-gateway.sh | 4 -- test/supernode_client/main.go | 2 +- 9 files changed, 37 insertions(+), 119 deletions(-) diff --git a/cmd/ipfs/daemon.go b/cmd/ipfs/daemon.go index cb525591932..c734b7ba32b 100644 --- a/cmd/ipfs/daemon.go +++ b/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 { diff --git a/cmd/ipfswatch/main.go b/cmd/ipfswatch/main.go index f8011bd9502..4c69a35b823 100644 --- a/cmd/ipfswatch/main.go +++ b/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)), } diff --git a/core/corehttp/gateway.go b/core/corehttp/gateway.go index 0d9453e1e7a..d0cf597bf16 100644 --- a/core/corehttp/gateway.go +++ b/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) -} diff --git a/core/corehttp/gateway_handler.go b/core/corehttp/gateway_handler.go index f6f8705598f..8313e877e25 100644 --- a/core/corehttp/gateway_handler.go +++ b/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() { diff --git a/core/corehttp/gateway_test.go b/core/corehttp/gateway_test.go index f8a01aa5291..b966d329fab 100644 --- a/core/corehttp/gateway_test.go +++ b/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) diff --git a/misc/completion/ipfs-completion.bash b/misc/completion/ipfs-completion.bash index afa320f0270..0b88ea4c66e 100644 --- a/misc/completion/ipfs-completion.bash +++ b/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 diff --git a/test/sharness/t0061-daemon-opts.sh b/test/sharness/t0061-daemon-opts.sh index baf1a51817d..ba8385fb2fe 100755 --- a/test/sharness/t0061-daemon-opts.sh +++ b/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 && diff --git a/test/sharness/t0110-gateway.sh b/test/sharness/t0110-gateway.sh index ea1497b938d..7556ef4d45a 100755 --- a/test/sharness/t0110-gateway.sh +++ b/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 && diff --git a/test/supernode_client/main.go b/test/supernode_client/main.go index 76b0be0ddb9..23ac04e81cc 100644 --- a/test/supernode_client/main.go +++ b/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 {