Ditching GnuPG #261

silverdaz · 2018-03-07T17:57:23Z

We totally remove the dependency towards GnuPG (2.1) (even in the bootstrap step)

There is no need to use the gpg-agent, the socket forwarder/proxy and patch the GnuPG binary to locate the agent socket in a non-temporary location on disk. Updated RPMs are not necessary anymore.

We created a python script that follows RFC4880 for decryption. The script does not load the whole file to decrypt in memory, but it streams it. This allows us to handle huge files.
The code uses generators: A simple way to instantiate a function and run it step-by-step. This is how the script "produces" decrypted data as it goes.

The keyserver was also updated and answers request over HTTPS. This resolves issue #259.
The ReST endpoints are easy to adjust (if necessary) and described in the extracted documentation.

Finally, for the docker deployment, we removed many images. We keep only one base image (an upgraded centos-base including python) and the inbox image. The Terraform deployment is also updated accordingly.

Run the following asciinema demo to see it in action.

No GnuPG. Just Python.

The private key material is decrypted. It is up to the library creating the keys to parse the bytes streams and retrieve the MPIs (along with the key type). See section 5.5 of RFC4880 (https://tools.ietf.org/html/rfc4880#section-5.5)

No GnuPG. Just Python.

The private key material is decrypted. It is up to the library creating the keys to parse the bytes streams and retrieve the MPIs (along with the key type). See section 5.5 of RFC4880 (https://tools.ietf.org/html/rfc4880#section-5.5)

blankdots · 2018-03-08T06:22:19Z

deployments/terraform/instances/inbox/boot.sh

+
+pip3.6 uninstall -y lega
+pip3.6 install pika==0.11.0 fusepy
+pip3.6 install git+https://github.com/NBISweden/LocalEGA.git@feature/pgp


maybe the feature/pgp branch should not be hardcoded?

True. But it must be right now in the PR, and then another PR will follow when that PR is merged into dev.

blankdots · 2018-03-08T06:23:50Z

deployments/terraform/instances/vault/cloud_init.tpl

@@ -53,6 +53,7 @@ bootcmd:
 runcmd:
  - mkfs -t btrfs -f /dev/vdb
  - pip3.6 uninstall -y lega
+  - pip3.6 install pika==0.11.0 psycopg2==2.7.3.2
  - pip3.6 install git+https://github.com/NBISweden/LocalEGA.git@feature/inbox-fuse


is there a branch feature/inbox-fuse ?

There was :p .... and that should be updated. Good catch!

blankdots · 2018-03-08T06:55:09Z

deployments/terraform/instances/workers/cloud_init.tpl

-  - pip3.6 install git+https://github.com/NBISweden/LocalEGA.git@feature/inbox-fuse
-  - systemctl start ega-socket-forwarder.service ega-socket-forwarder.socket
-  - systemctl enable ega-socket-forwarder.service ega-socket-forwarder.socket
+  - pip3.6 install pika==0.11.0 pycryptodomex==3.4.7 psycopg2==2.7.3.2 cryptography==2.1.3


psycopg2==2.7.4

dtitov

Great job, but I have to mention that we should probably try to avoid such a huge PR's: it's really hard to review everything at once.

dtitov · 2018-03-08T12:41:33Z

.travis.yml

@@ -7,10 +9,12 @@ before_install:
  # https://elk-docker.readthedocs.io/#es-not-starting-max-map-count
  # mostly used by ELK stack; Solving issue #252
  # - sudo sysctl -w vm.max_map_count=262144
+  - pip3.6 install pgpy


Why do we need Python on the "host" machine? Is it for replacing PGP keys generation in bootstrapping from GPG to Python implementation? Do we really need this replacement? I feel like it's redundant because GPG is pre-installed on Ubuntu and we don't need to install anything else.

No we don't, but here is the issue we fixed with it.
I explain.

We can use GnuPG to generate a key (as we used to do) and export the public key. However, to export the secret key, we need to be at the prompt to type the passphrase. So, that doesn't work for us (and tell me how we could make it work if you know), so we generate the public/private keys in another way (using the PGPy python module).

I see. Isn't that what we need for this to work: https://www.gnupg.org/documentation/manuals/gnupg/Unattended-GPG-key-generation.html ?

No, it was exactly what we used to generate the key.
That generates a keyring, and you can export the public from it.
However, to export the secrete key, the passphrase is needed (and prompted).

dtitov · 2018-03-08T12:45:26Z

deployments/docker/bootstrap/instance.sh

-chmod 744 ${PRIVATE}/${INSTANCE}/gpg/public.key
-rm -f ${PRIVATE}/${INSTANCE}/gen_key
-${GPG_CONF} --kill gpg-agent
+# Python 3.6


The same concern as above: I'm not sure if we need to replace PGP implementation on the host machine for bootstrapping.

dtitov · 2018-03-08T12:48:14Z

deployments/docker/images/common/Dockerfile

-FROM centos:7.4.1708
-LABEL maintainer "Frédéric Haziza, NBIS"
+FROM python:3.6-alpine3.4
+LABEL maintainer "NBIS SysDevs"


I've been waiting for this to happen for so many months :) Finally, the maintainer label is expanded to more than one person :)

viklund

It's hard to have good comments on the code since there's so much bitwhacking and such.

This looks to me like a more or less one to one implementation of the pgp protocol. It would be nice with a highlevel overview on how the pgp decryption thing works. Explain what a packet is for example. And have some idea about the flow through the different pieces of the code. I usually don't ask for documentation but I think it would be nice with more documentation on all of the packet classes.

viklund · 2018-03-08T14:53:06Z

lega/openpgp/utils.py

+    return data
+
+
+# See 3.7.1.3


3.7.1.3 what?

RFC 4880

That's the one describing the OpenPGP standard.

Ok, so the comment can be

# See 3.7.1.3 of RFC 4880

viklund · 2018-03-08T15:01:38Z

lega/openpgp/__main__.py

+                    LOG.debug("###### Decrypting session key")
+                    # Note: decrypt_session_key does not know yet the key ID.
+                    #       It will parse the packet and then contact the keyserver
+                    #       to retrieve the private_key material


I got confused by this comment. I would like something on the lines of:

Note: decrypt_session_key does not know yet the key ID. It will parse the packet and then use the provided function, fetch_private_key, to retrieve the private_key material```

viklund · 2018-03-08T15:02:49Z

lega/openpgp/iobuf.py

+import io
+
+class IOBuf(object):
+    """ Some description


Yes, that would be nice.

viklund · 2018-03-08T15:03:49Z

lega/openpgp/packet.py

+
+PACKET_TYPES = {} # Will be updated below
+
+def parse_one(data):


parse_next_packet(data)?

viklund · 2018-03-08T15:06:21Z

lega/openpgp/packet.py

+
+        filename_length = read_1(self.data)
+        if filename_length == 0:
+            # then sensitive file


I don't understand this comment.

Sorry, it should have been on line 523

viklund · 2018-03-08T15:19:02Z

lega/openpgp/utils.py

+        return f'OpenPGP Error: {self.msg}'
+
+
+def read_1(data, buf=None):


I think read_X function s should be more generic read_bytes(num, data, buf=None) and also the get_intX should be byte2int(b, width) (width is not technically needed unless you want to assert). This is unless there is a big performance hit by doing it this way. If there's a performance hit I think longer names would be better read_1_byte and so forth.

I think all of these functions should be called read_1_byte, read_2_bytes and so forth. To make it easeier to read. And get_int2 should perhaps be called byte2int(b, width) and then just have it dynamiclly do the right convert.

That comment was confusing, you can ignore the last paragraph.

That does not solve the problem on Travis

They are uploaded to docker.io already and we don't build the images.

blankdots

I bless this worthy for merge

Ditching GnuPG

Frédéric Haziza and others added 30 commits February 11, 2018 22:36

Parsing a few packets

cdcdf3c

This can now decrypt the test file, given a test key and a passphrase.

0dc32f4

No GnuPG. Just Python.

Progress on recursion and streaming

af2ce2a

Adding a script entrypoint and moving code around

b241324

Adding cryptography in requirement

e6d1b49

Fixing the issues with the cleardata buffer

ad71776

Using generators. First step towards streaming

5a39a3c

Using generators to process data stream

625368f

Streaming solution for PGP

d96a7b6

Updating unlock for private key to only return the material

cf3d017

Updating the consumer generator and changing the loglevel to CRITICAL

2349fe1

#257 basic keyserver with caching

f7e3405

new cache mechanism

c024710

#259 caching and unlock request

393f363

#259 check ttl and docker image

cf502e5

#259 fix for ttl

754049a

Parsing a few packets

214b821

This can now decrypt the test file, given a test key and a passphrase.

26b89bb

No GnuPG. Just Python.

Progress on recursion and streaming

7871a73

Adding a script entrypoint and moving code around

2ee0337

Adding cryptography in requirement

cf2fa1a

Fixing the issues with the cleardata buffer

7454465

Using generators. First step towards streaming

6b6275a

Using generators to process data stream

c2b6f55

Streaming solution for PGP

b7d7eb3

Updating unlock for private key to only return the material

c727986

Updating the consumer generator and changing the loglevel to CRITICAL

00a014f

Adding docstrings documentation

538fb2c

Timing delay?

c655000

silverdaz added this to the Sprint 24 milestone Mar 7, 2018

silverdaz requested review from viklund and blankdots March 7, 2018 17:57

Removing debug code and unbuffered flag

3d68a9a

blankdots reviewed Mar 8, 2018

View reviewed changes

blankdots mentioned this pull request Mar 8, 2018

Capturing service startup and exit status #257

Closed

Fixing typos

117b4cf

dtitov reviewed Mar 8, 2018

View reviewed changes

Making the keyserver port configurable

256a455

viklund suggested changes Mar 8, 2018

View reviewed changes

viklund reviewed Mar 8, 2018

View reviewed changes

Frédéric Haziza added 9 commits March 8, 2018 16:36

Including review comments from Johan.

f2db3f2

Adding a general description for how a packet is structured

cc737ae

Renaming the read_X functions

d593f3c

Bootstrapping back in a container, and not on the host

bd31d95

Updating some permissions.

6307220

That does not solve the problem on Travis

Ingestigating travis issue

240f025

Making travis user own the private directory

a6d6d9a

Adding back the fake Message Broker for Central EGA

9ad747b

Trying 2 big images (base and inbox) containing everything.

fd0d6bf

They are uploaded to docker.io already and we don't build the images.

blankdots approved these changes Mar 9, 2018

View reviewed changes

viklund approved these changes Mar 9, 2018

View reviewed changes

switch back to common

5226e0b

dtitov approved these changes Mar 9, 2018

View reviewed changes

blankdots merged commit d6055c4 into dev Mar 9, 2018

blankdots deleted the feature/pgp branch March 22, 2018 13:35

viklund pushed a commit that referenced this pull request Nov 22, 2018

Merge pull request #261 from NBISweden/feature/pgp

8ff713d

Ditching GnuPG

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ditching GnuPG #261

Ditching GnuPG #261

silverdaz commented Mar 7, 2018 •

edited

Loading

blankdots Mar 8, 2018

silverdaz Mar 8, 2018

blankdots Mar 8, 2018

silverdaz Mar 8, 2018

blankdots Mar 8, 2018

dtitov left a comment

dtitov Mar 8, 2018

silverdaz Mar 8, 2018

dtitov Mar 8, 2018

silverdaz Mar 8, 2018

dtitov Mar 8, 2018

dtitov Mar 8, 2018

viklund left a comment

viklund Mar 8, 2018

silverdaz Mar 8, 2018

viklund Mar 8, 2018

viklund Mar 8, 2018

viklund Mar 8, 2018

silverdaz Mar 8, 2018

viklund Mar 8, 2018

viklund Mar 8, 2018

silverdaz Mar 8, 2018 •

edited

Loading

viklund Mar 8, 2018

viklund Mar 8, 2018 •

edited

Loading

blankdots left a comment


		PACKET_TYPES = {} # Will be updated below

		def parse_one(data):

		return f'OpenPGP Error: {self.msg}'


		def read_1(data, buf=None):

Ditching GnuPG #261

Ditching GnuPG #261

Conversation

silverdaz commented Mar 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dtitov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

viklund left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

silverdaz Mar 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

viklund Mar 8, 2018 • edited Loading

Choose a reason for hiding this comment

blankdots left a comment

Choose a reason for hiding this comment

silverdaz commented Mar 7, 2018 •

edited

Loading

silverdaz Mar 8, 2018 •

edited

Loading

viklund Mar 8, 2018 •

edited

Loading