Change in vdsm[ovirt-3.5]: jsonrpc: filter params while logging secure method invocation

nsoffer at redhat.com nsoffer at redhat.com
Sat May 16 20:31:55 UTC 2015


Nir Soffer has posted comments on this change.

Change subject: jsonrpc: filter params while logging secure method invocation
......................................................................


Patch Set 2:

(3 comments)

https://gerrit.ovirt.org/#/c/40961/2/lib/yajsonrpc/__init__.py
File lib/yajsonrpc/__init__.py:

Line 494
Line 495
Line 496
Line 497
Line 498
Same:

    if req.method in self.SECURE_RETURN:
        protected_res = protect_passwords(picklecopy(res))
    else:
        protected_res = res

And log protected_res instead of res


Line 454:                    'StoragePool.connectStorageServer',
Line 455:                    'StoragePool.disconnectStorageServer',
Line 456:                    'StoragePool.validateStorageServerConnection',
Line 457:                    'VM.desktopLogin',
Line 458:                    'VM.setTicket')
Host_fenceNode is missing - logging it only in TRACE level is not good enough.

Same for virt methods containing passwords - please look at the master patch to find the methods that need protection.
Line 459: 
Line 460:     SECURE_RETURN = ('Host.getDeviceList')
Line 461: 
Line 462:     def __init__(self, bridge, threadFactory=None):


Line 474:                              'Host_getStats', 'StorageDomain_getStats',
Line 475:                              'VM_getStats', 'Host_fenceNode'):
Line 476:             logLevel = logging.TRACE
Line 477:         self._log_message(logLevel, 'Calling', self.SECURE_ARGS, req.method,
Line 478:                           req.params)
This works, but is more complicated than needed, and is harder to follow. I think the only change needed is:

    if req.method in self.SECURE_ARGS:
        protected_params = protect_passwords(picklecopy(req.params))
    else:
        protected_params = req.params

And log protected_params instead of req.params. No need to add _log_message to log two different messages that need different protection.

If you like to keep the code cleaner, I think this can help:

    def protected_params(self, method, args):
        if method in self.SECURE_ARGS:
            return protect_passwords(picklecopy(args))
        return args

    def _protected_result(self, method, result):
        if method in self.SECURE_RETURN:
            return protect_passwords(picklecopy(result))
        return result

Then you just do:

    protected_params = self._protected_args(req.method, req.params)

Or:

    protected_res = self._protected_result(req.method, res)

We don't want to mix logging formatting and password protection logic.
Line 479: 
Line 480:         try:
Line 481:             method = getattr(self._bridge, mangledMethod)
Line 482:         except AttributeError:


-- 
To view, visit https://gerrit.ovirt.org/40961
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I34a36a361d2be28343352ae83ceac94b86162a12
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: ovirt-3.5
Gerrit-Owner: Piotr Kliczewski <piotr.kliczewski at gmail.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski at gmail.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list