Fix authorization webhook implementation and add tests (ContainerSSH#537

) * Fix authz webhook not being called Signed-off-by: Nikos Tsipinakis <nikos@tsipinakis.com> * Add codecov support for coverage reporting Signed-off-by: Nikos Tsipinakis <nikos@tsipinakis.com> * Unify authz configuration with the other methods Authz webhook configuration will now be placed under the 'webhook' key to be in line with the other authentication methods. Signed-off-by: Nikos Tsipinakis <nikos@tsipinakis.com> * Add authz support to testauthconfig server Signed-off-by: Nikos Tsipinakis <nikos@tsipinakis.com> * Test authz in integration Signed-off-by: Nikos Tsipinakis <nikos@tsipinakis.com> --------- Signed-off-by: Nikos Tsipinakis <nikos@tsipinakis.com>
tsipinakis · Oct 7, 2023 · ad1233a · ad1233a
1 parent 13dbb35
commit ad1233a
Show file tree

Hide file tree

Showing 8 changed files with 284 additions and 70 deletions.
diff --git a/.github/workflows/pipeline.yml b/.github/workflows/pipeline.yml
@@ -87,7 +87,7 @@ jobs:
       - name: Run go test
         run: |
           set -euo pipefail
-          go test -json -p 1 -v ./... 2>&1 | tee /tmp/gotest.log
+          go test -coverprofile=coverage.txt -covermode=atomic -json -p 1 -v ./... 2>&1 | tee /tmp/gotest.log
       - name: Format log output
         if: always()
         run: |
@@ -100,3 +100,7 @@ jobs:
           name: test-log
           path: /tmp/gotest.log
           if-no-files-found: error
+      - name: Upload coverage reports to Codecov
+        uses: codecov/codecov-action@v3
+        env:
+          CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }}
diff --git a/cmd/containerssh-testauthconfigserver/main.go b/cmd/containerssh-testauthconfigserver/main.go
@@ -3,16 +3,16 @@
 // This OpenAPI document describes the API endpoints that are required for implementing an authentication
 // and configuration server for ContainerSSH. (See https://github.com/containerssh/libcontainerssh for details.)
 //
-//     Schemes: http, https
-//     Host: localhost
-//     BasePath: /
-//     Version: 0.5.0
+//	Schemes: http, https
+//	Host: localhost
+//	BasePath: /
+//	Version: 0.5.0
 //
-//     Consumes:
-//     - application/json
+//	Consumes:
+//	- application/json
 //
-//     Produces:
-//     - application/json
+//	Produces:
+//	- application/json
 //
 // swagger:meta
 package main
