On Wed, Feb 05, 2014 at 06:36:31PM +0100, Santi Raffa wrote:
> On Wed, Feb 5, 2014 at 4:54 PM, Jose A. Lopes <[email protected]> wrote:
> >> + if field in constants.PRIVATE_PARAMETERS_BLACKLIST:
> >> + if not field.endswith("_cluster"):
> >
> > This will break as soon as someone changes the parameters list.
> > This should be documented in the constant, I guess.
>
> There kind of is no good place to document this. The parameter names
> are defined in OpParams.hs, which is very long. Maybe some sort of
> Template Haskell wizardry, later on, can assert that if Private is
> involved in the type of the variable then the resulting parameter name
> must be protected?
The parameter names are defined in the
'Ganeti.Constants.privateParametersBlacklist' module. Please add a
comment to this list explaining the difference between the parameters
and that parameter names ending with '_cluster' are treated specially.
We want to prevent people in the future from adding parameters to this
list and getting unexpected results.
> >> + data[field] = PrivateDict(value)
> >> + else:
> >> + for os in data[field]:
> >> + value[os] = PrivateDict(value[os])
> >> + else:
> >> + todo.append(value)
> >> + else: # Values
> >> + pass
> >
> > This is a strange algorithm. Have you considered doing a recursive
> > traversal of the object tree?
>
> Yes, this function was originally that and was then turned into this
> purely procedural style for performance given that this is such a
> high-traffic zone.
Please add a comment to this function explaining that it is written
like this and not recursively for performance reasons.
> >> signed_dict = LoadJson(txt)
> >> +
> >> + WrapPrivateValues(signed_dict)
> >> +
> >> if not isinstance(signed_dict, dict):
> >> raise errors.SignatureError("Invalid external message")
> >
> > Can't the line below fail if the WrapPrivateValues above returns an
> > instance of PrivateDict?
>
> Yes. Luckily, PrivateDict subclasses dict, so the check passes.
Cool!
> >> +def EncodeWithoutPrivate(obj):
> >> + if isinstance(obj, Private):
> >> + return None
> >> + raise TypeError(repr(obj) + " is not JSON serializable")
> >
> > The message should mention the Private class.
>
> These functions get passed to the JSON library and they are used
> whenever they don't know what to do with an object; in this case, they
> instruct the library to use None for Private values. For things that
> are not Private, we still don't know how to deal with them. That's why
> the generic exception is raised.
Doesn't the exception message get shown to the user?
> Here's the interdiff to be split across the relevant commits:
>
> diff --git a/lib/config.py b/lib/config.py
> index 8db03c2..2542e5b 100644
> --- a/lib/config.py
> +++ b/lib/config.py
> @@ -2490,7 +2490,7 @@ class ConfigWriter(object):
> self._BumpSerialNo()
> txt = serializer.DumpJson(
> self._config_data.ToDict(_with_private=True),
> - private_decoder=serializer.EncodeWithPrivateFields
> + private_encoder=serializer.EncodeWithPrivateFields
> )
>
> getents = self._getents()
> diff --git a/lib/rpc/client.py b/lib/rpc/client.py
> index 47ce583..5e2fb93 100644
> --- a/lib/rpc/client.py
> +++ b/lib/rpc/client.py
> @@ -121,7 +121,7 @@ def FormatRequest(method, args, version=None):
>
> # Serialize the request
> return serializer.DumpJson(request,
> -
> private_decoder=serializer.EncodeWithPrivateFields)
> +
> private_encoder=serializer.EncodeWithPrivateFields)
>
>
> def CallRPCMethod(transport_cb, method, args, version=None):
> diff --git a/lib/rpc/node.py b/lib/rpc/node.py
> index e584956..dd50817 100644
> --- a/lib/rpc/node.py
> +++ b/lib/rpc/node.py
> @@ -504,7 +504,7 @@ class _RpcClientBase:
> pnbody = dict(
> (n,
> serializer.DumpJson(prep_fn(n, encode_args_fn(n)),
> -
> private_decoder=serializer.EncodeWithPrivateFields))
> +
> private_encoder=serializer.EncodeWithPrivateFields))
> for n in node_list
> )
>
> diff --git a/lib/serializer.py b/lib/serializer.py
> index b5b6353..5230c21 100644
> --- a/lib/serializer.py
> +++ b/lib/serializer.py
> @@ -45,17 +45,20 @@ from ganeti import constants
> _RE_EOLSP = re.compile("[ \t]+$", re.MULTILINE)
>
>
> -def DumpJson(data, private_decoder=None):
> +def DumpJson(data, private_encoder=None):
> """Serialize a given object.
>
> @param data: the data to serialize
> @return: the string representation of data
> + @param private_encoder: specify L{serializer.EncodeWithPrivateFields} if
> you
> + require the produced JSON to also contain private
> + parameters. Otherwise, they will encode to null.
>
> """
> - if private_decoder is None:
> + if private_encoder is None:
> # Do not leak private fields by default.
> - private_decoder = EncodeWithoutPrivate
> - encoded = simplejson.dumps(data, default=private_decoder)
> + private_encoder = EncodeWithoutPrivateFields
> + encoded = simplejson.dumps(data, default=private_encoder)
>
> txt = _RE_EOLSP.sub("", encoded)
> if not txt.endswith("\n"):
> @@ -81,6 +84,11 @@ def LoadJson(txt):
>
>
> def WrapPrivateValues(json):
> + """Crawl a JSON decoded structure for private values and wrap them.
> +
> + @param json: the json-decoded value to protect.
> + """
> +
Newlines here are not correct:
"""Blah.
Blah.
"""
code
> todo = [json]
>
> while todo:
> @@ -111,17 +119,18 @@ def WrapPrivateValues(json):
>
>
> def DumpSignedJson(data, key, salt=None, key_selector=None,
> - private_decoder=None):
> + private_encoder=None):
> """Serialize a given object and authenticate it.
>
> @param data: the data to serialize
> @param key: shared hmac key
> @param key_selector: name/id that identifies the key (in case there are
> multiple keys in use, e.g. in a multi-cluster environment)
> + @param private_encoder: see L{DumpJson}
> @return: the string representation of data signed by the hmac key
>
> """
> - txt = DumpJson(data, private_decoder=private_decoder)
> + txt = DumpJson(data, private_encoder=private_encoder)
> if salt is None:
> salt = ""
> signed_dict = {
> @@ -364,7 +373,7 @@ class PrivateDict(dict):
> return returndict
>
>
> -def EncodeWithoutPrivate(obj):
> +def EncodeWithoutPrivateFields(obj):
> if isinstance(obj, Private):
> return None
> raise TypeError(repr(obj) + " is not JSON serializable")
> diff --git a/src/Ganeti/Constants.hs b/src/Ganeti/Constants.hs
> index 961e3ba..f300981 100644
> --- a/src/Ganeti/Constants.hs
> +++ b/src/Ganeti/Constants.hs
> @@ -4754,7 +4754,8 @@ glusterPortDefault = 24007
Comment here about each of these parameters and that parameters ending
with '_cluster' are special when it comes to
serialization/deserialization.
Thanks,
Jose
> privateParametersBlacklist :: [String]
> privateParametersBlacklist = [ "osparams_private"
> , "osparams_secret"
> - , "osparams_private_cluster" ]
> + , "osparams_private_cluster"
> + ]
>
> -- | Warn the user that the logging level is too low for production use.
> debugModeConfidentialityWarning :: String
> diff --git a/test/hs/Test/Ganeti/OpCodes.hs b/test/hs/Test/Ganeti/OpCodes.hs
> index 714d39a..3591480 100644
> --- a/test/hs/Test/Ganeti/OpCodes.hs
> +++ b/test/hs/Test/Ganeti/OpCodes.hs
> @@ -590,7 +590,7 @@ case_py_compat_types = do
> \ for op in decoded]\n\
> \print serializer.Dump(\
> \ encoded,\
> - \ private_decoder = serializer.EncodeWithPrivateFields)"
> + \ private_encoder=serializer.EncodeWithPrivateFields)"
> serialized
> >>= checkPythonResult
> let deserialised =
> diff --git a/test/py/ganeti.serializer_unittest.py
> b/test/py/ganeti.serializer_unittest.py
> index 4a7874c..20ee4f1 100755
> --- a/test/py/ganeti.serializer_unittest.py
> +++ b/test/py/ganeti.serializer_unittest.py
> @@ -57,7 +57,7 @@ class TestSerializer(testutils.GanetiTestCase):
> def _TestSerializer(self, dump_fn, load_fn):
> _dump_fn = lambda data: dump_fn(
> data,
> - private_decoder=serializer.EncodeWithPrivateFields
> + private_encoder=serializer.EncodeWithPrivateFields
> )
> for data in self._TESTDATA:
> self.failUnless(_dump_fn(data).endswith("\n"))
> @@ -78,7 +78,7 @@ class TestSerializer(testutils.GanetiTestCase):
> def _TestSigned(self, dump_fn, load_fn):
> _dump_fn = lambda *args, **kwargs: dump_fn(
> *args,
> - private_decoder=serializer.EncodeWithPrivateFields,
> + private_encoder=serializer.EncodeWithPrivateFields,
> **kwargs
> )
> for data in self._TESTDATA:
> @@ -201,7 +201,7 @@ class TestPrivate(unittest.TestCase):
> self.assertIn(
> "egg",
> serializer.Dump(pDict,
> - private_decoder=serializer.EncodeWithPrivateFields)
> + private_encoder=serializer.EncodeWithPrivateFields)
> )
>
> def testDictGet(self):
>
>
> --
> Raffa Santi
> Google Germany GmbH
> Dienerstr. 12
> 80331 München
>
>
> Registergericht und -nummer: Hamburg, HRB 86891
> Sitz der Gesellschaft: Hamburg
> Geschäftsführer: Graham Law, Christine Elizabeth Flores
--
Jose Antonio Lopes
Ganeti Engineering
Google Germany GmbH
Dienerstr. 12, 80331, München
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Graham Law, Christine Elizabeth Flores
Steuernummer: 48/725/00206
Umsatzsteueridentifikationsnummer: DE813741370
