Skip to content
This repository has been archived by the owner. It is now read-only.

Feature webhook #163

Merged
merged 17 commits into from Apr 24, 2017

Conversation

@mainephd
Copy link

mainephd commented Apr 11, 2017

Addresses issue #331

The resource DSL has been updated to include a webhook_token attribute that allows the end user to configure an arbitrary string to be used to trigger a resource to perform a check.

The webhook would be http://{yourserver}/api/v1/teams/:team_name/pipelines/:pipeline_name/resources/:resource_name/check/webhook?access_token=somevalue

There is also a convenience method to generate a token value
http://{yourserver}/api/v1/teams/:team_name/auth/access_token

A separate pull request would need to be submitted to pull in the dependency on https://github.com/satori/go.uuid

@pivotal-issuemaster

This comment has been minimized.

Copy link

pivotal-issuemaster commented Apr 11, 2017

@mainephd Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@pivotal-issuemaster

This comment has been minimized.

Copy link

pivotal-issuemaster commented Apr 11, 2017

@mainephd Thank you for signing the Contributor License Agreement!

@concourse-bot

This comment has been minimized.

Copy link

concourse-bot commented Apr 11, 2017

Hi there!

We use Pivotal Tracker to provide visibility into what our team is working on. A story for this issue has been automatically created.

The current status is as follows:

This comment, as well as the labels on the issue, will be automatically updated as the status in Tracker changes.

Copy link
Member

vito left a comment

Thanks a bunch for taking this on! Lots of people have wanted this.

Feedback:

I think we can do without the token generation endpoint. I'm not sure how a user would invoke it, and it's easy enough to just put an arbitrary string in the pipeline .yml. Having it be simple static config in the pipeline means you can just mash on the keyboard to make one, and rotate it by just reconfiguring the pipeline.

I also think we should move away from the generic access_token idea. I know in the original issue we mentioned the idea of having generateable tokens passed in query params as a precursor to this feature, but we've since moved away from that approach for security reasons. For example, intermediate routing tiers will often include query params in access logs, so if we go with that approach for general tokens, that'll be an easy leak.

So, I think it's better to special-case this endpoint's auth rules and not make it look generic. The severity of a leak is very low[1], so using query params is still fine with me, I just don't want it to look like a general API. Sorry for not going back to the original issue and updating it with our thoughts on this.

