launchpad-reviewers team mailing list archive

["Thiago F. Pappacena" <pappacena@xxxxxxxxx>]

LGTM. Just added a couple of minor notes for coding style.

Let's see wgrant's opinion.

Diff comments:

> diff --git a/lp_signing/auth.py b/lp_signing/auth.py
> index e78a46b..f73ce77 100644
> --- a/lp_signing/auth.py
> +++ b/lp_signing/auth.py
> @@ -78,6 +85,28 @@ class BoxedRequest(Request):
>              super().is_json or
>              self.mimetype == "application/x-boxed-json")  # pragma: no cover
>  
> +    def _get_client_public_key(self):

This method seems to be called outside of this case. Wouldn't it be better to remove the underscore in the beginning?

Same for _get_nonce, as BoxedResponse seems to be using it too.

> +        try:
> +            header = self.headers["X-Client-Public-Key"]
> +        except KeyError:
> +            raise DataValidationError.single("Client public key not provided")
> +        try:
> +            return PublicKey(header.encode("UTF-8"), encoder=Base64Encoder)
> +        except Exception:
> +            raise DataValidationError.single("Cannot decode client public key")
> +
> +    def _get_nonce(self, header_name):
> +        try:
> +            header = self.headers[header_name]
> +        except KeyError:
> +            raise DataValidationError.single(
> +                "{} header not provided".format(header_name))
> +        try:
> +            return base64.b64decode(header.encode("UTF-8"), validate=True)
> +        except Exception:
> +            raise InvalidNonce.single(
> +                "Cannot decode {} header".format(header_name))
> +
>      def _get_data_for_json(self, cache):
>          """Read, authenticate, and decrypt incoming data.
>  
> @@ -95,27 +127,17 @@ class BoxedRequest(Request):
>              failed.
>          """
>          data = self.get_data(cache)
> -        if ("X-Client-Public-Key" not in self.headers or
> -                "X-Nonce" not in self.headers):
> -            raise DataValidationError.single("Request not authenticated")
> -        try:
> -            client_public_key = PublicKey(
> -                self.headers["X-Client-Public-Key"].encode("UTF-8"),
> -                encoder=Base64Encoder)
> -        except Exception:
> -            raise DataValidationError.single("Cannot decode client public key")
> -        try:
> -            nonce = base64.b64decode(
> -                self.headers["X-Nonce"].encode("UTF-8"), validate=True)
> -        except Exception:
> -            raise InvalidNonce.single("Cannot decode nonce")
> +        client_public_key = self._get_client_public_key()
> +        nonce = self._get_nonce("X-Nonce")

I would prefer to have a `_get_request_nonce()` and `_get_response_nonce()` (using the original `_get_nonce`) to avoid hard-coding the header name around, but since it seems to be only used once for each case (`X-Nonce` and `X-Response-Nonce`), this is fine too.

>          Nonce.check(nonce)
> -        for private_key in boxed_authentication.service_private_keys:
> +        for i, private_key in enumerate(
> +                boxed_authentication.service_private_keys):
>              try:
>                  box = Box(private_key, client_public_key)
>                  data = box.decrypt(data, nonce, encoder=Base64Encoder)
>              except Exception:
>                  continue
> +            self.private_key_index = i
>              self.client = Client.getByPublicKey(client_public_key)
>              if self.client is None:
>                  raise DataValidationError.single(


-- 
https://code.launchpad.net/~cjwatson/lp-signing/+git/lp-signing/+merge/381514
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/lp-signing:encrypt-responses into lp-signing:master.