Smart-Soft Proxy SSO plugin #266

evbevz · 2017-09-15T15:36:26Z

deps: msktutil, cyrus-sasl-gssapi.
MIT kerberos implementation.
When Web-proxy uses LDAP connectors for authentication, SSO plugin gets the first LDAP connector from proxy authentication settings and makes SSO available for that connector.
Use kerberos check-list tab for easy setup, resolve common configuration mistakes, generate/delete/show keytab and test SSO.

fabianfrz

@fichtner If this takes longer, it might be impossible to do a re-review in time.

fabianfrz · 2017-09-16T08:12:44Z

www/proxy-sso/src/opnsense/service/templates/OPNsense/ProxySSO/krb5.conf

+{% if helpers.exists('OPNsense.ProxySSO.ADKerberosImplementation') and OPNsense.ProxySSO.ADKerberosImplementation == 'W2003' %}
+	 default_tgs_enctypes = rc4-hmac des-cbc-crc des-cbc-md5
+	 default_tkt_enctypes = rc4-hmac des-cbc-crc des-cbc-md5
+	 permitted_enctypes = rc4-hmac des-cbc-crc des-cbc-md5


indent mismatch

fabianfrz · 2017-09-16T08:13:50Z

www/proxy-sso/src/opnsense/service/templates/OPNsense/ProxySSO/kerberos.sso.conf

+{%          if method != "Local Database" %}
+{%              for server in helpers.toList('system.authserver') %}
+{%                  if server.type == 'ldap' and server.name == method %}
+{%                      if ldap_method.append(server) %}


empty if statement?

Yes, see above.

fabianfrz · 2017-09-16T08:16:55Z

www/proxy-sso/src/opnsense/scripts/OPNsense/ProxySSO/squid-gen-keytab.sh

+	e) ENCTYPES="$OPTARG" ;;
+	b) BASENAME="$OPTARG" ;;
+	u) USERNAME="$OPTARG" ;;		# LDAP admin username
+	p) PASSWORD="$OPTARG" ;;		# LDAP admin password


indent issue

fabianfrz · 2017-09-16T08:23:26Z

www/proxy-sso/src/opnsense/mvc/app/views/OPNsense/ProxySSO/index.volt

+        <button class="btn btn-primary" id="DeleteKeytab" type="button"><b>{{ lang._('Delete keytab') }}</b></button>
+        <button class="btn btn-primary" id="ShowKeytab" type="button"><b>{{ lang._('Show keytab') }}</b></button>
+        <br/>
+        <br/>


to keep the distance, it would be better to use CSS because br depends on many settings (like font size) while margin-bottom with a pixel distance will always stay the same.

fabianfrz · 2017-09-16T08:32:34Z

www/proxy-sso/src/opnsense/mvc/app/controllers/OPNsense/ProxySSO/Api/ServiceController.php

+            $hostname = 'HTTP/' . $cnf['system']['hostname'];
+            $domain = $cnf['system']['domain'];
+            $kerbname = substr(strtoupper($cnf['system']['hostname']), 0, 13) . "-K";
+            $winver = !isset($cnf['OPNsense']['ProxySSO']['ADKerberosImplementation']) || $cnf['OPNsense']['ProxySSO']['ADKerberosImplementation'] == 'W2008' ? '2008' : '2003';


Why are you not using your model for that?

$mdl = new ProxySSO(); $value = (string)$mdl->ADKerberosImplementation;

should work to access this field if the model is imported.

fabianfrz · 2017-09-16T09:10:30Z

www/proxy-sso/src/opnsense/mvc/app/views/OPNsense/ProxySSO/index.volt

