Comment 4 Pierre Ossman 2018-06-04 15:41:06 CEST

Most of the discussion has been around info messages, but we also need to handle error messages fully. Right now we handle those if they are part of PAM rejecting the login, but we should also handle them when things succeed or when they are shown between prompts.

Comment 8 Samuel Mannehed 2018-07-05 16:50:18 CEST

Tester should verify:

* that the first PAM_PROMPT_ECHO_OFF uses password from login form

* that a PAM conversation with PAM messages, PAM errors and PAM prompts works

* that the UI works as expected
  - timeouts
  - errors from javascript code
  - errors from python code
  - disabled/unavailable javascript handling

* that the error feedback is sufficient and proper (via the UI to the user and via the log to the administrator)

* that there are no stray files (fifos in /var/run/thinlinc)

* that there are no stray processes (tl-pamapp)

* that seeding the login form is possible by using query parameters (not password)

* that performing a login through query parameters is possible (loginsubmit=1)

* that password from login form is passed to first PAM_PROMPT_ECHO_OFF when having a PAM_PROMPT_ECHO_ON before the real password prompt

Comment 9 Samuel Mannehed 2018-07-05 16:52:15 CEST

The work done on this bug also fixed these two bugs:

* Bug 6221 - the UI layout has been fixed
* Bug 5658 - the process doesn't hang and the limit is now 1024 bytes

Comment 10 Samuel Mannehed 2018-07-05 16:57:15 CEST

Created attachment 880 [details]
Latest version of pam_tester

Check the code header documentation for usage.

Build:

  gcc -shared  -fPIC -lpam pam_tester.c -o pam_tester.so

Install:

  sudo cp pam_tester.so /lib64/security/
  sudo chcon -u system_u /lib64/security/pam_tester.so

Comment 11 Samuel Mannehed 2018-07-06 10:41:01 CEST

(In reply to comment #8)
>   - errors from javascript code
>   - errors from python code

example of error from javascript code:

  WebSockets not supported

example of error from python code:

  Authentication Failed

Comment 12 Pierre Ossman 2018-07-06 11:10:19 CEST

Some minor issues:

(In reply to comment #8)
> Tester should verify:
> 
> * that the first PAM_PROMPT_ECHO_OFF uses password from login form
> 

Works fine.

> * that a PAM conversation with PAM messages, PAM errors and PAM prompts works
> 

Tested messages, errors and prompts. No issues.

> * that the UI works as expected
>   - timeouts

Works:

 * Automatic "Next" works.

Broken:

 * After a login timeout, I get a username field with "$username".
 * After a login timeout, tl-pamapp crashes with glibc complaining about double free
 * After a login timeout, we are returned via a GET, not a POST, giving a rather nasty URL

>   - errors from javascript code
>   - errors from python code

Works

>   - disabled/unavailable javascript handling

Works

> 
> * that the error feedback is sufficient and proper (via the UI to the user and
> via the log to the administrator)
> 

Looks good.

Minor nitpick:

The logging for failures is a bit technical, mentioning PAM conversation and tasks. Can't we just say "Authentication failed for user foo:"/"Access denied for user foo:"?

> * that there are no stray files (fifos in /var/run/thinlinc)
> 

I have one pair, but the time corresponds to the above crash so it might have something to do with that.

> * that there are no stray processes (tl-pamapp)
> 

No problem.

> * that seeding the login form is possible by using query parameters (not
> password)
> 

Works.

> * that performing a login through query parameters is possible (loginsubmit=1)
> 

Works

> * that password from login form is passed to first PAM_PROMPT_ECHO_OFF when
> having a PAM_PROMPT_ECHO_ON before the real password prompt

Works.

Comment 16 Samuel Mannehed 2018-07-06 16:21:55 CEST

(In reply to comment #12)
> > * that the UI works as expected
> >   - timeouts
> 
> Broken:
> 
>  * After a login timeout, I get a username field with "$username".
>  * After a login timeout, we are returned via a GET, not a POST, giving a rather nasty URL

Fixed in r33484

>  * After a login timeout, tl-pamapp crashes with glibc complaining about double free

Not fixed yet. The problem seems to be that the signal handler in tl-pamapp is calling pam_end(), perhaps it would be better to just do a _exit(1) right away. Doing _exit() in the signal handler seems to fix the crash, but more testing is needed.

----

Other issues that has been discussed:

 * Should we keep the Javascript timeout? It adds a lot of code complexity for very limited benefits. If we remove it we need to ensure that the user gets helpful errors when returning after the tl-pamapp process has timed out and when the fifos has been removed.

 * Do we need the acks in tl-pamapp? It seems like tlwebaccess can read the fifos even after tl-pamapp has exited.

 * We should silently eat up stderr messages in tlwebaccess to avoid having stderr messages from tl-pamapp being written to the webaccess log.

Comment 17 Pierre Ossman 2018-07-09 11:34:17 CEST

We also lost the fd cleanup prexec function along the way, so tl-pamapp inherits some fds it shouldn't.

Comment 25 Pierre Ossman 2018-07-11 10:31:31 CEST

Everything should be fixed now.

Comment 26 Pierre Ossman 2018-07-11 10:41:31 CEST

(In reply to comment #16)
> (In reply to comment #12)
> > > * that the UI works as expected
> > >   - timeouts
> > 
> > Broken:
> > 
> >  * After a login timeout, I get a username field with "$username".
> >  * After a login timeout, we are returned via a GET, not a POST, giving a rather nasty URL
> 
> Fixed in r33484
> 

Works.

> >  * After a login timeout, tl-pamapp crashes with glibc complaining about double free
> 

Works now.

> 
>  * Should we keep the Javascript timeout? It adds a lot of code complexity for
> very limited benefits. If we remove it we need to ensure that the user gets
> helpful errors when returning after the tl-pamapp process has timed out and
> when the fifos has been removed.
> 

Removed, but still getting proper timeout errors.

>  * Do we need the acks in tl-pamapp? It seems like tlwebaccess can read the
> fifos even after tl-pamapp has exited.
> 

We do, so they stay. Not sure why things worked without them during our previous testing.

>  * We should silently eat up stderr messages in tlwebaccess to avoid having
> stderr messages from tl-pamapp being written to the webaccess log.

Works.

(In reply to comment #17)
> We also lost the fd cleanup prexec function along the way, so tl-pamapp
> inherits some fds it shouldn't.

Works.

Comment 27 Pierre Ossman 2018-07-11 10:46:50 CEST

> - The Web UI should guide you through the PAM conversation step by step
> 

Works well with all kinds of prompts.

> - Info, error and prompts should presented to the user in the correct order 
> 

Indeed they are.

> - Info messages should be shown for X seconds before the user is taken to next step. User should be able to skip forward by hitting a 'next' button.
> 

Works.

> - The Web UI initial form should contain username and password. The first PAM_PROMPT_ECHO_OFF that is received from PAM should be seeded with the provided password from login form.
> 

Works.

> - Info and error messages received before the first prompt should be shown to the user.

Works.