Skip to content
This repository has been archived by the owner on Feb 14, 2024. It is now read-only.

Support unsigned command verification into the tool #15

Closed
wants to merge 3 commits into from

Conversation

zsims
Copy link
Contributor

@zsims zsims commented Oct 3, 2018

This closes #14 by allowing a list of commands to be specified that can remain unsigned. This ensures plugins are still verified, and/or the step is still verified if a step signature is found.

I'll look at a "command eval" style approach in a future PR -- e.g. any script in the repository may be executed by default.

@zsims zsims self-assigned this Oct 3, 2018
@zsims zsims requested a review from lox October 3, 2018 10:21
@zsims
Copy link
Contributor Author

zsims commented Oct 3, 2018

I'm wondering how configurable this needs to be @lox... I'm thinking this could instead be some hard coded rules that are:

  1. Allow any local/repository script (no command eval style); and/or
  2. Allow buildkite-signed-pipeline upload <a local file here>

Keen for thoughts, as the above is really the behaviour most people would want (I imagine)

@zsims
Copy link
Contributor Author

zsims commented Oct 3, 2018

Note that the rules get slightly complex if plugins are referenced, but the kingpin command parsing could be used to disambiguate buildkite-signed-pipeline upload, e.g.

if strings.HasPrefix(command, "buildkite-signed-pipeline upload") {
  // do kingpin parsing of `command`
  // work out what the `file` is (if any)
  // check to make sure `file` is a file in the repository
}

@zsims zsims closed this Oct 3, 2018
@zsims zsims reopened this Oct 3, 2018
@zsims
Copy link
Contributor Author

zsims commented Oct 3, 2018

I'll look at this as part of #16 -- as this PR closes the possibility of plugins to be injected into "allowed commands"

@lox
Copy link
Contributor

lox commented Oct 3, 2018

I reckon it might be a good idea to write up in the README "attack scenarios", e.g what potential threats we are aiming to solve and scenarios and how buildkite-signed-pipeline defends against them. Otherwise it's easy to forget edge cases when we are adding new features. If I get a chance I'll make a start!

@zsims
Copy link
Contributor Author

zsims commented Oct 3, 2018

Good idea, will help shape this :-)

log.Println("✅ Allowing allow-listed command as no signature is specified, and no plugins are referenced")
return nil
}
log.Println("❗️Forcing signature check as pstep has an expected signature or referenced plugins")
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo? pstep

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good pickup! Indeed

Copy link
Contributor

@lox lox left a comment

Choose a reason for hiding this comment

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

This is looking really good, nice one @zsims

@zsims
Copy link
Contributor Author

zsims commented Oct 5, 2018

After writing up the attack scenarios (great suggestion, many thanks 😁) I'm not convinced this level of flexibility is needed as from what I've seen consumers want to be able to:

  1. Call builkite-signed-pipeline with any arguments; or
  2. Run a script in their repository

Do you think it's worth addressing those first? This PR won't let you call buildkite-signed-pipeline upload mypipeline.yml unless you explicitly allow it -- which is hard to do given it's pipeline specific where this tool runs on agents

@lox
Copy link
Contributor

lox commented Oct 9, 2018

After some thought, I completely agree. I reckon we should keep it simple and not introduce this!

@zsims
Copy link
Contributor Author

zsims commented Oct 9, 2018

Awesome, will look at a simple PR that let's through unsigned buildkite-signed-pipeline upload invocations.

@zsims zsims closed this Oct 9, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add an allow list of commands that can be un-signed
2 participants