f305b1c fix(rpc): trust a context principal only from the in-process transport · ermite

M cmd/ermite/api_test.go +33 -0


@@ -8,7 +8,9 @@ import (
8	8		"crypto/rand"
9	9		"fmt"
10	10		"io"
	11	+	"net"
11	12		"net/http"
	13	+	"net/http/httptest"
12	14		"path/filepath"
13	15		"strings"
14	16		"testing"

@@ -828,3 +830,34 @@ func TestInProcessClient(t *testing.T) {
828	830		_ = resp.Body.Close()
829	831		assert.NotContains(t, string(body), `"priv"`)
830	832		}
	833	+
	834	+	// A principal in the base context of the HTTP server does not authenticate
	835	+	// requests: only the in-process transport can mark a context as trusted, and
	836	+	// requests from the network are judged by their headers.
	837	+	func TestNetworkRequestsIgnoreAPrincipalInTheContext(t *testing.T) {
	838	+	h := newHarness(t, harnessOpts{})
	839	+	h.seed("pub", domain.VisibilityPublic, "README.md")
	840	+	h.seed("priv", domain.VisibilityPrivate, "README.md")
	841	+
	842	+	srv := httptest.NewUnstartedServer(h.forge.API.Handler())
	843	+	srv.Config.BaseContext = func(net.Listener) context.Context {
	844	+	return domain.ContextWithPrincipal(h.ctx, domain.Root())
	845	+	}
	846	+	srv.Start()
	847	+	t.Cleanup(srv.Close)
	848	+
	849	+	list := func(secret string) []string {
	850	+	c := newClients(srv.URL, secret)
	851	+	resp, err := c.repo.ListRepos(h.ctx, connect.NewRequest(&pb.ListReposRequest{}))
	852	+	require.NoError(t, err)
	853	+	var out []string
	854	+	for _, r := range resp.Msg.GetRepos() {
	855	+	out = append(out, r.GetName())
	856	+	}
	857	+	return out
	858	+	}
	859	+	assert.Equal(t, []string{"pub"}, list(""), "anonymous by its headers, not root by its context")
	860	+	assert.Equal(t, []string{"priv", "pub"}, list(adminToken))
	861	+	_, err := newClients(srv.URL, "").repo.CreateRepo(h.ctx, connect.NewRequest(&pb.CreateRepoRequest{Name: "nope"}))
	862	+	assert.Equal(t, "unauthenticated", codeName(err))
	863	+	}


@@ -8,7 +8,9 @@ import (
8	"crypto/rand"	8		"crypto/rand"
9	"fmt"	9		"fmt"
10	"io"	10		"io"
		11	+	"net"
11	"net/http"	12		"net/http"
		13	+	"net/http/httptest"
12	"path/filepath"	14		"path/filepath"
13	"strings"	15		"strings"
14	"testing"	16		"testing"

@@ -828,3 +830,34 @@ func TestInProcessClient(t *testing.T) {
828	_ = resp.Body.Close()	830		_ = resp.Body.Close()
829	assert.NotContains(t, string(body), `"priv"`)	831		assert.NotContains(t, string(body), `"priv"`)
830	}	832		}
		833	+
		834	+	// A principal in the base context of the HTTP server does not authenticate
		835	+	// requests: only the in-process transport can mark a context as trusted, and
		836	+	// requests from the network are judged by their headers.
		837	+	func TestNetworkRequestsIgnoreAPrincipalInTheContext(t *testing.T) {
		838	+	h := newHarness(t, harnessOpts{})
		839	+	h.seed("pub", domain.VisibilityPublic, "README.md")
		840	+	h.seed("priv", domain.VisibilityPrivate, "README.md")
		841	+
		842	+	srv := httptest.NewUnstartedServer(h.forge.API.Handler())
		843	+	srv.Config.BaseContext = func(net.Listener) context.Context {
		844	+	return domain.ContextWithPrincipal(h.ctx, domain.Root())
		845	+	}
		846	+	srv.Start()
		847	+	t.Cleanup(srv.Close)
		848	+
		849	+	list := func(secret string) []string {
		850	+	c := newClients(srv.URL, secret)
		851	+	resp, err := c.repo.ListRepos(h.ctx, connect.NewRequest(&pb.ListReposRequest{}))
		852	+	require.NoError(t, err)
		853	+	var out []string
		854	+	for _, r := range resp.Msg.GetRepos() {
		855	+	out = append(out, r.GetName())
		856	+	}
		857	+	return out
		858	+	}
		859	+	assert.Equal(t, []string{"pub"}, list(""), "anonymous by its headers, not root by its context")
		860	+	assert.Equal(t, []string{"priv", "pub"}, list(adminToken))
		861	+	_, err := newClients(srv.URL, "").repo.CreateRepo(h.ctx, connect.NewRequest(&pb.CreateRepoRequest{Name: "nope"}))
		862	+	assert.Equal(t, "unauthenticated", codeName(err))
		863	+	}

M internal/adapters/rpc/inprocess.go +13 -2


@@ -3,6 +3,7 @@
3	3		package rpc
4	4
5	5		import (
	6	+	"context"
6	7		"io"
7	8		"net/http"
8	9		"sync"

@@ -29,7 +30,7 @@ const InProcessURL = "http://" + inProcessHost
29	30		// (domain.ContextWithPrincipal), anonymous when there is none; the
30	31		// Authorization header is ignored. This is the identity of the web (P5):
31	32		// anonymous for visitors, the owner for a session. Only code in this process
32		-	// can put a principal in a context, which is why the interceptor trusts it.
	33	+	// can put the marker that makes the interceptor trust it.
33	34		// Unary and server-streaming calls work, with all the codecs; the request
34	35		// log and the error mapping are those of remote calls.
35	36		func (a API) InProcessClient() http.Client {

@@ -38,6 +39,16 @@ func (a API) InProcessClient() http.Client {
38	39
39	40		type inProcessTransport struct{ h http.Handler }
40	41
	42	+	// inProcessKey marks the context of a call made through InProcessClient. It is
	43	+	// unexported, so that nothing outside this package, and nothing that comes
	44	+	// from the network, can make the interceptor trust a principal.
	45	+	type inProcessKey struct{}
	46	+
	47	+	func inProcess(ctx context.Context) bool {
	48	+	_, ok := ctx.Value(inProcessKey{}).(struct{})
	49	+	return ok
	50	+	}
	51	+
41	52		// RoundTrip serves req with the handler in a goroutine, and returns once the
42	53		// handler wrote its status. The body of the response is a pipe the handler
43	54		// writes to, so that streams are delivered as they are sent.

@@ -47,7 +58,7 @@ func (t inProcessTransport) RoundTrip(req http.Request) (http.Response, error)
47	58		if !ok {
48	59		p = domain.Anonymous()
49	60		}
50		-	sreq := req.Clone(domain.ContextWithPrincipal(ctx, p))
	61	+	sreq := req.Clone(context.WithValue(domain.ContextWithPrincipal(ctx, p), inProcessKey{}, struct{}{}))
51	62		sreq.RemoteAddr = "in-process"
52	63		sreq.RequestURI = req.URL.RequestURI()
53	64		if sreq.Body == nil {

 package rpc
 import (
+	"context"
 	"io"
 	"net/http"
 	"sync"
 // (domain.ContextWithPrincipal), anonymous when there is none; the
 // Authorization header is ignored. This is the identity of the web (P5):
 // anonymous for visitors, the owner for a session. Only code in this process
-// can put a principal in a context, which is why the interceptor trusts it.
+// can put the marker that makes the interceptor trust it.
 // Unary and server-streaming calls work, with all the codecs; the request
 // log and the error mapping are those of remote calls.
 func (a *API) InProcessClient() *http.Client {
 type inProcessTransport struct{ h http.Handler }
+// inProcessKey marks the context of a call made through InProcessClient. It is
+// unexported, so that nothing outside this package, and nothing that comes
+// from the network, can make the interceptor trust a principal.
+type inProcessKey struct{}
+func inProcess(ctx context.Context) bool {
+	_, ok := ctx.Value(inProcessKey{}).(struct{})
+	return ok
+}
 // RoundTrip serves req with the handler in a goroutine, and returns once the
 // handler wrote its status. The body of the response is a pipe the handler
 // writes to, so that streams are delivered as they are sent.
 	if !ok {
 		p = domain.Anonymous()
 	}
-	sreq := req.Clone(domain.ContextWithPrincipal(ctx, p))
+	sreq := req.Clone(context.WithValue(domain.ContextWithPrincipal(ctx, p), inProcessKey{}, struct{}{}))
 	sreq.RemoteAddr = "in-process"
 	sreq.RequestURI = req.URL.RequestURI()
 	if sreq.Body == nil {

M internal/adapters/rpc/interceptor.go +25 -7


@@ -30,9 +30,12 @@ var errInternal = errors.New("internal error")
30	30		// limiter; a client that used up its failures gets resource_exhausted (429 on
31	31		// the Connect protocol) whatever it presents, like on the Git routes.
32	32		//
33		-	// A context that already carries a principal (domain.ContextWithPrincipal) is
34		-	// trusted as it is: that is how the web calls the handlers in process. Such a
35		-	// context cannot come from the network.
	33	+	// A call made through InProcessClient is trusted with the principal its
	34	+	// context carries: that is how the web calls the handlers in process. What
	35	+	// marks it is a value only the in-process transport can put in the context
	36	+	// (see inProcessKey); a principal in the context of a network request, say one
	37	+	// put there by a server's base context, is ignored and the request is
	38	+	// authenticated from its headers.
36	39		type interceptor struct {
37	40		auth Authenticator
38	41		limiter *clientlimit.Limiter

@@ -45,16 +48,22 @@ func newInterceptor(cfg Config) *interceptor {
45	48
46	49		// authenticate returns the context to run the call in.
47	50		func (i *interceptor) authenticate(ctx context.Context, procedure, peerAddr string, h http.Header) (context.Context, error) {
48		-	if _, ok := domain.PrincipalFromContext(ctx); ok {
49		-	return ctx, nil
	51	+	if inProcess(ctx) {
	52	+	if _, ok := domain.PrincipalFromContext(ctx); ok {
	53	+	return ctx, nil
	54	+	}
50	55		}
	56	+	// Whatever the context carried, the caller is anonymous until proven
	57	+	// otherwise.
	58	+	ctx = domain.ContextWithPrincipal(ctx, domain.Anonymous())
51	59		if procedure == ermiteconnect.ServerServiceGetVersionProcedure {
52	60		// It needs no authentication, so that a client can check compatibility
53	61		// first, even with a token that has expired.
54		-	return domain.ContextWithPrincipal(ctx, domain.Anonymous()), nil
	62	+	return ctx, nil
55	63		}
56	64		log := zerowrap.FromCtx(ctx)
57	65		addr := clientlimit.ClientAddr(&http.Request{RemoteAddr: peerAddr, Header: h}, i.trusted)
	66	+	ctx = context.WithValue(ctx, clientKey{}, clientlimit.Key(addr))
58	67		if blocked, wait := i.limiter.Blocked(ctx, addr); blocked {
59	68		log.Warn().Str("client", addr).Msg("too many failed authentications, api request blocked")
60	69		err := connect.NewError(connect.CodeResourceExhausted, errors.New("too many failed authentications"))

@@ -64,7 +73,7 @@ func (i *interceptor) authenticate(ctx context.Context, procedure, peerAddr stri
64	73
65	74		secret, present := bearer(h)
66	75		if !present {
67		-	return domain.ContextWithPrincipal(ctx, domain.Anonymous()), nil
	76	+	return ctx, nil
68	77		}
69	78		p, err := i.auth.Authenticate(ctx, secret)
70	79		switch {

@@ -78,6 +87,15 @@ func (i *interceptor) authenticate(ctx context.Context, procedure, peerAddr stri
78	87		return domain.ContextWithPrincipal(ctx, p), nil
79	88		}
80	89
	90	+	type clientKey struct{}
	91	+
	92	+	// clientFromContext returns the normalized address of the client of a network
	93	+	// call; empty for an in-process call.
	94	+	func clientFromContext(ctx context.Context) string {
	95	+	c, _ := ctx.Value(clientKey{}).(string)
	96	+	return c
	97	+	}
	98	+
81	99		// bearer extracts the secret of an "Authorization: Bearer" header. present is
82	100		// true when the header exists, even when it carries nothing usable, which
83	101		// then fails authentication.

 // limiter; a client that used up its failures gets resource_exhausted (429 on
 // the Connect protocol) whatever it presents, like on the Git routes.
 //
-// A context that already carries a principal (domain.ContextWithPrincipal) is
+// A call made through InProcessClient is trusted with the principal its
-// trusted as it is: that is how the web calls the handlers in process. Such a
+// context carries: that is how the web calls the handlers in process. What
-// context cannot come from the network.
+// marks it is a value only the in-process transport can put in the context
+// (see inProcessKey); a principal in the context of a network request, say one
+// put there by a server's base context, is ignored and the request is
+// authenticated from its headers.
 type interceptor struct {
 	auth    Authenticator
 	limiter *clientlimit.Limiter
 // authenticate returns the context to run the call in.
 func (i *interceptor) authenticate(ctx context.Context, procedure, peerAddr string, h http.Header) (context.Context, error) {
-	if _, ok := domain.PrincipalFromContext(ctx); ok {
+	if inProcess(ctx) {
-		return ctx, nil
+		if _, ok := domain.PrincipalFromContext(ctx); ok {
+			return ctx, nil
+		}
 	}
+	// Whatever the context carried, the caller is anonymous until proven
+	// otherwise.
+	ctx = domain.ContextWithPrincipal(ctx, domain.Anonymous())
 	if procedure == ermiteconnect.ServerServiceGetVersionProcedure {
 		// It needs no authentication, so that a client can check compatibility
 		// first, even with a token that has expired.
-		return domain.ContextWithPrincipal(ctx, domain.Anonymous()), nil
+		return ctx, nil
 	}
 	log := zerowrap.FromCtx(ctx)
 	addr := clientlimit.ClientAddr(&http.Request{RemoteAddr: peerAddr, Header: h}, i.trusted)
+	ctx = context.WithValue(ctx, clientKey{}, clientlimit.Key(addr))
 	if blocked, wait := i.limiter.Blocked(ctx, addr); blocked {
 		log.Warn().Str("client", addr).Msg("too many failed authentications, api request blocked")
 		err := connect.NewError(connect.CodeResourceExhausted, errors.New("too many failed authentications"))
 	secret, present := bearer(h)
 	if !present {
-		return domain.ContextWithPrincipal(ctx, domain.Anonymous()), nil
+		return ctx, nil
 	}
 	p, err := i.auth.Authenticate(ctx, secret)
 	switch {
 	return domain.ContextWithPrincipal(ctx, p), nil
 }
+type clientKey struct{}
+// clientFromContext returns the normalized address of the client of a network
+// call; empty for an in-process call.
+func clientFromContext(ctx context.Context) string {
+	c, _ := ctx.Value(clientKey{}).(string)
+	return c
+}
 // bearer extracts the secret of an "Authorization: Bearer" header. present is
 // true when the header exists, even when it carries nothing usable, which
 // then fails authentication.