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

Allow buildkite-agent to run a job when JWK is unavailable but failure behaviour is set to warn #2945

Conversation

CheeseStick
Copy link
Contributor

@CheeseStick CheeseStick commented Aug 21, 2024

Context

Recently, while working on an agent migration plan, I noticed that the agent without JWK configuration is not compatible with an agent with JWK, even when the failure behaviour is set to warn.

Because the agent that started with the JWK configuration adds a job signature to the pipeline's job when it uploads, a requested job can fail if it lands on the agent that doesn't have a JWK key configuration.

Changes

This PR updates a job runner not to reject a signed job when JWK is not configured and verification-failure-behaviour is set to warn. It also adds a new error, SignalReasonUnableToVerifySignature, to the job runner so the misconfiguration can be monitored.

Testing

  • Tests have run locally (with go test ./...). Buildkite employees may check this if the pipeline has run automatically.
    image
  • Code is formatted (with go fmt ./...)

@wolfeidau
Copy link
Contributor

@CheeseStick thanks for posting this PR, we have just merged some changes which overlap with this code.

The fix for missing JWKS and warnings is a good pickup, we need to impove how we handle this, however the change to SignalReasonUnableToVerifySignature is something I need to dig into further as there are some security concerns.

That said having some specific log messages which are logged in the agent is what we are leaning towards.

Let me pull this down and do some testing.

@CheeseStick
Copy link
Contributor Author

@CheeseStick thanks for posting this PR, we have just merged some changes which overlap with this code.

The fix for missing JWKS and warnings is a good pickup, we need to impove how we handle this, however the change to SignalReasonUnableToVerifySignature is something I need to dig into further as there are some security concerns.

That said having some specific log messages which are logged in the agent is what we are leaning towards.

Let me pull this down and do some testing.

Thanks!!! 🙇

@wolfeidau wolfeidau force-pushed the junjung/pipeline-verification/failure-behaviour branch from c29b174 to 1bd8d64 Compare September 6, 2024 01:32
@wolfeidau
Copy link
Contributor

We have reviewed this soluton, and based on recent exprience, this change also makes it clearer when builds fail because agents aren't configured with a key, overall this is a plus.

Copy link
Contributor

@wolfeidau wolfeidau left a comment

Choose a reason for hiding this comment

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

Looks good 👍🏻

@wolfeidau wolfeidau merged commit fc6e3ac into buildkite:main Sep 6, 2024
1 check 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
Development

Successfully merging this pull request may close these issues.

2 participants