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

feat: Crypt4GH support using LocalStorage #262

Merged
merged 1 commit into from
Sep 11, 2024
Merged

Conversation

mmalenic
Copy link
Member

@mmalenic mmalenic commented Sep 11, 2024

Changes

  • Add Crypt4GH support for LocalStorage. The process is:
    • The server is configured with a private_key and recipient_public_key.
    • Encrypted files are decrypted by htsget-rs using the private_key.
    • Byte ranges are calculated on unencrypted data.
    • Byte ranges are converted to encrypted ranges, and the Crypt4GH headers are reencrypted and served to the client along with edit lists.
    • The client is expected to decrypt the resulting concatenated bytes using their private key, which corresponds to the recipient_public_key.
  • Storage is significantly refactored, it now uses dynamic traits and some heap-allocated indirection. I think this was necesssary because otherwise there would be a large amount of less readable generic types on many structs in htsget-storage and htsget-search. This would be coupled with many more #[cfg(feature = "c4gh-experimental")] flags, and I thought it would be best to reduce these.
    • The C4GHStorage now wraps any other StorageTrait and decrypts the stream.

A lot of this code is copied from the existing https://github.com/umccr/htsget-rs/tree/crypt4gh branch, except it's been tidied up and made a bit more generic so that it supports other storage interfaces, not just UrlStorage.

I think a lot of the code under https://github.com/umccr/htsget-rs/blob/feat/crypt4gh-storage/htsget-storage/src/c4gh/edit.rs and https://github.com/umccr/htsget-rs/blob/feat/crypt4gh-storage/htsget-storage/src/c4gh/mod.rs belongs in the Crypt4GH-Rust repository eventually.

@mmalenic mmalenic self-assigned this Sep 11, 2024
@mmalenic mmalenic added the enhancement New feature or request label Sep 11, 2024
@brainstorm brainstorm added this pull request to the merge queue Sep 11, 2024
Merged via the queue into main with commit 7a2023e Sep 11, 2024
5 checks passed
@brainstorm
Copy link
Member

brainstorm commented Sep 11, 2024

Shall we deprecated PR #223, @mmalenic ?

Answer IRL: No because NBIS depends on a few bits in there still.

@brainstorm
Copy link
Member

brainstorm commented Sep 12, 2024

A few bits to address after discussing IRL:

  • S3 and URL storage support after this PR.
  • Rename c4gh-experimental to just experimental as we can reuse it with out-of-spec features such as BED support in htsget.
  • Drop object_type naming in favor of crypt4gh_keys (or similar) in the resolver configuration. Related to Crypt4GH secrets manager #264.
  • README.md: Don't stop at samtools view output erroring on EOF, explain why it errors and show a solution for it?
  • Document third party htsget python client support for crypt4gh.

@mmalenic
Copy link
Member Author

mmalenic commented Sep 19, 2024

#265 addresses point 2, and will allow the remaining points to be implemented in another PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants