nearlyfreespeech: fix authentication

[robryk]

This change fixes the issue where the path that was hashed was missing the initial slash. There is a bug in Golang's stdlib that surfaces when one uses url.Parse with a URL that has an empty path and no slash just after the domainname (https://golang.org/issue/58605). We add a workaround for that issue.

Also, this change adds a golden value test for the contents of the authentication header, to guard against changes caused by e.g. updates to Golang stdlib. Sadly, they will need updating if we make a deliberate change to the requests. I think they are still the best we can do without a reference implementation to compare against in a test.

Fixes #1998

[ldez]

What is the fix of the issue exactly? Because the code seems the same as before but just with global vars.

[robryk]

The previous version hashed URL paths that were missing the initial slash. The linked golang bug describes how one ends up with such paths.

[ldez]

Your PR doesn't change that: the BaseURL is join with some path so the "missing" / on the baseURL has no impact.

[robryk]

Sadly, it does, because net/url.Parse and/or net/url.JoinPath are buggy (see https://golang.org/issue/58605).

[ldez]

sorry but your changes don't fix JoinPath.

[robryk]

JoinPath works "incorrectly" if the original URL has empty Path. My change causes the base URL not to have empty Path.

It's debatable whether this is an issue of JoinPath: net/url's documentation claims that only relative URLs (which in particular have empty Host) should have a Path that doesn't start with a slash. Parse creates URL structs that do not satisfy that requirement (see https://go.dev/play/p/H6H902lJTam).

[ldez]

https://go.dev/play/p/kzSf05kp_ll

[robryk]

Note that https://github.com/go-acme/lego/blob/aeec5be129cfe1230ef9ab19bc48c3a5eddd411a/providers/dns/nearlyfreespeech/internal/client.go#L77 looks at endpoint.Path and not endpoint.String().

[ldez]

I think I understand now: https://go.dev/play/p/5XnhHCPfwce

But your fix is not at the right place.

[ldez]

you just have to add the following code inside the createSignature function:

// Workaround for https://golang.org/issue/58605
uri = "/" + strings.TrimLeft(uri, "/")

[ldez]

I would like to summarize: your explanation was not clear to me because the problem is related to JoinPath and not url.Parse. It's just a misunderstanding, I'm sorry I was a bit rude.

[robryk]

the problem is related to JoinPath and not url.Parse

That's at least debatable.

url.Parse parses http://a.com and http://a.com/ into two different structs. The former gets parsed into a struct that golang documentation claims is not a valid way to represent that URL (the field docstring says "path (relative paths may omit leading slash)", which implies that absolute ones cannot). See https://github.com/golang/go/issues/58605#issuecomment-1438790371 for someone else expressing this in different words.

[ldez]

My previous message was just to say sorry and explain my reaction. So yes everything is debatable but now we have to move forward on this PR.

WDYT about my proposal: https://github.com/go-acme/lego/pull/1999#discussion_r1299271371

[robryk]

Please take a look. I've also split this into two commits (which I should have done in the first place).

[robryk]

Would you prefer to take this PR over? I don't understand the reasons for the changes you request, so I'm effectively acting as an interface to a text editor.

[robryk]

(reposting from commit comments on request)

I'm afraid the merged version is broken in a different way: we use a different salt when computing the input to the hash (one line above) compared to the one we send. Thus, when the server attempts to verify the authHeader by recomputing the hash using the provided salt, it will get a different result and reject the request.

[ldez]

yes, my fault, I will fix that.

[ldez]

Fixed in a7befed6e8effcbe6792eea0dfae9cefd6661a24