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

[Rob Crittenden]

Petr Viktorin wrote:
> On 01/23/2014 02:17 PM, Petr Viktorin wrote:
>> On 01/22/2014 08:04 PM, Rob Crittenden wrote:
>>> Petr Viktorin wrote:
>>>> On 01/20/2014 05:21 PM, Rob Crittenden wrote:
>>>>> Petr Viktorin wrote:
>>>>>> On 01/17/2014 10:24 PM, Rob Crittenden wrote:
>>>>>>> Implement an IPA RESTful Foreman-compatible smart proxy. This
>>>>>>> exposes
>>>>>>> hosts and hostgroups via an unauthenticated REST API. The idea is
>>>>>>> that
>>>>>>> this service runs on the Foreman server and only listens on local
>>>>>>> ports.
>>>>>>>
>>>>>>> It is a CherryPy-based server and that handles the majority of REST
>>>>>>> for us.
>>>>>>>
>>>>>>> I included some tests, they can be executed with: nosetests -v
>>>>>>> smartproxy/tests
>>>>>>
>>>>>> Why is it not a part of ipatests?
>>>>>
>>>>> I can move it if it's a show-stopper. It seemed specific to this one
>>>>> directory so I stuck it there. It isn't relevant for most testing and
>>>>> requires some manual configuration (though CI could handle it).
>>>>
>>>> Not strictly a show stopper, but please move it. At the very least it
>>>> should end up in the freeipa-tests package.
>>>
>>> Moved.
>>
>> Thanks!
>>
>>>> A lot of the tests (integration, webUI) need manual configuration, so
>>>> this would be no exception. Of course the tests should be skipped if
>>>> the
>>>> configuration was not done, and the config instructions should be added
>>>> to/linked from http://www.freeipa.org/page/Testing
>>>
>>> Hmm, maybe. There are instructions to set up the environment in the man
>>> page. Testing beyond that consists of ./make-test tests/test_smartproxy
>>>
>>> I can add that testing bit once the patch is approved I suppose.
>>>
>>> rob
>>
>> Please add python-kerberos >= 1.1-13 to Requires and BuildRequires;
>> pylint fails with lower versions.
>> Are there plans to release python-kerberos-1.1-13.fc20, or will this be
>> f21+ only?
>>
>
>
> I've noticed gssproxy-0.3.1 is not available in rawhide (but it is in
> f20). I've sent a mail to Günther; meanwhile I'll use the f20 package
> (may the releng gods be merciful).
>
>
> Here are my thoughts on the patch.
>
> The URL endpoint /ipa/rest suggests that if we implement a complete REST
> API for IPA it would live here. Is the API supposed to be
> future-compatible? (The API itself seems to be a good subset of a
> complete REST API, but can we easily add an frontend with
> authentication, i18n, etc. here later, and keep the limitations for
> unauthenticated access?)
> Perhaps /ipa/smartproxy would be a better choice?

It was future-proofing. I'm fine with changing the URI, it is probably a 
good thing to save that name.

>
> I assume we're in control of the REST API design. Is that correct?
> According to RFC 2616, the POST verb should create a sub-resource of
> what's at the given URI. In other words, it would be more correct to use:
>      /ipa/rest/host        POST  Create a host entry  {'hostname': ...}
> and same for hostgroup.
> Foreman's DHCP and DNS APIs seem to do this right.

Sure, mistake on my part.

> Is returning HTML for errors a Foreman requirement? I think it's a weird
> thing to do in an API.

Yeah, beats me. I couldn't find anything defining the output format so I 
figured I'd return it in something renderable by a browser. I'm open to 
suggestions. I guess there is no reason it can't be json as well.

> I don't think IPACommand should be a class. It has no state, and it
> looks to me like if one of GET/POST/DELETE happens to not be overridden
> in an exposed subclass, there's a potential security issue -- any IPA
> command can be called. Could you use simple functions instead?

I guess so. I don't see it as a security risk though, as it would 
require changing the module and we already specify that this is 
unauthenticated, so if someone can hack the binary, they can already 
call arbitrary commands, class or no class. Still, the user will be 
limited to what they can do on the IPA side.

> In IPACommand.Command, there's lot of duplicate error handling blocks.
> You can handle more error classes at once with:
>      except (errors.DuplicateEntry, errors.DNSNotARecordError) as e:
> Instead of `str(e)`, please report the errors as
> "{e.__class__.__name__}: {e}" -- the message is sometimes unusable
> without the exception type (e.g. a KeyError from {}['result'] would only
> have 'result' as message).

I did that in an attempt of readability since there is so much overlap. 
I'll take another look and see what it looks like combined again.

> Passing *args, **options to Command closes the door for any (future)
> parametrization. Consider making the signature e.g. Command(self,
> command, args, options, success_stastus=200).

Not sure I follow but I'll take a look.

> Instead of `if fqdn == None:` use `if fqdn is None:`.

Arg!

> It would be great if you passed `api` around explicitly, instead of
> relying on the global instance.

Sure.

>
> Some import issues in test_smartproxy/resttest.py:
> resttest.py:24:8:  Unused import 'sys'
> resttest.py:25:8:  Unused import 'socket'
> resttest.py:26:8:  Unused import 'nose'
>
> `requests` is a third-party library, python-requests in Fedora; it
> should be in the specfile. (Dogtag pulls it in for now, but when
> ipa-server-ca is a separate package things will break.)
> According to PEP8:
>> Imports should be grouped in the following order:
>>     1. standard library imports
>>     2. related third party imports
>>     3. local application/library specific imports [ipa* in our case]
>> You should put a blank line between each group of imports.
> Normally I'm not pedantic about this, but it does help spotting mistakes
> like the above.

I don't know that I'd have caught the spec thing. I think the tests 
change was due to my moving things around, I just didn't re-run pylint 
against it before resubmitting.

> I'm not a fan of the test code copied from test_xmlrpc, but I guess we
> have better things to do than refactoring that :(

I'll see if I can be less lazy. That code was copied when it was outside 
of ipatests, I can probably drop most if not all of that.

> But there should at least be a check that skips the tests if the smart
> proxy is not available.

I figured that since this was run only when requested that sort of takes 
care of itself. I'll see about adding a test for the port or something.

>
> In the ipa-rest man page, "Add this to the top of the file, before any
> other services" doesn't specify what file should be edited.
> Also, maybe the page should mention that `systemd start
> ipa-rest.service` starts the server on port 8090.

Sure.

>
> It's probably an error at my side, but I couldn't get a response from
> the server: I got:
>     <html><body>The server encountered an unexpected condition which
> prevented it from fulfilling the request.</body></html>
> and in the log I see:
>     CCacheError: did not receive Kerberos credentials
> I'm not familiar with gssproxy, but I don't see how it knows that it
> should authenticate as the "rest" user.
>
> Should /var/log/ipa-rest.errors be world-readable?
>
> What is gssproxy.conf.snippet for? The same info is in the man page.

gssproxy is magic. Basically you configure a keytab with the 
credentials, then configure gssproxy to handle those credentials (e.g. 
the bits from the man page) then whenever there is a GSS need for those 
credentials, gssproxy steps in and handles everything for you. You'll 
notice that the smartproxy can't even read its own keytab. It doesn't 
need to!

So just configure gssproxy, /etc/gssproxy/gssproxy.conf, per the map 
page, with 0.3.1 and restart both daemons and it should just work.

>
> Also please fix:
> ./ipalib/util.py:61:56: E703 statement ends with a semicolon
> ./ipalib/util.py:63:54: E703 statement ends with a semicolon

Bleh, sure.

thanks

rob