diff --git a/actions.go b/actions.go
index 61f0abfbf40f3a3d5e68424e57b2dd3ff207b546..f805d7792bc0d3866d95f3822262d73c729ebd92 100644
--- a/actions.go
+++ b/actions.go
@@ -173,12 +173,23 @@ func (s *AccountService) RecoverPassword(ctx context.Context, tx TX, req *Passwo
if err != nil {
return err
}
- // TODO: authenticate with the secret recovery password.
+
+ // Authenticate the secret recovery password.
+ if !pwhash.ComparePassword(tx.GetUserRecoveryEncryptedPassword(ctx, user), req.RecoveryPassword) {
+ return ErrUnauthorized
+ }
+
ctx = context.WithValue(ctx, authUserCtxKey, req.Username)
return s.withRequest(ctx, tx, req, user, func(ctx context.Context) error {
+ s.audit.Log(ctx, ResourceID{}, "password reset via account recovery")
+
// Change the user password (the recovery password does not change).
- return s.changeUserPasswordAndUpdateEncryptionKeys(ctx, tx, user, req.RecoveryPassword, req.Password)
+ if err := s.changeUserPasswordAndUpdateEncryptionKeys(ctx, tx, user, req.RecoveryPassword, req.Password); err != nil {
+ return err
+ }
+ // Disable 2FA.
+ return s.disable2FA(ctx, tx, user)
})
}
diff --git a/actions_test.go b/actions_test.go
index c0423b31e5dcc0b2e9a14dd0799c1591da80b102..f9cf9245ae4cb85f43897edef67a0e132a11b678 100644
--- a/actions_test.go
+++ b/actions_test.go
@@ -5,6 +5,7 @@ import (
"errors"
"testing"
+ "git.autistici.org/ai3/go-common/pwhash"
sso "git.autistici.org/id/go-sso"
)
@@ -12,6 +13,7 @@ type fakeBackend struct {
users map[string]*User
resources map[string]map[string]*Resource
passwords map[string]string
+ recoveryPasswords map[string]string
appSpecificPasswords map[string][]*AppSpecificPasswordInfo
encryptionKeys map[string][]*UserEncryptionKey
}
@@ -62,9 +64,19 @@ func (b *fakeBackend) SetUserPassword(_ context.Context, user *User, password st
}
func (b *fakeBackend) SetPasswordRecoveryHint(_ context.Context, user *User, hint, response string) error {
+ b.users[user.Name].PasswordRecoveryHint = hint
+ b.recoveryPasswords[user.Name] = response
return nil
}
+func (b *fakeBackend) GetUserEncryptedPassword(_ context.Context, user *User) string {
+ return b.passwords[user.Name]
+}
+
+func (b *fakeBackend) GetUserRecoveryEncryptedPassword(_ context.Context, user *User) string {
+ return b.recoveryPasswords[user.Name]
+}
+
func (b *fakeBackend) SetResourcePassword(_ context.Context, r *Resource, password string) error {
b.passwords[r.ID.String()] = password
return nil
@@ -138,9 +150,13 @@ func (v *fakeValidator) Validate(tkt, nonce, service string, _ []string) (*sso.T
}, nil
}
-func (b *fakeBackend) addUser(user *User) {
+func (b *fakeBackend) addUser(user *User, pw, rpw string) {
b.users[user.Name] = user
b.resources[user.Name] = make(map[string]*Resource)
+ b.passwords[user.Name] = pwhash.Encrypt(pw)
+ if rpw != "" {
+ b.recoveryPasswords[user.Name] = pwhash.Encrypt(rpw)
+ }
for _, r := range user.Resources {
b.resources[user.Name][r.ID.String()] = r
}
@@ -154,6 +170,7 @@ func createFakeBackend() *fakeBackend {
"": make(map[string]*Resource),
},
passwords: make(map[string]string),
+ recoveryPasswords: make(map[string]string),
appSpecificPasswords: make(map[string][]*AppSpecificPasswordInfo),
encryptionKeys: make(map[string][]*UserEncryptionKey),
}
@@ -177,7 +194,7 @@ func createFakeBackend() *fakeBackend {
},
},
},
- })
+ }, "password", "recoverypassword")
return fb
}
@@ -267,6 +284,9 @@ func TestService_ChangePassword(t *testing.T) {
newPassword string
expectedOk bool
}{
+ // First, fail cur_password authentication.
+ {"BADPASS", "new_password", false},
+
// Ordering is important as it is meant to emulate
// setting the password, failing to reset it, then
// succeeding.
@@ -524,3 +544,27 @@ func TestService_ResetResourcePassword(t *testing.T) {
t.Fatal("oops, it appears that the password was stored in cleartext on the backend")
}
}
+
+func TestService_Recovery(t *testing.T) {
+ svc, tx := testService("")
+
+ // Bad recovery response.
+ err := svc.RecoverPassword(context.Background(), tx, &PasswordRecoveryRequest{
+ Username: "testuser",
+ RecoveryPassword: "BADPASS",
+ Password: "new_password",
+ })
+ if err == nil {
+ t.Fatal("oops, recovered account with bad password")
+ }
+
+ // Successful account recovery.
+ err = svc.RecoverPassword(context.Background(), tx, &PasswordRecoveryRequest{
+ Username: "testuser",
+ RecoveryPassword: "recoverypassword",
+ Password: "new_password",
+ })
+ if err != nil {
+ t.Fatalf("RecoverPassword failed: %v", err)
+ }
+}
diff --git a/backend/model.go b/backend/model.go
index aa7590b19eb3694d7f30e9bfef7d8c0d27ac5d6e..9a9fa95b8d13a5391523fb27e0bb95733e87ef38 100644
--- a/backend/model.go
+++ b/backend/model.go
@@ -258,6 +258,24 @@ func (tx *backendTX) SetUserPassword(ctx context.Context, user *accountserver.Us
return nil
}
+func (tx *backendTX) GetUserEncryptedPassword(ctx context.Context, user *accountserver.User) string {
+ values := tx.readAttributeValues(ctx, tx.getUserDN(user), passwordLDAPAttr)
+ if len(values) > 0 {
+ // Remove legacy LDAP encryption prefix.
+ return strings.TrimPrefix(values[0], "{crypt}")
+ }
+ return ""
+}
+
+func (tx *backendTX) GetUserRecoveryEncryptedPassword(ctx context.Context, user *accountserver.User) string {
+ values := tx.readAttributeValues(ctx, tx.getUserDN(user), recoveryResponseLDAPAttr)
+ if len(values) > 0 {
+ // Remove legacy LDAP encryption prefix.
+ return strings.TrimPrefix(values[0], "{crypt}")
+ }
+ return ""
+}
+
func (tx *backendTX) SetPasswordRecoveryHint(ctx context.Context, user *accountserver.User, hint, response string) error {
dn := tx.getUserDN(user)
tx.setAttr(dn, recoveryHintLDAPAttr, hint)
diff --git a/errors.go b/errors.go
index 3f7788eef4821b30d7d24ea7c0e577451463fce1..50a7c2299112c3f332fb056cce648846c7ddad0f 100644
--- a/errors.go
+++ b/errors.go
@@ -4,7 +4,7 @@ import "errors"
var (
// ErrUnauthorized means that the request failed due to lack of authorization.
- ErrUnauthorized = errors.New("unauthorized")
+ ErrUnauthorized = newAuthError(errors.New("unauthorized"))
// ErrUserNotFound is returned when a user object is not found.
ErrUserNotFound = errors.New("user not found")
diff --git a/integrationtest/integration_test.go b/integrationtest/integration_test.go
index 039b7d3890bd32f20a779b19d95f467cb0ec7952..0b00418f6a41de88ad258d285006c157cfa8e4cd 100644
--- a/integrationtest/integration_test.go
+++ b/integrationtest/integration_test.go
@@ -198,11 +198,7 @@ func TestIntegration_ChangeUserPassword(t *testing.T) {
Username: "uno@investici.org",
SSO: c.ssoTicket("uno@investici.org"),
},
- // Since the user in the test fixture does
- // *not* have encryption key, and
- // authenticateWithPassword is currently a
- // no-op, this test will pass.
- CurPassword: "ha ha, really not the current password",
+ CurPassword: "password",
},
Password: "new_password",
}, nil)
diff --git a/integrationtest/testdata/test1.ldif b/integrationtest/testdata/test1.ldif
index 9bf6880683b247b165f7b48ccd62866470c24af8..b2a5140a188774340156146effda8eedb7c1236f 100644
--- a/integrationtest/testdata/test1.ldif
+++ b/integrationtest/testdata/test1.ldif
@@ -18,9 +18,8 @@ givenName: Private
shadowLastChange: 12345
shadowWarning: 7
preferredLanguage: it
-userPassword:: e2NyeXB0fSQ2JG1wVzdTZHZROG52OFJabE8kcFNqbm1hLi9CZDNIWU1hL29sMGZ
- FRmZrS3pWRjAxUkkzMnpISEoxNnc2V2xaajFuSFVzMmd4ZXZIVDdoemdELnJ5Lk1BZWM3REptZVZ5
- WEtkeDV0QTE=
+userPassword:: e2NyeXB0fSQ2JG1wVzdTZHZROG52OFJabE8kSTRmQldkVUpFeVVsb0dpdVpnYm8yNTlVVFpJMi9y
+ Vk5QODdZU04zVE0ud2F5MmR2UndYNmE0NHVVV2drWC9ac25HOGF3R0RYWFRmMDVNVXhNbGlnSDA=
dn: mail=uno@investici.org,uid=uno@investici.org,ou=People,dc=example,dc=com
status: active
diff --git a/service.go b/service.go
index 3c82d693b3164f8ecb3a9bda09edfa7e6dfd8170..4f2c06d04db91a9ee07adcaa1318b4ed5f440aae 100644
--- a/service.go
+++ b/service.go
@@ -6,6 +6,7 @@ import (
"log"
"reflect"
+ "git.autistici.org/ai3/go-common/pwhash"
"git.autistici.org/id/go-sso"
)
@@ -54,6 +55,13 @@ type TX interface {
DeleteApplicationSpecificPassword(context.Context, *User, string) error
SetUserTOTPSecret(context.Context, *User, string) error
DeleteUserTOTPSecret(context.Context, *User) error
+
+ // The Get*EncryptedPassword methods allow us to perform
+ // authentication within the accountserver, skipping one round
+ // trip to the auth-server, given that we already have the
+ // necessary information.
+ GetUserEncryptedPassword(context.Context, *User) string
+ GetUserRecoveryEncryptedPassword(context.Context, *User) string
}
// FindResourceRequest contains parameters for searching a resource by name.
@@ -177,7 +185,7 @@ func (s *authService) authorizeAdminGeneric(ctx context.Context, tx TX, ssoTicke
// Requests are allowed if the SSO ticket corresponds to an admin, or if
// it identifies the same user that we're querying.
if !s.isAdmin(tkt) {
- return nil, newAuthError(ErrUnauthorized)
+ return nil, ErrUnauthorized
}
ctx = context.WithValue(ctx, authUserCtxKey, tkt.User)
@@ -194,7 +202,7 @@ func (s *authService) authorizeAdmin(ctx context.Context, tx TX, req RequestBase
// Requests are allowed if the SSO ticket corresponds to an admin, or if
// it identifies the same user that we're querying.
if !s.isAdmin(tkt) {
- return nil, nil, newAuthError(ErrUnauthorized)
+ return nil, nil, ErrUnauthorized
}
ctx = context.WithValue(ctx, authUserCtxKey, tkt.User)
@@ -213,7 +221,7 @@ func (s *authService) authorizeUser(ctx context.Context, tx TX, req RequestBase)
// Requests are allowed if the SSO ticket corresponds to an admin, or if
// it identifies the same user that we're querying.
if !s.isAdmin(tkt) && tkt.User != req.Username {
- return nil, nil, newAuthError(ErrUnauthorized)
+ return nil, nil, ErrUnauthorized
}
user, err := getUserOrDie(ctx, tx, req.Username)
@@ -225,8 +233,14 @@ func (s *authService) authorizeUser(ctx context.Context, tx TX, req RequestBase)
// user password. Used for account-privileged operations related to
// credential manipulation.
func (s *authService) authorizeUserWithPassword(ctx context.Context, tx TX, req PrivilegedRequestBase) (context.Context, *User, error) {
- // TODO: call out to the auth-server?
- return s.authorizeUser(ctx, tx, req.RequestBase)
+ rctx, user, err := s.authorizeUser(ctx, tx, req.RequestBase)
+ if err != nil {
+ return nil, nil, err
+ }
+ if !pwhash.ComparePassword(tx.GetUserEncryptedPassword(ctx, user), req.CurPassword) {
+ return nil, nil, ErrUnauthorized
+ }
+ return rctx, user, nil
}
// Extended version of authorizeUser that checks access to a specific
@@ -249,7 +263,7 @@ func (s *authService) authorizeResource(ctx context.Context, tx TX, req Resource
}
if !s.isAdmin(tkt) && !canAccessResource(tkt.User, r) {
- return nil, nil, newAuthError(ErrUnauthorized)
+ return nil, nil, ErrUnauthorized
}
ctx = context.WithValue(ctx, authUserCtxKey, tkt.User)