Also, given that this is a single string value that we're just checking for a match on, it's more of a password. So maybe we should either use basic auth in some way, or just call the query param and config value in the pipeline webhook_password? (I'd suggest being able to configure username/password as it would be nice to just use basic auth rather than a query param, as those won't be logged, but there's not really much point in having the user configure both...)

[1]: All they can do is resource check immediately rather than 1 minute from now, and they can't even inject anything for the check.


if accessToken == "" {
logger.Info("malformed-request", lager.Data{"error": "missing access_token"})
w.WriteHeader(http.StatusBadRequest)

This comment has been minimized.

Copy link
@vito

vito Apr 13, 2017

Member

Should this be a 401?

This comment has been minimized.

Copy link
@mainephd

mainephd Apr 14, 2017

Author

i waffled on whether to send a 400 vs a 401. It technically is a bad request because all of the required attributes aren't present but based on your equating the token to a password then 401 make sense. So I can go either way.

accessToken := r.URL.Query().Get("access_token")

if accessToken == "" {
logger.Info("malformed-request", lager.Data{"error": "missing access_token"})

This comment has been minimized.

Copy link
@vito

vito Apr 13, 2017

Member

This can probably just say no-access-token (or no-password, given my larger comment) and not log the error bit.

This comment has been minimized.

Copy link
@mainephd

mainephd Apr 14, 2017

Author

If the basic-auth approach is taken would the built in basic_auth_validator need to be used or setup as some middleware for this endpoint? The logger statement is easy enough to change so i'll make the change.

This comment has been minimized.

Copy link
@mainephd

mainephd Apr 14, 2017

Author

And you are welcome. It was cool to finally dig into the code that has made our lives ALOT easier to manage in terms of development.

This comment has been minimized.

Copy link
@vito

vito Apr 18, 2017

Member

I wouldn't lean on the existing BasicAuthValidator as that is specifically for team-configured basic auth. This is sort of its own way of checking auth, particular to this endpoint, so it could just interpret the Authorization header itself, with it having been ignored by all the other middleware.

Maybe it could just statically check for webhook as the user and the configured password as the password?


token := pipelineResource.Config.WebhookToken
if token != accessToken {
logger.Info("invalid-token-error", lager.Data{"error": fmt.Sprintf("Actual token %s does not match expected token %s", accessToken, token)})

This comment has been minimized.

Copy link
@vito

vito Apr 13, 2017

Member

No need for the error bit here; I don't think we should log either token.

This comment has been minimized.

Copy link
@mainephd

mainephd Apr 14, 2017

Author

Agreed.

Stderr: scanErr.Stderr,
}
w.Header().Set("Content-Type", "application/json")
w.WriteHeader(http.StatusBadRequest)

This comment has been minimized.

Copy link
@vito

vito Apr 13, 2017

Member

This shouldn't be a 400 error. Maybe 500? But maybe this is existing bad UX with the checking endpoint; I'm guessing this was copypasta.

We should also not print the stderr for the webhook case; if someone manages to acquire the password, this could lead to further information leakage.

How about just check for an error and log it? Same with the case error branch. Guess this case could just be removed at that point.

This comment has been minimized.

Copy link
@mainephd

mainephd Apr 14, 2017

Author

This was indeed copypasta. :) So if an error occurs (essentially all of the switch statement) then just return a 500 error?

newHandler = auth.WrapHandler(newHandler, wrappa.getTokenValidator, wrappa.userContextReader)
} else {
newHandler = auth.WrapHandler(newHandler, wrappa.authValidator, wrappa.userContextReader)
}
wrapped[name] = auth.CSRFValidationHandler(newHandler, rejector, wrappa.userContextReader)
if name != atc.CheckResourceWebHook {

This comment has been minimized.

Copy link
@vito

vito Apr 13, 2017

Member

Why's this needed?

This comment has been minimized.

Copy link
@billimek

billimek Apr 13, 2017

We were expecting this question! @mainephd can probably provide more context, but:

CSRFValidationHandler expects the client to be authenticated for all API calls unless they are a GET (e.g. DownloadCLI, ListAllPipelines, etc). Issue here is that the webhook is a POST, and won't get the exception inside CSRFValidationHandler, hence this check.

See

if r.Method != http.MethodGet && r.Method != http.MethodHead && r.Method != http.MethodOptions {
for context of the exception logic inside CSRFValidationHandler

This comment has been minimized.

Copy link
@mainephd

mainephd Apr 14, 2017

Author

Without this special case the webhook caller would have to include the X-Csrf-Token header on each request and shouldn't be available for the caller to access otherwise the CSRF protection becomes useless. So thus we added the special case. We wanted to do it more generically but that would have been a bigger change.

This comment has been minimized.

Copy link
@vito

vito Apr 18, 2017

Member

Hmm I don't think this will be required for the actual use-case though. The X-Csrf-Token validation is only checked if the user has the ATC-Authorization cookie present. Requests made to this endpoint will have neither Authorization nor ATC-Authorization, as the endpoint has its own special auth rules (the query param).

This comment has been minimized.

Copy link
@mainephd

mainephd Apr 19, 2017

Author

So because of this line https://github.com/concourse/atc/blob/master/auth/cookie_set_handler.go#L16 it would only check that the X-Csrf-Token is specified. I have tested locally in no-auth mode and it works as you described with the ATC-Authorization header is not present

newHandler = auth.WrapHandler(newHandler, wrappa.getTokenValidator, wrappa.userContextReader)
} else {
newHandler = auth.WrapHandler(newHandler, wrappa.authValidator, wrappa.userContextReader)
}
wrapped[name] = auth.CSRFValidationHandler(newHandler, rejector, wrappa.userContextReader)
if name != atc.CheckResourceWebHook {

This comment has been minimized.

Copy link
@vito

vito Apr 18, 2017

Member

Hmm I don't think this will be required for the actual use-case though. The X-Csrf-Token validation is only checked if the user has the ATC-Authorization cookie present. Requests made to this endpoint will have neither Authorization nor ATC-Authorization, as the endpoint has its own special auth rules (the query param).

}

scanner := s.scannerFactory.NewResourceScanner(pipelineDB, dbPipeline)
err = scanner.ScanFromVersion(logger, resourceName, nil)

This comment has been minimized.

Copy link
@vito

vito Apr 18, 2017

Member

This should actually check from the current version, as in

fromVersion := reqBody.From
if fromVersion == nil {
latestVersion, found, err := pipelineDB.GetLatestVersionedResource(resourceName)
if err != nil {
logger.Info("failed-to-get-latest-versioned-resource", lager.Data{"error": err.Error()})
w.WriteHeader(http.StatusInternalServerError)
return
}
if found {
fromVersion = atc.Version(latestVersion.Version)
}
}
scanner := s.scannerFactory.NewResourceScanner(pipelineDB, dbPipeline)
err = scanner.ScanFromVersion(logger, resourceName, fromVersion)

The reason for this is that a hook may be triggered by someone pushing 100 commits. We want to find all the ones since the current version, not just the last of the 100 commits.

accessToken := r.URL.Query().Get("access_token")

if accessToken == "" {
logger.Info("malformed-request", lager.Data{"error": "missing access_token"})

This comment has been minimized.

Copy link
@vito

vito Apr 18, 2017

Member

I wouldn't lean on the existing BasicAuthValidator as that is specifically for team-configured basic auth. This is sort of its own way of checking auth, particular to this endpoint, so it could just interpret the Authorization header itself, with it having been ignored by all the other middleware.

Maybe it could just statically check for webhook as the user and the configured password as the password?

@@ -14,6 +14,7 @@ type CookieSetHandler struct {

func (handler CookieSetHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
cookie, err := r.Cookie(AuthCookieName)

This comment has been minimized.

Copy link
@vito

vito Apr 18, 2017

Member

Don't need this change; mind rebasing it out just to reduce files touched?

This comment has been minimized.

Copy link
@mainephd
@@ -330,7 +330,7 @@ func (config TaskConfig) validateInputContainsNames() []string {
type TaskRunConfig struct {
Path string `json:"path" yaml:"path"`
Args []string `json:"args,omitempty" yaml:"args"`
Dir string `json:"dir",omitempty" yaml:"dir"`

This comment has been minimized.

Copy link
@vito

vito Apr 18, 2017

Member

Nice catch, though did you see this breaking things? Should this be its own release note?

This comment has been minimized.

Copy link
@mainephd

mainephd Apr 18, 2017

Author

When i ran the unit test locally it did not break things. It probably should be noted in a release cause it could be broken. Locally the the unit test were not all passing even on a fresh clone of the concourse repo.

This comment has been minimized.

Copy link
@vito

vito Apr 24, 2017

Member

I'll track this as a separate bug and see if it actually affected anything. Thanks!

This comment has been minimized.

Copy link
@mainephd

mainephd Apr 24, 2017

Author

So I am clear. Do you want me to revert my change or leave it in tact?

This comment has been minimized.

Copy link
@vito

vito Apr 24, 2017

Member

You can leave it - the separate bug is in our Tracker just so I can do explicit acceptance on it.

@@ -112,7 +112,7 @@ func indexHtml() (*asset, error) {
return nil, err
}

info := bindataFileInfo{name: "index.html", size: 2615, mode: os.FileMode(420), modTime: time.Unix(1491867544, 0)}

This comment has been minimized.

Copy link
@vito

vito Apr 18, 2017

Member

These changes probably shouldn't be here?

This comment has been minimized.

Copy link
@mainephd

mainephd Apr 18, 2017

Author

I am not sure how this line was removed. I'll have to do some digging to see how it got removed.

@mainephd

This comment has been minimized.

Copy link
Author

mainephd commented Apr 20, 2017

@billimek @vito
The acceptance test seems to be behave more consistently locally when i change the default timeout and polling configurations of the Eventually assertions from

SetDefaultEventuallyTimeout(10 * time.Second)
SetDefaultEventuallyPollingInterval(100 * time.Millisecond)

to

 SetDefaultEventuallyTimeout(20 * time.Second)
 SetDefaultEventuallyPollingInterval(1 * time.Second)

Is this a change I should commit or leave it alone?

Copy link
Member

vito left a comment

Almost there! Left one request for change. Sorry for the delay.

@@ -330,7 +330,7 @@ func (config TaskConfig) validateInputContainsNames() []string {
type TaskRunConfig struct {
Path string `json:"path" yaml:"path"`
Args []string `json:"args,omitempty" yaml:"args"`
Dir string `json:"dir",omitempty" yaml:"dir"`

This comment has been minimized.

Copy link
@vito

vito Apr 24, 2017

Member

I'll track this as a separate bug and see if it actually affected anything. Thanks!


return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
resourceName := rata.Param(r, "resource_name")
accessToken := r.URL.Query().Get("access_token")

This comment has been minimized.

Copy link
@vito

vito Apr 24, 2017

Member

Can we rename this to webhook_token? It has specific handling for this endpoint, whereas access_token sounds like a generic API for passing a token to all handlers.

This comment has been minimized.

Copy link
@mainephd

mainephd Apr 24, 2017

Author

That is easy enough.

@vito

This comment has been minimized.

Copy link
Member

vito commented Apr 24, 2017

Awesome, thanks! 🎊

@vito
vito approved these changes Apr 24, 2017
@vito

This comment has been minimized.

Copy link
Member

vito commented Apr 24, 2017

The CI failure is in acceptance/ - not your fault. I've logged a bug to have us clean that up, it's really annoying for PRs. Merging!

@vito vito merged commit 90e3f22 into concourse:master Apr 24, 2017
1 of 2 checks passed
1 of 2 checks passed
concourse-ci/status Concourse CI build failure
Details
ci/pivotal-cla Thank you for signing the Contributor License Agreement!
Details
@vito vito added this to the v3.0.0 milestone May 13, 2017
@vito vito added the efficiency label May 14, 2017
@mainephd mainephd deleted the mainephd:feature-webhook branch Sep 17, 2017
@trivigy

This comment has been minimized.

Copy link

trivigy commented Apr 18, 2018

What is the right way to deal with check_every when setting the webhook_token?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
6 participants
You can’t perform that action at this time.