Non-ASCII password on user creation leads to 'User(...) already exists' #1555

Closed
ansemjo opened this Issue Nov 29, 2016 · 5 comments

Comments

Projects
None yet
4 participants
@ansemjo
Contributor

ansemjo commented Nov 29, 2016

Looking for possible NAS solutions and I stumbled over Rockstor today. I am already successfully using btrfs on my laptop, so this looks very interesting to me.

Anyway. When attempting to create the first user through the WebGUI wizard I used a password with non-ASCII characters which lead to the following issue. (As can be seen in the screenshots, this is reproducible when simply creating a new user.)

screenshot from 2016-11-29 22-13-30

Traceback (most recent call last):
  File "/opt/rockstor/src/rockstor/rest_framework_custom/generic_view.py", line 40, in _handle_exception
    yield
  File "/opt/rockstor/src/rockstor/storageadmin/views/user.py", line 152, in post
    usermod(invar['username'], invar['password'])
  File "/opt/rockstor/src/rockstor/system/users.py", line 130, in usermod
    out, err = p.communicate(input=passwd)
  File "/usr/lib64/python2.7/subprocess.py", line 800, in communicate
    return self._communicate(input)
  File "/usr/lib64/python2.7/subprocess.py", line 1401, in _communicate
    stdout, stderr = self._communicate_with_poll(input)
  File "/usr/lib64/python2.7/subprocess.py", line 1465, in _communicate_with_poll
    input_offset += os.write(fd, chunk)
UnicodeEncodeError: 'ascii' codec can't encode character u'\x**' in position 0: ordinal not in range(128)

u'\x**' contains the hex representation of the character, censored here.

There appears to be no hint on allowed characters and no password validation whatsoever? Fair enough. Change the password to all-ASCII characters and try again:

screenshot from 2016-11-29 22-14-47

Traceback (most recent call last):
  File "/opt/rockstor/eggs/gunicorn-0.16.1-py2.7.egg/gunicorn/workers/sync.py", line 34, in run
    client, addr = self.socket.accept()
  File "/usr/lib64/python2.7/socket.py", line 202, in accept
    sock, addr = self._sock.accept()
error: [Errno 11] Resource temporarily unavailable

Oops.

As a workaround I used a different username. This is all for testing purposes on a VM anyway, so it's not a big deal. The username later appears in the list of Other system users and needs to be removed via CLI.

I'd suggest either adding validation as it appears to be done for the username or edit the python script to perform whatever encoding has to be performed before the user creation? Unfortunately, I haven't done any python yet myself ..

I am running Rockstor version 3.8.15-0, kernel 4.6.0-1.el7.elrepo.x86_64.

@phillxnet

This comment has been minimized.

Show comment
Hide comment
@phillxnet

phillxnet Nov 30, 2016

Member

@ansemjo Thanks for the excellent bug report, and nice find. There is as you say definitely a need for improvement here. As to your "Unfortunately, I haven't done any python yet myself ..." I wouldn't let that bother you as it is incredibly accessible. I would like to encourage you to take a look at the indicated code and have a dabble.

We have a Developers section of the Contributing to Rockstor - Overview in the official docs. This walks you through getting setup to submit fixes. And you have already gotten started with the first bit (this issue) and are obviously not new to GitHub so that bit's all sorted.

I agree that we need better user facing info on allowed characters so that's just adding a popup hint. Followed by a little more input validation. I know it's easier said than done but I suspect it's going to be easier than you think.

Thanks again and if you fancy having a go then just note here that you are dabbling and ask questions as you go. Also the Rockstor forum is pretty active and friendly and all current developer are also members.

Member

phillxnet commented Nov 30, 2016

@ansemjo Thanks for the excellent bug report, and nice find. There is as you say definitely a need for improvement here. As to your "Unfortunately, I haven't done any python yet myself ..." I wouldn't let that bother you as it is incredibly accessible. I would like to encourage you to take a look at the indicated code and have a dabble.

We have a Developers section of the Contributing to Rockstor - Overview in the official docs. This walks you through getting setup to submit fixes. And you have already gotten started with the first bit (this issue) and are obviously not new to GitHub so that bit's all sorted.

I agree that we need better user facing info on allowed characters so that's just adding a popup hint. Followed by a little more input validation. I know it's easier said than done but I suspect it's going to be easier than you think.

Thanks again and if you fancy having a go then just note here that you are dabbling and ask questions as you go. Also the Rockstor forum is pretty active and friendly and all current developer are also members.

