Skip to content

feat(compose-spec): Add content property for secrets in compose-spec.json#669

Merged
glours merged 2 commits intocompose-spec:mainfrom
idsulik:fix-issue-12033
Sep 9, 2024
Merged

feat(compose-spec): Add content property for secrets in compose-spec.json#669
glours merged 2 commits intocompose-spec:mainfrom
idsulik:fix-issue-12033

Conversation

@idsulik
Copy link
Copy Markdown
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

@idsulik idsulik requested a review from ndeloof as a code owner July 30, 2024 15:31
@idsulik idsulik mentioned this pull request Jul 30, 2024
Closed
@ndeloof
Copy link
Copy Markdown
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

ndeloof
ndeloof requested changes Jul 30, 2024
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
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
ndeloof
ndeloof reviewed Jul 30, 2024
View reviewed changes
@idsulik idsulik requested a review from ndeloof July 30, 2024 17:43
@ndeloof
Copy link
Copy Markdown
Collaborator

ndeloof commented Jul 31, 2024 •
edited
Loading

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

idsulik
idsulik commented Jul 31, 2024
View reviewed changes
schema/schema.go Outdated

// 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
Copy Markdown
Contributor Author

@idsulik idsulik Jul 31, 2024

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
Copy Markdown
Collaborator

@ndeloof ndeloof Jul 31, 2024

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
Copy Markdown
Contributor Author

@idsulik idsulik Jul 31, 2024

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
Copy Markdown
Contributor Author

@idsulik idsulik Jul 31, 2024

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
ndeloof
ndeloof reviewed Sep 4, 2024
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Collaborator

ndeloof commented Sep 4, 2024

oh indeed.

@ndeloof
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
glours
glours approved these changes Sep 9, 2024
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

idsulik commented Sep 9, 2024

@glours done

@glours glours enabled auto-merge (rebase) September 9, 2024 13:56
@idsulik @ndeloof
fix(secrets): Fix secrets environment to content mapping
c3bc0bf 
Signed-off-by: Suleiman Dibirov <idsulik@gmail.com>
@idsulik @ndeloof
loaders fixes
e00c25f 
Signed-off-by: Suleiman Dibirov <idsulik@gmail.com>
@glours glours merged commit 9601b45 into compose-spec:main Sep 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ndeloof ndeloof ndeloof approved these changes

@glours glours glours approved these changes

@jhrotko jhrotko Awaiting requested review from jhrotko

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[BUG] regression in compose secrets handling for include files between versions Docker version 26.1.4, build 5650f9b and 27.1.1, build 6312585

3 participants

@idsulik @ndeloof @glours