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.

Sign up for GitHub

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

fix ssec: missing base64 encoding and md5 hash #225

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
+15 −6
Open

fix ssec: missing base64 encoding and md5 hash #225

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d94457c
fixed wrong errorhandling for customerKey size check
gschei Nov 6, 2024
7eddb13
fixed missing ssec: base64 encoding and md5 hash were missing
gschei Nov 10, 2024
abf14db
updatd customerKey errorhandling
gschei Nov 24, 2024
3e1572f
updated backupstoragelocation-readme regarding customerKeyEncryptionFile
gschei Dec 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions backupstoragelocation.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,12 +74,13 @@ spec:
kmsKeyId: "502b409c-4da1-419f-a16e-eif453b3i49f"

# 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.
# stored in S3. The referenced file should exist within the velero container and
# should contain a 32-byte string. It is typically mounted from a secret.
#
# The customerKeyEncryptionFile points to a mounted secret within the velero container.
# Add the below values to the velero cloud-credentials secret:
# Eg. add to the velero "cloud-credentials" secret this entry with the base64 encoded key
# (will be decoded when the secret is mounted)
# customer-key: <your_b64_encoded_32byte_string>
Copy link
Member

Choose a reason for hiding this comment

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

Should this line be uncommented? or is the encoded customer-key only supposed to be populated by the object_store.go?

Copy link
Member

Choose a reason for hiding this comment

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

Can it be expected that someone put in encoded 32 byte string instead of specifying the file name in the key below this one?
If so

Suggested change
# customer-key: <your_b64_encoded_32byte_string>
customer-key: <your_b64_encoded_32byte_string>

Copy link
Author

Choose a reason for hiding this comment

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

The function readCustomerKey currently only supports reading the secret from a file mounted inside the velero container. So the example here shows how to put the key into the secret, therefore it is just a comment and not part of the BSL configuration. I think it make sense, not to provide the senstive key as parameter, but only as secret.

# The default value below points to the already mounted secret.
# The value below points to the already mounted secret.
#
# Cannot be used in conjunction with kmsKeyId.
#
Expand Down
12 changes: 10 additions & 2 deletions velero-plugin-for-aws/object_store.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ package main

import (
"context"
"crypto/md5"
"encoding/base64"
"fmt"
"io"
"os"
Expand Down Expand Up @@ -74,6 +76,7 @@ type ObjectStore struct {
s3Uploader *manager.Uploader
kmsKeyID string
sseCustomerKey string
sseCustomerKeyMd5 string
signatureVersion string
serverSideEncryption string
tagging string
Expand Down Expand Up @@ -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))
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

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.

hash := md5.Sum([]byte(customerKey))
o.sseCustomerKeyMd5 = base64.StdEncoding.EncodeToString(hash[:])
}

if publicURL != "" {
Expand Down Expand Up @@ -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 "", errors.Errorf("contents of %s (%s) are not exactly 32 bytes", customerKeyEncryptionFileKey, customerKeyEncryptionFile)
}

key := string(keyBytes)
Expand All @@ -267,6 +272,7 @@ func (o *ObjectStore) PutObject(bucket, key string, body io.Reader) error {
case o.sseCustomerKey != "":
input.SSECustomerAlgorithm = aws.String("AES256")
input.SSECustomerKey = &o.sseCustomerKey
input.SSECustomerKeyMD5 = &o.sseCustomerKeyMd5
// otherwise, use the SSE algorithm specified, if any
case o.serverSideEncryption != "":
input.ServerSideEncryption = types.ServerSideEncryption(o.serverSideEncryption)
Expand Down Expand Up @@ -298,6 +304,7 @@ func (o *ObjectStore) ObjectExists(bucket, key string) (bool, error) {
if o.sseCustomerKey != "" {
input.SSECustomerAlgorithm = aws.String("AES256")
input.SSECustomerKey = &o.sseCustomerKey
input.SSECustomerKeyMD5 = &o.sseCustomerKeyMd5
}

log.Debug("Checking if object exists")
Expand Down Expand Up @@ -329,6 +336,7 @@ func (o *ObjectStore) GetObject(bucket, key string) (io.ReadCloser, error) {
if o.sseCustomerKey != "" {
input.SSECustomerAlgorithm = aws.String("AES256")
input.SSECustomerKey = &o.sseCustomerKey
input.SSECustomerKeyMD5 = &o.sseCustomerKeyMd5
}

output, err := o.s3.GetObject(context.Background(), input)
Expand Down
Loading