Implement basic form of `snapcraft register-key`

[cjwatson]

This adds a basic register-key command that can be used to register keys
from your snap keyring.

We don't yet have all the prescribed error modes in place, and nor are
we able to tell which keys are already enabled. That requires some more
work on the store side.

[snappy-m-o]

Can one of the admins verify this patch?

[snappy-m-o]

Can one of the admins verify this patch?

[cjwatson]

Note that https://code.launchpad.net/~cjwatson/snap-assertions-service/public-key-headers-endpoint/+merge/302612 and https://code.launchpad.net/~cjwatson/software-center-agent/less-terrible-account-key/+merge/302789 need to be deployed before we can go ahead with this, but it should be possible to review this in the meantime to speed things up.

[sergiusens]

Seems travis needs some deps installed

  Running setup.py install for pygpgme ... error
    Complete output from command /usr/bin/python3 -u -c "import setuptools, tokenize;__file__='/tmp/pip-build-c3o2kata/pygpgme/setup.py';exec(compile(getattr(tokenize, 'open', open)(__file__).read().replace('\r\n', '\n'), __file__, 'exec'))" install --record /tmp/pip-iwyk7pry-record/install-record.txt --single-version-externally-managed --compile:
    running install
    running build
    running build_py
    creating build
    creating build/lib.linux-x86_64-3.5
    creating build/lib.linux-x86_64-3.5/gpgme
    copying gpgme/editutil.py -> build/lib.linux-x86_64-3.5/gpgme
    copying gpgme/__init__.py -> build/lib.linux-x86_64-3.5/gpgme
    running build_ext
    building 'gpgme._gpgme' extension
    creating build/temp.linux-x86_64-3.5
    creating build/temp.linux-x86_64-3.5/src
    x86_64-linux-gnu-gcc -pthread -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -g -fstack-protector-strong -Wformat -Werror=format-security -Wdate-time -D_FORTIFY_SOURCE=2 -fPIC -I/usr/include/python3.5m -c src/gpgme.c -o build/temp.linux-x86_64-3.5/src/gpgme.o
    In file included from src/gpgme.c:22:0:
    src/pygpgme.h:24:19: fatal error: gpgme.h: No such file or directory
    compilation terminated.
    error: command 'x86_64-linux-gnu-gcc' failed with exit status 1

[cjwatson]

Thanks. I'll fix that in a bit - the store interface is moving quite quickly here so I want to get that settled down first.

[sergiusens]

El 16/08/16 a las 13:10, Colin Watson escribió:

Thanks. I'll fix that in a bit - the store interface is moving quite
quickly here so I want to get that settled down first.

Indeed, not a stranger to that story :-)

[elopio]

we usually try to avoid variable names like this. Specially for somebody like me who doesn't speak great English, ctx is less readable than context.

[elopio]

could you group these four conditions in a function with a nice name explaining what this check is about?
like if not _is_usable_key(key), and then in _is_usable_key a comment about what makes a key usable.

[elopio]

Here too, it would be nice to group the statement in a function. _is_mainkey_usable? I'm not sure about the name.

[elopio]

I would move this to a function too, but I tend to write way too many functions :p I won't complain about this one, just saying...

[elopio]

Maybe display is not the right word here, because it's not displaying anything. What about _get_formatted_fingerprint or _format_fingerprint ?

[elopio]

I don't like assertions. I think it's better to make an if and raise the exception explicitly. The "assert" statement can be disabled and the "if" can't, but that's not likely to happen. My main problem with assertions is that they are intended to discover errors from the programmer, things that should never happen. And for me, that's what unit tests are for. So if you already have a test that makes sure this condition can't happen, the statement is not useful. And if this can happen during the normal operation of the program, that's what exceptions are for, and they are clearer.

[elopio]

Here again, display is not the right verb. get or format seem better.

[elopio]

s/ctx/context
On the line above I was wondering what fpr was, then I saw you assigned it to "fingerprint", that's lovely :D

[elopio]

@sergiusens this feels like a decorator, right? We are duplicating it in many places, we could just tag the functions that require authentication instead.

[sergiusens]

El 16/08/16 a las 23:01, Leo Arias escribió:

In snapcraft/_store.py
#726 (comment):

+def register_key(query):

• keys = list(_get_usable_secret_keys(query=query))
• if not keys:

•    raise RuntimeError(
  

•        'You have no usable GPG secret keys matching "{}".'.format(query))
  

• key = _select_key(keys)
• fingerprint = key.subkeys[0].fpr
• ctx = gpgme.Context()
• key_data = io.BytesIO()
• ctx.export(fingerprint.encode('ascii'), key_data)
• logger.info('Registering GPG key ...')
• store = storeapi.StoreClient()
• try:

•    store.register_key(key_data.getvalue())
  

• except storeapi.errors.InvalidCredentialsError:

@sergiusens https://github.com/sergiusens this feels like a decorator,
right? We are duplicating it in many places, we could just tag the
functions that require authentication instead.

Maybe, let's look into that after this lands.

[elopio]

"expected" sounds confusing to me. What do you think about: "The GPG key ... will be used to sign your assertions"? I'm not sure it's better though.

[elopio]

This is great, thanks for sharing this @cjwatson !

How hard would it be to write an integration test for the happy path? We are using pyexpect, which makes it easy to handle the input. But I'm wondering about creating the gpg key to be selected. We would have to patch the ~/.gnupg path to use a temp file and maybe we can insert a fake key in there.

[cjwatson]

I've refactored this to use the new snap key management interfaces (https://docs.google.com/document/d/1Bkn9sLgXqkGsgV1eBWO935z5jZJiJQCL24muqw2zD7U). I know that Travis is currently failing; I need to get it to install snapd, but I need to wait for a new enough version to land in xenial-updates first.

At least a basic integration test should be feasible, and I'm working on that now.

[sergiusens]

https://bugs.launchpad.net/snapcraft/+bug/1612730

[cjwatson]

retest this please

[josepht]

Colin said 'retest this please'

[sergiusens]

ok to test

[elopio]

is there a bug to track this?

[cjwatson]

I've filed https://bugs.launchpad.net/bugs/1621441 and added a reference here.

[elopio]

When I run register-key without a key, I get:

./bin/snapcraft register-key
'NoneType' object is not iterable

[elopio]

can you please use format instead?

[elopio]

please use format.

[cjwatson]

The register-key 'NoneType' object is not iterable error is really a snapd bug, and I've sent a fix for that as snapcore/snapd#1869. I've also made a change on this side to tolerate the peculiar output.