Patch reviews of authorization code

system · January 15, 2010, 8:26am

bradymiller wrote on Friday, January 15, 2010:

hey,

from ‘code review’ tracker here:
https://sourceforge.net/tracker/?func=detail&aid=2932246&group_id=60081&atid=1245239

Haven’t tested this yet, but note this so far in a read through:
1. No trailing or leading whitespaces in the xl functions (explained here: http://www.openmedsoftware.org/wiki/Development_Policies#Internationalization ) line 237 in the patch is one example in being incorrect
2. Do not put special characters in the xl functions, such as \n (line 806). Need to break it up into lines that a translator would be able to understand (i know not ideal, but is the nature of the beast).
3. line 915 i incorrect for xl(). should be +<INPUT TYPE=“Submit” VALUE="<?php xl(‘Save Changes’,‘e’); ?>" onClick=“return userInputValidation()”> (note I moved the quotations outside the xl function)
4. formData function has trim option . for example line formData(‘username’,‘G’,true) would trim (thus no need for putting it in a trim function). see openemr/library/formdata.inc.php for functions (also nice description there).

Couple quick questions about the client certificates:
1) Is this needed for MU (my impression was that only one method was needed, ie just name/password)
2) Does this work in a typical clinic where mutliple physicians are using multiple computers (ie. computers in shared physicians rooms and computers in shared patients rooms)
3) Does this work on linux and windows servers, or is it OS specific
4) Do any other EMR’s (certified or non-certified) use this mechanism?

Don’t get me wrong. I think it’s a nifty cool feature. Just wondering if it should be completely off be default, and then turned on and set up post-install. I’m also wondering the same thing for the forcing of https. Just thinking out loud.

-brady

system · January 15, 2010, 5:46pm

tmccormi wrote on Friday, January 15, 2010:

We did some testing of this a bit last night at MI2 and found a couple of things that need to be thought about. I’ll let Aron post the details, but basically I agree that SSL and Certificates should be OFF by default. Trying to do an upgrade of an existing system requires you to run the setup scripts, for one. Some systems don’t have HTTPS enabled at all because they are not used outside of an Intranet (ie, completely behind a firewall or not connected to the outside at all).

Password security, rules and aging should remain on to meet MU, in my opinion.

Tony

system · January 16, 2010, 9:46pm

bradymiller wrote on Saturday, January 16, 2010:

hey,

Still haven’t been able to test this secondary to host of minor issues (default globals settings, email stuff, htttps stuff, etc.), but wanted to add a bit more criticism over the conceptual part of this code, which is also related to Tony’s input.

This code introduces three functions SSL certificates, https forcing, and password security rules that should not be pushed on the entire OpenEMR open source user base. They should be built in a fashion where they are all options that can be turned off or on via interface/globals.php in a very granular fashion.

For example, the password security stuff would turn into three or so option in globals.php (ie, for whether to use secure passwords, grace time (set 0 if grace time is infinite), password expiration (set 0 if there is no expiration), set password history(from 0-3)) with full explanation of what these settings do there.

The https forcing is something in my mind at least, which should be optional also.

The SSL certification stuff truly doesn’t make any sense to me for all openemr users(see my questions regarding this in my first post), but I can see situations where a specific OpenEMR instance may want to take advantage of it, so is a potentially good contribution. However, I don’t think it has any place in the setup.php script, and could be configured post-install. There are several reasons why the setup.php script is not a good place:
1) In future, that script will rarely be used anyways. Currently, the xampp package, ubuntu package, and the appliance bypass the setup.php script. As OpenEMR gets more popular, more platform specific packages will be built that also do not use setup.php.
2) You also miss the upgraders. If you instead focused on a mechanisms after the setup.php script, you’d give many more users the option to use this function.
3) If one day the community demands this in the setup.php (doubtful…), then easy to integrate it (for example like the php-gacl install script is integrated)

Now, specifically regarding Tony’s input. What options to choose as default are not really that vital at this point. What is vital is to realize what should be an option; thus placing the appropriate mechanism around it. To me, the password strengthening obviously falls into this, and is a simple mod to the current code. So, if we do decide to make strong password default at release time, then a user whom doesn’t want it, can always turn it off.

-brady

system · January 18, 2010, 5:45pm

sunsetsystems wrote on Monday, January 18, 2010:

Wow, lots of new code here. I have only had time to skim it, but here are some comments.

* Agree 100% that all of this stuff needs to be optional.

* The wiki page at http://www.openmedsoftware.org/wiki/2._SSL_Configuration suggests that only server-side certificates are used, however it looks like the code is dealing with client-side (browser) certificates and even getting the user name from the certificate (why is that done?). What’s really going on?

