[Freeipa-devel] [PATCH] 0210 frontend: fix output validation for multiple type choices

Tue Jul 19 10:40:56 UTC 2016

On 19.7.2016 12:31, Alexander Bokovoy wrote:
> On Tue, 19 Jul 2016, Jan Cholasta wrote:
>> On 19.7.2016 11:36, Alexander Bokovoy wrote:
>>> On Tue, 19 Jul 2016, Jan Cholasta wrote:
>>>> On 19.7.2016 10:40, Alexander Bokovoy wrote:
>>>>> On Tue, 19 Jul 2016, Jan Cholasta wrote:
>>>>>> On 18.7.2016 10:12, Alexander Bokovoy wrote:
>>>>>>> On Mon, 18 Jul 2016, Jan Cholasta wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> On 16.7.2016 12:46, Alexander Bokovoy wrote:
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> I had some time and was blocked by these bugs to do my tickets
>>>>>>>>> so I
>>>>>>>>> actually fixed these three problems that are assigned to Martin
>>>>>>>>> Babinsky. Hopefully, Martin wouldn't be offended by that. :)
>>>>>>>>>
>>>>>>>>> ------
>>>>>>>>>
>>>>>>>>> Output entry elements may have multiple types allowed. We need to
>>>>>>>>> check
>>>>>>>>> all of them to properly validate the output. Right now, thin
>>>>>>>>> client
>>>>>>>>> receives type specifications for elements as tuples of types, so
>>>>>>>>> what is seen as 'None' on the server side becomes (type(None),)
>>>>>>>>> tuple
>>>>>>>>> on the thin client side.
>>>>>>>>>
>>>>>>>>> Change validation to account this by processing each separate type
>>>>>>>>> of the element and account for both None and type(None). Raise
>>>>>>>>> type
>>>>>>>>> error only if all of the type checks failed.
>>>>>>>>>
>>>>>>>>> https://fedorahosted.org/freeipa/ticket/6061
>>>>>>>>
>>>>>>>> NACK, this only hides the real issue, which is that
>>>>>>>> trustconfig-show
>>>>>>>> (and automember-set-default in #6037) claims to return the primary
>>>>>>>> key
>>>>>>>> of the object in the 'value' output field, but the object does not
>>>>>>>> have a primary key, so the client rightfully expects None.
>>>>>>> Why did it work before introducing thin client?
>>>>>>
>>>>>> Because I took the liberty of not putting any extra effort just to
>>>>>> keep old hacks working on thin client. In this case,
>>>>>> output.PrimaryKey
>>>>>> was used for the 'value' output field before, which always allowed
>>>>>> unicode in addition to the primary key type. On thin client,
>>>>>> output.Output with the primary key type is used instead, which is
>>>>>> less
>>>>>> forgiving and uncovers bogus command definitions. (There aren't any
>>>>>> besides the ones already mentioned, I remember fixing them but the
>>>>>> commit got lost somehow. Oh well.)
>>>>> This means thin client would not work against old server which would
>>>>> return a non-primary key value. I think it is unacceptable.
>>>>
>>>> Not true, as this only applies to servers with API schema (4.4+).
>>> Only because thin client does not work against servers without API
>>> schema at all:
>>>
>>> [root at f24-master ~]# ipa -vv -e xmlrpc_uri=https://id.vda.li/ipa/xml
>>> config-show
>>> ipa: INFO: trying https://id.vda.li/ipa/json
>>> ipa: INFO: Request: {
>>>   "id": 0,    "method": "ping",    "params": [
>>>       [],        {}
>>>   ]
>>> }
>>> ipa: INFO: Response: {
>>>   "error": null,    "id": 0,    "principal": "admin at VDA.LI",
>>> "result": {
>>>       "messages": [
>>>           {
>>>               "code": 13001,                "message": "API Version
>>> number was not sent, forward compatibility not guaranteed. Assuming
>>> server's API version, 2.156",                "name": "VersionMissing",
>>>               "type": "warning"
>>>           }
>>>       ],        "summary": "IPA server version 4.2.3. API version 2.156"
>>>   },    "version": "4.2.3"
>>> }
>>> ipa: INFO: Forwarding 'config_show/1' to json server
>>> 'https://id.vda.li/ipa/json'
>>> ipa: INFO: Request: {
>>>   "id": 0,    "method": "config_show/1",    "params": [
>>>       [],        {
>>>           "version": "2.210"
>>>       }
>>>   ]
>>> }
>>> ipa: INFO: Response: {
>>>   "error": {
>>>       "code": 905,        "message": "unknown command 'config_show/1'",
>>>       "name": "CommandError"
>>>   },    "id": 0,    "principal": "admin at VDA.LI",    "result": null,
>>> "version": "4.2.3"
>>> }
>>> ipa: ERROR: unknown command 'config_show/1'
>>
>> Works fine for me:
>>
>> $ rpm -q freeipa-client
>> freeipa-client-4.4.0.201607180602GITa2891af3-0.fc24.x86_64
>>
>> $ ipa ping
>> -------------------------------------------
>> IPA server version 4.2.3. API version 2.156
>> -------------------------------------------
>>
>> $ ipa config-show
>>  Maximum username length: 32
>>  Home directory base: /home
>>  Default shell: /bin/sh
>>  Default users group: ipausers
>>  Default e-mail domain: abc.idm.lab.eng.brq.redhat.com
>>  Search time limit: -1
>>  Search size limit: -1
>>  User search fields: uid,givenname,sn,telephonenumber,ou,title
>>  Group search fields: cn,description
>>  Enable migration mode: FALSE
>>  Certificate Subject base: O=ABC.IDM.LAB.ENG.BRQ.REDHAT.COM
>>  Password Expiration Notification (days): 4
>>  Password plugin features: AllowNThash
>>  SELinux user map order:
>> guest_u:s0$xguest_u:s0$user_u:s0$staff_u:s0-s0:c0.c1023$unconfined_u:s0-s0:c0.c1023
>>
>>  Default SELinux user: unconfined_u:s0-s0:c0.c1023
>>  Default PAC types: nfs:NONE, MS-PAC
>>
>>
>> Try removing ~/.cache/ipa/servers/<server hostname> - if you
>> reinstalled the server in the last hour (the default cache TTL), it
>> should help.
> Yes, this did help, thanks. I didn't reinstall the server at all but I
> ran ipa command before adding CA cert of that install to my client's NSS
> database. I guess it was the cache from that attempt that broke it.

Hmm, that does not sound right. If there was no CA cert for the server, 
then it wasn't possible to talk to it, so there should not have been any 
information cached for it. I will investigate further, but it might be a 
bug in caching.

>
> Coming back to trustconfig-show, older server response's 'value' field
> is ignored. I guess we can actually remove the value completely because
> it is of no use anywhere.

That's even a better fix :-)

>
> As to other commands without primary key and returning value, may be you
> can revive your patch?

I think Martin already did that in the other thread: 
<https://www.redhat.com/archives/freeipa-devel/2016-July/msg00264.html>

-- 
Jan Cholasta