Skip to content

Commit

Permalink
feat: make notification to unknown recipients configurable (#3075)
Browse files Browse the repository at this point in the history
Added the ability to configure whether the system should notify unknown recipients, if some tries to recover their account or verify their address ("anti-account-enumeration measures").

BREAKING CHANGES: By default, Kratos no longer sends out these Emails. If you want to keep notifying unknown addresses (keep the current behavior), set `selfservice.flows.recovery.notify_unknown_recipients` to `true` for recovery, or `selfservice.flows.verification.notify_unknown_recipients` for verification flows.

Closes #2345 
Closes #2585
  • Loading branch information
jonas-jonas authored Feb 14, 2023
1 parent 9c0b68c commit 1a5ead4
Show file tree
Hide file tree
Showing 25 changed files with 394 additions and 114 deletions.
11 changes: 11 additions & 0 deletions driver/config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -150,13 +150,15 @@ const (
ViperKeySelfServiceRecoveryUI = "selfservice.flows.recovery.ui_url"
ViperKeySelfServiceRecoveryRequestLifespan = "selfservice.flows.recovery.lifespan"
ViperKeySelfServiceRecoveryBrowserDefaultReturnTo = "selfservice.flows.recovery.after." + DefaultBrowserReturnURL
ViperKeySelfServiceRecoveryNotifyUnknownRecipients = "selfservice.flows.recovery.notify_unknown_recipients"
ViperKeySelfServiceVerificationEnabled = "selfservice.flows.verification.enabled"
ViperKeySelfServiceVerificationUI = "selfservice.flows.verification.ui_url"
ViperKeySelfServiceVerificationRequestLifespan = "selfservice.flows.verification.lifespan"
ViperKeySelfServiceVerificationBrowserDefaultReturnTo = "selfservice.flows.verification.after." + DefaultBrowserReturnURL
ViperKeySelfServiceVerificationAfter = "selfservice.flows.verification.after"
ViperKeySelfServiceVerificationBeforeHooks = "selfservice.flows.verification.before.hooks"
ViperKeySelfServiceVerificationUse = "selfservice.flows.verification.use"
ViperKeySelfServiceVerificationNotifyUnknownRecipients = "selfservice.flows.verification.notify_unknown_recipients"
ViperKeyDefaultIdentitySchemaID = "identity.default_schema_id"
ViperKeyIdentitySchemas = "identity.schemas"
ViperKeyHasherAlgorithm = "hashers.algorithm"
Expand Down Expand Up @@ -656,10 +658,15 @@ func (p *Config) SelfServiceFlowRecoveryBeforeHooks(ctx context.Context) []SelfS
func (p *Config) SelfServiceFlowVerificationBeforeHooks(ctx context.Context) []SelfServiceHook {
return p.selfServiceHooks(ctx, ViperKeySelfServiceVerificationBeforeHooks)
}

func (p *Config) SelfServiceFlowVerificationUse(ctx context.Context) string {
return p.GetProvider(ctx).String(ViperKeySelfServiceVerificationUse)
}

func (p *Config) SelfServiceFlowVerificationNotifyUnknownRecipients(ctx context.Context) bool {
return p.GetProvider(ctx).BoolF(ViperKeySelfServiceVerificationNotifyUnknownRecipients, false)
}

func (p *Config) SelfServiceFlowSettingsBeforeHooks(ctx context.Context) []SelfServiceHook {
return p.selfServiceHooks(ctx, ViperKeySelfServiceSettingsBeforeHooks)
}
Expand Down Expand Up @@ -1193,6 +1200,10 @@ func (p *Config) SelfServiceFlowRecoveryRequestLifespan(ctx context.Context) tim
return p.GetProvider(ctx).DurationF(ViperKeySelfServiceRecoveryRequestLifespan, time.Hour)
}

func (p *Config) SelfServiceFlowRecoveryNotifyUnknownRecipients(ctx context.Context) bool {
return p.GetProvider(ctx).BoolF(ViperKeySelfServiceRecoveryNotifyUnknownRecipients, false)
}

func (p *Config) SelfServiceLinkMethodLifespan(ctx context.Context) time.Duration {
return p.GetProvider(ctx).DurationF(ViperKeyLinkLifespan, time.Hour)
}
Expand Down
13 changes: 13 additions & 0 deletions driver/config/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -514,6 +514,7 @@ func TestViperProvider_Defaults(t *testing.T) {
assert.True(t, p.SelfServiceStrategy(ctx, "password").Enabled)
assert.True(t, p.SelfServiceStrategy(ctx, "profile").Enabled)
assert.True(t, p.SelfServiceStrategy(ctx, "link").Enabled)
assert.True(t, p.SelfServiceStrategy(ctx, "code").Enabled)
assert.False(t, p.SelfServiceStrategy(ctx, "oidc").Enabled)
},
},
Expand All @@ -531,6 +532,15 @@ func TestViperProvider_Defaults(t *testing.T) {
assert.True(t, p.SelfServiceStrategy(ctx, "oidc").Enabled)
},
},
{
init: func() *config.Config {
return config.MustNew(t, l, os.Stderr, configx.WithConfigFiles("stub/.kratos.notify-unknown-recipients.yml"), configx.SkipValidation())
},
expect: func(t *testing.T, p *config.Config) {
assert.True(t, p.SelfServiceFlowRecoveryNotifyUnknownRecipients(ctx))
assert.True(t, p.SelfServiceFlowVerificationNotifyUnknownRecipients(ctx))
},
},
} {
t.Run(fmt.Sprintf("case=%d", k), func(t *testing.T) {
p := tc.init()
Expand All @@ -546,6 +556,9 @@ func TestViperProvider_Defaults(t *testing.T) {
assert.False(t, p.SelfServiceStrategy(ctx, "link").Enabled)
assert.True(t, p.SelfServiceStrategy(ctx, "code").Enabled)
assert.False(t, p.SelfServiceStrategy(ctx, "oidc").Enabled)

assert.False(t, p.SelfServiceFlowRecoveryNotifyUnknownRecipients(ctx))
assert.False(t, p.SelfServiceFlowVerificationNotifyUnknownRecipients(ctx))
})
}

Expand Down
6 changes: 6 additions & 0 deletions driver/config/stub/.kratos.notify-unknown-recipients.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
selfservice:
flows:
recovery:
notify_unknown_recipients: true
verification:
notify_unknown_recipients: true
12 changes: 12 additions & 0 deletions embedx/config.schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -1230,6 +1230,12 @@
"code"
],
"default": "code"
},
"notify_unknown_recipients": {
"title": "Notify unknown recipients",
"description": "Whether to notify recipients, if verification was requested for their address.",
"type": "boolean",
"default": false
}
}
},
Expand Down Expand Up @@ -1281,6 +1287,12 @@
"code"
],
"default": "code"
},
"notify_unknown_recipients": {
"title": "Notify unknown recipients",
"description": "Whether to notify recipients, if recovery was requested for their account.",
"type": "boolean",
"default": false
}
}
},
Expand Down
64 changes: 42 additions & 22 deletions selfservice/strategy/code/code_sender.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ package code