+        $("#CreateKeytab").click(function() {
+            ajaxCall(url="/api/proxysso/service/createkeytab", sendData={
+                        "admin_login":$("#admin_username").val(),
+                        "admin_password":$("#admin_password").val()}, callback=function(data,status) {


too large indent

fabianfrz · 2017-09-16T09:10:40Z

www/proxy-sso/src/opnsense/mvc/app/views/OPNsense/ProxySSO/index.volt

+        $("#TestKerbLogin").click(function() {
+            ajaxCall(url="/api/proxysso/service/testkerblogin", sendData={
+                        "login":$("#username").val(),
+                        "password":$("#password").val()}, callback=function(data,status) {


fabianfrz · 2017-09-16T09:13:16Z

www/proxy-sso/src/opnsense/mvc/app/views/OPNsense/ProxySSO/index.volt

+        {{ partial("layout_partials/base_form",['fields':testingCreateForm,'id':'frm_TestingCreate'])}}
+        <button class="btn btn-primary" id="CreateKeytab" type="button"><b>{{ lang._('Create keytab') }}</b></button>
+        <button class="btn btn-primary" id="DeleteKeytab" type="button"><b>{{ lang._('Delete keytab') }}</b></button>
+        <button class="btn btn-primary" id="ShowKeytab" type="button"><b>{{ lang._('Show keytab') }}</b></button>


This is the frontend, if possible try clean english here (Key Table for example). This is important for translation and for end users.

fabianfrz · 2017-09-16T09:14:51Z

www/proxy-sso/src/opnsense/scripts/OPNsense/ProxySSO/kerberos_test.sh

+PASSWORD="${PASSWORD%\'}"
+echo "${PASSWORD}" | sed 's/\\//g' > ${PASS_TMP}
+
+/usr/local/bin/kinit ${USERNAME} < ${PASS_TMP}


is that safe against RCE?

I think so.
Password escaped before this action call:

$pass = escapeshellarg($this->request->getPost("password"));

And additional quoting performed in script (%\').

fabianfrz · 2017-09-16T09:15:53Z

www/proxy-sso/src/opnsense/service/templates/OPNsense/ProxySSO/krb5.conf

+{%      for method in OPNsense.proxy.forward.authentication.method.split(",") %}
+{%          for server in helpers.toList('system.authserver') %}
+{%              if server.type == 'ldap' and server.name == method %}
+{%                      if ldap.append(server) %}{% endif %}


Yes, it is a way to provide value changes from inner loop to outer variable (ldap) in jinja2.
ldap.append(server) makes it working.

If you would do it like this, there would not be an if:
https://github.com/opnsense/plugins/blob/master/net/mdns-repeater/src/opnsense/service/templates/OPNsense/MDNSRepeater/mdnsrepeater#L7

Yes you are right.

evbevz · 2017-09-18T09:17:19Z

@fabianfrz all items are fixed.

fichtner · 2017-09-18T09:29:17Z

www/proxy-sso/+POST_DEINSTALL.post

@@ -0,0 +1,5 @@
+rm /usr/local/etc/squid/pre-auth/20-negotiate.auth.conf


I don't have time for a full review yet, but this needs "rm -f" in case the file is not there

I have this on my TODO to provide a proper revoke of templates on deinstall for automation: #46

fichtner · 2017-09-18T09:31:11Z

www/proxy-sso/+POST_DEINSTALL.post

@@ -0,0 +1,5 @@
+rm /usr/local/etc/squid/pre-auth/20-negotiate.auth.conf
+if [ -f /var/run/squid/squid.pid ]; then
+	service squid reload


I know what it's trying to achieve, but it's tricky as it doesn't work on a stock FreeBSD, it would kill the service. In OPNsense it works, maybe we need some type of reload command embedding in the package Makefile to signal reverse dependencies of base plugins?

PLUGIN_USES= squid

pointing to the squid.inc plugin file and doing a reload if possible?

in any case, a configd call would be far safer here, preferably in the background

fichtner · 2017-09-18T09:32:19Z

www/proxy-sso/Makefile

+PLUGIN_COMMENT=		Kerberos authentication module
+PLUGIN_DEPENDS=		msktutil cyrus-sasl-gssapi
+PLUGIN_MAINTAINER=	evbevz@gmail.com
+PLUGIN_WWW=		http://smart-soft.ru


should upgrade to HTTPS if possible ;)

fichtner · 2017-09-18T09:33:03Z

www/proxy-sso/Makefile

@@ -0,0 +1,8 @@
+PLUGIN_NAME=		proxy-sso


are you ok with replacing the old plugin? the name would be web-proxy-sso or web-proxy-sso2, although that gets confusing...

no problem, as you wish )

fichtner · 2017-09-18T09:33:21Z

www/proxy-sso/Makefile

@@ -0,0 +1,8 @@
+PLUGIN_NAME=		proxy-sso
+PLUGIN_VERSION=		1.3


if we replace the old one, can this be 2.0 instead? what do you think?

Great. I can do this post merge, no need to shuffle it all around now :)

fichtner · 2017-09-18T09:35:18Z

www/proxy-sso/src/etc/inc/plugins.inc.d/proxy_sso.inc

+function proxy_sso_configure()
+{
+    return [
+        'webproxy_reconfigure' => ['proxy_sso_squid_reconfigure'],


does this work in the core or do we need to merge a pr? I vaguely remember something

This needs opnsense/core#1627
Without this pull-request, administrator should reconfigure proxy-sso manually after reconfiguring proxy.

this changes slightly with the merge: the facility is "webproxy" and in reconfigure you need to check for "reconfigure" argument, it looks like this:

https://github.com/opnsense/core/blob/master/src/etc/inc/plugins.inc.d/openvpn.inc#L37

Some changes have to be done in connection with acceptance of opnsense/core#1627

Done: 651ce70

in that case the line now reads something like:

'webproxy' => ['proxy_sso_squid_reconfigure:2'],

but the second argument to the function is the "reconfigure" statement like

https://github.com/opnsense/core/blob/master/src/etc/inc/plugins.inc.d/openvpn.inc#L1172

Why? I don't understand why I need set count of parameters to 2 when function proxy_sso_squid_reconfigure() has no parameters.

because the plugin configure endpoint was simplified. we can pass all kinds of arguments here, we don't have to encode the type of the call in the name of the endpoint, so "webproxy_reconfigure" becomes "webproxy":

opnsense/core@b769f35#diff-7ab5c46a2101377cc2113c978d5a9676R34

aha, understood )

fichtner · 2017-09-18T09:38:55Z

www/proxy-sso/src/opnsense/service/conf/actions.d/actions_proxysso.conf

+message:create keytab
+
+[deletekeytab]
+command:( [ ! -f /usr/local/etc/squid/squid.keytab ] && echo "No keytab file" ) || rm /usr/local/etc/squid/squid.keytab


ideally this should be like a wrapper shell script for keytab operations to avoid bouncing every tweak / change through the service configuration. but this is only a recommendation from my side :)

I did not want to produce more essences, than minimum.

it's fine for now. if it grows more complex we need to reconsider

fabianfrz

Seems like its almost OK and may be merged as soon as the change requests of @fichtner are in.

fichtner · 2017-09-19T19:02:35Z

opnsense/core#1627 was merged with a tiny bit of changes...

fabianfrz · 2017-09-20T16:58:07Z

www/proxy-sso/src/opnsense/mvc/app/controllers/OPNsense/ProxySSO/Api/ServiceController.php

+use \OPNsense\Core\Config;
+use \OPNsense\ProxySSO\ProxySSO;
+
+class ServiceController extends \OPNsense\Proxy\Api\ServiceController


Is this base controller still an requirement?

fichtner · 2017-09-25T08:50:06Z

Merged, thanks! Will fix up remaining issues post-merged.

(cherry picked from commit e2a42db) (cherry picked from commit 7416607) (cherry picked from commit 94df25e) (cherry picked from commit c75320e) (cherry picked from commit 2d603f3) (cherry picked from commit 14e1595)

evbevz added 9 commits August 30, 2017 04:55

Smart-Soft Proxy SSO plugin

b9eff68

Make config item reference in MVC manner.

d446945

Fix indentation

b106a5c

Fix indentation

e35434f

Frontend fixes: identations, styles, captions

d9e7a1a

Checklist logic tuning.

0c5c26d

UI menu XML make shorter

8a310b6

Fix post-deinstall script.

667ff35

Templates: make set variables by 'do' statement instead 'if'

548cad6

fichtner self-assigned this Sep 15, 2017

fabianfrz suggested changes Sep 16, 2017

View reviewed changes

fichtner reviewed Sep 18, 2017

View reviewed changes

fabianfrz approved these changes Sep 19, 2017

View reviewed changes

fichtner mentioned this pull request Sep 19, 2017

Add plugins_reconfigure to squid opnsense/core#1627

Merged

Adapt reconfigure action to webproxy plugins hook.

651ce70

fabianfrz reviewed Sep 20, 2017

View reviewed changes

fichtner mentioned this pull request Sep 21, 2017

web-proxy-sso: TODO list for public release #43

Closed

fabianfrz added the feature Adding new functionality label Sep 22, 2017

Fix webproxy hook.

5e4a314

fichtner merged commit e2a42db into opnsense:master Sep 25, 2017

		@@ -0,0 +1,5 @@
		rm /usr/local/etc/squid/pre-auth/20-negotiate.auth.conf

Smart-Soft Proxy SSO plugin #266

Smart-Soft Proxy SSO plugin #266

Conversation

evbevz commented Sep 15, 2017

fabianfrz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evbevz commented Sep 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evbevz Sep 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evbevz Sep 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabianfrz left a comment

Choose a reason for hiding this comment

fichtner commented Sep 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fichtner commented Sep 25, 2017

evbevz commented Sep 18, 2017 •

edited

Loading

evbevz Sep 18, 2017 •

edited

Loading

evbevz Sep 18, 2017 •

edited

Loading