Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: remove duplicate queries during settings flow and use better index hint for credentials lookup #4193

Merged
merged 23 commits into from
Nov 7, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions cmd/clidoc/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -309,6 +309,8 @@ func validateAllMessages(path string) error {
info := &types.Info{
Defs: make(map[*ast.Ident]types.Object),
}

//nolint:staticcheck
var pack *ast.Package
for _, p := range packs {
if p.Name == "text" {
Expand Down
1 change: 1 addition & 0 deletions cmd/hashers/argon2/calibrate.go
Original file line number Diff line number Diff line change
Expand Up @@ -246,6 +246,7 @@ Please note that the values depend on the machine you run the hashing on. If you
case res.MaxMem > conf.localConfig.DedicatedMemory:
_, _ = progressPrinter.Printf("The required memory was %s more than the maximum allowed of %s.\n", res.MaxMem-maxMemory, conf.localConfig.DedicatedMemory)

//nolint:gosec // disable G115
conf.localConfig.Memory -= (res.MaxMem - conf.localConfig.DedicatedMemory) / bytesize.ByteSize(reqPerMin)
_, _ = progressPrinter.Printf("Decreasing memory to %s\n", conf.localConfig.Memory)
// too slow
Expand Down
1 change: 1 addition & 0 deletions courier/courier_dispatcher.go
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ func (c *courier) DispatchQueue(ctx context.Context) error {
maxRetries := c.deps.CourierConfig().CourierMessageRetries(ctx)
pullCount := c.deps.CourierConfig().CourierWorkerPullCount(ctx)

//nolint:gosec // disable G115
messages, err := c.deps.CourierPersister().NextMessages(ctx, uint8(pullCount))
if err != nil {
if errors.Is(err, ErrQueueEmpty) {
Expand Down
12 changes: 8 additions & 4 deletions driver/config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -550,10 +550,14 @@ func (p *Config) HasherArgon2(ctx context.Context) *Argon2 {
// warn about usage of default values and point to the docs
// warning will require https://github.com/ory/viper/issues/19
return &Argon2{
Memory: p.GetProvider(ctx).ByteSizeF(ViperKeyHasherArgon2ConfigMemory, Argon2DefaultMemory),
Iterations: uint32(p.GetProvider(ctx).IntF(ViperKeyHasherArgon2ConfigIterations, int(Argon2DefaultIterations))),
Parallelism: uint8(p.GetProvider(ctx).IntF(ViperKeyHasherArgon2ConfigParallelism, int(Argon2DefaultParallelism))),
SaltLength: uint32(p.GetProvider(ctx).IntF(ViperKeyHasherArgon2ConfigSaltLength, int(Argon2DefaultSaltLength))),
Memory: p.GetProvider(ctx).ByteSizeF(ViperKeyHasherArgon2ConfigMemory, Argon2DefaultMemory),
//nolint:gosec // disable G115
Iterations: uint32(p.GetProvider(ctx).IntF(ViperKeyHasherArgon2ConfigIterations, int(Argon2DefaultIterations))),
//nolint:gosec // disable G115
Parallelism: uint8(p.GetProvider(ctx).IntF(ViperKeyHasherArgon2ConfigParallelism, int(Argon2DefaultParallelism))),
//nolint:gosec // disable G115
SaltLength: uint32(p.GetProvider(ctx).IntF(ViperKeyHasherArgon2ConfigSaltLength, int(Argon2DefaultSaltLength))),
//nolint:gosec // disable G115
KeyLength: uint32(p.GetProvider(ctx).IntF(ViperKeyHasherArgon2ConfigKeyLength, int(Argon2DefaultKeyLength))),
ExpectedDuration: p.GetProvider(ctx).DurationF(ViperKeyHasherArgon2ConfigExpectedDuration, Argon2DefaultDuration),
ExpectedDeviation: p.GetProvider(ctx).DurationF(ViperKeyHasherArgon2ConfigExpectedDeviation, Argon2DefaultDeviation),
Expand Down
3 changes: 2 additions & 1 deletion hash/hash_comparator.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ import (

//nolint:staticcheck
//lint:ignore SA1019 compatibility for imported passwords
"golang.org/x/crypto/md4" //#nosec G501 -- compatibility for imported passwords
"golang.org/x/crypto/md4" //nolint:gosec // disable G115 G501 -- compatibility for imported passwords
"golang.org/x/crypto/pbkdf2"
"golang.org/x/crypto/scrypt"

Expand Down Expand Up @@ -159,6 +159,7 @@ func CompareArgon2id(_ context.Context, password []byte, hash []byte) error {
}

// Derive the key from the other password using the same parameters.
//nolint:gosec // disable G115
otherHash := argon2.IDKey(password, salt, p.Iterations, uint32(p.Memory), p.Parallelism, p.KeyLength)

return comparePasswordHashConstantTime(hash, otherHash)
Expand Down
1 change: 1 addition & 0 deletions hash/hasher_argon2.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ func NewHasherArgon2(c Argon2Configuration) *Argon2 {
}

func toKB(mem bytesize.ByteSize) uint32 {
//nolint:gosec // disable G115
return uint32(mem / bytesize.KB)
}

Expand Down
2 changes: 1 addition & 1 deletion identity/manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -370,7 +370,7 @@ func (e *CreateIdentitiesError) Find(ident *Identity) *FailedIdentity {
return nil
}
func (e *CreateIdentitiesError) ErrOrNil() error {
if e.failedIdentities == nil || len(e.failedIdentities) == 0 {
if len(e.failedIdentities) == 0 {
return nil
}
return e
Expand Down
1 change: 1 addition & 0 deletions internal/client-go/go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ github.com/golang/protobuf v1.2.0/go.mod h1:6lQm79b+lXiMfvg/cZm0SGofjICqVBUtrP5y
golang.org/x/net v0.0.0-20180724234803-3673e40ba225/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4=
golang.org/x/net v0.0.0-20190108225652-1e06a53dbb7e h1:bRhVy7zSSasaqNksaRZiA5EEI+Ei4I1nO5Jh72wfHlg=
golang.org/x/net v0.0.0-20190108225652-1e06a53dbb7e/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4=
golang.org/x/oauth2 v0.0.0-20190604053449-0f29369cfe45/go.mod h1:gOpvHmFTYa4IltrdGE7lF6nIHvwfUNPOp7c8zoXwtLw=
golang.org/x/sync v0.0.0-20181221193216-37e7f081c4d4 h1:YUO/7uOKsKeq9UokNS62b8FYywz3ker1l1vDZRCRefw=
golang.org/x/sync v0.0.0-20181221193216-37e7f081c4d4/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM=
golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ=
Expand Down
18 changes: 8 additions & 10 deletions persistence/sql/identity/persister_identity.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,6 @@ import (
"sync"
"time"

"github.com/ory/kratos/x/events"

"github.com/ory/x/crdbx"

"github.com/gobuffalo/pop/v6"
"github.com/gofrs/uuid"
"github.com/pkg/errors"
Expand All @@ -33,7 +29,9 @@ import (
"github.com/ory/kratos/persistence/sql/update"
"github.com/ory/kratos/schema"
"github.com/ory/kratos/x"
"github.com/ory/kratos/x/events"
"github.com/ory/x/contextx"
"github.com/ory/x/crdbx"
"github.com/ory/x/errorsx"
"github.com/ory/x/otelx"
"github.com/ory/x/pagination/keysetpagination"
Expand Down Expand Up @@ -806,11 +804,11 @@ func identifiersTableNameWithIndexHint(con *pop.Connection) string {
ici := "identity_credential_identifiers"
switch con.Dialect.Name() {
case "cockroach":
ici += "@identity_credential_identifiers_nid_i_ici_idx"
ici += "@identity_credential_identifiers_ici_nid_i_idx"
case "sqlite3":
ici += " INDEXED BY identity_credential_identifiers_nid_i_ici_idx"
ici += " INDEXED BY identity_credential_identifiers_ici_nid_i_idx"
case "mysql":
ici += " USE INDEX(identity_credential_identifiers_nid_i_ici_idx)"
ici += " USE INDEX(identity_credential_identifiers_ici_nid_i_idx)"
default:
// good luck 🤷‍♂️
}
Expand Down Expand Up @@ -932,7 +930,7 @@ func (p *IdentityPersister) ListIdentities(ctx context.Context, params identity.
)
}

if params.IdsFilter != nil && len(params.IdsFilter) != 0 {
if len(params.IdsFilter) > 0 {
wheres += `
AND identities.id in (?)
`
Expand Down Expand Up @@ -987,15 +985,15 @@ func (p *IdentityPersister) ListIdentities(ctx context.Context, params identity.
}
case identity.ExpandFieldVerifiableAddresses:
addrs := make([]identity.VerifiableAddress, 0)
if err := con.Where("nid = ?", nid).Where("identity_id IN (?)", identityIDs).Order("id").All(&addrs); err != nil {
if err := con.Where("identity_id IN (?)", identityIDs).Where("nid = ?", nid).Order("id").All(&addrs); err != nil {
return sqlcon.HandleError(err)
}
for _, addr := range addrs {
identitiesByID[addr.IdentityID].VerifiableAddresses = append(identitiesByID[addr.IdentityID].VerifiableAddresses, addr)
}
case identity.ExpandFieldRecoveryAddresses:
addrs := make([]identity.RecoveryAddress, 0)
if err := con.Where("nid = ?", nid).Where("identity_id IN (?)", identityIDs).Order("id").All(&addrs); err != nil {
if err := con.Where("identity_id IN (?)", identityIDs).Where("nid = ?", nid).Order("id").All(&addrs); err != nil {
return sqlcon.HandleError(err)
}
for _, addr := range addrs {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
DROP INDEX IF EXISTS identity_credential_identifiers_nid_ici_i_idx;
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
CREATE INDEX IF NOT EXISTS identity_credential_identifiers_nid_ici_i_idx
ON identity_credential_identifiers (nid ASC, identity_credential_id ASC, identifier ASC);
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
DROP INDEX identity_credential_identifiers_nid_ici_i_idx ON identity_credential_identifiers;
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
CREATE INDEX identity_credential_identifiers_nid_ici_i_idx
ON identity_credential_identifiers (nid ASC, identity_credential_id ASC, identifier ASC);
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
CREATE INDEX IF NOT EXISTS identity_credential_identifiers_identity_credential_id_idx
ON identity_credential_identifiers (identity_credential_id ASC);

DROP INDEX IF EXISTS identity_credential_identifiers_ici_nid_i_idx;
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
CREATE INDEX IF NOT EXISTS identity_credential_identifiers_ici_nid_i_idx
ON identity_credential_identifiers (identity_credential_id ASC, nid ASC, identifier ASC);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alnr i changed the index structure to use identity_credential_id as the prefix (it is also the highest cardinality) and then we can use it both for FK and the query credentials


DROP INDEX IF EXISTS identity_credential_identifiers_identity_credential_id_idx;
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
CREATE INDEX identity_credential_identifiers_identity_credential_id_idx
ON identity_credential_identifiers (identity_credential_id ASC);

DROP INDEX identity_credential_identifiers_ici_nid_i_idx ON identity_credential_identifiers;
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
CREATE INDEX identity_credential_identifiers_ici_nid_i_idx
ON identity_credential_identifiers (identity_credential_id ASC, nid ASC, identifier ASC);

DROP INDEX identity_credential_identifiers_identity_credential_id_idx ON identity_credential_identifiers;
2 changes: 1 addition & 1 deletion selfservice/flow/error.go
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ func NewFlowReplacedError(message *text.Message) *ReplacedError {
return &ReplacedError{
DefaultError: x.ErrGone.WithID(text.ErrIDSelfServiceFlowReplaced).
WithError("self-service flow replaced").
WithReasonf(message.Text),
WithReason(message.Text),
}
}

Expand Down
Loading
Loading