Skip to content

Commit

Permalink
Merge pull request #508 from cyberark/decode-base64-k8s-secrets
Browse files Browse the repository at this point in the history
Decode base64 K8s secrets
  • Loading branch information
gl-johnson authored Mar 21, 2023
2 parents b44c3c8 + 4274d14 commit ed42696
Show file tree
Hide file tree
Showing 11 changed files with 135 additions and 52 deletions.
4 changes: 2 additions & 2 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -12,5 +12,5 @@ junit.xml
# Temporary directory to store the CyberArk proxy CA certificate
build_ca_certificate/

# Ignore generated policy files
deploy/policy/generated/
# Ignore generated policy files and manifests
deploy/**/generated/
3 changes: 3 additions & 0 deletions deploy/config/k8s/k8s-secret.yml
Original file line number Diff line number Diff line change
Expand Up @@ -9,4 +9,7 @@ stringData:
var_with_spaces: secrets/var with spaces
var_with_pluses: secrets/var+with+pluses
var_with_umlaut: secrets/umlaut
var_with_encoded:
id: secrets/encoded
content-type: base64
non-conjur-key: some-value
3 changes: 3 additions & 0 deletions deploy/config/openshift/k8s-secret.yml
Original file line number Diff line number Diff line change
Expand Up @@ -9,4 +9,7 @@ stringData:
var_with_spaces: secrets/var with spaces
var_with_pluses: secrets/var+with+pluses
var_with_umlaut: secrets/umlaut
var_with_encoded:
id: secrets/encoded
content-type: base64
non-conjur-key: some-value
5 changes: 5 additions & 0 deletions deploy/dev/config/k8s/secrets-provider-init-container.sh.yml
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,11 @@ spec:
secretKeyRef:
name: test-k8s-secret
key: var_with_umlaut
- name: VARIABLE_WITH_ENCODED_SECRET
valueFrom:
secretKeyRef:
name: test-k8s-secret
key: var_with_encoded
- name: NON_CONJUR_SECRET
valueFrom:
secretKeyRef:
Expand Down
5 changes: 5 additions & 0 deletions deploy/dev/config/k8s/secrets-provider-k8s-rotation.sh.yml
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,11 @@ spec:
secretKeyRef:
name: test-k8s-secret
key: var_with_umlaut
- name: VARIABLE_WITH_ENCODED_SECRET
valueFrom:
secretKeyRef:
name: test-k8s-secret
key: var_with_encoded
- name: NON_CONJUR_SECRET
valueFrom:
secretKeyRef:
Expand Down
1 change: 1 addition & 0 deletions deploy/policy/load_policies.sh
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ conjur variable set -i secrets/test_secret -v "some-secret"
conjur variable set -i "secrets/var with spaces" -v "some-secret"
conjur variable set -i "secrets/var+with+pluses" -v "some-secret"
conjur variable set -i "secrets/umlaut" -v "some-secret"
conjur variable set -i "secrets/encoded" -v "c2VjcmV0LXZhbHVl" # == secret-value
conjur variable set -i secrets/url -v "postgresql://test-app-backend.app-test.svc.cluster.local:5432"
conjur variable set -i secrets/username -v "some-user"
conjur variable set -i secrets/password -v "7H1SiSmYp@5Sw0rd"
Expand Down
1 change: 1 addition & 0 deletions deploy/policy/templates/conjur-secrets.template.sh.yml
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ cat << EOL
- !variable var with spaces
- !variable var+with+pluses
- !variable umlaut
- !variable encoded
- !variable url
- !variable username
- !variable password
Expand Down
16 changes: 10 additions & 6 deletions deploy/utils.sh
Original file line number Diff line number Diff line change
Expand Up @@ -332,6 +332,7 @@ deploy_chart() {
}

