NBISweden / LocalEGA

Please go to to https://github.com/EGA-archive/LocalEGA instead
Apache License 2.0
4 stars 1 forks source link

GA4GH #303

Closed silverdaz closed 6 years ago

silverdaz commented 6 years ago

This PR implements the GA4GH feature. It is essentially the same architecture as before, but the cryptographic steps are done using the GA4GH standard, and not PGP+custom format for the vault.

Moreover, the vault is backed by an S3 storage.

In more details:

The cryptographic part have been moved to the LocalEGA-cryptor repo, as a separate package.

Finally, the bootstrap script does not start multiple instances. It's only one, say lega, along with the cega stubs.

silverdaz commented 6 years ago

@dtitov: Could you please help updating the tests, since there is no "instance" anymore? Containers are simply named db, mq, ingest, s3, verify...etc...(and cega-mq, cega-users, cega-eureka). So context.getTargetInstance() is not necessary.

Unless you want to minimize the changes and just use one instance called lega.

dtitov commented 6 years ago

@silverdaz, yes, I will.

dtitov commented 6 years ago

I've fixed almost all the tests except the ones with successfull encryption scenario, because test-suite at the moment is not able to encrypt files with Crypt4GH. But I'll try to add this functionality tomorrow.

There's one more failing test: Scenario: A.3 User exists in Central EGA and tries to connect to LocalEGA, but the inbox was not created for him. Looks like mounting is done somehow differently now, so I have to change simulation of "inbox not created" event, but I'm not sure how...

blankdots commented 6 years ago

Unit tests partly re-factored, will continue that

silverdaz commented 6 years ago

@dtitov : Have you seen that: https://github.com/AlexanderSenf/Crypt4GH ?

dtitov commented 6 years ago

@silverdaz, yes, it's a draft and Alexander haven't promised that it's production ready. Also, there's no encryption stream there - only decryption stream. And it's not published to Maven as well :(

Anyway, now we have this: https://github.com/uio-bmi/crypt4gh :)

blankdots commented 6 years ago

the lega/conf/loggers also need to be modified or removed if we are not going to use all of them

silverdaz commented 6 years ago

ok, no problem. But why do the loggers matter?

silverdaz commented 6 years ago

I don't mind having another implementation in Java somewhere else. It's fine.

Just out of curiosity: I think the implementation of Alexander does encryption too. Am I seeing this wrong? https://github.com/AlexanderSenf/Crypt4GH/blob/master/src/crypt4gh/Crypt4gh.java#L151-L195

dtitov commented 6 years ago

That's correct, it's just not wrapped as an OutputStream.

silverdaz commented 6 years ago

and?

dtitov commented 6 years ago

And I believe it should be.

But why are we discussing it here at all? I'm not sure this PR is the right place. I'm fixing the test-suite for this PR, as you asked.

silverdaz commented 6 years ago

I'm not discussing. I'm just pointing out the other implementation to help.

dtitov commented 6 years ago

Thank you.

silverdaz commented 6 years ago

But for the discussion: I don't think the testsuite should necessarily implement the encryption as users will do. It is not a bad thing to have it, of course, but I think that encrypting a small file and hard-coding it in the testsuite is simple enough, and we can find some other way to test big files.

silverdaz commented 6 years ago

crypt4gh is not complicated, so if the testsuite already has encryption, then it's all good!

silverdaz commented 6 years ago

Side note: I've got the feeling that we can have very soon another PR with data-out, including a step in the testsuite that decrypts files (as regular https streams or as htsget streams) ;-)

dtitov commented 6 years ago

Exactly!

dtitov commented 6 years ago

Tests are green: both locally and on Travis.

The architecture looks good to me, although I haven't investigated the Python code in details. But since Stefan approved the PR, I'm performing the merge.