* What are the setup requirements before this SSL stuff will work? How are the certificates signed? How do they get configured into Apache? Will it work with web servers other than Apache?

* In globals.php you have ‘$web_root = “https://127.0.0.1:2010/”;’. $web_root is to be a relative path, don’t use it to force HTTPS. That can be handled in the web server configuration.

* Put spaces around binary operators for readability. For example not ‘if ($_SERVER!=“on”)’, but ‘if ($_SERVER != “on”)’.

* Some of the code needs more comments. Be kind!

* Watch out for insecure use of form data. user_admin.php looks insecure with statements like this:
$result = sqlStatement(“select active from users where id={$_GET}”);

* 3_1_0-to-3_2_0_upgrade.sql is the wrong place for your SQL updates. Use 3_2_0-to-3_3_0_upgrade.sql.

* Don’t use 0000-00-00 as a default date, use null instead to indicate no date. The 0000-00-00 thing is an ugly construct that is peculiar to MySQL.

Rod
www.sunsetsystems.com

system · January 18, 2010, 5:48pm

sunsetsystems wrote on Monday, January 18, 2010:

SourceForge messed up my line breaks again. Trying again:

* Agree 100% that all of this stuff needs to be optional.

* The wiki page at http://www.openmedsoftware.org/wiki/2._SSL_Configuration suggests that only server-side certificates are used, however it looks like the code is dealing with client-side (browser) certificates and even getting the user name from the certificate (why is that done?). What’s really going on?

* What are the setup requirements before this SSL stuff will work? How are the certificates signed? How do they get configured into Apache? Will it work with web servers other than Apache?

* In globals.php you have ‘$web_root = “https://127.0.0.1:2010/”;’. $web_root is to be a relative path, don’t use it to force HTTPS. That can be handled in the web server configuration.

* Put spaces around binary operators for readability. For example not ‘if ($_SERVER!=“on”)’, but ‘if ($_SERVER != “on”)’.

* Some of the code needs more comments. Be kind!

* Watch out for insecure use of form data. user_admin.php looks insecure with statements like this: $result = sqlStatement(“select active from users where id={$_GET}”);

* 3_1_0-to-3_2_0_upgrade.sql is the wrong place for your SQL updates. Use 3_2_0-to-3_3_0_upgrade.sql.

* Don’t use 0000-00-00 as a default date, use null instead to indicate no date. The 0000-00-00 thing is an ugly construct that is peculiar to MySQL.

Rod
www.sunsetsystems.com

system · January 18, 2010, 5:52pm

sunsetsystems wrote on Monday, January 18, 2010:

SourceForge messed up my formatting again. Here it is again:

* Agree 100% that all of this stuff needs to be optional.

* The wiki page at http://www.openmedsoftware.org/wiki/2._SSL_Configuration suggests that only server-side certificates are used, however it looks like the code is dealing with client-side (browser) certificates and even getting the user name from the certificate (why is that done?). What’s really going on?

* What are the setup requirements before this SSL stuff will work? How are the certificates signed? How do they get configured into Apache? Will it work with web servers other than Apache?

* In globals.php you have ‘$web_root = “https://127.0.0.1:2010/”;’. $web_root is to be a relative path, don’t use it to force HTTPS. That can be handled in the web server configuration.

* Put spaces around binary operators for readability.

* Some of the code needs more comments. Be kind!

* Watch out for insecure use of form data. user_admin.php looks insecure with statements like this: $result = sqlStatement(“select active from users where id={$_GET}”);

* 3_1_0-to-3_2_0_upgrade.sql is the wrong place for your SQL updates. Use 3_2_0-to-3_3_0_upgrade.sql.

* Don’t use 0000-00-00 as a default date, use null instead to indicate no date. The 0000-00-00 thing is an ugly construct that is peculiar to MySQL.

Rod
www.sunsetsystems.com

system · January 18, 2010, 6:21pm

sunsetsystems wrote on Monday, January 18, 2010:

Ignore my confusion about the client side certificates, I just noticed http://www.openmedsoftware.org/wiki/3._Client_Side_certificates .

However I’m concerned about a couple of things here. What if the user does not have a working email address? What if one computer is in a common area, used by several different users?

And administrator/user documentation is still critical.

Rod
www.sunsetsystems.com

system · January 18, 2010, 7:16pm

sunsetsystems wrote on Monday, January 18, 2010:

