donders-institute / hpc-webhook Goto Github PK

The HTTP method should be DELETE and the URL should be:

http(s)://{QaasHost}:{QaasPort}/configuration/{WebhookID}

It should just return HTTP code 200 on success, 404 if the {WebhookID} is not found.

Question: how to be sure the webhook is deleted by the owner of it, not any random user?

@rutgervandeelen

It looks to me that this will result in removing existing table after restart of the Postgresql container. Is it intended?

https://github.com/Donders-Institute/hpc-qaas/blob/612d7ee18355cc1d12e149b9d6306ec048fa0c55/init/01-initialize-database.sh#L5

@rutgervandeelen

I think it is safer to also match the username (and/or group name) when retrieving webhook information from the database.

https://github.com/Donders-Institute/hpc-qaas/blob/7c1510eb010636e5dc327069ca40c8657e1ad685/internal/server/database.go#L96

Since we have a working directory for each webhook under user's home directory, I think we could make use it to allow creating webhook-specific secret using an approach similar to the .htpasswd file.

For example, after a user creates a webhook,

$ hpcutil webhook create {qsubScript}

The user can optionally secure it by using the client:

$ hpcutil webhook secure {webhookID} --secret {webhookSecret}

or these two steps can be combined in one command:

$ hpcutil webhook create {subscript} --secret {webhookSecret}

Under the hood, the client tool writes the (oneway-hashed) secret in a file (e.g. secret) in the webhook's working directory.

When there is a trigger to the webhook, the server checks whether there is such secret file available in its working directory, if so, it tries to match the secret received from the HTTP request header to the secret in the file. The following qsub command is only performed if there is a match.

If the webhook folder doesn't have the secret file in it, the trigger is then accepted without the check. This allows the user to remove (and reset) the secret easily by just remove the secret file.

roboos@mentat005> hpcutil webhook list
ERRO[0000] error retrieving webhook info from the QaaS server: 404 Not Found (HTTP CODE: 404) 
ERRO[0000] error retrieving webhook info from the QaaS server: 404 Not Found (HTTP CODE: 404)

Perhaps this is due to you not having completed the install yet. In that case, please do let me know when it is ready.

@rutgervandeelen

It's nice to see that you return whatever in the database Item back as the information; but I miss the full Webhook URL which needs to be constructed by taking the Hash of the Webhook and the base URL of the webhook trigger. Please also provide it as part of the returned Webhook info.

https://github.com/Donders-Institute/hpc-qaas/blob/96650f36c11ec6adb4aa0f1faa50e15a344ec6c7/internal/server/configuration.go#L28

@rutgervandeelen

In the code referred below, you seem to check and create a SSH key pair per user. This is to my understanding not necessary.

The entire QaaS can just use one key pair with the private key held by (and only accessible for) the QaaS service user; while the public key is distributed into users' ~/.ssh/authorized_keys file. This already allows QaaS service user to SSH into individual user's account.

https://github.com/Donders-Institute/hpc-qaas/blob/9bb5d69688f42c2e565113340027bb9cdacca184/internal/server/handlers.go#L81

@rutgervandeelen

I just noticed that the URL returned from the database.getRow doesn't contain the server.WebhookPath (see the link below). If one takes that URL and call it directly, the 404 NOT FOUND is returned.

https://github.com/Donders-Institute/hpc-qaas/blob/c9285264bbadb48536d1d95da4f30f18b256f123/internal/server/database.go#L135

so that triggers from GitHub (and maybe others) will not be terminated after exceeding the expected response time.

GitHub expects the webhook server to response within 10 seconds, otherwise it terminates the call. See this document from GitHub.

When it happens, the NGINX proxy server reports the following error log:

access.hpc-webhook.log-20191019.gz:192.30.252.99 - - [18/Oct/2019:13:19:09 +0200] "POST /webhook/91580b45-03e0-4b6f-9001-2c76e7ac4e4a HTTP/1.1" 499 0 "-" "GitHub-Hookshot/3b07851"
access.hpc-webhook.log-20191019.gz:140.82.115.245 - - [18/Oct/2019:13:34:08 +0200] "POST /webhook/91580b45-03e0-4b6f-9001-2c76e7ac4e4a HTTP/1.1" 499 0 "-" "GitHub-Hookshot/3b07851"
access.hpc-webhook.log-20191019.gz:140.82.115.251 - - [18/Oct/2019:15:40:31 +0200] "POST /webhook/91580b45-03e0-4b6f-9001-2c76e7ac4e4a HTTP/1.1" 499 0 "-" "GitHub-Hookshot/3b07851"
access.hpc-webhook.log-20191019.gz:192.30.252.97 - - [18/Oct/2019:23:48:16 +0200] "POST /webhook/91580b45-03e0-4b6f-9001-2c76e7ac4e4a HTTP/1.1" 499 0 "-" "GitHub-Hookshot/6c1acbb"
access.hpc-webhook.log-20191020.gz:140.82.115.245 - - [19/Oct/2019:15:45:18 +0200] "POST /webhook/91580b45-03e0-4b6f-9001-2c76e7ac4e4a HTTP/1.1" 499 0 "-" "GitHub-Hookshot/6c1acbb"
access.hpc-webhook.log-20191020.gz:140.82.115.249 - - [19/Oct/2019:20:43:59 +0200] "POST /webhook/91580b45-03e0-4b6f-9001-2c76e7ac4e4a HTTP/1.1" 499 0 "-" "GitHub-Hookshot/6c1acbb"
access.hpc-webhook.log-20191020.gz:192.30.252.98 - - [19/Oct/2019:23:46:51 +0200] "POST /webhook/91580b45-03e0-4b6f-9001-2c76e7ac4e4a HTTP/1.1" 499 0 "-" "GitHub-Hookshot/6c1acbb"

SSH connection needs to be closed when done. This is currently not the case.

Perhaps refactor connector.go and execute.go
and replace ssh.Dial convenience function call with something else:

c, err := net.DialTimeout(network, addr, timeout)
if err != nil {
    return nil, err
}
conn, chans, reqs, err := ssh.NewClientConn(c, addr, config)
if err != nil {
    return nil, err
}

// will close the underlying net.Conn
defer conn.Close 

client := ssh.NewClient(c, chans, reqs)

I recommend to rename this application from hpc-qaas to hpc-webhook

Motivation:

1. to make the name consistent with the hpcutil interface
2. to make the name consistent with its use on github and other external services
3. I tend to mistype it as "quaas", since a "q" is usually followed by a "u"

The primary application is to have a webhook server, which executes jobs on our compute cluster using qsub. The cluster itself is not the service, neither is qsub the service.

Since the script.sh file is not to be executed itself, I propose to call it script. That aligns better with payload, which ends up in the same directory.

The HTTP method should be GET, and the URL should be

http(s)://{QaasHost}:{QaasPort}/configuration/{webhookId}

@rutgervandeelen

Please return the ConfigurationResponse after the new webhoook is registered. This allows the client to give user a feedback with the Webhook URL he/she can then use to registry it on a Webhook trigger (e.g. on GitHub).

You seem to return the ConfigurationResponse in the WebhookHandler, which I don't think is necessary as I don't think the Webhook trigger (such as GitHub) will be able to process this response.

https://github.com/Donders-Institute/hpc-qaas/blob/9bb5d69688f42c2e565113340027bb9cdacca184/internal/server/handlers.go#L113

for consistency it would be better if the directory would not have an "s" in its name.