[apparmor] [PATCH v2] json support for tools (logprof and genprof)

[Christian Boltz]

Hello,

Am Mittwoch, 22. März 2017, 19:24:04 CEST schrieb Goldwyn Rodrigues:
> From: Goldwyn Rodrigues <rgoldwyn at suse.com>
> 
> This adds JSON support for tools in order to be able to talk to
> other utilities such as Yast.
> 
> The json is one per line, in order to differentiate between multiple
> records. This is based on work presented by Christian Boltz some time
> back.
> 
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn at suse.com>
> 
> ---
> Changes since v1:
>  - implementation of set_json_mode(), write_json()
>  - Changed the way output is provided to keep input the same. This
> would write in either of two formats: text or json, but will keep
> input the same. This helps in keeping localizations in place. I so
> wish UI was a class.. - Removed all yast calls.

A generic notice first: make check complained about some whitespace 
issues. Please always run "make check" in the utils directory before 
sending a patch ;-)  (even if your changes are probably not covered by 
unittests)

Speaking about tests - I'd still like to see an unittest for the json 
interface ;-)  (maybe a sample to-be-updated profile, an audit.log sniplet, 
the expected JSON, predefined answers and finally the expected profile.)


I tried to use aa-logprof --json, but all I get is:

python3 aa-logprof --json
Traceback (most recent call last):
  File "aa-logprof", line 56, in <module>
    apparmor.do_logprof_pass(logmark)
  File "/home/cb/apparmor/HEAD-clean/utils/apparmor/aa.py", line 1905, in do_logprof_pass
    aaui.UI_Info(_('Reading log entries from %s.') % logfile)
  File "/home/cb/apparmor/HEAD-clean/utils/apparmor/ui.py", line 61, in UI_Info
    write_json(jsonout)
  File "/home/cb/apparmor/HEAD-clean/utils/apparmor/ui.py", line 43, in write_json
    sys.stdout.write(json.dumps(jsonout, soft_keys=False, separators=(',', ': ')) + '\n')
  File "/usr/lib64/python3.6/json/__init__.py", line 238, in dumps
    **kw).encode(obj)
TypeError: __init__() got an unexpected keyword argument 'soft_keys'

Is that meant to be sort_keys instead of soft_keys?

After changing that, it finally worked. I get a prompt like (linebreaks 
added for readability)

{
"title": null,
"headers": {"Profil": "/usr/bin/konversation","Pfad": "/sys/devices/pci0000:00/0000:00:02.0/vendor","Neuer Modus": "r","Schweregrad": 4},
"explanation": null,
"options": ["/sys/devices/pci0000:00/0000:00:02.0/vendor r,","/sys/devices/pci*/*/vendor r,"],
"menu_items": ["Erl(a)uben","[A(b)lehnen]","(I)gnorieren","(G)lob","Glob with (E)xtension","(N)eu","Audi(t)","Abb(r)echen","En(d)e"]
}

I'm missing one thing here - the preselected option.
In most cases, the first option is preselected - but after using (G)lob,
Glob with (E)xtension or (N)ew, the newly added option will be 
selected, and the new option is in most (not all!) cases the last one.

> diff --git a/utils/apparmor/ui.py b/utils/apparmor/ui.py
> index bfbde8c..e6fcd1c 100644
> --- a/utils/apparmor/ui.py
> +++ b/utils/apparmor/ui.py

> -from apparmor.yasti import yastLog, SendDataToYast, GetDataFromYast 

It seems ui.py was the only user of yastLog(), so you can (and should)
drop it from yasti.py.

SendDataToYast and GetDataFromYast  are still called from aa.py. Please
check if you need to change something there.

From a quick look, the most important affected functions are:
- get_profile() - you'll see this in action with aa-genprof if a profile
  exists in /usr/share/apparmor/extra-profiles/ and you select 
  "view profile"
- yast_select_and_upload_profiles() - dead code because we don't have
  a profile repo currently
- save_profiles() - SendDataToYaST is used for "view changes"

There are some more functions that check for YaST - simply search aa.py
for "Yast" and "yast".

Note: Ideally, aa.py should know nothing about the UI used, so it might
be a good idea to move this code to ui.py unless that makes things 
terribly difficult.

I'd guess a (to-be-written) UI_view_in_pager() function or extending 
UI_LongMessage() to handle text mode (instead of deleting it) could 
get the job done ;-)

> @@ -47,18 +56,17 @@ def UI_Info(text):
>      debug_logger.info(text)
>      if UI_mode == 'text':
>          sys.stdout.write(text + '\n')
> -    else:
> -        yastLog(text)
> +    elif UI_mode == 'json':
> +        jsonout = {'info': text}
> +        write_json(jsonout)

What happens if someone accidently sets UI_mode to "foo"? (Hint: no output,
and you'll have a hard time to find out _why_ there isn't any output.)
I know this is unlikely, but I'd still prefer to be on the safe side.

What about:

    if UI_mode == 'json':
        ...
    else:  # text mode
        ...

You could also add an else branch that raises an exception("unknown 
UI_mode") - but that might be too much ;-)

This comment applies to several sections, so please adjust this in the
whole file.

> @@ -138,36 +140,31 @@ def UI_YesNoCancel(text, default):
> ...
> +            jsonout = {'dialog': 'yesnocancal', 'text': text, 'default': default}

As Seth already noted in v1, this should be yesnocanc_e_l ;-)

> @@ -405,7 +388,10 @@ class PromptQuestion(object):
> 
>              prompt += ' / '.join(menu_items)
> 
> -            sys.stdout.write(prompt + '\n')
> +            if UI_mode == 'json':
> +                sys.stdout.write(json.dumps(jsonprompt,
> sort_keys=False,  separators=(',', ': ')) + '\n') 

Please use write_json() ;-)


That all said - v2 looks much better than v1 :-)


Regards,

Christian Boltz
-- 
Bevor es Mißverständnisse gibt:  Ja, ich bin willenloser Lustsklave
der Göttin "Versionitis", und für ein 'ß' hinter der Versionsnummer
tue ich _alles_                               [Ratti in suse-linux]
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20170402/56a8295e/attachment.pgp>