Merge pull request #2284 from openziti/fix.2282.fix.oidc.totp.flow

Fix.2282.fix.OIDC.totp.flow

controller/oidc_auth/login.go

@@ -6,6 +6,7 @@ import (
 	"encoding/json"
 	"fmt"
 	"github.com/openziti/edge-api/rest_model"
+	"github.com/openziti/foundation/v2/errorz"
 	"github.com/openziti/ziti/controller/apierror"
 	"github.com/openziti/ziti/controller/model"
 	"github.com/pkg/errors"
@@ -157,6 +158,7 @@ func renderTotp(w http.ResponseWriter, id string, err error) {
 }
 
 func renderPage(w http.ResponseWriter, pageTemplate *template.Template, id string, err error) {
+	w.Header().Set("content-type", "text/html; charset=utf-8")
 	var errMsg string
 	errDisplay := "none"
 	if err != nil {
@@ -184,20 +186,26 @@ func (l *login) checkTotp(w http.ResponseWriter, r *http.Request) {
 
 	if err != nil {
 		renderJsonApiError(w, err)
+		return
 	}
 
 	bodyContentType, err := negotiateBodyContentType(r)
 
 	if err != nil {
-		renderJsonApiError(w, err)
+		if responseType == JsonContentType {
+			renderJsonApiError(w, err)
+			return
+		}
+		http.Error(w, fmt.Sprintf("cannot process body content type: %s", err), http.StatusBadRequest)
+		return
 	}
 
 	id := ""
 	code := ""
 	if bodyContentType == FormContentType {
 		err := r.ParseForm()
 		if err != nil {
-			http.Error(w, fmt.Sprintf("cannot parse form:%s", err), http.StatusInternalServerError)
+			http.Error(w, fmt.Sprintf("cannot parse form:%s", err), http.StatusBadRequest)
 			return
 		}
 		id = r.FormValue("id")
@@ -225,19 +233,25 @@ func (l *login) checkTotp(w http.ResponseWriter, r *http.Request) {
 	authRequest, verifyErr := l.store.VerifyTotp(ctx, code, id)
 
 	if verifyErr != nil {
-		renderTotp(w, id, err)
-		return
+		if responseType == JsonContentType {
+			renderJsonApiError(w, &errorz.ApiError{
+				Code:    "INVALID TOTP CODE",
+				Message: "an invalid TOTP code was supplied",
+				Status:  http.StatusBadRequest,
+			})
+			return
+		} else {
+			renderTotp(w, id, verifyErr)
+			return
+		}
 	}
 
 	if !authRequest.HasAmr(AuthMethodSecondaryTotp) {
-		renderTotp(w, id, errors.New("invalid TOTP code"))
-	}
-
-	if responseType == HtmlContentType {
-		http.Redirect(w, r, l.callback(r.Context(), id), http.StatusFound)
+		renderTotp(w, id, errors.New("TOTP supplied but not enabled or required on identity"))
 	}
 
-	renderJson(w, http.StatusOK, &rest_model.Empty{})
+	callbackUrl := l.callback(r.Context(), id)
+	http.Redirect(w, r, callbackUrl, http.StatusFound)
 }
 
 func (l *login) authenticate(w http.ResponseWriter, r *http.Request) {
@@ -284,6 +298,10 @@ func (l *login) authenticate(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
+	authRequest.SdkInfo = credentials.SdkInfo
+	authRequest.EnvInfo = credentials.EnvInfo
+	authRequest.AuthTime = time.Now()
+
 	if authRequest.SecondaryTotpRequired && !authRequest.HasAmr(AuthMethodSecondaryTotp) {
 		w.Header().Set(TotpRequiredHeader, "true")
 		if responseType == HtmlContentType {
@@ -295,10 +313,6 @@ func (l *login) authenticate(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	authRequest.AuthTime = time.Now()
-	authRequest.SdkInfo = credentials.SdkInfo
-	authRequest.EnvInfo = credentials.EnvInfo
-
 	callbackUrl := l.callback(r.Context(), credentials.AuthRequestId)
 	http.Redirect(w, r, callbackUrl, http.StatusFound)
 }

controller/oidc_auth/provider.go

@@ -48,6 +48,8 @@ func NewNativeOnlyOP(ctx context.Context, env model.Env, config Config) (http.Ha
 	return http.HandlerFunc(func(writer http.ResponseWriter, request *http.Request) {
 		r := request.WithContext(context.WithValue(request.Context(), contextKeyHttpRequest, request))
 		r = request.WithContext(context.WithValue(r.Context(), contextKeyTokenState, &TokenState{}))
+		r = request.WithContext(op.ContextWithIssuer(r.Context(), config.Issuer))
+
 		oidcHandler.ServeHTTP(writer, r)
 	}), nil
 

go.mod

@@ -55,7 +55,7 @@ require (
 	github.com/openziti/jwks v1.0.3
 	github.com/openziti/metrics v1.2.56
 	github.com/openziti/runzmd v1.0.49
-	github.com/openziti/sdk-golang v0.23.39
+	github.com/openziti/sdk-golang v0.23.40
 	github.com/openziti/secretstream v0.1.21
 	github.com/openziti/storage v0.3.0
 	github.com/openziti/transport/v2 v2.0.138

go.sum

@@ -582,8 +582,8 @@ github.com/openziti/metrics v1.2.56 h1:sOX5SCdK2Kx2vci+2PWIXDedbyRDUWylM3xJmmISi
 github.com/openziti/metrics v1.2.56/go.mod h1:nATuueUtFF5PDhwBwbq93g8sjpQZmf0yo2rySOnOUEY=
 github.com/openziti/runzmd v1.0.49 h1:WAnHH/aNAuGTORtHq146P02e2ZuGQaiyjht13OpgllE=
 github.com/openziti/runzmd v1.0.49/go.mod h1:sFSEo3SJOMPdE52WZ6OGHnFTuxS9NqcrGc3VYoR5qzA=
-github.com/openziti/sdk-golang v0.23.39 h1:e+FJ8h0jxP1NaRZq4eIafMpxqe+UK0T43bKhdyGZPdQ=
-github.com/openziti/sdk-golang v0.23.39/go.mod h1:5wt3h/TCeC/YqNLfdTAMEEAJvQqCCJYKYXt4Dmbcj64=
+github.com/openziti/sdk-golang v0.23.40 h1:GvONB0uFDerPqRYy2f+W+SjYemGPrh6zZLBQTkIsi9g=
+github.com/openziti/sdk-golang v0.23.40/go.mod h1:2PRGfYgwpSBrYKtYsjmq5o5fvF1PmkVtvmxwqMqVhx4=
 github.com/openziti/secretstream v0.1.21 h1:r4xN8/CzSEvxZFFYGSztrlhMtIvk3B+SQcq2zgZ4Tb4=
 github.com/openziti/secretstream v0.1.21/go.mod h1:1lfAnS8gBHsKZiPbRRK1sularbAsqizN6tWUEuZSfo0=
 github.com/openziti/storage v0.3.0 h1:DH2SN8GYy7rSlBZM9X5W1Dv2b2qZ8kSKyt0iivokVMw=

tests/mfa_ziti_test.go

@@ -1077,7 +1077,7 @@ func Test_MFA(t *testing.T) {
 }
 
 func computeMFACode(secret string) string {
-	now := int64(time.Now().UTC().Unix() / 30)
+	now := time.Now().UTC().Unix() / 30
 	code := dgoogauth.ComputeCode(secret, now)
 
 	//pad leading 0s to 6 characters

tests/sdk_auth_test.go

@@ -1,10 +1,13 @@
+//go:build apitests
+
 package tests
 
 import (
 	"crypto/x509"
 	"github.com/golang-jwt/jwt/v5"
 	"github.com/google/uuid"
 	"github.com/openziti/edge-api/rest_client_api_client/current_api_session"
+	"github.com/openziti/edge-api/rest_client_api_client/current_identity"
 	"github.com/openziti/edge-api/rest_management_api_client/auth_policy"
 	"github.com/openziti/edge-api/rest_management_api_client/external_jwt_signer"
 	management_identity "github.com/openziti/edge-api/rest_management_api_client/identity"
@@ -62,6 +65,109 @@ func TestSdkAuth(t *testing.T) {
 		ctx.Req.NotNil(apiSession.GetToken())
 	})
 
+	t.Run("oidc client cert + TOTP MFA, ca pool on client can authenticate", func(t *testing.T) {
+		ctx.testContextChanged(t)
+
+		testIdMfa := ctx.AdminManagementSession.RequireNewIdentityWithOtt(true)
+		testIdMfaCerts := ctx.completeOttEnrollment(testIdMfa.Id)
+
+		certCreds := edge_apis.NewCertCredentials([]*x509.Certificate{testIdMfaCerts.cert}, testIdMfaCerts.key)
+
+		client := edge_apis.NewClientApiClient([]*url.URL{clientApiUrl}, ctx.ControllerConfig.Id.CA(), func(strings chan string) {
+			strings <- ""
+		})
+		apiSession, err := client.Authenticate(certCreds, nil)
+
+		ctx.Req.NoError(err)
+		ctx.Req.NotNil(client)
+		ctx.Req.NotNil(apiSession)
+		ctx.Req.NotNil(apiSession.GetToken())
+
+		t.Run("can enroll in MFA TOTP", func(t *testing.T) {
+			ctx.testContextChanged(t)
+
+			enrollMfaParams := &current_identity.EnrollMfaParams{}
+			enrollMfaResp, err := client.API.CurrentIdentity.EnrollMfa(enrollMfaParams, nil)
+
+			ctx.Req.NoError(err)
+			ctx.Req.NotNil(enrollMfaResp)
+			ctx.Req.NotNil(enrollMfaResp.Payload)
+			ctx.Req.NotNil(enrollMfaResp.Payload.Data)
+
+			detailMfaParams := &current_identity.DetailMfaParams{}
+			detailMfaResp, err := client.API.CurrentIdentity.DetailMfa(detailMfaParams, nil)
+			ctx.Req.NoError(err)
+			ctx.Req.NotNil(detailMfaResp)
+			ctx.Req.NotNil(detailMfaResp.Payload)
+			ctx.Req.NotNil(detailMfaResp.Payload.Data)
+			ctx.Req.NotEmpty(detailMfaResp.Payload.Data.ProvisioningURL)
+
+			parsedUrl, err := url.Parse(detailMfaResp.Payload.Data.ProvisioningURL)
+			ctx.Req.NoError(err)
+
+			queryParams, err := url.ParseQuery(parsedUrl.RawQuery)
+			ctx.Req.NoError(err)
+			secrets := queryParams["secret"]
+			ctx.Req.NotNil(secrets)
+			ctx.Req.NotEmpty(secrets)
+
+			mfaSecret := secrets[0]
+
+			code := computeMFACode(mfaSecret)
+
+			verifyMfaParams := &current_identity.VerifyMfaParams{
+				MfaValidation: &rest_model.MfaCode{
+					Code: ToPtr(code),
+				},
+			}
+
+			verifyMfaResp, err := client.API.CurrentIdentity.VerifyMfa(verifyMfaParams, nil)
+			ctx.Req.NoError(err)
+			ctx.Req.NotNil(verifyMfaResp)
+
+			t.Run("can authenticate with newly enrolled TOTP MFA", func(t *testing.T) {
+				ctx.testContextChanged(t)
+
+				client := edge_apis.NewClientApiClient([]*url.URL{clientApiUrl}, ctx.ControllerConfig.Id.CA(), func(strings chan string) {
+					parsedUrl, err := url.Parse(detailMfaResp.Payload.Data.ProvisioningURL)
+					ctx.Req.NoError(err)
+
+					queryParams, err := url.ParseQuery(parsedUrl.RawQuery)
+					ctx.Req.NoError(err)
+					secrets := queryParams["secret"]
+					ctx.Req.NotNil(secrets)
+					ctx.Req.NotEmpty(secrets)
+
+					mfaSecret := secrets[0]
+
+					code := computeMFACode(mfaSecret)
+
+					strings <- code
+				})
+				client.SetUseOidc(true)
+				apiSession, err := client.Authenticate(certCreds, nil)
+
+				ctx.Req.NoError(err)
+				ctx.Req.NotNil(apiSession)
+				ctx.Req.NotNil(apiSession.GetToken())
+			})
+
+			t.Run("authenticating with invalid totp code returns an error", func(t *testing.T) {
+				ctx.testContextChanged(t)
+
+				client := edge_apis.NewClientApiClient([]*url.URL{clientApiUrl}, ctx.ControllerConfig.Id.CA(), func(strings chan string) {
+					const invalidCode = "00000"
+					strings <- invalidCode
+				})
+				client.SetUseOidc(true)
+				apiSession, err := client.Authenticate(certCreds, nil)
+
+				ctx.Req.Error(err)
+				ctx.Req.Nil(apiSession)
+			})
+		})
+	})
+
 	t.Run("client updb, ca pool on cert can authenticate", func(t *testing.T) {
 		ctx.testContextChanged(t)
 

zititest/go.mod

@@ -16,7 +16,7 @@ require (
 	github.com/openziti/fablab v0.5.60
 	github.com/openziti/foundation/v2 v2.0.47
 	github.com/openziti/identity v1.0.81
-	github.com/openziti/sdk-golang v0.23.39
+	github.com/openziti/sdk-golang v0.23.40
 	github.com/openziti/storage v0.3.0
 	github.com/openziti/transport/v2 v2.0.138
 	github.com/openziti/ziti v0.28.3

zititest/go.sum

@@ -606,8 +606,8 @@ github.com/openziti/metrics v1.2.56 h1:sOX5SCdK2Kx2vci+2PWIXDedbyRDUWylM3xJmmISi
 github.com/openziti/metrics v1.2.56/go.mod h1:nATuueUtFF5PDhwBwbq93g8sjpQZmf0yo2rySOnOUEY=
 github.com/openziti/runzmd v1.0.49 h1:WAnHH/aNAuGTORtHq146P02e2ZuGQaiyjht13OpgllE=
 github.com/openziti/runzmd v1.0.49/go.mod h1:sFSEo3SJOMPdE52WZ6OGHnFTuxS9NqcrGc3VYoR5qzA=
-github.com/openziti/sdk-golang v0.23.39 h1:e+FJ8h0jxP1NaRZq4eIafMpxqe+UK0T43bKhdyGZPdQ=
-github.com/openziti/sdk-golang v0.23.39/go.mod h1:5wt3h/TCeC/YqNLfdTAMEEAJvQqCCJYKYXt4Dmbcj64=
+github.com/openziti/sdk-golang v0.23.40 h1:GvONB0uFDerPqRYy2f+W+SjYemGPrh6zZLBQTkIsi9g=
+github.com/openziti/sdk-golang v0.23.40/go.mod h1:2PRGfYgwpSBrYKtYsjmq5o5fvF1PmkVtvmxwqMqVhx4=
 github.com/openziti/secretstream v0.1.21 h1:r4xN8/CzSEvxZFFYGSztrlhMtIvk3B+SQcq2zgZ4Tb4=
 github.com/openziti/secretstream v0.1.21/go.mod h1:1lfAnS8gBHsKZiPbRRK1sularbAsqizN6tWUEuZSfo0=
 github.com/openziti/storage v0.3.0 h1:DH2SN8GYy7rSlBZM9X5W1Dv2b2qZ8kSKyt0iivokVMw=