From 4643e488dbc71faf0b0b717ef5018d77f8457f32 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Michaud?= Date: Thu, 13 Feb 2020 03:12:37 +0100 Subject: [PATCH] [MISC] Fail with an error message when X-Forwarded-* headers are missing (#631) * Fail with an error message when X-Forwarded-* headers are missing. * Remove useless comments. --- internal/handlers/errors.go | 4 +- .../handlers/handler_register_u2f_step1.go | 10 ++ .../handler_register_u2f_step1_test.go | 96 +++++++++++++++++++ internal/handlers/handler_sign_u2f_step1.go | 11 ++- .../handlers/handler_sign_u2f_step1_test.go | 42 ++++++++ internal/handlers/handler_verify_test.go | 4 +- internal/middlewares/errors.go | 5 + internal/middlewares/identity_verification.go | 10 ++ .../middlewares/identity_verification_test.go | 43 ++++++++- 9 files changed, 217 insertions(+), 8 deletions(-) create mode 100644 internal/handlers/handler_register_u2f_step1_test.go create mode 100644 internal/handlers/handler_sign_u2f_step1_test.go diff --git a/internal/handlers/errors.go b/internal/handlers/errors.go index ea4b7f71..875433d0 100644 --- a/internal/handlers/errors.go +++ b/internal/handlers/errors.go @@ -9,5 +9,5 @@ const InternalError = "Internal error." // UnauthorizedError is the error message sent when the user is not authorized. const UnauthorizedError = "You're not authorized." -var errMissingXForwardedHost = errors.New("Missing header X-Fowarded-Host used to detect target URL") -var errMissingXForwardedProto = errors.New("Missing header X-Fowarded-Proto used to detect target URL") +var errMissingXForwardedHost = errors.New("Missing header X-Fowarded-Host") +var errMissingXForwardedProto = errors.New("Missing header X-Fowarded-Proto") diff --git a/internal/handlers/handler_register_u2f_step1.go b/internal/handlers/handler_register_u2f_step1.go index a12f5061..ff114a9b 100644 --- a/internal/handlers/handler_register_u2f_step1.go +++ b/internal/handlers/handler_register_u2f_step1.go @@ -24,6 +24,16 @@ var SecondFactorU2FIdentityStart = middlewares.IdentityVerificationStart(middlew }) func secondFactorU2FIdentityFinish(ctx *middlewares.AutheliaCtx, username string) { + if ctx.XForwardedProto() == nil { + ctx.Error(errMissingXForwardedProto, operationFailedMessage) + return + } + + if ctx.XForwardedHost() == nil { + ctx.Error(errMissingXForwardedHost, operationFailedMessage) + return + } + appID := fmt.Sprintf("%s://%s", ctx.XForwardedProto(), ctx.XForwardedHost()) ctx.Logger.Tracef("U2F appID is %s", appID) var trustedFacets = []string{appID} diff --git a/internal/handlers/handler_register_u2f_step1_test.go b/internal/handlers/handler_register_u2f_step1_test.go new file mode 100644 index 00000000..89bfa2a2 --- /dev/null +++ b/internal/handlers/handler_register_u2f_step1_test.go @@ -0,0 +1,96 @@ +package handlers + +import ( + "fmt" + "testing" + "time" + + "github.com/authelia/authelia/internal/middlewares" + "github.com/authelia/authelia/internal/mocks" + "github.com/dgrijalva/jwt-go" + "github.com/golang/mock/gomock" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/suite" +) + +type HandlerRegisterU2FStep1Suite struct { + suite.Suite + + mock *mocks.MockAutheliaCtx +} + +func (s *HandlerRegisterU2FStep1Suite) SetupTest() { + s.mock = mocks.NewMockAutheliaCtx(s.T()) + + userSession := s.mock.Ctx.GetSession() + userSession.Username = "john" + s.mock.Ctx.SaveSession(userSession) +} + +func (s *HandlerRegisterU2FStep1Suite) TearDownTest() { + s.mock.Close() +} + +func createToken(secret string, username string, action string, expiresAt time.Time) string { + claims := &middlewares.IdentityVerificationClaim{ + StandardClaims: jwt.StandardClaims{ + ExpiresAt: expiresAt.Unix(), + Issuer: "Authelia", + }, + Action: action, + Username: username, + } + token := jwt.NewWithClaims(jwt.SigningMethodHS256, claims) + ss, _ := token.SignedString([]byte(secret)) + return ss +} + +func newFinishArgs() middlewares.IdentityVerificationFinishArgs { + return middlewares.IdentityVerificationFinishArgs{ + ActionClaim: U2FRegistrationAction, + IsTokenUserValidFunc: func(ctx *middlewares.AutheliaCtx, username string) bool { return true }, + } +} + +func (s *HandlerRegisterU2FStep1Suite) TestShouldRaiseWhenXForwardedProtoIsMissing() { + token := createToken(s.mock.Ctx.Configuration.JWTSecret, "john", U2FRegistrationAction, + time.Now().Add(1*time.Minute)) + s.mock.Ctx.Request.SetBodyString(fmt.Sprintf("{\"token\":\"%s\"}", token)) + + s.mock.StorageProviderMock.EXPECT(). + FindIdentityVerificationToken(gomock.Eq(token)). + Return(true, nil) + + s.mock.StorageProviderMock.EXPECT(). + RemoveIdentityVerificationToken(gomock.Eq(token)). + Return(nil) + + SecondFactorU2FIdentityFinish(s.mock.Ctx) + + assert.Equal(s.T(), 200, s.mock.Ctx.Response.StatusCode()) + assert.Equal(s.T(), "Missing header X-Fowarded-Proto", s.mock.Hook.LastEntry().Message) +} + +func (s *HandlerRegisterU2FStep1Suite) TestShouldRaiseWhenXForwardedHostIsMissing() { + s.mock.Ctx.Request.Header.Add("X-Forwarded-Proto", "http") + token := createToken(s.mock.Ctx.Configuration.JWTSecret, "john", U2FRegistrationAction, + time.Now().Add(1*time.Minute)) + s.mock.Ctx.Request.SetBodyString(fmt.Sprintf("{\"token\":\"%s\"}", token)) + + s.mock.StorageProviderMock.EXPECT(). + FindIdentityVerificationToken(gomock.Eq(token)). + Return(true, nil) + + s.mock.StorageProviderMock.EXPECT(). + RemoveIdentityVerificationToken(gomock.Eq(token)). + Return(nil) + + SecondFactorU2FIdentityFinish(s.mock.Ctx) + + assert.Equal(s.T(), 200, s.mock.Ctx.Response.StatusCode()) + assert.Equal(s.T(), "Missing header X-Fowarded-Host", s.mock.Hook.LastEntry().Message) +} + +func TestShouldRunHandlerRegisterU2FStep1Suite(t *testing.T) { + suite.Run(t, new(HandlerRegisterU2FStep1Suite)) +} diff --git a/internal/handlers/handler_sign_u2f_step1.go b/internal/handlers/handler_sign_u2f_step1.go index c5532f66..95016132 100644 --- a/internal/handlers/handler_sign_u2f_step1.go +++ b/internal/handlers/handler_sign_u2f_step1.go @@ -12,7 +12,15 @@ import ( // SecondFactorU2FSignGet handler for initiating a signing request. func SecondFactorU2FSignGet(ctx *middlewares.AutheliaCtx) { - userSession := ctx.GetSession() + if ctx.XForwardedProto() == nil { + ctx.Error(errMissingXForwardedProto, operationFailedMessage) + return + } + + if ctx.XForwardedHost() == nil { + ctx.Error(errMissingXForwardedHost, operationFailedMessage) + return + } appID := fmt.Sprintf("%s://%s", ctx.XForwardedProto(), ctx.XForwardedHost()) var trustedFacets = []string{appID} @@ -23,6 +31,7 @@ func SecondFactorU2FSignGet(ctx *middlewares.AutheliaCtx) { return } + userSession := ctx.GetSession() keyHandleBytes, publicKeyBytes, err := ctx.Providers.StorageProvider.LoadU2FDeviceHandle(userSession.Username) if err != nil { diff --git a/internal/handlers/handler_sign_u2f_step1_test.go b/internal/handlers/handler_sign_u2f_step1_test.go new file mode 100644 index 00000000..4b8ecf8f --- /dev/null +++ b/internal/handlers/handler_sign_u2f_step1_test.go @@ -0,0 +1,42 @@ +package handlers + +import ( + "testing" + + "github.com/authelia/authelia/internal/mocks" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/suite" +) + +type HandlerSignU2FStep1Suite struct { + suite.Suite + + mock *mocks.MockAutheliaCtx +} + +func (s *HandlerSignU2FStep1Suite) SetupTest() { + s.mock = mocks.NewMockAutheliaCtx(s.T()) +} + +func (s *HandlerSignU2FStep1Suite) TearDownTest() { + s.mock.Close() +} + +func (s *HandlerSignU2FStep1Suite) TestShouldRaiseWhenXForwardedProtoIsMissing() { + SecondFactorU2FSignGet(s.mock.Ctx) + + assert.Equal(s.T(), 200, s.mock.Ctx.Response.StatusCode()) + assert.Equal(s.T(), "Missing header X-Fowarded-Proto", s.mock.Hook.LastEntry().Message) +} + +func (s *HandlerSignU2FStep1Suite) TestShouldRaiseWhenXForwardedHostIsMissing() { + s.mock.Ctx.Request.Header.Add("X-Forwarded-Proto", "http") + SecondFactorU2FSignGet(s.mock.Ctx) + + assert.Equal(s.T(), 200, s.mock.Ctx.Response.StatusCode()) + assert.Equal(s.T(), "Missing header X-Fowarded-Host", s.mock.Hook.LastEntry().Message) +} + +func TestShouldRunHandlerSignU2FStep1Suite(t *testing.T) { + suite.Run(t, new(HandlerSignU2FStep1Suite)) +} diff --git a/internal/handlers/handler_verify_test.go b/internal/handlers/handler_verify_test.go index 9273ddd4..3d435f23 100644 --- a/internal/handlers/handler_verify_test.go +++ b/internal/handlers/handler_verify_test.go @@ -72,7 +72,7 @@ func TestShouldRaiseWhenNoHeaderProvidedToDetectTargetURL(t *testing.T) { defer mock.Close() _, err := getOriginalURL(mock.Ctx) assert.Error(t, err) - assert.Equal(t, "Missing header X-Fowarded-Proto used to detect target URL", err.Error()) + assert.Equal(t, "Missing header X-Fowarded-Proto", err.Error()) } func TestShouldRaiseWhenNoXForwardedHostHeaderProvidedToDetectTargetURL(t *testing.T) { @@ -82,7 +82,7 @@ func TestShouldRaiseWhenNoXForwardedHostHeaderProvidedToDetectTargetURL(t *testi mock.Ctx.Request.Header.Set("X-Forwarded-Proto", "https") _, err := getOriginalURL(mock.Ctx) assert.Error(t, err) - assert.Equal(t, "Missing header X-Fowarded-Host used to detect target URL", err.Error()) + assert.Equal(t, "Missing header X-Fowarded-Host", err.Error()) } func TestShouldRaiseWhenXForwardedProtoIsNotParseable(t *testing.T) { diff --git a/internal/middlewares/errors.go b/internal/middlewares/errors.go index 1ee2e5bc..78b1a018 100644 --- a/internal/middlewares/errors.go +++ b/internal/middlewares/errors.go @@ -1,8 +1,13 @@ package middlewares +import "errors" + // InternalError is the error message sent when there was an internal error but it should // be hidden to the end user. In that case the error should be in the server logs. const InternalError = "Internal error." // UnauthorizedError is the error message sent when the user is not authorized. const UnauthorizedError = "You're not authorized." + +var errMissingXForwardedHost = errors.New("Missing header X-Fowarded-Host") +var errMissingXForwardedProto = errors.New("Missing header X-Fowarded-Proto") diff --git a/internal/middlewares/identity_verification.go b/internal/middlewares/identity_verification.go index cb664eba..8410c661 100644 --- a/internal/middlewares/identity_verification.go +++ b/internal/middlewares/identity_verification.go @@ -49,6 +49,16 @@ func IdentityVerificationStart(args IdentityVerificationStartArgs) RequestHandle return } + if ctx.XForwardedProto() == nil { + ctx.Error(errMissingXForwardedProto, operationFailedMessage) + return + } + + if ctx.XForwardedHost() == nil { + ctx.Error(errMissingXForwardedHost, operationFailedMessage) + return + } + link := fmt.Sprintf("%s://%s%s?token=%s", ctx.XForwardedProto(), ctx.XForwardedHost(), args.TargetEndpoint, ss) diff --git a/internal/middlewares/identity_verification_test.go b/internal/middlewares/identity_verification_test.go index 7d394de2..e5917e01 100644 --- a/internal/middlewares/identity_verification_test.go +++ b/internal/middlewares/identity_verification_test.go @@ -42,7 +42,6 @@ func TestShouldFailStartingProcessIfUserHasNoEmailAddress(t *testing.T) { middlewares.IdentityVerificationStart(newArgs(retriever))(mock.Ctx) - // Return 200 KO assert.Equal(t, 200, mock.Ctx.Response.StatusCode()) assert.Equal(t, "User does not have any email", mock.Hook.LastEntry().Message) } @@ -60,7 +59,6 @@ func TestShouldFailIfJWTCannotBeSaved(t *testing.T) { args := newArgs(defaultRetriever) middlewares.IdentityVerificationStart(args)(mock.Ctx) - // Return 200 KO assert.Equal(t, 200, mock.Ctx.Response.StatusCode()) assert.Equal(t, "cannot save", mock.Hook.LastEntry().Message) } @@ -70,6 +68,8 @@ func TestShouldFailSendingAnEmail(t *testing.T) { defer mock.Close() mock.Ctx.Configuration.JWTSecret = "abc" + mock.Ctx.Request.Header.Add("X-Forwarded-Proto", "http") + mock.Ctx.Request.Header.Add("X-Forwarded-Host", "host") mock.StorageProviderMock.EXPECT(). SaveIdentityVerificationToken(gomock.Any()). @@ -82,16 +82,53 @@ func TestShouldFailSendingAnEmail(t *testing.T) { args := newArgs(defaultRetriever) middlewares.IdentityVerificationStart(args)(mock.Ctx) - // Return 200 KO assert.Equal(t, 200, mock.Ctx.Response.StatusCode()) assert.Equal(t, "no notif", mock.Hook.LastEntry().Message) } +func TestShouldFailWhenXForwardedProtoHeaderIsMissing(t *testing.T) { + mock := mocks.NewMockAutheliaCtx(t) + defer mock.Close() + + mock.Ctx.Configuration.JWTSecret = "abc" + mock.Ctx.Request.Header.Add("X-Forwarded-Host", "host") + + mock.StorageProviderMock.EXPECT(). + SaveIdentityVerificationToken(gomock.Any()). + Return(nil) + + args := newArgs(defaultRetriever) + middlewares.IdentityVerificationStart(args)(mock.Ctx) + + assert.Equal(t, 200, mock.Ctx.Response.StatusCode()) + assert.Equal(t, "Missing header X-Fowarded-Proto", mock.Hook.LastEntry().Message) +} + +func TestShouldFailWhenXForwardedHostHeaderIsMissing(t *testing.T) { + mock := mocks.NewMockAutheliaCtx(t) + defer mock.Close() + + mock.Ctx.Configuration.JWTSecret = "abc" + mock.Ctx.Request.Header.Add("X-Forwarded-Proto", "http") + + mock.StorageProviderMock.EXPECT(). + SaveIdentityVerificationToken(gomock.Any()). + Return(nil) + + args := newArgs(defaultRetriever) + middlewares.IdentityVerificationStart(args)(mock.Ctx) + + assert.Equal(t, 200, mock.Ctx.Response.StatusCode()) + assert.Equal(t, "Missing header X-Fowarded-Host", mock.Hook.LastEntry().Message) +} + func TestShouldSucceedIdentityVerificationStartProcess(t *testing.T) { mock := mocks.NewMockAutheliaCtx(t) defer mock.Close() mock.Ctx.Configuration.JWTSecret = "abc" + mock.Ctx.Request.Header.Add("X-Forwarded-Proto", "http") + mock.Ctx.Request.Header.Add("X-Forwarded-Host", "host") mock.StorageProviderMock.EXPECT(). SaveIdentityVerificationToken(gomock.Any()).