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

gateway: clean up its surface, and remove BlockList #2874

Merged
merged 1 commit into from
Jun 20, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 5 additions & 28 deletions cmd/ipfs/daemon.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import (
_ "net/http/pprof"
"os"
"sort"
"strings"
"sync"

"gx/ipfs/QmPpRcbNUXauP3zWZ1NJMLWpe4QnmEHrd2ba2D3yqWznw7/go-multiaddr-net"
Expand Down Expand Up @@ -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),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this true?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The option itself is still there, but not being used anymore, lines 365 ff. were the only usage. I left the option there for now so that you don't get Error: Unrecognized option 'unrestricted-api' if you were really using it. The new behaviour is as if --unrestricted-api was always true.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new behaviour is as if --unrestricted-api was always true.

I believe this has since been reverted. But this comment should've raised flags.

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),
Expand Down Expand Up @@ -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/"),
Expand Down Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we are using defaults, the if block can be removed and just assign to cfg.Gateway.Writable.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mh okay, we shouldn't be using Default(false) with this one. From what I can tell, --writable if present is supposed to override cfg.Gateway.Writable.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So... the second return value (writableOptionFound) is separate from defaults -- it tells specifically whether the option was passed on the CLI.

So this assumption is still true and everything's good as-is:

From what I can tell, --writable if present is supposed to override cfg.Gateway.Writable.

In other words, the default value for --writable is never used -- only if it has actually been passed.

}

gwLis, err := manet.Listen(gatewayMaddr)
Expand All @@ -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 {
Expand Down
8 changes: 7 additions & 1 deletion cmd/ipfswatch/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)),
}
Expand Down
68 changes: 8 additions & 60 deletions core/corehttp/gateway.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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)
}
10 changes: 2 additions & 8 deletions core/corehttp/gateway_handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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() {
Expand Down
8 changes: 7 additions & 1 deletion core/corehttp/gateway_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{}
Expand All @@ -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)
Expand Down
12 changes: 6 additions & 6 deletions misc/completion/ipfs-completion.bash
Original file line number Diff line number Diff line change
Expand Up @@ -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 \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this needs to come back too.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ack right! thanks

--help"
}

Expand Down Expand Up @@ -314,7 +314,7 @@ _ipfs_resolve()

_ipfs_stats()
{
_ipfs_comp "bw --help"
_ipfs_comp "bw --help"
}

_ipfs_stats_bw()
Expand Down Expand Up @@ -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
Expand Down
11 changes: 1 addition & 10 deletions test/sharness/t0061-daemon-opts.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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 &&
Expand Down
4 changes: 0 additions & 4 deletions test/sharness/t0110-gateway.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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 &&
Expand Down
2 changes: 1 addition & 1 deletion test/supernode_client/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ func run() error {

opts := []corehttp.ServeOption{
corehttp.CommandsOption(cmdCtx(node, repoPath)),
corehttp.GatewayOption(false, nil),
corehttp.GatewayOption(),
}

if *cat {
Expand Down