launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Answers to your questions:

1: Good question.  I think I'd prefer to maximise the amount of information we preserve when injecting the key, so (b), and the relevant timestamp would be the modification time of the key on the filesystem.  (We don't have distinct created and updated timestamps here.)

2: We shouldn't be trying to assemble a Key object outside lp-signing, and it shouldn't be made JSON-serialisable; Key is part of its internal database model, not part of its external interface.  (b) is the correct approach here, except that you should add a "description" field as in /generate.  I also think it would be better for /inject to compute the fingerprint rather than having to be given it (since the fingerprint is redundant with the public key).

I'd add a new Key.inject constructor alongside Key.generate.  It would be a bit like Key.generate, except that instead of generating a key it would write the public key to a temporary file on disk and extract its fingerprint.  It would then call cls.new to create a Key object.

3: I don't think this is necessary.  That's for LP to keep track of.

(I've only skimmed the rest of the MP for now, since of course most of it was contingent on answers to these questions.  But it looks broadly along the lines of what I was expecting - thanks!)

Diff comments:

> diff --git a/lp_signing/webapi.py b/lp_signing/webapi.py
> index 9bc068c..9103483 100644
> --- a/lp_signing/webapi.py
> +++ b/lp_signing/webapi.py
> @@ -165,3 +165,43 @@ def sign_message():
>          "public-key": base64.b64encode(key.public_key).decode("UTF-8"),
>          "signed-message": base64.b64encode(signed_message).decode("UTF-8"),
>          }, 200
> +
> +
> +inject_api = service.api("/inject", "inject_key", methods=["POST"])
> +
> +
> +@inject_api.view(introduced_at="1.0")
> +@validate_body({
> +    "type": "object",
> +    "properties": {
> +        "key-type": {
> +            "type": "string",
> +            "enum": [item.token for item in KeyType],
> +            },
> +        "lp-key": {"type": "string"},
> +        },
> +    "required": ["key-type", "lp-key"],
> +    })
> +@validate_output({
> +    "type": "object",
> +    "properties": {
> +        "fingerprint": {"type": "string"},
> +        "public-key": {"type": "string"},  # base64
> +        },
> +    "required": ["fingerprint", "public-key"],
> +    })
> +def inject_key():
> +    payload = request.get_json()
> +    # key_type = KeyType.items[payload["key-type"]]
> +    try:
> +        key = base64.b64decode(
> +            payload["lp-key"].encode("UTF-8"), validate=True)
> +    except binascii.Error:
> +        raise DataValidationError.single("Cannot decode message")
> +
> +    key.addAuthorization(request.client)
> +    store.commit()
> +    return {
> +        "fingerprint": key.fingerprint,
> +        "public-key": base64.b64encode(key.public_key).decode("UTF-8"),

Returning the fingerprint makes sense if we're computing it, but there's probably no point in returning the public key since it's part of the request anyway.

> +        }, 200


-- 
https://code.launchpad.net/~ilasc/lp-signing/+git/lp-signing/+merge/378734
Your team Launchpad code reviewers is requested to review the proposed merge of ~ilasc/lp-signing:inject-api into lp-signing:master.