Following up up on my previous comment… I’d suggest always requiring password authentication via the OpenEMR login page. The browser certificate can be thought of as a way to authenticate the *location* of the user, to keep out the professional bad guys, not as a way to identify the user.

Also regarding emailing a certificate to the user - even if they have working reliable email, it is likely to be insecure. There needs to be another option for delivering the certificate.

I deleted my original comments with the formatting errors.

Rod
www.sunsetsystems.com

system · January 20, 2010, 1:31pm

visolveemr wrote on Wednesday, January 20, 2010:

Hi Team,

Thanks for your time in reviewing the patch and your comments.

- We will make both SSL certificates and Strong Password Checking optional and off by default
- We will move the SSL configuration screen from setup.php to a separate menu item under the Administration menu.

The SSL client certificates do not replace username/password login. Users still need to enter a valid username/password after the client certificate is validated by Apache.

Related MUO - “Verify that a person or entity seeking access to electronic health information is the one claimed and is authorized to access such information”

Here goes the answers for some of your queries:-

Emailing certificates is insecure

If required, we can provide a button for the admin to download the certificate to his local machine (through the secure HTTPS connection). From there, it is up to the admin to securely handoff the certificate to the user (by CD, USB stick drive, etc).

One computer - used by several people

As mentioned above, username/password login is still required after client certificate validation.

When a user logs in, the audit log will store
- The username
- The IP address of the client machine
- The client certificate name

Having a separate certificate for each user allows the user to install the certificate on other non-shared machines.

What are the setup requirements before this SSL stuff will work? How are the certificates signed? How do they get configured into Apache? Will it work with web servers other than Apache?

The requirements are
- The Apache mod_ssl.so/mod_ssl.dll library.
- The PHP library php_openssl.so/php_openssl.dll
On Linux, the pre-compiled Apache/PHP packages (Ubuntu, Redhat) always come
with the libraries above.
On Windows, the OpenEMR XAMPP download also comes with the libraries above.
C:/xampp/apache/modules/mod_ssl.so
C:/xampp/php/ext/php_openssl.dll
The client certificates are signed by a Certificate Authority (CA)
certificate/key file. The CA file is specified in globals.php. Currently,
OpenEMR creates its own CA certificate during setup (setup.php). We will
move this behavior to a separate Administration screen, outside of setup.
The user must manually configure Apache to use the certificates.
The setup.php screen displays the Apache config lines that need to
be added. We will move this screen outside of setup.
Yes, X.509 SSL certificates have a standard format (see rfc2459),
and will work with any webserver.

Xl translations

Brady, I’ve seen the special characters ‘.’ ‘:’ in so many places in openemr. What are the special charaters that are not allowed in openemr?

Hope this helps.

Thanks
ViCarePlus team

system · January 21, 2010, 10:08am

bradymiller wrote on Thursday, January 21, 2010:

hey,

I’m not quite clear what your asking. When I make a english constant, I avoid symbols. If I do need to include a symbol(like a period or exclamation), I include it in the translated constant though. I usually attempt to use a constant that has already been used, which are listed here: openemr/contrib/util/language_translations/currentConstants.txt

Here are some good links that cover the xl() function:
http://www.openmedsoftware.org/wiki/OpenEMR_Internationalization_Configuration#Developer_Instructions
http://www.openmedsoftware.org/wiki/Development_Policies#Internationalization

-brady

system · January 21, 2010, 5:54pm

visolveemr wrote on Thursday, January 21, 2010:

Brady,

You’d mentioned that special characters are not allowed in xl. I’d seen in some places (in the existing openemr code) where the characters like ‘:’ are used in the xl function.

Ex: login.php
xl(‘Username:’,‘e’)
xl(‘Password:’,‘e’)

We’ll remove the ‘\n’ character from the ‘xl’ calls…

Thanks
ViCarePlus team

system · January 21, 2010, 6:14pm

bradymiller wrote on Thursday, January 21, 2010:

Vicare,
Sorry about that. I should of known what you were referring to; it was late last night. The best thing to do is just put yourself in the place of the translators, whom usually have no technical knowledge, when they start translating from the google spreadsheet here: http://spreadsheets.google.com/ccc?key=0AtTW60zHo6HzcGg0UE9JMGJHM1NsSWpuYkh0Snl4Q0E&hl=en http://spreadsheets.google.com/ccc?key=0AtTW60zHo6HzcGg0UE9JMGJHM1NsSWpuYkh0Snl4Q0E&hl=en. So, they’ll be able to deal with normal punctuation characters, however there should be no other special characters that a layperson would not understand (like \n for example). If had the \n character, most translators would likely ignore it, or worse try to translate it.

-brady