[Freeipa-devel] [PATCH] 1106 IPA REST smart proxy

[Rob Crittenden]

Petr Viktorin wrote:
> On 03/10/2014 08:55 PM, Rob Crittenden wrote:
>> Rob Crittenden wrote:
>>> Petr Viktorin wrote:
>>>> On 02/27/2014 10:18 PM, Rob Crittenden wrote:
>>>>> Rob Crittenden wrote:
>>>>> Updated patch based on feedback from Foreman team. I added a new URI,
>>>>> /features, which Foreman uses to determine what capabilities a proxy
>>>>> has.
>>>>>
>>>>> rob
>>>>
>>>> On my VMs, where the first request is handled properly but the server
>>>> hangs on the second one. I gave you access to the machines for
>>>> investigation.
>>>
>>> Sent you something out-of-band but in short, I wasn't able to reproduce
>>> on your reproducing VMs :-( Ping me tomorrow and we'll try it together.
>
> It ended up a combination of my mistake and a bug in GSSProxy. At least
> you found the bug. https://fedorahosted.org/gss-proxy/ticket/121
>
>>>> Please add the Python libraries (python-cherrypy, python-requests,
>>>> python-kerberos) to BuildRequires. Otherwise the build fails due to
>>>> pylint errors.
>>>
>>> Fixed.
>>>
>>>>
>>>> In the man page:
>>>>
>>>>> +Create a host or user whose credentials will be used by the server to
>>>>> make requests and add it to the role:
>>>>> +
>>>>> + $ ipa user\-add \-\-first=Smartproxy \-\-last=Serversmartproxy
>>>>> + $ ipa role\-add\-member \-\-users=smartproxy 'Smartproxy management'
>>>>
>>>> the first command should be
>>>>      ipa user-add smartproxy --first=Smartproxy --last=Serversmartproxy
>>>> since by default the username is 'sserversmartproxy'.
>>>
>>> The problem was a missing space before smartproxy. I have the login name
>>> last in this example. Fixed.
>>>
>>>>
>>>> A nitpick regarding the systemd service: according to [0], Type=forking
>>>> should be avoided. Is there a reason against setting Type=simple, and
>>>> removing the PID file?
>>>
>>> Because I wasn't able to get this working with cherrypy daemon mode.
>>> AFAICT it forks itself and systemd wouldn't end up with the right pid so
>>> can't stop the service.
>>
>> And now the updated patch. The changes are super-minor.
>>
>> rob
>>
>
> One last nitpick: the IPAErrors get encoded as JSON but the
> Content-Encoding is set to text/html. It's a one-line change so I went
> ahead and tested with it. ACK from me if you agree.

+1

Here are a couple more enhancements I'm considering, this seems simpler 
than inter-diff since it is so small. Here is why I made the changes, in 
order:

I doubled the calls to create the connection but one isn't in a 
try/except!? Remove the obvious one.

We currently completely eat GSSAPI errors, I figure we should log failures.

IPA stores the principal in the request context so using that will save 
a GSSAPI call (and as we learned, a lock in gssproxy).

I included your content-type change.

rob


diff --git a/smartproxy/ipa-smartproxy b/smartproxy/ipa-smartproxy
index d5e8f22..ba50cef 100755
--- a/smartproxy/ipa-smartproxy
+++ b/smartproxy/ipa-smartproxy
@@ -32,6 +32,7 @@ from cherrypy.process import plugins
  from ipalib import api
  from ipalib import errors
  from ipalib import util
+from ipalib.request import context
  from ipaserver.rpcserver import json_encode_binary
  from ipapython.version import VERSION

@@ -83,12 +84,12 @@ def Command(command, *args, **options):
              message="Not a local request"
          )

-    if not api.Backend.rpcclient.isconnected():
-        api.Backend.rpcclient.connect()
      try:
          if not api.Backend.rpcclient.isconnected():
              api.Backend.rpcclient.connect()
      except errors.CCacheError, e:
+        cherrypy.log(msg="Kerberos error: %s" % e,
+            context='IPA', traceback=False)
          raise IPAError(
              status=401,
              message=e
@@ -171,9 +172,11 @@ class IPAError(cherrypy.HTTPError):
              error = {'message':
                       'Unable to handle error message type %s' % 
type(self._mess
age)}

+        principal = getattr(context, 'principal', None)
+        response.headers["Content-Type"] = "application/json;charset=
utf-8"
          response.body = json.dumps({'error': error,
                                      'id': 0,
-                                    'principal': util.get_current_principal
(),
+                                    'principal': principal,
                                      'result': None,
                                      'version': VERSION},
                                      sort_keys=True, indent=2)