fix ssec: missing base64 encoding and md5 hash #225

gschei · 2024-11-10T16:54:39Z

This PR is based on #224 and replaces PR #217 (which has some code duplication and is not complete).

It fixes these issues:

the SSEC-CustomerKey is stored in the Velero Container as plain 32 byte key and must be base64 encoded before sending it to AWS.
as shown in PR 217 MD5 hash is missing for head/get/put operations

I tested this change successfully with an encrypted backup and restore with Velero and S3 Bucket.

Signed-off-by: Gilbert Scheiblhofer <gilbert.scheiblhofer@gmx.at>

tonobo · 2024-11-22T21:35:36Z

I'm pretty new to velero, i tried this mr locally and noticed there are issues while accessing the logs and futher contents from the bucket, probably caused by not passing the sse-c headers. am i holding it wrong or is there still something missing.

kaovilai · 2024-11-22T21:38:28Z

velero-plugin-for-aws/object_store.go

@@ -242,7 +247,7 @@ func readCustomerKey(customerKeyEncryptionFile string) (string, error) {
 	fileHandle.Close()

 	if nBytes != 32 {
-		return "", errors.Wrapf(err, "contents of %s (%s) are not exactly 32 bytes", customerKeyEncryptionFileKey, customerKeyEncryptionFile)
+		return "", fmt.Errorf("contents of %s (%s) are not exactly 32 bytes", customerKeyEncryptionFileKey, customerKeyEncryptionFile)


Is this necessary? This will mean the log trace won't propagate to velero pod logs with stack trace

The point of the fix is that err is always nil in this situation and errors.Wrapf here always returns nil, so to velero looks like key was read successfully (but is ""). I encountered this situation during testing, having a key < 32 bytes and no errors showed up in velero (but overall encryption did not work).

With this change, now the error is shown in the Backup-Resource correctly (but no stacktrace though).

ahh.. got it. Make sense. Here's a revised suggestion.

Suggested change

return "", fmt.Errorf("contents of %s (%s) are not exactly 32 bytes", customerKeyEncryptionFileKey, customerKeyEncryptionFile)

return "", errors.Errorf("contents of %s (%s) are not exactly 32 bytes", customerKeyEncryptionFileKey, customerKeyEncryptionFile)

thanks..PR updated

kaovilai · 2024-11-22T21:39:49Z

velero-plugin-for-aws/object_store.go

@@ -186,7 +189,9 @@ func (o *ObjectStore) Init(config map[string]string) error {
 		if err != nil {
 			return err
 		}
-		o.sseCustomerKey = customerKey
+		o.sseCustomerKey = base64.StdEncoding.EncodeToString([]byte(customerKey))


Why are we encoding string to []byte then string again?

So, your idea is to change the function readCustomerKey to return a []byte instead of a string? Would make sense, I can update the PR for it.

I see now that the description says "<your_b64_encoded_32byte_string>" which is wrong, the key must be plain and is then encoded to base64 before sending it.

Does "the key must be plain" holds true prior to this PR? Are we creating breaking change from customerKey is base64 -> customerKey must be plain?

Is the reason you are adding encoding here is because you wanted plain key instead?

Yes, the question is, should the key be stored plain or base64 encoded in the container? I assume plain because here

velero-plugin-for-aws/velero-plugin-for-aws/object_store.go

Line 237 in cedace1

keyBytes := make([]byte, 32)

we read exactly 32 bytes. If the key would be base64 encoded, then it would be longer then 32 bytes, and the mentioned line would be incorrect and needs to be fixed.

gschei · 2024-11-23T10:24:56Z

I'm pretty new to velero, i tried this mr locally and noticed there are issues while accessing the logs and futher contents from the bucket, probably caused by not passing the sse-c headers. am i holding it wrong or is there still something missing.

To test it successfully, you must mount the encryption key into the velero container (eg. from a secret) and then reference the file containing the key in the config of the BackupStorageLocation. See description here: https://github.com/vmware-tanzu/velero-plugin-for-aws/blob/main/backupstoragelocation.md.

I see now that the description says "<your_b64_encoded_32byte_string>" which is wrong, the key must be plain and is then encoded to base64 before sending it.

Signed-off-by: Gilbert Scheiblhofer <gilbert.scheiblhofer@gmx.at>

tonobo · 2024-11-24T19:51:43Z

@gschei you're right that's not my concern. I get it to work after checking the actual code not the problem, but once i enable the encryption part i'm unable to call velero backup logs for example. I always get an common error message as it's not passing the credentials somewhere. Local and within the container.

tonobo · 2024-11-24T21:57:40Z

EDIT: Nevermind, handled totally differnent

besides that, the internal kopia data mover seems to have trouble as well.

 data path backup failed: Failed to run data path service for DataUpload
    daily-pvc-backup-20241124210813-lkshf: error to initialize data path: error to
    boost backup repository connection backups-apps-kopia: error to connect backup
    repo: error to connect repo with storage: error to connect to repository: repository
    not initialized in the provided storage'

Can you confirm csi data movement works with sse-c backup locations?

P.S by dropping the sse-c key at all, it just works.

gschei · 2024-11-25T06:55:48Z

@gschei you're right that's not my concern. I get it to work after checking the actual code not the problem, but once i enable the encryption part i'm unable to call velero backup logs for example. I always get an common error message as it's not passing the credentials somewhere. Local and within the container.

Yes I see velero backup logs creates a DownloadRequest Resources which does not contain the Encryption Headers. Looks like object_store.CreateSignedURL needs to be extended too with Encryption Parameters.

gschei · 2024-11-26T21:05:30Z

It looks like AWS getSignedUrl dies not support SSECustomerKey, see https://docs.aws.amazon.com/AWSJavaScriptSDK/latest/AWS/S3.html#getSignedUrl-property

Note:

Not all operation parameters are supported when using pre-signed URLs. Certain parameters, such as SSECustomerKey, ACL, Expires, ContentLength, or Tagging must be provided as headers when sending a request. If you are using pre-signed URLs to upload from a browser and need to use these fields, see createPresignedPost().

kaovilai · 2024-11-26T21:24:46Z

getSignedUrl

is currently required until vmware-tanzu/velero#6167 gets implemented.

gschei · 2024-12-10T20:56:41Z

One additional remark regarding the encryption key: original implementation ac31cd9 (from feb/22) stated that the key should be a "32 byte string". Later (oct/23) a863977 a documentation update was done which apparently was wrong because it contradicts the original statement, saying that the key should now be a "base64 encoded 32 byte string" which is not correct, because within the code only the first 32 bytes are read anyway (and base64 endoding will make the string longer).

From my point of view current implementation is not working at the moment - once due to missing base64 (or alternatively correct parsing of base64 encoded key) and also due to missing md5 hash - a problem which appeared later.

velero backup logs statement is generally not working for encrypted backups and will also not work after merging this PR. But creation and restore of encrypted backups would work now.

@kaovilai @reasonerjt Could you please have a look at this again and merge if OK?

kaovilai · 2024-12-10T21:12:40Z

Please update readme and or backupstoragelocation.md to reflect this PR or the current state of using customerKey since you're probably most versed in how to use this PR as is.

kaovilai · 2024-12-10T21:14:27Z

velero-plugin-for-aws/backupstoragelocation.md

Lines 76 to 87 in 2dd6bcb

    
           # Specify the file that contains the SSE-C customer key to enable customer key encryption of the backups 
        
           # stored in S3. The referenced file should contain a 32-byte string. 
        
           #   
        
           # The customerKeyEncryptionFile points to a mounted secret within the velero container. 
        
           # Add the below values to the velero cloud-credentials secret: 
        
           # customer-key: <your_b64_encoded_32byte_string> 
        
           # The default value below points to the already mounted secret. 
        
           #  
        
           # Cannot be used in conjunction with kmsKeyId. 
        
           # 
        
           # Optional (defaults to "", which means SSE-C is disabled). 
        
           customerKeyEncryptionFile: "/credentials/customer-key"

Is this accurate as is?

Signed-off-by: Gilbert Scheiblhofer <gilbert.scheiblhofer@gmx.at>

gschei · 2024-12-10T21:59:18Z

@kaovilai I clarified the description. The point is, when I base64 encode the key within the kubernetes secret, it will be automatically base64 decoded by kubernetes when mounted into the container.

backupstoragelocation.md

kaovilai

This lgtm. Just some clarifications. Thanks for the PR!

reasonerjt

lgtm, thanks

gschei added 2 commits November 6, 2024 18:22

fixed wrong errorhandling for customerKey size check

d94457c

Signed-off-by: Gilbert Scheiblhofer <gilbert.scheiblhofer@gmx.at>

fixed missing ssec: base64 encoding and md5 hash were missing

7eddb13

Signed-off-by: Gilbert Scheiblhofer <gilbert.scheiblhofer@gmx.at>

github-actions bot requested review from blackpiglet and shubham-pampattiwar November 10, 2024 16:54

github-actions bot assigned gschei Nov 10, 2024

gschei changed the title ~~gschei/fix ssec base64 md5~~ fix ssec: missing base64 encoding and md5 hash Nov 10, 2024

gschei mentioned this pull request Nov 10, 2024

Send md5 of customer key #217

Closed

kaovilai reviewed Nov 22, 2024

View reviewed changes

updatd customerKey errorhandling

abf14db

Signed-off-by: Gilbert Scheiblhofer <gilbert.scheiblhofer@gmx.at>

blackpiglet requested a review from reasonerjt November 25, 2024 03:20

updated backupstoragelocation-readme regarding customerKeyEncryptionFile

3e1572f

Signed-off-by: Gilbert Scheiblhofer <gilbert.scheiblhofer@gmx.at>

kaovilai reviewed Dec 10, 2024

View reviewed changes

backupstoragelocation.md Show resolved Hide resolved

kaovilai approved these changes Dec 10, 2024

View reviewed changes

kaovilai mentioned this pull request Dec 16, 2024

fixed wrong errorhandling for customerKey size check #224

Closed

reasonerjt approved these changes Dec 16, 2024

View reviewed changes

reasonerjt merged commit e5b0f0d into vmware-tanzu:main Dec 16, 2024
2 checks passed

This was referenced Dec 16, 2024

Add SSECustomerKeyMD5 support #226

Closed

[Cherry-pick-to-1.11] fixed missing ssec: base64 encoding and md5 hash were missing #228

Merged

This was referenced Dec 17, 2024

OADP-5388: velero#225 Various SSE-C & MD5 Hash fixes openshift/velero-plugin-for-aws#63

Merged

[velero-plugin-for-aws:v1.9.1] Backup fails with customer provided key encryption vmware-tanzu/velero#7693

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix ssec: missing base64 encoding and md5 hash #225

fix ssec: missing base64 encoding and md5 hash #225

gschei commented Nov 10, 2024

tonobo commented Nov 22, 2024

kaovilai Nov 22, 2024

gschei Nov 23, 2024

kaovilai Nov 24, 2024

gschei Nov 24, 2024

kaovilai Nov 22, 2024

gschei Nov 23, 2024

kaovilai Nov 24, 2024

gschei Nov 24, 2024

gschei commented Nov 23, 2024

tonobo commented Nov 24, 2024

tonobo commented Nov 24, 2024 •

edited

Loading

gschei commented Nov 25, 2024

gschei commented Nov 26, 2024

kaovilai commented Nov 26, 2024

gschei commented Dec 10, 2024

kaovilai commented Dec 10, 2024

kaovilai commented Dec 10, 2024

gschei commented Dec 10, 2024

kaovilai left a comment

reasonerjt left a comment

	return "", fmt.Errorf("contents of %s (%s) are not exactly 32 bytes", customerKeyEncryptionFileKey, customerKeyEncryptionFile)
	return "", errors.Errorf("contents of %s (%s) are not exactly 32 bytes", customerKeyEncryptionFileKey, customerKeyEncryptionFile)

fix ssec: missing base64 encoding and md5 hash #225

fix ssec: missing base64 encoding and md5 hash #225

Conversation

gschei commented Nov 10, 2024

tonobo commented Nov 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gschei commented Nov 23, 2024

tonobo commented Nov 24, 2024

tonobo commented Nov 24, 2024 • edited Loading

gschei commented Nov 25, 2024

gschei commented Nov 26, 2024

kaovilai commented Nov 26, 2024

gschei commented Dec 10, 2024

kaovilai commented Dec 10, 2024

kaovilai commented Dec 10, 2024

gschei commented Dec 10, 2024

kaovilai left a comment

Choose a reason for hiding this comment

reasonerjt left a comment

Choose a reason for hiding this comment

tonobo commented Nov 24, 2024 •

edited

Loading