@ansemjo

This comment has been minimized.

Show comment
Hide comment
@ansemjo

ansemjo Nov 30, 2016

Contributor

Sure, I'll try and look into it. :) Thanks for the links to the docs!

I found src/rockstor/storageadmin/views/user.py already, which appears to perform some username validation in _validate_input .. so maybe that's a good place to start.


Wed 30 Nov 20:05:34 CET 2016

Just for later reference, need these packages to build on the VM:

  • gcc
  • python-devel
  • postgresql-devel
  • zeromq-devel
Contributor

ansemjo commented Nov 30, 2016

Sure, I'll try and look into it. :) Thanks for the links to the docs!

I found src/rockstor/storageadmin/views/user.py already, which appears to perform some username validation in _validate_input .. so maybe that's a good place to start.


Wed 30 Nov 20:05:34 CET 2016

Just for later reference, need these packages to build on the VM:

  • gcc
  • python-devel
  • postgresql-devel
  • zeromq-devel
@ansemjo

This comment has been minimized.

Show comment
Hide comment
@ansemjo

ansemjo Nov 30, 2016

Contributor

Still no validation or hints for weak passwords etc. but one can create users with non-ascii characters in their passwords with the above PR.

Contributor

ansemjo commented Nov 30, 2016

Still no validation or hints for weak passwords etc. but one can create users with non-ascii characters in their passwords with the above PR.

@MFlyer

This comment has been minimized.

Show comment
Hide comment
@MFlyer

MFlyer Dec 1, 2016

Member

Hi @ansemjo,
happy to see a new contributor, welcome aboard! 🚀

Still no validation or hints for weak passwords

This could be a nice small enhancement you can (if you want) accomplish 3 different ways:

  • easy: add a tooltip to password field suggesting not weak passwords (lower/upper case password with digits and symbols)
  • less easy: force users to have a strong password via javascript regex
  • less easy 2: force it with regex on backend to (paranoia mode)

My personal opinion: I always try to think about my 90yrs old grandma using computers so I'd get option 1 (I tell you, but I don't force you - if you're a sysadmin/super user managing Rockstor & sensible data I suppose you'll create a strong password, if not...my grandma will kick you! :P)

Anyway, have fun!
Mirko

Member

MFlyer commented Dec 1, 2016

Hi @ansemjo,
happy to see a new contributor, welcome aboard! 🚀

Still no validation or hints for weak passwords

This could be a nice small enhancement you can (if you want) accomplish 3 different ways:

  • easy: add a tooltip to password field suggesting not weak passwords (lower/upper case password with digits and symbols)
  • less easy: force users to have a strong password via javascript regex
  • less easy 2: force it with regex on backend to (paranoia mode)

My personal opinion: I always try to think about my 90yrs old grandma using computers so I'd get option 1 (I tell you, but I don't force you - if you're a sysadmin/super user managing Rockstor & sensible data I suppose you'll create a strong password, if not...my grandma will kick you! :P)

Anyway, have fun!
Mirko

@ansemjo

This comment has been minimized.

Show comment
Hide comment
@ansemjo

ansemjo Dec 1, 2016

Contributor

Thanks for the warm welcome! 👻

If I were to add any kind of validation I'd probably do it in the backend but I'd also only force a minimum length of, say, 6-8 characters or so. No hard restrictions on complexity. I would add that as hints or tooltips, like you suggested.

However, I would also seperate those concerns. Yes, it would be a nice addition and I'll probably try my hand at it too when I can find some time for it. But it is not strictly necessary to fix the issue at hand .. personally, I'd consider a seperate issue / PR at a later time for that.

Contributor

ansemjo commented Dec 1, 2016

Thanks for the warm welcome! 👻

If I were to add any kind of validation I'd probably do it in the backend but I'd also only force a minimum length of, say, 6-8 characters or so. No hard restrictions on complexity. I would add that as hints or tooltips, like you suggested.

However, I would also seperate those concerns. Yes, it would be a nice addition and I'll probably try my hand at it too when I can find some time for it. But it is not strictly necessary to fix the issue at hand .. personally, I'd consider a seperate issue / PR at a later time for that.

@schakrava schakrava closed this in #1559 Dec 17, 2016

@schakrava schakrava added the bug label Jan 4, 2017

@schakrava schakrava added this to the Pinnacles milestone Jan 4, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment