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 <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Federico Simoncelli <[email protected]> Gerrit-Reviewer: Francesco Romani <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: Piotr Kliczewski <[email protected]> Gerrit-Reviewer: Yaniv Bronhaim <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
