Auth: make publicurl required and add stricter validation (#2458)

* make auth.publicurl required and add stricter validation

* fix broken tests

* add publicURL tests

* feedback and testfix

* use latest stable version of Go in e2e tests

.github/workflows/e2e-tests.yaml

@@ -23,6 +23,11 @@ jobs:
       - name: Checkout
         uses: actions/checkout@v3
 
+      - name: Set up Go
+        uses: actions/setup-go@v4
+        with:
+          go-version: 'stable'
+
       - name: Set up QEMU
         uses: docker/setup-qemu-action@v2
 

auth/api/iam/api_test.go

@@ -27,6 +27,7 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"go.uber.org/mock/gomock"
+	"net/url"
 	"testing"
 )
 
@@ -41,7 +42,6 @@ func TestWrapper_GetOAuthAuthorizationServerMetadata(t *testing.T) {
 
 		require.NoError(t, err)
 		assert.IsType(t, GetOAuthAuthorizationServerMetadata200JSONResponse{}, res)
-
 	})
 	t.Run("error - not a did", func(t *testing.T) {
 		//400
@@ -112,17 +112,23 @@ func statusCodeFrom(err error) int {
 
 type testCtx struct {
 	client        *Wrapper
+	authnServices *auth.MockAuthenticationServices
 	documentOwner *vdr.MockDocumentOwner
 }
 
 func newTestClient(t testing.TB) *testCtx {
+	publicURL, err := url.Parse("https://nuts.nl")
+	require.NoError(t, err)
 	ctrl := gomock.NewController(t)
-	ctx := &testCtx{
-		documentOwner: vdr.NewMockDocumentOwner(ctrl),
-	}
-	ctx.client = &Wrapper{
-		auth: auth.NewAuthInstance(auth.Config{PublicURL: "https://example.com"}, nil, nil, nil, nil, nil, nil),
-		vdr:  ctx.documentOwner,
+	authnServices := auth.NewMockAuthenticationServices(ctrl)
+	authnServices.EXPECT().PublicURL().Return(publicURL).AnyTimes()
+	documentOwner := vdr.NewMockDocumentOwner(ctrl)
+	return &testCtx{
+		authnServices: authnServices,
+		documentOwner: documentOwner,
+		client: &Wrapper{
+			auth: authnServices,
+			vdr:  documentOwner,
+		},
 	}
-	return ctx
 }

auth/auth.go

@@ -20,6 +20,7 @@ package auth
 
 import (
 	"errors"
+	"fmt"
 	"github.com/nuts-foundation/nuts-node/vdr/didservice"
 	"github.com/nuts-foundation/nuts-node/vdr/types"
 	"net/url"
@@ -37,9 +38,6 @@ import (
 	"github.com/nuts-foundation/nuts-node/vcr"
 )
 
-// ErrMissingPublicURL is returned when the publicUrl is missing from the config
-var ErrMissingPublicURL = errors.New("auth.publicurl must be set in strictmode")
-
 const contractValidity = 60 * time.Minute
 
 var _ AuthenticationServices = (*Auth)(nil)
@@ -57,6 +55,7 @@ type Auth struct {
 	pkiProvider     pki.Provider
 	shutdownFunc    func()
 	vdrInstance     types.VDR
+	publicURL       *url.URL
 }
 
 // Name returns the name of the module.
@@ -76,11 +75,7 @@ func (auth *Auth) V2APIEnabled() bool {
 
 // PublicURL returns the public URL of the node.
 func (auth *Auth) PublicURL() *url.URL {
-	if auth.config.PublicURL == "" {
-		panic("auth.publicurl must be set")
-	}
-	publicURL, _ := url.Parse(auth.config.PublicURL)
-	return publicURL
+	return auth.publicURL
 }
 
 // ContractNotary returns an implementation of the ContractNotary interface.
@@ -123,8 +118,17 @@ func (auth *Auth) Configure(config core.ServerConfig) error {
 	}
 
 	// TODO: this is verifier/signer specific
-	if auth.config.PublicURL == "" && config.Strictmode {
-		return ErrMissingPublicURL
+	if auth.config.PublicURL == "" {
+		return errors.New("invalid auth.publicurl: must provide url")
+	}
+	var err error
+	if config.Strictmode {
+		auth.publicURL, err = core.ParsePublicURL(auth.config.PublicURL, false, "https")
+	} else {
+		auth.publicURL, err = core.ParsePublicURL(auth.config.PublicURL, true, "http", "https")
+	}
+	if err != nil {
+		return fmt.Errorf("invalid auth.publicurl: %w", err)
 	}
 
 	auth.contractNotary = notary.NewNotary(notary.Config{

auth/auth_test.go

@@ -40,6 +40,7 @@ func TestAuth_Configure(t *testing.T) {
 	t.Run("ok", func(t *testing.T) {
 		config := DefaultConfig()
 		config.ContractValidators = []string{"uzi"}
+		config.PublicURL = "https://nuts.nl"
 		ctrl := gomock.NewController(t)
 		pkiMock := pki.NewMockProvider(ctrl)
 		pkiMock.EXPECT().AddTruststore(gomock.Any())   // uzi
@@ -49,16 +50,7 @@ func TestAuth_Configure(t *testing.T) {
 
 		i := NewAuthInstance(config, vdrInstance, vcr.NewTestVCRInstance(t), crypto.NewMemoryCryptoInstance(), nil, nil, pkiMock)
 
-		_ = i.Configure(tlsServerConfig)
-	})
-
-	t.Run("error - no publicUrl", func(t *testing.T) {
-		authCfg := TestConfig()
-		authCfg.Irma.SchemeManager = "pbdf"
-		i := testInstance(t, authCfg)
-		cfg := core.NewServerConfig()
-		cfg.Strictmode = true
-		assert.Equal(t, ErrMissingPublicURL, i.Configure(*cfg))
+		require.NoError(t, i.Configure(tlsServerConfig))
 	})
 
 	t.Run("error - IRMA config failure", func(t *testing.T) {
@@ -89,7 +81,6 @@ func TestAuth_Configure(t *testing.T) {
 
 	t.Run("error - TLS required in strict mode", func(t *testing.T) {
 		authCfg := TestConfig()
-		authCfg.PublicURL = "https://example.com"
 		i := testInstance(t, authCfg)
 		serverConfig := core.NewServerConfig()
 		serverConfig.Strictmode = true
@@ -105,6 +96,34 @@ func TestAuth_Configure(t *testing.T) {
 		err := i.Configure(tlsServerConfig)
 		assert.ErrorIs(t, err, assert.AnError)
 	})
+	t.Run("public url", func(t *testing.T) {
+		type test struct {
+			strict bool
+			pURL   string
+			errStr string
+		}
+		tt := []test{
+			{true, "", "invalid auth.publicurl: must provide url"},
+			{true, ":invalid", "invalid auth.publicurl: parse \":invalid\": missing protocol scheme"},
+			{true, "https://127.0.0.1", "invalid auth.publicurl: hostname is IP"},
+			{true, "https://example.com", "invalid auth.publicurl: hostname is RFC2606 reserved"},
+			{true, "https://localhost", "invalid auth.publicurl: hostname is RFC2606 reserved"},
+			{true, "http://nuts.nl", "invalid auth.publicurl: scheme must be https"},
+
+			{false, "", "invalid auth.publicurl: must provide url"},
+			{false, ":invalid", "invalid auth.publicurl: parse \":invalid\": missing protocol scheme"},
+			{false, "https://127.0.0.1", "invalid auth.publicurl: hostname is IP"},
+			{false, "something://nuts.nl", "invalid auth.publicurl: scheme must be http or https"},
+		}
+		authCfg := TestConfig()
+		cfg := core.NewServerConfig()
+		for _, test := range tt {
+			authCfg.PublicURL = test.pURL
+			i := testInstance(t, authCfg)
+			cfg.Strictmode = test.strict
+			assert.EqualError(t, i.Configure(*cfg), test.errStr, "test config: url=%s; strict=%s", test.pURL, test.strict)
+		}
+	})
 }
 
 func TestAuth_Name(t *testing.T) {

auth/test.go

@@ -31,6 +31,7 @@ import (
 func TestConfig() Config {
 	config := DefaultConfig()
 	config.ContractValidators = []string{"dummy"}
+	config.PublicURL = "https://nuts.nl"
 	return config
 }
 

core/url.go

@@ -18,7 +18,14 @@
 
 package core
 
-import "strings"
+import (
+	"errors"
+	"fmt"
+	"net"
+	"net/url"
+	"slices"
+	"strings"
+)
 
 // JoinURLPaths works like path.Join but for URLs; it won't remove double slashes.
 // It makes sures there is only one slash between the parts.
@@ -35,3 +42,62 @@ func JoinURLPaths(parts ...string) string {
 	}
 	return result
 }
+
+// ParsePublicURL parses the given input string as URL and asserts that
+// it has a scheme and that it is in the allowedSchemes if provided,
+// it is not an IP address, and
+// it is not (depending on allowReserved) a reserved address or TLD as described in RFC2606 or https://www.ietf.org/archive/id/draft-chapin-rfc2606bis-00.html.
+func ParsePublicURL(input string, allowReserved bool, allowedSchemes ...string) (*url.URL, error) {
+	parsed, err := url.Parse(input)
+	if err != nil {
+		return nil, err
+	}
+	if parsed.Scheme == "" || parsed.Hostname() == "" {
+		return nil, errors.New("url must contain scheme and host")
+	}
+	if len(allowedSchemes) > 0 && !slices.Contains(allowedSchemes, parsed.Scheme) {
+		return nil, fmt.Errorf("scheme must be %s", strings.Join(allowedSchemes, " or "))
+	}
+	if net.ParseIP(parsed.Hostname()) != nil {
+		return nil, errors.New("hostname is IP")
+	}
+	if !allowReserved && isReserved(parsed) {
+		return nil, errors.New("hostname is RFC2606 reserved")
+	}
+	return parsed, nil
+}
+
+// isReserved returns true if URL uses any of the reserved TLDs or addresses
+func isReserved(URL *url.URL) bool {
+	parts := strings.Split(strings.ToLower(URL.Hostname()), ".")
+	tld := parts[len(parts)-1]
+	if slices.Contains(reservedTLDs, tld) {
+		return true
+	}
+
+	if len(parts) > 1 {
+		l2address := strings.Join(parts[len(parts)-2:], ".")
+		return slices.Contains(reservedAddresses, l2address)
+	}
+
+	return false
+}
+
+var reservedTLDs = []string{
+	"", // no domain specified
+	"corp",
+	"example",
+	"home",
+	"host",
+	"invalid",
+	"lan",
+	"local",
+	"localdomain",
+	"localhost",
+	"test",
+}
+var reservedAddresses = []string{
+	"example.com",
+	"example.net",
+	"example.org",
+}