neicnordic / sensitive-data-archive

https://neic-sda.readthedocs.io
3 stars 6 forks source link

feat: configurable allow uncrypted download #887

Closed nanjiangshu closed 2 weeks ago

nanjiangshu commented 1 month ago

Related issue(s) and PR(s)
This PR fixes #868

Description By default, downloading unencrypted files is disabled and will return a 400 (BadRequest) error. This is however configurable. When serveUnencryptedData: true is set in the config file under the item app, download unencrypted files will be allowed.

How to test Use this branch from the starter-kit-storage-and-interfaces to start the stack. Then obtain the token and key by

token=$(curl -s -k https://localhost:8080/tokens | jq -r '.[0]')
sda-cli createKey c4gh
pubkey=$(base64 -w0 c4gh.pub.pem)

After that,

# this should return 400
curl -o /dev/null -s -w "%{http_code}\n" -H "Authorization: Bearer $token" -H -k http://localhost:8443/files/FILE0000001 

# this should return 400
curl -o /dev/null -s -w "%{http_code}\n" -H "Authorization: Bearer $token" -H -k http://localhost:8443/s3/DATASET0001//htsnexus_test_NA12878.bam  # unenrypted

# this should return 200
curl -o /dev/null -s -w "%{http_code}\n" -H "Client-Public-Key: $pubkey" -H "Authorization: Bearer $token" http://localhost:8443/s3-encrypted/DATASET0001/htsnexus_test_NA12878.bam 

Try to set allowUnencryptedDownload: true in the config file and all of the above three commands should return 200

pontus commented 1 month ago

What are the required use cases here? With bigpicture perspective, I don't see that we need have the same server serve both encrypted.

If that goes for all your use cases as well, I would feel better about just looking at this setting rather than what's now set as c.Param("type") == "encrypted".

jbygdell commented 1 month ago

What are the required use cases here? With bigpicture perspective, I don't see that we need have the same server serve both encrypted.

If that goes for all your use cases as well, I would feel better about just looking at this setting rather than what's now set as c.Param("type") == "encrypted".

This is more of a stepping stone right now. There are some rather heavy refactoring required here, merge S3 and S3-encrypted which would remove the c.Param("type") == "encrypted" But in the end the app should have one global flag that specifies how the data is delivered, encrypted (default) or unencrypted.

aaperis commented 3 weeks ago

Looks good, just fix the failure in the one of the tests and then I will approve

This should hopefully be resolved once the PR is rebased on main.

nanjiangshu commented 3 weeks ago

Looks good, just fix the failure in the one of the tests and then I will approve

This should hopefully be resolved once the PR is rebased on main.

Very good guess @aaperis ! Tests are passed after rebasing to the main branch @kostas-kou