set_config_directory_path() {
export DEV_CONFIG_DIR="dev/config/k8s"
export CONFIG_DIR="config/k8s"
if [[ "$PLATFORM" = "openshift" ]]; then
export CONFIG_DIR="config/openshift"
Expand Down Expand Up @@ -380,8 +381,9 @@ deploy_init_env() {
echo "Running Deployment Manifest"

if [[ "$DEV" = "true" ]]; then
./dev/config/k8s/secrets-provider-init-container.sh.yml > ./dev/config/k8s/secrets-provider-init-container.yml
$cli_with_timeout apply -f ./dev/config/k8s/secrets-provider-init-container.yml
mkdir -p $DEV_CONFIG_DIR/generated
$DEV_CONFIG_DIR/secrets-provider-init-container.sh.yml > $DEV_CONFIG_DIR/generated/secrets-provider-init-container.yml
$cli_with_timeout apply -f $DEV_CONFIG_DIR/generated/secrets-provider-init-container.yml

$cli_with_timeout "get pods --namespace=$APP_NAMESPACE_NAME --selector app=init-env --no-headers | wc -l"
else
Expand All @@ -407,8 +409,9 @@ deploy_k8s_rotation_env() {
echo "Running Deployment Manifest"

if [[ "$DEV" = "true" ]]; then
./dev/config/k8s/secrets-provider-k8s-rotation.sh.yml > ./dev/config/k8s/secrets-provider-k8s-rotation.yml
$cli_with_timeout apply -f ./dev/config/k8s/secrets-provider-k8s-rotation.yml
mkdir -p $DEV_CONFIG_DIR/generated
$DEV_CONFIG_DIR/secrets-provider-k8s-rotation.sh.yml > $DEV_CONFIG_DIR/generated/secrets-provider-k8s-rotation.yml
$cli_with_timeout apply -f $DEV_CONFIG_DIR/generated/secrets-provider-k8s-rotation.yml

$cli_with_timeout "get pods --namespace=$APP_NAMESPACE_NAME --selector app=test-app --no-headers | wc -l"
else
Expand Down Expand Up @@ -649,8 +652,9 @@ deploy_push_to_file() {
deployment_name="test-env"

if [[ "$DEV" = "true" ]]; then
"./dev/config/k8s/$dev_yaml_file_name.sh.yml" > "./dev/config/k8s/$dev_yaml_file_name.yml"
$cli_with_timeout apply -f "./dev/config/k8s/$dev_yaml_file_name.yml"
mkdir -p $DEV_CONFIG_DIR/generated
"$DEV_CONFIG_DIR/$dev_yaml_file_name.sh.yml" > "$DEV_CONFIG_DIR/generated/$dev_yaml_file_name.yml"
$cli_with_timeout apply -f "$DEV_CONFIG_DIR/generated/$dev_yaml_file_name.yml"

$cli_with_timeout "get pods --namespace=$APP_NAMESPACE_NAME --selector app=$deployment_name --no-headers | wc -l"
else
Expand Down
3 changes: 3 additions & 0 deletions pkg/log/messages/error_messages.go
Original file line number Diff line number Diff line change
Expand Up @@ -84,3 +84,6 @@ const CSPFK058E string = "CSPFK058E Could not rename temporary file '%s' to '%s'
const CSPFK059E string = "CSPFK059E Could not delete temporary file '%s'. Truncated file."
const CSPFK060E string = "CSPFK060E Could not delete temporary file '%s'. File may be left on disk."
const CSPFK061E string = "CSPFK061E Could not write content to temporary file for '%s'"

// Secrets Decoding
const CSPFK064E string = "CSPFK064E Failed to decode secret '%s' with '%s' content-type. Reason: %s"
59 changes: 40 additions & 19 deletions pkg/secrets/k8s_secrets_storage/provide_conjur_secrets.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package k8ssecretsstorage
import (
"bytes"
"context"
"encoding/base64"
"fmt"
"strings"

Expand Down Expand Up @@ -344,25 +345,7 @@ func (p K8sProvider) updateRequiredK8sSecrets(
spanCtx, span := tracer.Start(p.traceContext, "Update K8s Secrets")
defer span.End()

// Create a map of entries to be added to the 'Data' fields of each
// K8s Secret. Each entry will map an application secret name to
// a value retrieved from Conjur.
newSecretData := map[string]map[string][]byte{}
for variableID, secretValue := range conjurSecrets {
dests := p.secretsState.updateDestinations[variableID]
for _, dest := range dests {
k8sSecretName := dest.k8sSecretName
secretName := dest.secretName
// If there are no data entries for this K8s Secret yet, initialize
// its map of data entries.
if newSecretData[k8sSecretName] == nil {
newSecretData[k8sSecretName] = map[string][]byte{}
}
newSecretData[k8sSecretName][secretName] = secretValue
}
// Null out the secret value
conjurSecrets[variableID] = []byte{}
}
newSecretData := p.createSecretData(conjurSecrets)

// Update K8s Secrets with the retrieved Conjur secrets
for k8sSecretName, secretData := range newSecretData {
Expand Down Expand Up @@ -401,3 +384,41 @@ func (p K8sProvider) updateRequiredK8sSecrets(

return updated, nil
}

// createSecretData creates a map of entries to be added to the 'Data' fields
// of each K8s Secret. Each entry will map an application secret name to a
// value retrieved from Conjur. If a secret has a 'base64' content type, the
// resulting secret value will be decoded.
func (p K8sProvider) createSecretData(conjurSecrets map[string][]byte) map[string]map[string][]byte {
secretData := map[string]map[string][]byte{}
for variableID, secretValue := range conjurSecrets {
dests := p.secretsState.updateDestinations[variableID]
for _, dest := range dests {
k8sSecretName := dest.k8sSecretName
secretName := dest.secretName
// If there are no data entries for this K8s Secret yet, initialize
// its map of data entries.
if secretData[k8sSecretName] == nil {
secretData[k8sSecretName] = map[string][]byte{}
}

// Check if the secret value should be decoded in this K8s Secret
if dest.contentType == "base64" {
decodedSecretValue, err := base64.StdEncoding.DecodeString(string(secretValue))
if err != nil {
// Log the error as a warning but still provide the original secret value
p.log.warn(messages.CSPFK064E, secretName, dest.contentType, err.Error())
secretData[k8sSecretName][secretName] = secretValue
} else {
secretData[k8sSecretName][secretName] = decodedSecretValue
}
} else {
secretData[k8sSecretName][secretName] = secretValue
}
}
// Null out the secret value
conjurSecrets[variableID] = []byte{}
}

return secretData
}
87 changes: 62 additions & 25 deletions pkg/secrets/k8s_secrets_storage/provide_conjur_secrets_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -139,17 +139,28 @@ func assertErrorLogged(msg string, args ...interface{}) assertFunc {
}
}

func assertInfoLogged(expected bool, msg string, args ...interface{}) assertFunc {
func assertLogged(expected bool, logLevel string, msg string, args ...interface{}) assertFunc {
return func(t *testing.T, mocks testMocks, updated bool, err error, desc string) {
infoStr := fmt.Sprintf(msg, args...)
logStr := fmt.Sprintf(msg, args...)
var logDesc string
if expected {
logDesc = ", expected info log to contain: "
logDesc = fmt.Sprintf(", expected %s log to contain: ", logLevel)
} else {
logDesc = ", expected info log NOT to contain: "
logDesc = fmt.Sprintf(", expected %s log NOT to contain: ", logLevel)
}
newDesc := desc + logDesc + logStr
switch logLevel {
case "debug":
assert.Equal(t, expected, mocks.logger.DebugWasLogged(logStr), newDesc)
case "info":
assert.Equal(t, expected, mocks.logger.InfoWasLogged(logStr), newDesc)
case "warn":
assert.Equal(t, expected, mocks.logger.WarningWasLogged(logStr), newDesc)
case "error":
assert.Equal(t, expected, mocks.logger.ErrorWasLogged(logStr), newDesc)
default:
assert.Fail(t, "Invalid log level: "+logLevel)
}
newDesc := desc + logDesc + infoStr
assert.Equal(t, expected, mocks.logger.InfoWasLogged(infoStr), newDesc)
}
}

Expand Down Expand Up @@ -369,25 +380,50 @@ func TestProvide(t *testing.T) {
},
requiredSecrets: []string{"k8s-secret1", "k8s-secret2"},
asserts: []assertFunc{
// TODO - uncomment when decoding is implemented
// assertSecretsUpdated(
// expectedK8sSecrets{
// "k8s-secret1": {
// "test-decoding": "decoded-value-1",
// "test-decoding2": "decoded-value-2",
// },
// "k8s-secret2": {
// "test-still-encoded": "ZGVjb2RlZC12YWx1ZS0x",
// "test-still-encoded2": "ZGVjb2RlZC12YWx1ZS0y",
// },
// },
// expectedMissingValues{},
// false,
// ),
assertInfoLogged(true, messages.CSPFK022I, "test-decoding", "k8s-secret1"),
assertInfoLogged(true, messages.CSPFK022I, "test-decoding2", "k8s-secret1"),
assertInfoLogged(false, messages.CSPFK022I, "test-still-encoded", "k8s-secret2"),
assertInfoLogged(false, messages.CSPFK022I, "test-still-encoded2", "k8s-secret2"),
assertSecretsUpdated(
expectedK8sSecrets{
"k8s-secret1": {
"test-decoding": "decoded-value-1",
"test-decoding2": "decoded-value-2",
},
"k8s-secret2": {
"test-still-encoded": "ZGVjb2RlZC12YWx1ZS0x",
"test-still-encoded2": "ZGVjb2RlZC12YWx1ZS0y",
},
},
expectedMissingValues{},
false,
),
assertLogged(true, "info", messages.CSPFK022I, "test-decoding", "k8s-secret1"),
assertLogged(true, "info", messages.CSPFK022I, "test-decoding2", "k8s-secret1"),
assertLogged(false, "info", messages.CSPFK022I, "test-still-encoded", "k8s-secret2"),
assertLogged(false, "info", messages.CSPFK022I, "test-still-encoded2", "k8s-secret2"),
},
},
{
desc: "Plaintext secret with base64 content-type returns the secret as is",
k8sSecrets: k8sStorageMocks.K8sSecrets{
"k8s-secret1": {
"conjur-map": {
"secret1": map[string]interface{}{
"id": "conjur/var/path1",
"content-type": "base64",
},
},
},
},
requiredSecrets: []string{"k8s-secret1"},
asserts: []assertFunc{
assertSecretsUpdated(
expectedK8sSecrets{
"k8s-secret1": {
"secret1": "secret-value1",
},
},
expectedMissingValues{},
false,
),
assertLogged(true, "warn", messages.CSPFK064E, "secret1", "base64", "illegal base64 data"),
},
},
{
Expand Down Expand Up @@ -560,6 +596,7 @@ func TestProvide(t *testing.T) {
mocks := newTestMocks()
mocks.setPermissions(tc.denyConjurRetrieve, tc.denyK8sRetrieve,
tc.denyK8sUpdate)

for secretName, secretData := range tc.k8sSecrets {
mocks.kubeClient.AddSecret(secretName, secretData)
}
Expand Down

0 comments on commit ed42696

Please sign in to comment.