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

Add webhook package #614

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

kamaln7
Copy link
Contributor

@kamaln7 kamaln7 commented Apr 7, 2023

This adds a webhook package to be used by DigitalOcean users directly and consulted as a reference implementation of the request signing spec.

Example usage:

http.HandleFunc("/webhook", func(w http.ResponseWriter, req *http.Request) {
   // only allow POST requests
   if req.Method != http.MethodPost {
       w.WriteHeader(http.StatusMethodNotAllowed)
       return
   }


   // verify request authenticity
   err := webhook.VerifyHTTPRequest(req, "webhook-secret", webhook.VerificationOpts{})
   if err != nil {
       w.WriteHeader(http.StatusForbidden)
       w.Write([]byte(err.Error()))
       return
   }


   // read the request body
   eventJSON, err := io.ReadAll(req.Body)
   if err != nil {
       w.WriteHeader(http.StatusInternalServerError)
       w.Write([]byte(err.Error()))
       return
   }
   defer req.Body.Close()
  
   eventName := req.Header.Get(webhook.HTTPHeaderEventName)
   // parse the request body json into a structured type using the eventName to determine
   // which type of event it is
   // ...
})

Copy link
Member

@andrewsomething andrewsomething left a comment

Choose a reason for hiding this comment

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

For posterity, I wanted to carry over my comment from a discussion of this elsewhere. It would be great to have some package level comments that discuss the signature format and perhaps include the example from the PR description.

@imhunterand
Copy link

Hi @kamaln7

I have reviewed the provided Go code for the webhook/webhook_test.go and webhook/webhook.go files. I found a potential vulnerability related to insufficient verification of HTTP request headers, which can be exploited to bypass the signature verification process. This vulnerability arises due to the lack of verification of the Do-Event-Name header, which can be manipulated by an attacker to forge a valid-looking signature.

Here's a description of the vulnerability and a proof of concept:

The VerifyHTTPRequest function in the webhook.go file checks the Do-Signature header for the signature package and verifies it. However, it does not check the Do-Event-Name header, which is expected to contain the event name. An attacker can manipulate the Do-Event-Name header to forge a valid-looking signature, bypassing the signature verification process.

  • Create a malicious HTTP request with a custom Do-Event-Name header.
POST /webhook-endpoint HTTP/1.1
Host: your-webhook-server.com
Content-Type: application/json
Do-Signature: t=1633027200,v1=<legitimate-signature>
Do-Event-Name: admin.delete

{"key": "value"}
  • In the webhook.go file, locate the VerifyHTTPRequest function.
  • Modify the function to include the Do-Event-Name header verification.
func VerifyHTTPRequest(r *http.Request, secret string, opts VerificationOpts) error {
	header := r.Header.Get(HTTPHeaderSignature)
	if header == "" {
		return ErrNotSigned
	}

	eventName := r.Header.Get(HTTPHeaderEventName)
	if eventName == "" {
		return fmt.Errorf("missing event name")
	}

	// Verify the event name here, e.g., by comparing it against a list of allowed event names.

	sigPack, err := ParseSignaturePackage(header)
	if err != nil {
		return fmt.Errorf("parsing signature header: %w", err)
	}

	body, err := io.ReadAll(r.Body)
	if err != nil {
	

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.

None yet

3 participants