Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Configurable s3 endpoint for download #1163

Open
wants to merge 22 commits into
base: main
Choose a base branch
from

Conversation

aaperis
Copy link
Contributor

@aaperis aaperis commented Dec 4, 2024

Related issue(s) and PR(s)
This PR closes #750.

Description
With this PR download can either be deployed as serving either unencrypted or encrypted files. The serveUnencryptedData boolean is removed as well as the logic for download to always generate an internal c4gh key-pair (that would allow the service to serve unencrypted files in case serveUnencryptedData=true).

Main changes:

  • download can now be configured to only serve unencrypted files if a filepath to a c4gh private key file is provided. Internally, the code checks the validity of the provided key by using the supplied passphrase to retrieve the corresponding c4gh public key. The service will not start if this procedure fails and the service will serve unencrypted data only if the retrieved public key is non-empty. This ensures that there can be no misconfiguration by e.g. providing malfunctioning keys etc. For example, the crypt4gh library and therefore reencrypt will use any string provided as to it to encrypt a file but with this mechanism we avoid such a scary scenario.
  • if no filepath and passphrase are provided, then download will serve only encrypted files. This is the default behavior.

Other changes include:

  • go testsuite updates and some refactoring along the way
  • update of dev environment: in order to be able to test both setups at once, the docker compose files have been modified to include two instances of download listening at different ports, one serving encrypted and the other serving unencrypted files
  • update of integration tests, including a separate bash script for error messages returned to the user (according to the issue description)
  • update of helm chart

How to test
Integration tests pass.

@aaperis aaperis force-pushed the feature/unified-configurable-s3-endpoint-for-download branch from d5fc9f1 to d9c8803 Compare December 4, 2024 20:51
@aaperis aaperis changed the title Feature/unified configurable s3 endpoint for download Configurable s3 endpoint for download Dec 5, 2024
@aaperis aaperis force-pushed the feature/unified-configurable-s3-endpoint-for-download branch from d9c8803 to 73baacd Compare December 5, 2024 14:35
@aaperis aaperis self-assigned this Dec 9, 2024
@aaperis aaperis force-pushed the feature/unified-configurable-s3-endpoint-for-download branch 4 times, most recently from 9f68afa to d4e2439 Compare December 12, 2024 23:52
@aaperis aaperis marked this pull request as ready for review December 12, 2024 23:59
@aaperis aaperis requested a review from a team December 12, 2024 23:59
@aaperis aaperis force-pushed the feature/unified-configurable-s3-endpoint-for-download branch 2 times, most recently from 32a1128 to 058887a Compare December 13, 2024 08:10
charts/sda-svc/README.md Outdated Show resolved Hide resolved
charts/sda-svc/templates/download-deploy.yaml Outdated Show resolved Hide resolved
Comment on lines 234 to 235
{{- end }}
{{- if not .Values.global.vaultSecrets }}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
{{- end }}
{{- if not .Values.global.vaultSecrets }}
{{- end }}

charts/sda-svc/values.yaml Outdated Show resolved Hide resolved
Comment on lines 260 to 261
{{- if .Values.global.download.serveDecrypted }}
- name: c4gh-transient
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
{{- if .Values.global.download.serveDecrypted }}
- name: c4gh-transient
{{- if not .Values.global.vaultSecrets }}
{{- if .Values.global.download.serveDecrypted }}
- name: c4gh-transient

sda-download/internal/config/config_test.go Outdated Show resolved Hide resolved
sda-download/internal/config/config_test.go Outdated Show resolved Hide resolved
sda-download/internal/config/config_test.go Outdated Show resolved Hide resolved
sda-download/dev_utils/compose-no-tls.yml Outdated Show resolved Hide resolved
@@ -4,7 +4,6 @@ app:
serverkey: "./dev_utils/certs/download-key.pem"
port: "8443"
middleware: "default"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
middleware: "default"
middleware: "default"
c4gh:
passphrase: ""
privateKeyPath: ""

- use neic crypt4gh
- do not use pip
- silent curls
- internal c4gh key is optional
- base64 pubkey is created from loaded file
- modify tests
otherwise always serve encrypted files through /s3
- remove /s3-encrypted endpoint
- log warning if serving unencrypted
- remove serveUnencryptedData configuration
- add unencrypted download instance and config
for testing (un)encrypted download cases
and do a small refactoring
- add serveDecrypted object variable
- repurpose obsolete c4gh secret logic
- update README
@aaperis aaperis force-pushed the feature/unified-configurable-s3-endpoint-for-download branch from 4e96ea7 to eb58f24 Compare December 13, 2024 10:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[download] Configurable option for whether to support decrypted xor encrypted downloads.
2 participants