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(compose-spec): Add content property for secrets in compose-spec.json #669

Merged
merged 2 commits into from
Sep 9, 2024

Conversation

idsulik
Copy link
Contributor

@idsulik idsulik commented Jul 30, 2024

Fixes docker/compose#12033

Add content to secret property, because in case of environment value, we add "content" property but it wasn't defined in the spec

@ndeloof
Copy link
Collaborator

ndeloof commented Jul 30, 2024

we add "content" property but it wasn't defined in the spec

This is by design: we don't want sensible data to be exposed as plain text in a compose file

Copy link
Collaborator

@ndeloof ndeloof left a comment

Choose a reason for hiding this comment

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

This introduces a major risk for users to put sensible data in a compose file and publish by mistake, we can't let them shoot into their own feet

@idsulik
Copy link
Contributor Author

idsulik commented Jul 30, 2024

@ndeloof good point, thank you! pushed fix. Added validation to prevent from specifying "content" for the secrets property

@idsulik idsulik requested a review from ndeloof July 30, 2024 16:34
validation/validation.go Outdated Show resolved Hide resolved
@idsulik idsulik requested a review from ndeloof July 30, 2024 17:43
@ndeloof
Copy link
Collaborator

ndeloof commented Jul 31, 2024

SecretConfig already has a custom MarshalJSON so that content is excluded from json output. But with use of map[string]any for yaml processing, this isn't used by gojsonschema validator. Will need to find an equivalent hack

schema/schema.go Outdated
@@ -72,6 +73,19 @@ func Validate(config map[string]interface{}) error {
return nil
}

// removeSecretsContentProperty removes the content property from secrets
// we add the content key here loader/environment.go:66
func removeSecretsContentProperty(config map[string]interface{}) map[string]interface{} {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ndeloof here is an equivalent ) at least tmp solution

Copy link
Collaborator

Choose a reason for hiding this comment

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

but we loose value doing so

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we can copy for validation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pushed changes. now it clones the map and validate

@idsulik idsulik force-pushed the fix-issue-12033 branch 2 times, most recently from 774b32d to 37c46f9 Compare July 31, 2024 19:00
Copy link
Collaborator

@ndeloof ndeloof left a comment

Choose a reason for hiding this comment

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

AFAICT this won't catch a compose file using invalid attribute content to set secret, while value will be actually loaded.

@ndeloof
Copy link
Collaborator

ndeloof commented Sep 4, 2024

Suggestion: to avoid letting content be used in compose.yaml and miss it during validation, resolved value could be stored in map with key #value (with a #) which makes it an invalid key in yaml (until user set key with quote, but this can't be by accident then).

@idsulik
Copy link
Contributor Author

idsulik commented Sep 4, 2024

Suggestion: to avoid letting content be used in compose.yaml and miss it during validation, resolved value could be stored in map with key #value (with a #) which makes it an invalid key in yaml (until user set key with quote, but this can't be by accident then).

@ndeloof , if you mean this changes:

# environment.go
		if found, ok := environment[env]; ok {
			secret["#value"] = found
		}

then it won'r work:

secrets.one-secret-token Additional property #value is not allowed

@ndeloof
Copy link
Collaborator

ndeloof commented Sep 4, 2024

oh indeed.

@ndeloof
Copy link
Collaborator

ndeloof commented Sep 9, 2024

A possible workaround is to use a fake extension key x-#value that would be valid regarding the spec

@idsulik
Copy link
Contributor Author

idsulik commented Sep 9, 2024

A possible workaround is to use a fake extension key x-#value that would be valid regarding the spec

I'm not sure I get the idea if I rename

if found, ok := environment[env]; ok {
	secret["content"] = found
}

into

if found, ok := environment[env]; ok {
	secret["x-#value"] = found
}

Then how do we map this key into the Secret struct's content field?

@ndeloof
Copy link
Collaborator

ndeloof commented Sep 9, 2024

Then how do we map this key into the Secret struct's content field?

need to add a DecodeMapstructure func to FileObjectConfig so it detects this custom key is set and use it as value

@idsulik
Copy link
Contributor Author

idsulik commented Sep 9, 2024

@ndeloof pushed changes.
p.s. extracted the key name into constant for consistency

@ndeloof ndeloof requested review from glours and jhrotko September 9, 2024 12:55
Copy link
Collaborator

@glours glours left a comment

Choose a reason for hiding this comment

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

LGTM
@idsulik can you sign-off your commits please?

@idsulik
Copy link
Contributor Author

idsulik commented Sep 9, 2024

@glours done

@glours glours enabled auto-merge (rebase) September 9, 2024 13:56
Signed-off-by: Suleiman Dibirov <idsulik@gmail.com>
Signed-off-by: Suleiman Dibirov <idsulik@gmail.com>
@glours glours merged commit 9601b45 into compose-spec:main Sep 9, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants