Skip to content

Conversation

@ItalyPaleAle
Copy link
Contributor

Description

This PR implements the Dapr crypto scheme v1, as defined in dapr/proposals#3

It includes the methods Encrypt and Decrypt which are high-level methods to encrypt and decrypt messages of arbitrary length (up to 256TB). These methods work on streams and don't hold more than 64KB of data in a buffer.

This PR includes extensive unit tests that cover all edge cases.

Example usage

To encrypt a message, you invoke the Encrypt method with a readable stream containing the plaintext data and an object containing options.

The most important option is WrapKeyFn, which is a function that, given a plaintext file key (randomly-generated by the Encrypt method), the name of a key in the vault, and the algorithm, returns the wrapped key. Callers are required to pass this method as a lambda, which will perform wrapping using a component (for example, Azure Key Vault).

enc, err := Encrypt(
    bytes.NewReader(message),
    EncryptOptions{
        WrapKeyFn: func(wrappedKey []byte, algorithm, keyName string, nonce, tag []byte) (plaintextKey jwk.Key, err error) {
            //…
        },
        KeyName:   keyName,
        Algorithm: algorithm,
    },
)

The method returns a readable stream enc which will have the data written to it.

The method does not accept a context because it is not needed when working with streams. Callers can stop the method by stopping the input stream. The WrapKeyFn should manage its context (for the network call) internally (i.e. inside the lambda).

The signature of the Decrypt method is similar, except it takes less options, with the only required one being an UnwrapKeyFn (same concept as key wrapping for Encrypt):

dec, err := Decrypt(
    enc,
    DecryptOptions{
        UnwrapKeyFn: unwrapKeyFn,
    },
)

Here too, enc is a readable stream to the encrypted document, and the returned dec is a readable stream containing the plaintext.

Issue reference

Tracked by: dapr/dapr#6147
Proposal: dapr/proposals#3

Checklist

  • Code compiles correctly
  • Created/updated tests

@codecov-commenter
Copy link

codecov-commenter commented Mar 29, 2023

Codecov Report

Merging #42 (c5b02dc) into main (24e430f) will increase coverage by 1.52%.
The diff coverage is 86.30%.

@@            Coverage Diff             @@
##             main      #42      +/-   ##
==========================================
+ Coverage   79.07%   80.60%   +1.52%     
==========================================
  Files          26       31       +5     
  Lines        1491     1856     +365     
==========================================
+ Hits         1179     1496     +317     
- Misses        233      259      +26     
- Partials       79      101      +22     
Impacted Files Coverage Δ
schemes/enc/v1/filekey.go 72.81% <72.81%> (ø)
schemes/enc/v1/scheme.go 87.28% <87.28%> (ø)
schemes/enc/v1/algorithms.go 100.00% <100.00%> (ø)
schemes/enc/v1/ciphers.go 100.00% <100.00%> (ø)
schemes/enc/v1/manifest.go 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@ItalyPaleAle ItalyPaleAle force-pushed the dapr-crypto-scheme-v1 branch 2 times, most recently from 1a49ff9 to 70ad786 Compare March 29, 2023 17:16
Signed-off-by: ItalyPaleAle <[email protected]>
@ItalyPaleAle ItalyPaleAle force-pushed the dapr-crypto-scheme-v1 branch from 70ad786 to 021cf6c Compare March 29, 2023 17:35
Signed-off-by: Alessandro (Ale) Segala <[email protected]>
Signed-off-by: ItalyPaleAle <[email protected]>
ItalyPaleAle and others added 3 commits April 4, 2023 23:51
@mukundansundar
Copy link

@ItalyPaleAle there seems to be a flaky test in here. Can you take a look?

@ItalyPaleAle
Copy link
Contributor Author

@mukundansundar looks like there was a bug in the crypto code but not in what was added with this PR. Fixed here: #44

Signed-off-by: ItalyPaleAle <[email protected]>
@ItalyPaleAle ItalyPaleAle force-pushed the dapr-crypto-scheme-v1 branch from cbcf722 to 3629d26 Compare April 9, 2023 04:31
@JoshVanL
Copy link
Contributor

JoshVanL commented Apr 12, 2023

@ItalyPaleAle not completely finished the code part of the review, but thought it was worth sending my comments/questions about the design early 🙂

Signed-off-by: ItalyPaleAle <[email protected]>
@ItalyPaleAle
Copy link
Contributor Author

Thanks @JoshVanL I have addressed your comments and made (almost all) the suggested changes so far.

@artursouza
Copy link
Contributor

I recommend not marking comments as resolved unless the reviewer agrees or is unresponsive.

Signed-off-by: ItalyPaleAle <[email protected]>
@artursouza artursouza merged commit 15a7040 into dapr:main Apr 18, 2023
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.

8 participants