mirror/opencloud
1
0
Fork 0
mirror of https://github.com/opencloud-eu/opencloud.git synced 2026-01-25 14:30:28 -05:00
Code Issues Packages Projects Releases Wiki Activity

Merge pull request #65 from butonic/fix-password-related-segfault

Browse Source 
fix password related segfaults
This commit is contained in:
Jörn Friedrich Dreyer
2020-07-23 13:23:43 +02:00
committed by GitHub
parent 42b8a7add5 65e3df2b1e
commit 54be6f4c2c
4 changed files with 81 additions and 18 deletions
Download Patch File Download Diff File Expand all files Collapse all files

5
changelog/unreleased/fix-segfault.md Normal file
View File

@@ -0,0 +1,5 @@
Bugfix: Prevent segfault when no password is set
Passwords are stored in a dedicated child struct of an account. We fixed several segfault conditions where the methods would try to unset a password when that child struct was not existing.
https://github.com/owncloud/ocis-accounts/pull/65

5
changelog/unreleased/tighten-screws.md Normal file
View File

@@ -0,0 +1,5 @@
Change: Tighten screws on usernames and email addresses
In order to match accounts to the OIDC claims we currently rely on the email address or username to be present. We force both to match the [W3C recommended regex](https://www.w3.org/TR/2016/REC-html51-20161101/sec-forms.html#valid-e-mail-address) with usernames having to start with a character or `_`. This allows the username to be presented and used in ACLs when integrating the os with the glauth LDAP service of ocis.
https://github.com/owncloud/ocis-accounts/pull/65

17
pkg/proto/v0/accounts.pb.micro_test.go
View File

@@ -389,6 +389,11 @@ func TestCreateExistingUser(t *testing.T) {
// All tests fail after running this
// https://github.com/owncloud/ocis-accounts/issues/62
func TestCreateAccountInvalidUserName(t *testing.T) {
resp, err := listAccounts(t)
checkError(t, err)
numAccounts := len(resp.GetAccounts())
testData := []string{
"",
"0",
@@ -400,14 +405,16 @@ func TestCreateAccountInvalidUserName(t *testing.T) {
_, err := createAccount(t, userName)
// Should give error
checkError(t, err)
if err == nil {
t.Fatalf("Expected an Error when creating user '%s' but got nil", userName)
}
}
// This throws a error in server
// resp contains no accounts
resp, _ := listAccounts(t)
// resp should have the same number of accounts
resp, err = listAccounts(t)
checkError(t, err)
assert.Equal(t, 0, len(resp.GetAccounts()))
assert.Equal(t, numAccounts, len(resp.GetAccounts()))
cleanUp(t)
}

72
pkg/service/v0/accounts.go
View File

@@ -233,7 +233,10 @@ func (s Service) ListAccounts(ctx context.Context, in *proto.ListAccountsRequest
s.expandMemberOf(a)
// remove password before returning
a.PasswordProfile.Password = ""
if a.PasswordProfile != nil {
a.PasswordProfile.Password = ""
}
out.Accounts = append(out.Accounts, a)
}
@@ -259,7 +262,9 @@ func (s Service) GetAccount(c context.Context, in *proto.GetAccountRequest, out
s.expandMemberOf(out)
// remove password
out.PasswordProfile.Password = ""
if out.PasswordProfile != nil {
out.PasswordProfile.Password = ""
}
return
}
@@ -273,6 +278,12 @@ func (s Service) CreateAccount(c context.Context, in *proto.CreateAccountRequest
if in.Account.Id == "" {
in.Account.Id = uuid.Must(uuid.NewV4()).String()
}
if !s.isValidUsername(in.Account.PreferredName) {
return merrors.BadRequest(s.id, "preferred_name '%s' must be at least the local part of an email", in.Account.PreferredName)
}
if !s.isValidEmail(in.Account.Mail) {
return merrors.BadRequest(s.id, "mail '%s' must be a valid email", in.Account.Mail)
}
if id, err = cleanupID(in.Account.Id); err != nil {
return merrors.InternalServerError(s.id, "could not clean up account id: %v", err.Error())
@@ -312,6 +323,12 @@ func (s Service) UpdateAccount(c context.Context, in *proto.UpdateAccountRequest
if in.Account.Id == "" {
return merrors.BadRequest(s.id, "account id missing")
}
if !s.isValidUsername(in.Account.PreferredName) {
return merrors.BadRequest(s.id, "preferred_name '%s' must be at least the local part of an email", in.Account.PreferredName)
}
if !s.isValidEmail(in.Account.Mail) {
return merrors.BadRequest(s.id, "mail '%s' must be a valid email", in.Account.Mail)
}
if id, err = cleanupID(in.Account.Id); err != nil {
return merrors.InternalServerError(s.id, "could not clean up account id: %v", err.Error())
@@ -342,19 +359,24 @@ func (s Service) UpdateAccount(c context.Context, in *proto.UpdateAccountRequest
out.Mail = in.Account.Mail // read only?
out.Description = in.Account.Description
if in.Account.PasswordProfile != nil && in.Account.PasswordProfile.Password != "" {
// encrypt password
c := crypt.New(crypt.SHA512)
if out.PasswordProfile.Password, err = c.Generate([]byte(in.Account.PasswordProfile.Password), nil); err != nil {
s.log.Error().Err(err).Str("id", id).Interface("account", loggableAccount(in.Account)).Msg("could not hash password")
return merrors.InternalServerError(s.id, "could not hash password: %v", err.Error())
if in.Account.PasswordProfile != nil {
if out.PasswordProfile == nil {
out.PasswordProfile = &proto.PasswordProfile{}
}
if in.Account.PasswordProfile.Password != "" {
// encrypt password
c := crypt.New(crypt.SHA512)
if out.PasswordProfile.Password, err = c.Generate([]byte(in.Account.PasswordProfile.Password), nil); err != nil {
s.log.Error().Err(err).Str("id", id).Interface("account", loggableAccount(in.Account)).Msg("could not hash password")
return merrors.InternalServerError(s.id, "could not hash password: %v", err.Error())
}
}
// lastPasswordChangeDateTime calculated, see password
out.PasswordProfile.PasswordPolicies = in.Account.PasswordProfile.PasswordPolicies
out.PasswordProfile.ForceChangePasswordNextSignIn = in.Account.PasswordProfile.ForceChangePasswordNextSignIn
out.PasswordProfile.ForceChangePasswordNextSignInWithMfa = in.Account.PasswordProfile.ForceChangePasswordNextSignInWithMfa
out.PasswordProfile.LastPasswordChangeDateTime = tsnow
}
// lastPasswordChangeDateTime calculated, see password
out.PasswordProfile.PasswordPolicies = in.Account.PasswordProfile.PasswordPolicies
out.PasswordProfile.ForceChangePasswordNextSignIn = in.Account.PasswordProfile.ForceChangePasswordNextSignIn
out.PasswordProfile.ForceChangePasswordNextSignInWithMfa = in.Account.PasswordProfile.ForceChangePasswordNextSignInWithMfa
// memberOf read only
// createdDateTime read only
@@ -382,7 +404,9 @@ func (s Service) UpdateAccount(c context.Context, in *proto.UpdateAccountRequest
}
// remove password
out.PasswordProfile.Password = ""
if out.PasswordProfile != nil {
out.PasswordProfile.Password = ""
}
return
}
@@ -425,3 +449,25 @@ func (s Service) DeleteAccount(c context.Context, in *proto.DeleteAccountRequest
s.log.Info().Str("id", id).Msg("deleted account")
return
}
// We want to allow email addresses as usernames so they show up when using them in ACLs on storages that allow intergration with our glauth LDAP service
// so we are adding a few restrictions from https://stackoverflow.com/questions/6949667/what-are-the-real-rules-for-linux-usernames-on-centos-6-and-rhel-6
// names should not start with numbers
var usernameRegex = regexp.MustCompile("^[a-zA-Z_][a-zA-Z0-9.!#$%&'*+/=?^_`{|}~-]*(@[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?(?:.[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?)*)*$")
func (s Service) isValidUsername(e string) bool {
if len(e) < 1 && len(e) > 254 {
return false
}
return usernameRegex.MatchString(e)
}
// regex from https://www.w3.org/TR/2016/REC-html51-20161101/sec-forms.html#valid-e-mail-address
var emailRegex = regexp.MustCompile("^[a-zA-Z0-9.!#$%&'*+/=?^_`{|}~-]+@[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?(?:.[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?)*$")
func (s Service) isValidEmail(e string) bool {
if len(e) < 3 && len(e) > 254 {
return false
}
return emailRegex.MatchString(e)
}