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

Include plugins in command step signatures #2292

Merged
merged 1 commit into from
Aug 15, 2023
Merged

Conversation

DrJosh9000
Copy link
Contributor

Plugins alter the execution of a step completely, so the list of plugins and their configurations must be included in the signature for it to be secure.

This PR currently encodes plugin configs into JSON. This is potentially not a great encoding (what stability guarantees do we really have?), but because this is how the whole pipeline is uploaded, it ought to round-trip well enough.

@DrJosh9000 DrJosh9000 requested a review from a team August 15, 2023 03:38
Copy link
Contributor

@triarius triarius left a comment

Choose a reason for hiding this comment

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

LGTM. Let's see what if anything breaks with the JSON representation of plugins.

func (c *CommandStep) SignedFields() (map[string]string, error) {
plugins := ""
if len(c.Plugins) > 0 {
// TODO: Reconsider using JSON here - is it stable enough?
Copy link
Contributor

Choose a reason for hiding this comment

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

Ordered Maps are an issue, yeah? I think we can overcome it if we marshal all the unordered stuff into ordered equivalents.

Another one could be floating point representation, and precision. If the backend or database changes the precision of a number, we could have an issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ordered Maps are an issue, yeah? I think we can overcome it if we marshal all the unordered stuff into ordered equivalents.

My thinking on this is: the pipeline parser ultimately shoves whatever ordered.DecodeYAML produces for the plugin config into Plugin.Config, which will tend to be either *ordered.Map[string, any] or nil, preserving input order. As long as the backend does it the same, it should be equal.

Another one could be floating point representation, and precision. If the backend or database changes the precision of a number, we could have an issue.

This kind of thing does concern me.

@DrJosh9000 DrJosh9000 merged commit 185c3c6 into main Aug 15, 2023
@DrJosh9000 DrJosh9000 deleted the sign-env-and-plugins branch August 15, 2023 05:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants