Improve separation of roles between backend and API on secrets management

actions.go

@@ -269,16 +269,11 @@ func (s *AccountService) ChangeUserPassword(ctx context.Context, tx TX, req *Cha
 		return err
 	}
 
-	// Set the encrypted password attribute on the user and email resources.
+	// Set the encrypted password attribute on the user (will set it on emails too).
 	encPass := pwhash.Encrypt(req.Password)
 	if err := tx.SetUserPassword(ctx, user, encPass); err != nil {
 		return newBackendError(err)
 	}
-	for _, r := range user.GetResourcesByType(ResourceTypeEmail) {
-		if err := tx.SetResourcePassword(ctx, r, encPass); err != nil {
-			return newBackendError(err)
-		}
-	}
 
 	return nil
 }

backend/model.go

@@ -12,6 +12,17 @@ import (
 	"git.autistici.org/ai3/accountserver"
 )
 
+const (
+	// Names of some well-known LDAP attributes.
+	totpSecretLDAPAttr        = "totpSecret"
+	preferredLanguageLDAPAttr = "preferredLanguage"
+	recoverQuestionLDAPAttr   = "recoverQuestion"
+	aspLDAPAttr               = "appSpecificPassword"
+	storagePublicKeyLDAPAttr  = "storagePublicKey"
+	storagePrivateKeyLDAPAttr = "storageEncryptedSecretKey"
+	passwordLDAPAttr          = "userPassword"
+)
+
 // Generic interface to LDAP - allows us to stub out the LDAP client while
 // testing.
 type ldapConn interface {
@@ -175,10 +186,10 @@ func s2l(s string) []string {
 
 func newUser(entry *ldap.Entry) (*accountserver.User, error) {
 	user := &accountserver.User{
-		Name:              entry.GetAttributeValue("uid"),
-		Lang:              entry.GetAttributeValue("preferredLanguage"),
-		Has2FA:            (entry.GetAttributeValue("totpSecret") != ""),
-		HasEncryptionKeys: (len(entry.GetAttributeValues("storageEncryptionKey")) > 0),
+		Name:   entry.GetAttributeValue("uid"),
+		Lang:   entry.GetAttributeValue(preferredLanguageLDAPAttr),
+		Has2FA: (entry.GetAttributeValue(totpSecretLDAPAttr) != ""),
+		//HasEncryptionKeys: (len(entry.GetAttributeValues("storageEncryptionKey")) > 0),
 		//PasswordRecoveryHint: entry.GetAttributeValue("recoverQuestion"),
 	}
 	if user.Lang == "" {
@@ -223,8 +234,9 @@ func (tx *backendTX) GetUser(ctx context.Context, username string) (*accountserv
 			// object, a shortcoming of the legacy A/I database model. Set
 			// them on the main User object.
 			if isObjectClass(entry, "virtualMailUser") {
-				user.PasswordRecoveryHint = entry.GetAttributeValue("recoverQuestion")
-				user.AppSpecificPasswords = getASPInfo(decodeAppSpecificPasswords(entry.GetAttributeValues("appSpecificPassword")))
+				user.PasswordRecoveryHint = entry.GetAttributeValue(recoverQuestionLDAPAttr)
+				user.AppSpecificPasswords = getASPInfo(decodeAppSpecificPasswords(entry.GetAttributeValues(aspLDAPAttr)))
+				user.HasEncryptionKeys = (entry.GetAttributeValue(storagePublicKeyLDAPAttr) != "")
 			}
 
 			// Parse the resource and add it to the User.
@@ -239,50 +251,37 @@ func (tx *backendTX) GetUser(ctx context.Context, username string) (*accountserv
 	return user, nil
 }
 
-func singleAttributeQuery(dn, attribute string) *ldap.SearchRequest {
-	return ldap.NewSearchRequest(
-		dn,
-		ldap.ScopeBaseObject,
-		ldap.NeverDerefAliases,
-		0,
-		0,
-		false,
-		"(objectClass=*)",
-		[]string{attribute},
-		nil,
-	)
-}
-
-func (tx *backendTX) readAttributeValues(ctx context.Context, dn, attribute string) []string {
-	req := singleAttributeQuery(dn, attribute)
-	result, err := tx.search(ctx, req)
-	if err != nil {
-		return nil
-	}
-	if len(result.Entries) < 1 {
-		return nil
-	}
-	return result.Entries[0].GetAttributeValues(attribute)
-}
-
 func (tx *backendTX) SetUserPassword(ctx context.Context, user *accountserver.User, encryptedPassword string) error {
-	tx.setAttr(tx.getUserDN(user), "userPassword", encryptedPassword)
+	dn := tx.getUserDN(user)
+	tx.setAttr(dn, passwordLDAPAttr, encryptedPassword)
+	for _, r := range user.GetResourcesByType(accountserver.ResourceTypeEmail) {
+		dn, _ = tx.backend.resources.GetDN(r.ID)
+		tx.setAttr(dn, passwordLDAPAttr, encryptedPassword)
+	}
 	return nil
 }
 
 func (tx *backendTX) GetUserEncryptionKeys(ctx context.Context, user *accountserver.User) ([]*accountserver.UserEncryptionKey, error) {
-	rawKeys := tx.readAttributeValues(ctx, tx.getUserDN(user), "storageEncryptionKey")
+	r := user.GetSingleResourceByType(accountserver.ResourceTypeEmail)
+	dn, _ := tx.backend.resources.GetDN(r.ID)
+	rawKeys := tx.readAttributeValues(ctx, dn, storagePrivateKeyLDAPAttr)
 	return decodeUserEncryptionKeys(rawKeys), nil
 }
 
 func (tx *backendTX) SetUserEncryptionKeys(ctx context.Context, user *accountserver.User, keys []*accountserver.UserEncryptionKey) error {
 	encKeys := encodeUserEncryptionKeys(keys)
-	tx.setAttr(tx.getUserDN(user), "storageEncryptionKey", encKeys...)
+	for _, r := range user.GetResourcesByType(accountserver.ResourceTypeEmail) {
+		dn, _ := tx.backend.resources.GetDN(r.ID)
+		tx.setAttr(dn, storagePrivateKeyLDAPAttr, encKeys...)
+	}
 	return nil
 }
 
 func (tx *backendTX) SetUserEncryptionPublicKey(ctx context.Context, user *accountserver.User, pub []byte) error {
-	tx.setAttr(tx.getUserDN(user), "storageEncryptionPublicKey", string(pub))
+	for _, r := range user.GetResourcesByType(accountserver.ResourceTypeEmail) {
+		dn, _ := tx.backend.resources.GetDN(r.ID)
+		tx.setAttr(dn, storagePublicKeyLDAPAttr, string(pub))
+	}
 	return nil
 }
 
@@ -296,50 +295,51 @@ func excludeASPFromList(asps []*appSpecificPassword, id string) []*appSpecificPa
 	return out
 }
 
-func (tx *backendTX) SetApplicationSpecificPassword(ctx context.Context, user *accountserver.User, info *accountserver.AppSpecificPasswordInfo, encryptedPassword string) error {
-	emailRsrc := user.GetSingleResourceByType(accountserver.ResourceTypeEmail)
-	if emailRsrc == nil {
-		return errors.New("no email resource")
-	}
-	emailDN, _ := tx.backend.resources.GetDN(emailRsrc.ID)
+func (tx *backendTX) setASPOnResource(ctx context.Context, r *accountserver.Resource, info *accountserver.AppSpecificPasswordInfo, encryptedPassword string) {
+	dn, _ := tx.backend.resources.GetDN(r.ID)
 
 	// Obtain the full list of ASPs from the backend and replace/append the new one.
-	asps := decodeAppSpecificPasswords(tx.readAttributeValues(ctx, emailDN, "appSpecificPassword"))
+	asps := decodeAppSpecificPasswords(tx.readAttributeValues(ctx, dn, aspLDAPAttr))
 	asps = append(excludeASPFromList(asps, info.ID), newAppSpecificPassword(*info, encryptedPassword))
 	outASPs := encodeAppSpecificPasswords(asps)
-
-	tx.setAttr(emailDN, "appSpecificPassword", outASPs...)
-	return nil
+	tx.setAttr(dn, aspLDAPAttr, outASPs...)
 }
 
-func (tx *backendTX) DeleteApplicationSpecificPassword(ctx context.Context, user *accountserver.User, id string) error {
-	emailRsrc := user.GetSingleResourceByType(accountserver.ResourceTypeEmail)
-	if emailRsrc == nil {
-		return errors.New("no email resource")
+func (tx *backendTX) SetApplicationSpecificPassword(ctx context.Context, user *accountserver.User, info *accountserver.AppSpecificPasswordInfo, encryptedPassword string) error {
+	for _, r := range user.GetResourcesByType(accountserver.ResourceTypeEmail) {
+		tx.setASPOnResource(ctx, r, info, encryptedPassword)
 	}
-	emailDN, _ := tx.backend.resources.GetDN(emailRsrc.ID)
+	return nil
+}
 
-	asps := decodeAppSpecificPasswords(tx.readAttributeValues(ctx, emailDN, "appSpecificPassword"))
+func (tx *backendTX) deleteASPOnResource(ctx context.Context, r *accountserver.Resource, id string) {
+	dn, _ := tx.backend.resources.GetDN(r.ID)
+	asps := decodeAppSpecificPasswords(tx.readAttributeValues(ctx, dn, aspLDAPAttr))
 	asps = excludeASPFromList(asps, id)
 	outASPs := encodeAppSpecificPasswords(asps)
+	tx.setAttr(dn, aspLDAPAttr, outASPs...)
+}
 
-	tx.setAttr(emailDN, "appSpecificPassword", outASPs...)
+func (tx *backendTX) DeleteApplicationSpecificPassword(ctx context.Context, user *accountserver.User, id string) error {
+	for _, r := range user.GetResourcesByType(accountserver.ResourceTypeEmail) {
+		tx.deleteASPOnResource(ctx, r, id)
+	}
 	return nil
 }
 
 func (tx *backendTX) SetUserTOTPSecret(ctx context.Context, user *accountserver.User, secret string) error {
-	tx.setAttr(tx.getUserDN(user), "totpSecret", secret)
+	tx.setAttr(tx.getUserDN(user), totpSecretLDAPAttr, secret)
 	return nil
 }
 
 func (tx *backendTX) DeleteUserTOTPSecret(ctx context.Context, user *accountserver.User) error {
-	tx.setAttr(tx.getUserDN(user), "totpSecret")
+	tx.setAttr(tx.getUserDN(user), totpSecretLDAPAttr)
 	return nil
 }
 
 func (tx *backendTX) SetResourcePassword(ctx context.Context, r *accountserver.Resource, encryptedPassword string) error {
 	dn, _ := tx.backend.resources.GetDN(r.ID)
-	tx.setAttr(dn, "userPassword", encryptedPassword)
+	tx.setAttr(dn, passwordLDAPAttr, encryptedPassword)
 	return nil
 }
 

backend/model_test.go

@@ -14,14 +14,27 @@ const (
 	testLDAPPort = 42871
 	testLDAPAddr = "ldap://127.0.0.1:42871"
 	testUser1    = "uno@investici.org"
+	testUser2    = "due@investici.org"
 )
 
 func startServerAndGetUser(t testing.TB) (func(), accountserver.Backend, *accountserver.User) {
+	return startServerAndGetUserWithName(t, testUser1)
+}
+
+func startServerAndGetUser2(t testing.TB) (func(), accountserver.Backend, *accountserver.User) {
+	return startServerAndGetUserWithName(t, testUser2)
+}
+
+func startServerAndGetUserWithName(t testing.TB, username string) (func(), accountserver.Backend, *accountserver.User) {
 	stop := ldaptest.StartServer(t, &ldaptest.Config{
-		Dir:   "../ldaptest",
-		Port:  testLDAPPort,
-		Base:  "dc=example,dc=com",
-		LDIFs: []string{"testdata/base.ldif", "testdata/test1.ldif"},
+		Dir:  "../ldaptest",
+		Port: testLDAPPort,
+		Base: "dc=example,dc=com",
+		LDIFs: []string{
+			"testdata/base.ldif",
+			"testdata/test1.ldif",
+			"testdata/test2.ldif",
+		},
 	})
 
 	b, err := NewLDAPBackend(testLDAPAddr, "cn=manager,dc=example,dc=com", "password", "dc=example,dc=com")
@@ -30,12 +43,12 @@ func startServerAndGetUser(t testing.TB) (func(), accountserver.Backend, *accoun
 	}
 
 	tx, _ := b.NewTransaction()
-	user, err := tx.GetUser(context.Background(), testUser1)
+	user, err := tx.GetUser(context.Background(), username)
 	if err != nil {
 		t.Fatal("GetUser", err)
 	}
 	if user == nil {
-		t.Fatalf("could not find test user %s", testUser1)
+		t.Fatalf("could not find test user %s", username)
 	}
 
 	return stop, b, user
@@ -81,6 +94,18 @@ func TestModel_GetUser(t *testing.T) {
 	}
 }
 
+func TestModel_GetUser_Has2FA(t *testing.T) {
+	stop, _, user := startServerAndGetUser2(t)
+	defer stop()
+
+	if !user.Has2FA {
+		t.Errorf("user %s does not appear to have 2FA enabled", testUser2)
+	}
+	if !user.HasEncryptionKeys {
+		t.Errorf("user %s does not appear to have encryption keys", testUser2)
+	}
+}
+
 func TestModel_GetUser_Group(t *testing.T) {
 	stop, _, user := startServerAndGetUser(t)
 	defer stop()
@@ -206,3 +231,72 @@ func TestModel_HasAnyResource(t *testing.T) {
 		t.Fatal("oops, found non existing resource")
 	}
 }
+
+func TestModel_SetUserPassword(t *testing.T) {
+	stop, b, user := startServerAndGetUser(t)
+	defer stop()
+
+	encPass := "encrypted password"
+
+	tx, _ := b.NewTransaction()
+	if err := tx.SetUserPassword(context.Background(), user, encPass); err != nil {
+		t.Fatal("SetUserPassword", err)
+	}
+	if err := tx.Commit(context.Background()); err != nil {
+		t.Fatal("Commit", err)
+	}
+
+	// Verify that the new password is set.
+	pwattr := tx.(*backendTX).readAttributeValues(
+		context.Background(),
+		"mail=uno@investici.org,uid=uno@investici.org,ou=People,dc=example,dc=com",
+		"userPassword",
+	)
+	if len(pwattr) == 0 {
+		t.Fatalf("no userPassword attribute on mail= object")
+	}
+	if len(pwattr) > 1 {
+		t.Fatalf("more than one userPassword found on mail= object")
+	}
+	if pwattr[0] != encPass {
+		t.Fatalf("bad userPassword, got %s, expected %s", pwattr[0], encPass)
+	}
+}
+
+func TestModel_SetUserEncryptionKeys_Add(t *testing.T) {
+	stop, b, user := startServerAndGetUser(t)
+	defer stop()
+
+	tx, _ := b.NewTransaction()
+	keys := []*accountserver.UserEncryptionKey{
+		{
+			ID:  accountserver.UserEncryptionKeyMainID,
+			Key: []byte("very secret key"),
+		},
+	}
+	if err := tx.SetUserEncryptionKeys(context.Background(), user, keys); err != nil {
+		t.Fatal("SetUserEncryptionKeys", err)
+	}
+	if err := tx.Commit(context.Background()); err != nil {
+		t.Fatal("Commit", err)
+	}
+}
+
+func TestModel_SetUserEncryptionKeys_Replace(t *testing.T) {
+	stop, b, user := startServerAndGetUser2(t)
+	defer stop()
+
+	tx, _ := b.NewTransaction()
+	keys := []*accountserver.UserEncryptionKey{
+		{
+			ID:  accountserver.UserEncryptionKeyMainID,
+			Key: []byte("very secret key"),
+		},
+	}
+	if err := tx.SetUserEncryptionKeys(context.Background(), user, keys); err != nil {
+		t.Fatal("SetUserEncryptionKeys", err)
+	}
+	if err := tx.Commit(context.Background()); err != nil {
+		t.Fatal("Commit", err)
+	}
+}

backend/testdata/test2.ldif

+dn: uid=due@investici.org,ou=People,dc=example,dc=com
+cn: due@investici.org
+gecos: due@investici.org
+gidNumber: 2000
+givenName: Private
+homeDirectory: /var/empty
+loginShell: /bin/false
+objectClass: top
+objectClass: person
+objectClass: posixAccount
+objectClass: shadowAccount
+objectClass: organizationalPerson
+objectClass: inetOrgPerson
+shadowLastChange: 12345
+shadowMax: 99999
+shadowWarning: 7
+sn: due@investici.org
+uid: due@investici.org
+uidNumber: 256799
+userPassword:: JGEyJDQkMzI3NjgkMSQwZDgyMzU1YjQ0Mzg0M2NmZDY4MjU1MzE4ZTVjYTdiZSRmNTQ0ODkxOTFiNWZlYzk2MDRlNWQ2ODZjMDQxZjJkNTFmOTgxOGY4ZTFmM2E4MDYzY2U3ZTEwMTE3OTc2OGI0
+totpSecret: ABCDEFGH
+
+dn: mail=due@investici.org,uid=due@investici.org,ou=People,dc=example,dc=com
+creationDate: 01-08-2013
+gidNumber: 2000
+host: host2
+mail: due@investici.org
+mailMessageStore: investici.org/due/
+objectClass: top
+objectClass: virtualMailUser
+originalHost: host2
+status: active
+uidNumber: 256799
+userPassword:: JGEyJDQkMzI3NjgkMSQwZDgyMzU1YjQ0Mzg0M2NmZDY4MjU1MzE4ZTVjYTdiZSRmNTQ0ODkxOTFiNWZlYzk2MDRlNWQ2ODZjMDQxZjJkNTFmOTgxOGY4ZTFmM2E4MDYzY2U3ZTEwMTE3OTc2OGI0
+storagePublicKey:: LS0tLS1CRUdJTiBQVUJMSUMgS0VZLS0tLS0KTUU0d0VBWUhLb1pJemowQ0FRWUZLNEVFQUNFRE9nQUVTeVVyVFhaaHRTeFpreityQjYwaFM4VnhINWozM3Ftbgphb3h2WG9IeG9vYU9Sc0x5TXNnVE5RVDR1bU1XdU12U3ROamszeWdWR2FNPQotLS0tLUVORCBQVUJMSUMgS0VZLS0tLS0K
+storageEncryptedSecretKey:: rffWTx7AjmhqD8li78Tal+7zfIbOFSyX4sKxKFa/bj5XAlWLyq7ANWiJB/0PRC1y8JBg+ezti5DjC5Ft82f5uwb2+3vIxjrxyz5vAmSUjRYo7o9alu5vLXRapsyhiYgJGmJrBJZxkQ9rGDXsM4OfZQNlxP4AVMobQFQU9X4QBUWFo2MwKuvwQiHg359hLufUrmr2bmjzPsU5Uj+8vAeQWHsVxWuUwUuob630A2V619iO5cp5nPzk5itYMNkdl1eR3KvUonvqwz++HLRJNqwh7qn2CjdUIA5ljexFg88UbNbzrpa+6Atmd4iXieYPewYHPHtuRFV3eHHlnBbv8VMcQdVZ0sqJokWDRvFjJbg=
+

backend/tx.go

@@ -86,7 +86,7 @@ func (tx *ldapTX) Commit(ctx context.Context) error {
 			mods[c.dn] = mr
 			dns = append(dns, c.dn)
 		}
-		tx.updateModifyRequest(mr, c)
+		tx.updateModifyRequest(ctx, mr, c)
 	}
 
 	// Now issue all ModifyRequests, one by one. Abort on the first error.
@@ -103,18 +103,49 @@ func (tx *ldapTX) Commit(ctx context.Context) error {
 	return nil
 }
 
-func (tx *ldapTX) updateModifyRequest(mr *ldap.ModifyRequest, attr ldapAttr) {
+func (tx *ldapTX) updateModifyRequest(ctx context.Context, mr *ldap.ModifyRequest, attr ldapAttr) {
 	old, ok := tx.cache[cacheKey(attr.dn, attr.attr)]
+
+	// Pessimistic approach: if we haven't seen this attribute
+	// before, try to fetch it from LDAP so we know if we need to
+	// perform an Add or a Replace.
+	if !ok {
+		log.Printf("tx: pessimistic fallback for %s %s", attr.dn, attr.attr)
+		oldFromLDAP := tx.readAttributeValues(ctx, attr.dn, attr.attr)
+		if len(oldFromLDAP) > 0 {
+			ok = true
+			old = oldFromLDAP
+		}
+	}
+
 	switch {
 	case ok && !stringListEquals(old, attr.values):
 		mr.Replace(attr.attr, attr.values)
 	case ok && attr.values == nil:
-		mr.Delete(attr.attr, nil)
+		mr.Delete(attr.attr, old)
 	case !ok && len(attr.values) > 0:
 		mr.Add(attr.attr, attr.values)
 	}
 }
 
+func (tx *ldapTX) readAttributeValues(ctx context.Context, dn, attr string) []string {
+	result, err := tx.search(ctx, ldap.NewSearchRequest(
+		dn,
+		ldap.ScopeBaseObject,
+		ldap.NeverDerefAliases,
+		0,
+		0,
+		false,
+		"(objectClass=*)",
+		[]string{attr},
+		nil,
+	))
+	if err == nil && len(result.Entries) > 0 {
+		return result.Entries[0].GetAttributeValues(attr)
+	}
+	return nil
+}
+
 func isEmptyModifyRequest(mr *ldap.ModifyRequest) bool {
 	return (len(mr.AddAttributes) == 0 &&
 		len(mr.DeleteAttributes) == 0 &&