@@ -40,73 +40,79 @@ type authHandler struct {
 
 // swagger:operation POST /password Authentication authPassword
 //
-// Password authentication
+// # Password authentication
 //
 // ---
 // parameters:
-// - name: request
-//   in: body
-//   description: The authentication request
-//   required: true
-//   schema:
+//   - name: request
+//     in: body
+//     description: The authentication request
+//     required: true
+//     schema:
 //     "$ref": "#/definitions/PasswordAuthRequest"
+//
 // responses:
-//   "200":
-//     "$ref": "#/responses/AuthResponse"
+//
+//	"200":
+//	  "$ref": "#/responses/AuthResponse"
 func (a *authHandler) OnPassword(meta metadata.ConnectionAuthPendingMetadata, Password []byte) (
 	bool,
 	metadata.ConnectionAuthenticatedMetadata,
 	error,
 ) {
 	if os.Getenv("CONTAINERSSH_ALLOW_ALL") == "1" ||
 		meta.Username == "foo" ||
-		meta.Username == "busybox" {
-		return true, meta.Authenticated(meta.Username), nil
+		meta.Username == "busybox" || meta.Username == "foonoauthz" {
+		return string(Password) == "bar", meta.Authenticated(meta.Username), nil
 	}
 	return false, meta.AuthFailed(), nil
 }
 
 // swagger:operation POST /pubkey Authentication authPubKey
 //
-// Public key authentication
+// # Public key authentication
 //
 // ---
 // parameters:
-// - name: request
-//   in: body
-//   description: The authentication request
-//   required: true
-//   schema:
+//   - name: request
+//     in: body
+//     description: The authentication request
+//     required: true
+//     schema:
 //     "$ref": "#/definitions/PublicKeyAuthRequest"
+//
 // responses:
-//   "200":
-//     "$ref": "#/responses/AuthResponse"
+//
+//	"200":
+//	  "$ref": "#/responses/AuthResponse"
 func (a *authHandler) OnPubKey(meta metadata.ConnectionAuthPendingMetadata, publicKey publicAuth.PublicKey) (
 	bool,
 	metadata.ConnectionAuthenticatedMetadata,
 	error,
 ) {
-	if meta.Username == "foo" || meta.Username == "busybox" {
-		return true, meta.Authenticated(meta.Username), nil
+	if meta.Username == "foo" || meta.Username == "busybox" || meta.Username == "foonoauthz" {
+		return false, meta.Authenticated(meta.Username), nil
 	}
 	return false, meta.AuthFailed(), nil
 }
 
 // swagger:operation POST /authz Authentication authz
 //
-// Authorization
+// # Authorization
 //
 // ---
 // parameters:
-// - name: request
-//   in: body
-//   description: The authorization request
-//   required: true
-//   schema:
+//   - name: request
+//     in: body
+//     description: The authorization request
+//     required: true
+//     schema:
 //     "$ref": "#/definitions/AuthorizationRequest"
+//
 // responses:
-//   "200":
-//     "$ref": "#/responses/AuthResponse"
+//
+//	"200":
+//	  "$ref": "#/responses/AuthResponse"
 func (a *authHandler) OnAuthorization(meta metadata.ConnectionAuthenticatedMetadata) (
 	bool,
 	metadata.ConnectionAuthenticatedMetadata,
@@ -127,14 +133,16 @@ type configHandler struct {
 //
 // ---
 // parameters:
-// - name: request
-//   in: body
-//   description: The configuration request
-//   schema:
+//   - name: request
+//     in: body
+//     description: The configuration request
+//     schema:
 //     "$ref": "#/definitions/ConfigRequest"
+//
 // responses:
-//   "200":
-//     "$ref": "#/responses/ConfigResponse"
+//
+//	"200":
+//	  "$ref": "#/responses/ConfigResponse"
 func (c *configHandler) OnConfig(request config.Request) (config.AppConfig, error) {
 	cfg := config.AppConfig{}
 
@@ -156,6 +164,8 @@ type handler struct {
 
 func (h *handler) ServeHTTP(writer goHttp.ResponseWriter, request *goHttp.Request) {
 	switch request.URL.Path {
+	case "/authz":
+		fallthrough
 	case "/password":
 		fallthrough
 	case "/pubkey":

diff --git a/config/auth.go b/config/auth.go
@@ -672,7 +672,7 @@ func (o *AuthGenericConfig) Validate() error {
 type AuthzConfig struct {
 	Method AuthzMethod `json:"method" yaml:"method" default:""`
 
-	AuthWebhookClientConfig `json:",inline" yaml:",inline"`
+	Webhook AuthWebhookClientConfig `json:"webhook" yaml:"webhook"`
 }
 
 // Validate validates the authorization configuration.
@@ -684,7 +684,7 @@ func (k *AuthzConfig) Validate() error {
 	case AuthzMethodDisabled:
 		return nil
 	case AuthzMethodWebhook:
-		return wrap(k.AuthWebhookClientConfig.Validate(), "webhook")
+		return wrap(k.Webhook.Validate(), "webhook")
 	default:
 		return newError("method", "BUG: invalid value for method for authorization: %s", k.Method)
 	}

diff --git a/internal/auth/authentication_factory.go b/internal/auth/authentication_factory.go
@@ -114,7 +114,7 @@ func NewAuthorizationProvider(
 	case config.AuthzMethodDisabled:
 		return nil, nil, nil
 	case config.AuthzMethodWebhook:
-		cli, err := NewWebhookClient(AuthenticationTypeAuthz, cfg.AuthWebhookClientConfig, logger, metrics)
+		cli, err := NewWebhookClient(AuthenticationTypeAuthz, cfg.Webhook, logger, metrics)
 		return cli, nil, err
 	default:
 		return nil, nil, fmt.Errorf("unsupported method: %s", cfg.Method)

diff --git a/internal/auth/webhook_client_impl.go b/internal/auth/webhook_client_impl.go
@@ -43,7 +43,13 @@ func (client *webhookClient) Authorize(
 		client.logger.Debug(err)
 		return &webhookClientContext{meta.AuthFailed(), false, err}
 	}
-	return client.processAuthzWithRetry(meta)
+
+	url := client.endpoint + "/authz"
+	authzRequest := auth.AuthorizationRequest{
+		ConnectionAuthenticatedMetadata: meta,
+	}
+
+	return client.processAuthzWithRetry(meta, url, authzRequest)
 }
 
 func (client *webhookClient) Password(
@@ -268,10 +274,9 @@ func (client *webhookClient) authServerRequest(endpoint string, requestObject in
 
 func (client *webhookClient) processAuthzWithRetry(
 	meta metadata.ConnectionAuthenticatedMetadata,
+	url string,
+	authzRequest interface{},
 ) AuthenticationContext {
-	url := client.endpoint + "/authz"
-	authzRequest := auth.AuthorizationRequest{}
-
 	ctx, cancel := context.WithTimeout(context.Background(), client.timeout)
 	defer cancel()
 	var lastError error

diff --git a/internal/auth/webhook_test.go b/internal/auth/webhook_test.go
@@ -70,6 +70,19 @@ func (h *handler) OnAuthorization(meta metadata.ConnectionAuthenticatedMetadata)
 	metadata.ConnectionAuthenticatedMetadata,
 	error,
 ) {
+	if meta.RemoteAddress.IP.String() != "127.0.0.1" {
+		return false, meta.AuthFailed(), fmt.Errorf("invalid IP: %s", meta.RemoteAddress.IP.String())
+	}
+	if meta.ConnectionID != "0123456789ABCDEF" {
+		return false, meta.AuthFailed(), fmt.Errorf("invalid connection ID: %s", meta.ConnectionID)
+	}
+	if meta.AuthenticatedUsername == "foo" {
+		return true, meta.AuthFailed(), nil
+	}
+	if meta.Username == "crash" {
+		// Simulate a database failure
+		return false, meta.AuthFailed(), fmt.Errorf("database error")
+	}
 	return false, meta.AuthFailed(), nil
 }
 
@@ -143,6 +156,24 @@ func TestAuth(t *testing.T) {
 				)
 				assert.NotEqual(t, nil, authenticationContext.Error())
 				assert.Equal(t, false, authenticationContext.Success())
+
+				authenticationContext = client.Authorize(
+					metadata.NewTestAuthenticatingMetadata("foo").Authenticated("foo"),
+				)
+				assert.Equal(t, nil, authenticationContext.Error())
+				assert.Equal(t, true, authenticationContext.Success())
+
+				authenticationContext = client.Authorize(
+					metadata.NewTestAuthenticatingMetadata("foo").Authenticated("foonoauthz"),
+				)
+				assert.Equal(t, nil, authenticationContext.Error())
+				assert.Equal(t, false, authenticationContext.Success())
+
+				authenticationContext = client.Authorize(
+					metadata.NewTestAuthenticatingMetadata("crash").Authenticated("crash"),
+				)
+				assert.NotEqual(t, nil, authenticationContext.Error())
+				assert.Equal(t, false, authenticationContext.Success())
 			},
 		)
 	}