import (
"context"
"net/http"
"net/url"

"github.com/gofrs/uuid"
Expand Down Expand Up @@ -59,21 +58,35 @@ func NewSender(deps senderDependencies) *Sender {
return &Sender{deps: deps}
}

// SendRecoveryCode sends a recovery code to the specified address.
// If the address does not exist in the store, an email is still being sent to prevent account
// enumeration attacks. In that case, this function returns the ErrUnknownAddress error.
func (s *Sender) SendRecoveryCode(ctx context.Context, r *http.Request, f *recovery.Flow, via identity.VerifiableAddressType, to string) error {
// SendRecoveryCode sends a recovery code to the specified address
//
// If the address does not exist in the store and dispatching invalid emails is enabled (CourierEnableInvalidDispatch is
// true), an email is still being sent to prevent account enumeration attacks. In that case, this function returns the
// ErrUnknownAddress error.
func (s *Sender) SendRecoveryCode(ctx context.Context, f *recovery.Flow, via identity.VerifiableAddressType, to string) error {
s.deps.Logger().
WithField("via", via).
WithSensitiveField("address", to).
Debug("Preparing recovery code.")

address, err := s.deps.IdentityPool().FindRecoveryAddressByValue(ctx, identity.RecoveryAddressTypeEmail, to)
if err != nil {
if err := s.send(ctx, string(via), email.NewRecoveryCodeInvalid(s.deps, &email.RecoveryCodeInvalidModel{To: to})); err != nil {
if errors.Is(err, sqlcon.ErrNoRows) {
notifyUnknownRecipients := s.deps.Config().SelfServiceFlowRecoveryNotifyUnknownRecipients(ctx)
s.deps.Audit().
WithField("via", via).
WithSensitiveField("email_address", address).
WithField("strategy", "code").
WithField("was_notified", notifyUnknownRecipients).
Info("Account recovery was requested for an unknown address.")
if !notifyUnknownRecipients {
// do nothing
} else if err := s.send(ctx, string(via), email.NewRecoveryCodeInvalid(s.deps, &email.RecoveryCodeInvalidModel{To: to})); err != nil {
return err
}
return ErrUnknownAddress
return errors.WithStack(ErrUnknownAddress)
} else if err != nil {
// DB error
return err
}

// Get the identity associated with the recovery address
Expand All @@ -90,7 +103,7 @@ func (s *Sender) SendRecoveryCode(ctx context.Context, r *http.Request, f *recov
CreateRecoveryCode(ctx, &CreateRecoveryCodeParams{
RawCode: rawCode,
CodeType: RecoveryCodeTypeSelfService,
ExpiresIn: s.deps.Config().SelfServiceCodeMethodLifespan(r.Context()),
ExpiresIn: s.deps.Config().SelfServiceCodeMethodLifespan(ctx),
RecoveryAddress: address,
FlowID: f.ID,
IdentityID: i.ID,
Expand Down Expand Up @@ -124,27 +137,34 @@ func (s *Sender) SendRecoveryCodeTo(ctx context.Context, i *identity.Identity, c
return s.send(ctx, string(code.RecoveryAddress.Via), email.NewRecoveryCodeValid(s.deps, &emailModel))
}

// SendVerificationCode sends a verification link to the specified address. If the address does not exist in the store, an email is
// still being sent to prevent account enumeration attacks. In that case, this function returns the ErrUnknownAddress
// error.
// SendVerificationCode sends a verification code & link to the specified address
//
// If the address does not exist in the store and dispatching invalid emails is enabled (CourierEnableInvalidDispatch is
// true), an email is still being sent to prevent account enumeration attacks. In that case, this function returns the
// ErrUnknownAddress error.
func (s *Sender) SendVerificationCode(ctx context.Context, f *verification.Flow, via identity.VerifiableAddressType, to string) error {
s.deps.Logger().
WithField("via", via).
WithSensitiveField("address", to).
Debug("Preparing verification code.")

address, err := s.deps.IdentityPool().FindVerifiableAddressByValue(ctx, via, to)
if err != nil {
if errors.Is(err, sqlcon.ErrNoRows) {
s.deps.Audit().
WithField("via", via).
WithSensitiveField("email_address", address).
Info("Sending out invalid verification via code email because address is unknown.")
if err := s.send(ctx, string(via), email.NewVerificationCodeInvalid(s.deps, &email.VerificationCodeInvalidModel{To: to})); err != nil {
return err
}
return errors.Cause(ErrUnknownAddress)
if errors.Is(err, sqlcon.ErrNoRows) {
notifyUnknownRecipients := s.deps.Config().SelfServiceFlowVerificationNotifyUnknownRecipients(ctx)
s.deps.Audit().
WithField("via", via).
WithField("strategy", "code").
WithSensitiveField("email_address", address).
WithField("was_notified", notifyUnknownRecipients).
Info("Address verification was requested for an unknown address.")
if !notifyUnknownRecipients {
// do nothing
} else if err := s.send(ctx, string(via), email.NewVerificationCodeInvalid(s.deps, &email.VerificationCodeInvalidModel{To: to})); err != nil {
return err
}
return errors.WithStack(ErrUnknownAddress)

} else if err != nil {
return err
}

Expand Down
83 changes: 69 additions & 14 deletions selfservice/strategy/code/code_sender_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,10 @@ import (
"encoding/base64"
"fmt"
"net/http"
"net/http/httptest"
"testing"
"time"

"github.com/ory/kratos/courier"
"github.com/ory/kratos/internal/testhelpers"

"github.com/stretchr/testify/assert"
Expand All @@ -38,14 +38,14 @@ func TestSender(t *testing.T) {
conf.MustSet(ctx, config.ViperKeyPublicBaseURL, "https://www.ory.sh/")
conf.MustSet(ctx, config.ViperKeyCourierSMTPURL, "smtp://foo@[email protected]/")
conf.MustSet(ctx, config.ViperKeyLinkBaseURL, "https://link-url/")
conf.MustSet(ctx, config.ViperKeySelfServiceRecoveryNotifyUnknownRecipients, true)
conf.MustSet(ctx, config.ViperKeySelfServiceVerificationNotifyUnknownRecipients, true)

u := &http.Request{URL: urlx.ParseOrPanic("https://www.ory.sh/")}

i := identity.NewIdentity(config.DefaultIdentityTraitsSchemaID)
i.Traits = identity.Traits(`{"email": "[email protected]"}`)
require.NoError(t, reg.IdentityManager().Create(context.Background(), i))

hr := httptest.NewRequest("GET", "https://www.ory.sh", nil)
require.NoError(t, reg.IdentityManager().Create(ctx, i))

t.Run("method=SendRecoveryCode", func(t *testing.T) {

Expand All @@ -54,15 +54,15 @@ func TestSender(t *testing.T) {
f, err := recovery.NewFlow(conf, time.Hour, "", u, code.NewStrategy(reg), flow.TypeBrowser)
require.NoError(t, err)

require.NoError(t, reg.RecoveryFlowPersister().CreateRecoveryFlow(context.Background(), f))
require.NoError(t, reg.RecoveryFlowPersister().CreateRecoveryFlow(ctx, f))

require.NoError(t, reg.CodeSender().SendRecoveryCode(context.Background(), hr, f, "email", "[email protected]"))
require.ErrorIs(t, reg.CodeSender().SendRecoveryCode(context.Background(), hr, f, "email", "[email protected]"), code.ErrUnknownAddress)
require.NoError(t, reg.CodeSender().SendRecoveryCode(ctx, f, "email", "[email protected]"))
require.ErrorIs(t, reg.CodeSender().SendRecoveryCode(ctx, f, "email", "[email protected]"), code.ErrUnknownAddress)
}

t.Run("case=with default templates", func(t *testing.T) {
recoveryCode(t)
messages, err := reg.CourierPersister().NextMessages(context.Background(), 12)
messages, err := reg.CourierPersister().NextMessages(ctx, 12)
require.NoError(t, err)
require.Len(t, messages, 2)

Expand All @@ -87,7 +87,7 @@ func TestSender(t *testing.T) {
conf.MustSet(ctx, config.ViperKeyCourierTemplatesRecoveryCodeInvalidEmail, fmt.Sprintf(`{ "subject": "base64://%s", "body": { "plaintext": "base64://%s", "html": "base64://%s" }}`, b64(subject+" invalid"), b64(body), b64(body)))
conf.MustSet(ctx, config.ViperKeyCourierTemplatesRecoveryCodeValidEmail, fmt.Sprintf(`{ "subject": "base64://%s", "body": { "plaintext": "base64://%s", "html": "base64://%s" }}`, b64(subject+" valid"), b64(body+" {{ .RecoveryCode }}"), b64(body+" {{ .RecoveryCode }}")))
recoveryCode(t)
messages, err := reg.CourierPersister().NextMessages(context.Background(), 12)
messages, err := reg.CourierPersister().NextMessages(ctx, 12)
require.NoError(t, err)
require.Len(t, messages, 2)

Expand All @@ -111,15 +111,15 @@ func TestSender(t *testing.T) {
f, err := verification.NewFlow(conf, time.Hour, "", u, code.NewStrategy(reg), flow.TypeBrowser)
require.NoError(t, err)

require.NoError(t, reg.VerificationFlowPersister().CreateVerificationFlow(context.Background(), f))
require.NoError(t, reg.VerificationFlowPersister().CreateVerificationFlow(ctx, f))

require.NoError(t, reg.CodeSender().SendVerificationCode(context.Background(), f, "email", "[email protected]"))
require.ErrorIs(t, reg.CodeSender().SendVerificationCode(context.Background(), f, "email", "[email protected]"), code.ErrUnknownAddress)
require.NoError(t, reg.CodeSender().SendVerificationCode(ctx, f, "email", "[email protected]"))
require.ErrorIs(t, reg.CodeSender().SendVerificationCode(ctx, f, "email", "[email protected]"), code.ErrUnknownAddress)
}

t.Run("case=with default templates", func(t *testing.T) {
verificationFlow(t)
messages, err := reg.CourierPersister().NextMessages(context.Background(), 12)
messages, err := reg.CourierPersister().NextMessages(ctx, 12)
require.NoError(t, err)
require.Len(t, messages, 2)

Expand All @@ -144,7 +144,7 @@ func TestSender(t *testing.T) {
conf.MustSet(ctx, config.ViperKeyCourierTemplatesVerificationCodeInvalidEmail, fmt.Sprintf(`{ "subject": "base64://%s", "body": { "plaintext": "base64://%s", "html": "base64://%s" }}`, b64(subject+" invalid"), b64(body), b64(body)))
conf.MustSet(ctx, config.ViperKeyCourierTemplatesVerificationCodeValidEmail, fmt.Sprintf(`{ "subject": "base64://%s", "body": { "plaintext": "base64://%s", "html": "base64://%s" }}`, b64(subject+" valid"), b64(body+" {{ .VerificationCode }}"), b64(body+" {{ .VerificationCode }}")))
verificationFlow(t)
messages, err := reg.CourierPersister().NextMessages(context.Background(), 12)
messages, err := reg.CourierPersister().NextMessages(ctx, 12)
require.NoError(t, err)
require.Len(t, messages, 2)

Expand All @@ -160,4 +160,59 @@ func TestSender(t *testing.T) {
})
})

t.Run("case=should be able to disable invalid email dispatch", func(t *testing.T) {
for _, tc := range []struct {
flow string
send func(t *testing.T)
configKey string
}{
{
flow: "recovery",
configKey: config.ViperKeySelfServiceRecoveryNotifyUnknownRecipients,
send: func(t *testing.T) {
s, err := reg.RecoveryStrategies(ctx).Strategy("code")
require.NoError(t, err)
f, err := recovery.NewFlow(conf, time.Hour, "", u, s, flow.TypeBrowser)
require.NoError(t, err)

require.NoError(t, reg.RecoveryFlowPersister().CreateRecoveryFlow(ctx, f))

err = reg.CodeSender().SendRecoveryCode(ctx, f, "email", "[email protected]")
require.ErrorIs(t, err, code.ErrUnknownAddress)
},
},
{
flow: "verification",
configKey: config.ViperKeySelfServiceVerificationNotifyUnknownRecipients,
send: func(t *testing.T) {
s, err := reg.VerificationStrategies(ctx).Strategy("code")
require.NoError(t, err)
f, err := verification.NewFlow(conf, time.Hour, "", u, s, flow.TypeBrowser)
require.NoError(t, err)

require.NoError(t, reg.VerificationFlowPersister().CreateVerificationFlow(ctx, f))

err = reg.CodeSender().SendVerificationCode(ctx, f, "email", "[email protected]")
require.ErrorIs(t, err, code.ErrUnknownAddress)
},
},
} {
t.Run("strategy="+tc.flow, func(t *testing.T) {

conf.Set(ctx, tc.configKey, false)

t.Cleanup(func() {
conf.Set(ctx, tc.configKey, true)
})

tc.send(t)

messages, err := reg.CourierPersister().NextMessages(ctx, 0)

require.ErrorIs(t, err, courier.ErrQueueEmpty)
require.Len(t, messages, 0)
})
}
})

}
2 changes: 1 addition & 1 deletion selfservice/strategy/code/strategy_recovery.go
Original file line number Diff line number Diff line change
Expand Up @@ -527,7 +527,7 @@ func (s *Strategy) recoveryHandleFormSubmission(w http.ResponseWriter, r *http.R
return s.HandleRecoveryError(w, r, f, body, err)
}

if err := s.deps.CodeSender().SendRecoveryCode(ctx, r, f, identity.VerifiableAddressTypeEmail, body.Email); err != nil {
if err := s.deps.CodeSender().SendRecoveryCode(ctx, f, identity.VerifiableAddressTypeEmail, body.Email); err != nil {
if !errors.Is(err, ErrUnknownAddress) {
return s.HandleRecoveryError(w, r, f, body, err)
}
Expand Down
6 changes: 6 additions & 0 deletions selfservice/strategy/code/strategy_recovery_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -596,6 +596,12 @@ func TestRecovery(t *testing.T) {
})

t.Run("description=should not be able to recover account that does not exist", func(t *testing.T) {
conf.Set(ctx, config.ViperKeySelfServiceRecoveryNotifyUnknownRecipients, true)

t.Cleanup(func() {
conf.Set(ctx, config.ViperKeySelfServiceRecoveryNotifyUnknownRecipients, false)
})

var check = func(t *testing.T, c *http.Client, flowType, email string) {
withValues := func(v url.Values) {
v.Set("email", email)
Expand Down
Loading

0 comments on commit 1a5ead4

Please sign in to comment.