www.cendio.com

Bug 5086

Summary: Web Access can't distinguish between PAM messages and prompts
Product: ThinLinc Reporter: Karl Mikaelsson <derfian@cendio.se>
Component: Web AccessAssignee: Henrik Andersson <hean01@cendio.se>
Status: CLOSED FIXED QA Contact: Bugzilla mail exporter <bugzilla-qa@cendio.se>
Severity: Normal    
Priority: P2 CC: hean01@cendio.se, samuel@cendio.se
Version: trunkKeywords: ossman_tester, relnotes
Target Milestone: 4.10.0   
Hardware: PC   
OS: Unknown   
Acceptance Criteria:
- The Web UI should guide you through the PAM conversation step by step - Info, error and prompts should presented to the user in the correct order - 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. - 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. - Info and error messages received before the first prompt should be shown to the user.
Bug Depends on:    
Bug Blocks: 5028, 6221    

------- Comment #4 From cendio 2018-06-04 15:41:06 -------
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 From cendio 2018-07-05 16:50:18 -------
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 From cendio 2018-07-05 16:52:15 -------
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 From cendio 2018-07-05 16:57:15 -------
Created an attachment (id=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 From cendio 2018-07-06 10:41:01 -------
(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 From cendio 2018-07-06 11:10:19 -------
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 From cendio 2018-07-06 16:21:55 -------
(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 From cendio 2018-07-09 11:34:17 -------
We also lost the fd cleanup prexec function along the way, so tl-pamapp
inherits some fds it shouldn't.
------- Comment #25 From cendio 2018-07-11 10:31:31 -------
Everything should be fixed now.
------- Comment #26 From cendio 2018-07-11 10:41:31 -------
(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 From cendio 2018-07-11 10:46:50 -------
> - 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.