feat: initial implementation #1

aschmahmann · 2024-08-16T21:08:32Z

This is an initial implementation of the DNS server using CoreDNS.

The main missing feature for getting this to work is integration with a distributed database to be shared between the primary and secondary nameservers. It might be possible to do this using zone files as well, but using something like dynamodb for now seems fine.

Note: we might want to bump the dependencies in https://github.com/ipfs/go-ds-dynamodb

aschmahmann · 2024-08-16T22:12:52Z

README.md

+To set an ACME challenge send an HTTP request to the server (for libp2p.direct this is registration.libp2p.direct)
+```shell
+curl -X POST "https://registration.libp2p.direct/v1/<peerID>/_acme-challenge" \
+-H "Authorization: Bearer <signature>.<public_key>"


Note on why JWT did not feel great because

It takes the entire payload and puts it in the authorization header

No standards around body hash, including public keys, etc.

IIUC most implementations don't seem to support secp256k1 and IMO we'd want to enable each of the 4 currently supported key types (RSA, Ed25519, ECDSA, Secp256k1)

Is this meant to follow RFC 6750? The "Bearer" Auth scheme is registered to that RFC https://www.iana.org/assignments/http-authschemes/http-authschemes.xhtml.

I haven't looked closer here, because I'm assuming we'll opt to use the HTTP Peer ID authentication flow instead.

aschmahmann · 2024-08-16T22:18:44Z

README.md

+curl -X POST "https://registration.libp2p.direct/v1/<peerID>/_acme-challenge" \
+-H "Authorization: Bearer <signature>.<public_key>"


Note: We could've avoided doing anything special with the auth if we'd relied on libp2p doing the auth for us.

About the protocol format:

custom wire protocol over libp2p seemed like overkill

HTTP over libp2p would be nice and use the exact same semantics but without the auth header

About using libp2p for the transport:

The initial connection establishment and loadbalancing stories don't seem as easy as we'd want at the moment

I don't want to have to embed a peerID into deployed applications (e.g. what if we have to rotate keys) and while we can get the other approaches working they feel not out of the box yet

Could use CA certs instead of peerIDs for the outbound connection, but that requires more new code in the libp2p implementations

We could use DNSAddr (e.g. /dnsaddr/registration.libp2p.direct without a peerID suffix) which seems fine (even more so with DNSSEC), although a bit of extra wrapper code likely needed in each implementation since IIRC it's not supported out of the box in most implementations

It'd be nice to evolve this into being able to do HTTP over libp2p and drop the auth header by doing one or both of:

Enable DNSAddr or CA based connection establishment

Land the HTTP PeerID Auth spec (yes it means two round-trips instead of one but that doesn't seem like a big deal)

The result means there'd be very little custom code here at all given the user is already leveraging a libp2p implementation.

custom wire protocol over libp2p seemed like overkill

Land the HTTP PeerID Auth spec (yes it means two round-trips instead of one but that doesn't seem like a big deal)

Both of these seem like desire-able things that we may want at some point in the future. Is it worth investigating now before we implement some other solution that quickly goes out of date?

custom wire protocol over libp2p seemed like overkill

This doesn't seem particularly desirable to bother with.

Land the HTTP PeerID Auth spec

WDYT @MarcoPolo, does it make more sense to land the spec then do this business with the signed envelopes?

Yes. The spec is very close to being ready. And if we have a use case, then I'd be very motivated to get it landed.

I see that we're essentially reusing the acme challenge as the authentication challenge. This is probably (?) okay for this use case, but I think it would better practice to not reuse challenges in this way. The HTTP Peer ID Auth flow would use a challenge just for the authentication (and that's where the extra round trip comes from).

aschmahmann · 2024-08-16T22:21:00Z

README.md

+}'
+```
+
+Where the signature is a base64 encoding of the signature for a [libp2p signed envelope](https://github.com/libp2p/specs/blob/master/RFC/0002-signed-envelopes.md)


It seemed like grabbing this spec meant less overhead for implementers given that those using signed peer records (i.e. for gossipsub which is in most implementations) already do most of this. However, as can be seen from the Go code here go-libp2p hid most of the internals away such that actually getting at them was painful and/or required copy-pasting.

Alternatives welcome (including if it's just tossing all this and landing the HTTP PeerID Auth spec

aschmahmann · 2024-08-16T22:33:07Z

README.md

+Where the signature is a base64 encoding of the signature for a [libp2p signed envelope](https://github.com/libp2p/specs/blob/master/RFC/0002-signed-envelopes.md)
+where:
+- The domain separation string is "peer-forge-domain-challenge"
+- The payload type is the ASCII string "/peer-forge-domain-challenge"


I wasn't planning on using a payload type and registering it in the codec table, however:

If using / is valid as an escape for a user defined string without relying on the table that works for me

While the spec says using / is fine and that it's also ok to leave it empty https://github.com/libp2p/specs/blob/master/RFC/0002-signed-envelopes.md#payload-type-information the Go implementation disagrees and has tests to enforce this https://github.com/libp2p/go-libp2p/blob/a0349afb888abafcf7991c183197859759134dfc/core/record/envelope_test.go#L145

@MarcoPolo @sukunrt might have thoughts on ^

aschmahmann · 2024-08-16T22:34:13Z

acme/writer.go

+			return
+		}
+
+		const ttl = time.Hour


No idea what the TTL should be here for these records given they shouldn't really be needed for very long. Thoughts?

aschmahmann · 2024-08-16T22:34:33Z

acme/writer.go

+	// TODO: Do we need to listen on the identify event instead?
+	// TODO: Where do we want to record this information if anywhere?


Calling these out

yes, and probably at least a log for now?

aschmahmann · 2024-08-16T22:36:24Z

ipparser/plugin.go

+	ForgeDomain string
+}
+
+const ttl = 1 * time.Hour


This seems like it could be set to infinity. Any thoughts on the number, a day, 7 days, longer?

aschmahmann · 2024-08-16T22:58:59Z

cmd/main.go

+func main() {
+	coremain.Run()
+}


Looks like some details need to get hammered out here between this and the tests. We could also choose to use the stock Caddy binary and use everything we have as plugins. Whatever makes deployment and testing easier

SgtPooki

just read the README.md and left some comments

README.md

SgtPooki · 2024-08-19T17:18:48Z

README.md

+curl -X POST "https://registration.libp2p.direct/v1/<peerID>/_acme-challenge" \
+-H "Authorization: Bearer <signature>.<public_key>"


custom wire protocol over libp2p seemed like overkill

Land the HTTP PeerID Auth spec (yes it means two round-trips instead of one but that doesn't seem like a big deal)

Both of these seem like desire-able things that we may want at some point in the future. Is it worth investigating now before we implement some other solution that quickly goes out of date?

README.md

aschmahmann · 2024-08-22T15:16:21Z

client/acme.go

+
+var libp2pDirectWssComponent = multiaddr.StringCast("/tls/sni/*.libp2p.direct/ws")
+
+func NewHostWithP2PForge(forgeDomain string, forgeRegistrationEndpoint string, opts ...libp2p.Option) (host.Host, error) {


Note: This function certainly, but most of this file is pretty WIP.

Will help to flesh this out with an end-to-end test that does:

Sets up the DNS server

Uses https://github.com/letsencrypt/pebble/ as a test ACME server that uses the DNS server

Have a libp2p client (e.g. using this function or something similar) that gets a certificate for itself using the test ACME server + DNS server

Have another libp2p client that can dial the first one using WSS with a custom client TLS config that allows the test CA cert

aschmahmann · 2024-08-29T19:30:24Z

client/acme.go

+
+var libp2pDirectWssComponent = multiaddr.StringCast("/tls/sni/*.libp2p.direct/ws")
+
+func NewHostWithP2PForge(forgeDomain string, forgeRegistrationEndpoint string, caEndpoint string, userEmail string, trustedRoots *x509.CertPool, onCertLoaded func(), allowPrivateForgeAddrs bool, opts ...libp2p.Option) (host.Host, error) {


This constructor definitely is not great. Wanted to get something out before spending more time here, suggestions welcome.

Seems like this could go a whole lot easier if we didn't have a bunch of couplings going on:

Need TLS Config to instantiate the host (unless we cast to Swarm and add a transport later?), but need the host (and its addresses) to setup the certmagic TLS config

Trying to add a transport or an address would mess with the users' expectations of options passing, but we can't say "maybe add this address or transport if not already present"

We need to setup a listener (which requires knowing the DNS name), but the DNS name is determined by the address

We need to dynamically change the names as the IP addresses change

Could potentially change this to setting up a host, erroring if the websocket transport already exists, and then adding it + it's addresses + another address factory wrapper.

client/acme.go

MarcoPolo

Just a first pass at the server side. I'll take a look at the client side next.

MarcoPolo · 2024-08-30T17:23:03Z

acme/reader.go

+type acmeReader struct {
+	Next        plugin.Handler
+	ForgeDomain string
+	Datastore   datastore.Datastore


I think we should just define the interface we actually need in this package and not require a full Datastore implementation.

MarcoPolo · 2024-08-30T17:26:23Z

acme/reader.go

+		m.SetReply(r)
+		m.Authoritative = true
+		m.Answer = answers
+		w.WriteMsg(&m)


MarcoPolo · 2024-08-30T17:33:49Z

acme/writer.go

+	mux     *mux.Router
+}
+
+func (c *acmeWriter) OnStartup() error {


nit: I think this should be Start() error. I know it's passed into an OnStartup function to register as a callback, but the function itself is about starting the acmeWriter. There's also precedent in Go stdlib for Start(...) error.

No strong feelings, I copied this from one of the CoreDNS examples https://github.com/coredns/coredns/blob/ee4d26b7807856efbf61ebebb2687df21561a787/plugin/health/setup.go#L22. If you want to change it feel free

MarcoPolo · 2024-08-30T17:43:58Z

acme/writer.go

+	c.nlSetup = true
+
+	c.mux.HandleFunc("/v1/{peerID}/_acme-challenge", func(w http.ResponseWriter, r *http.Request) {
+		authHeader := r.Header.Get("Authorization")


This will be so much better when we can use libp2p/go-libp2p#2854.

This could be something like:

authPeer := auth.ServerPeerIDAuth{ // ... Next: func(id peer.ID, w http.ResponseWriter, r *http.Request) { err := c.updateDB(id, r.Body()) // err handling w.WriteHeader(http.StatusOK) }, } //... c.mux.HandleFunc("/v1/_acme-challenge", authPeer)

Also is there a reason to have the peerID in the url?

For sure, that's why I flagged it earlier in #1 (comment).

Do you think we're close enough to landing things that I should just drop and rewrite to use the server peerID auth now? Alternatively can just wait a bit and we'll delete some code before we launch.

Also is there a reason to have the peerID in the url?
We can remove it. It was mostly there to mimic some existing APIs for DNS puts that take domain names as arguments.

It’s close. I just need a review from sukun and we’ll merge it. Very likely by next week.

MarcoPolo · 2024-08-30T17:46:48Z

acme/writer.go

+			return
+		}
+
+		// Value must be a base64url encoding of a SHA256 digest per https://datatracker.ietf.org/doc/html/rfc8555/#section-8.4


You should quote the relevant sentence verbatim "It MUST NOT contain any characters outside the base64url alphabet, including padding characters ("=")." As is, it's unclear if padding is allowed or not.

MarcoPolo · 2024-08-30T17:53:44Z

README.md

+To set an ACME challenge send an HTTP request to the server (for libp2p.direct this is registration.libp2p.direct)
+```shell
+curl -X POST "https://registration.libp2p.direct/v1/<peerID>/_acme-challenge" \
+-H "Authorization: Bearer <signature>.<public_key>"


Is this meant to follow RFC 6750? The "Bearer" Auth scheme is registered to that RFC https://www.iana.org/assignments/http-authschemes/http-authschemes.xhtml.

I haven't looked closer here, because I'm assuming we'll opt to use the HTTP Peer ID authentication flow instead.

client/challenge.go

acme/writer.go

MarcoPolo · 2024-08-30T17:58:50Z

acme/writer.go

+	// TODO: Do we need to listen on the identify event instead?
+	// TODO: Where do we want to record this information if anywhere?


yes, and probably at least a log for now?

ipparser/plugin.go

client/acme.go

glue records are fixed, bumping serial to see if it helps with resolution at 8.8.8.8

glue records are ip4-only, this is just making sure we remove any inconsistencies that could make google dns refuse resolution

…id ip.peerID.forge

…ID.forge

…user functions are executing

this allows us to avoid hardcoding them in Kubo

registration one is commented for now, as we should measure failures as well as success. docs in docs/METRICS.md

This enables `coredns_forge_acme_registrations_total{status}` which tracks number of request per response code, including ones that failed libp2pPeerIDAuth Also, switched to go-libp2p from main branch with http auth.

allows consumers like Kubo to adjust logger NOTE: certmagic.DefaultACME.Logger and certmagic.Default.Logger are used before we are able to set custom logger, so consumer likely will want to set them to the same one. We don't override upstream defaults in case application uses multiple instances, and some of them are not related to p2p-forge.

aims to solve race condition when NewDefault() from certmagic is used https://github.com/ipfs/kubo/pull/10521/files/e6e0b7a1dc350430c64438417bc5ed4a38171895#diff-52740993e6a197feddd69dc9db132b0b7babd2cf3c0b337bde3476b779c07a77

README.md

achingbrain · 2024-10-29T17:37:20Z

acme/writer.go

+				_, _ = w.Write([]byte(fmt.Sprintf("error storing value: %s", err)))
+				return
+			}
+			w.WriteHeader(http.StatusOK)


This should be w.WriteHeader(http.StatusNoContent) to cause a 204 since we send no content.

Co-authored-by: Alex Potsides <[email protected]>

lidel

Let's merge this, as the client shipped with Kubo 0.32.0-rc1, and we want to start producing docker images from main / staging.

Please fill separate issues / PRs for unaddressed issues.

assiming outdated comments are addressed, if not, please fill them as separate issues

aschmahmann force-pushed the feat/initial-implementation branch 3 times, most recently from 5928892 to 966722d Compare August 16, 2024 22:49

aschmahmann commented Aug 16, 2024

View reviewed changes

SgtPooki reviewed Aug 19, 2024

View reviewed changes

aschmahmann added 2 commits August 20, 2024 16:02

feat: initial implementation

83323ca

ci: add workflows

0ddd505

aschmahmann force-pushed the feat/initial-implementation branch from 0cdda1a to 0ddd505 Compare August 20, 2024 20:03

MarcoPolo reviewed Aug 20, 2024

View reviewed changes

README.md Outdated Show resolved Hide resolved

aschmahmann commented Aug 22, 2024

View reviewed changes

aschmahmann force-pushed the feat/initial-implementation branch 3 times, most recently from 8f4af5e to 5653d6c Compare August 29, 2024 18:34

aschmahmann added 2 commits August 29, 2024 15:35

feat: add acme client hook for libp2p

6566eb5

test: add more complete e2e testing

d1a0b6e

aschmahmann force-pushed the feat/initial-implementation branch from 5653d6c to d1a0b6e Compare August 29, 2024 19:35

aschmahmann commented Aug 29, 2024

View reviewed changes

aschmahmann added 2 commits August 29, 2024 15:56

fix: add HTTP 2 and 1.1 ALPNs

c7e5892

chore: update go-libp2p

bc873f6

MarcoPolo previously requested changes Aug 30, 2024

View reviewed changes

aschmahmann added 4 commits September 3, 2024 12:13

chore: switch from net.ParseIP to netip.ParseAddr

4b96c4a

fix: explicitly close test hosts

5f36c1a

fix: rename constructor to NewP2PForgeCertMgr

77c9330

feat: add options

999ba3c

MarcoPolo reviewed Sep 3, 2024

View reviewed changes

client/acme.go Outdated Show resolved Hide resolved

aschmahmann force-pushed the feat/initial-implementation branch from 6f12d69 to e4350da Compare September 4, 2024 19:35

aschmahmann added 2 commits September 4, 2024 15:57

feat: switch to http peerID auth

fab0c5b

chore: tidy up dependency imports

308b83a

aschmahmann force-pushed the feat/initial-implementation branch from e4350da to 308b83a Compare September 4, 2024 19:57

docs: add more clarification that base64url does not include padding

00b6c7c

tmp(acme-writer): force use of authentication env var

eef1770

aschmahmann force-pushed the feat/initial-implementation branch from 1f5b301 to eef1770 Compare September 16, 2024 20:10

aschmahmann and others added 18 commits September 16, 2024 16:29

fix(acme-reader): handle subsequent plugins correctly

7fd1cfb

chore: switch soa to ns1.libp2p.direct

a0ee13f

glue records are fixed, bumping serial to see if it helps with resolution at 8.8.8.8

chore: separate ip4 from ip6

709fad7

glue records are ip4-only, this is just making sure we remove any inconsistencies that could make google dns refuse resolution

respond with no answer but a successful query on peerID.forge and val…

f9d8f07

…id ip.peerID.forge

fix: normalize DNS names to lowercase

d060dac

respond with no answer but a successful query on _acme-challenge.peer…

4c9cc8a

…ID.forge

fix: escape arbitrary bytes before writing to log

587c7fa

fix(client): handle when our certificate is loaded even if no custom …

dbeb701

…user functions are executing

refactor: expose client defaults

2ecd19a

this allows us to avoid hardcoding them in Kubo

feat(client): WithUserAgent + WithForgeAuth

11dda24

feat: prometheus metrics + docs

4db6b55

registration one is commented for now, as we should measure failures as well as success. docs in docs/METRICS.md

feat(metrics): forge_acme_registrations_total

65145f8

This enables `coredns_forge_acme_registrations_total{status}` which tracks number of request per response code, including ones that failed libp2pPeerIDAuth Also, switched to go-libp2p from main branch with http auth.

chore: go-libp2p v0.37.0 and go 1.23

97f25ad

fix(client): use dedicated cert cache

f599f48

aims to solve race condition when NewDefault() from certmagic is used https://github.com/ipfs/kubo/pull/10521/files/e6e0b7a1dc350430c64438417bc5ed4a38171895#diff-52740993e6a197feddd69dc9db132b0b7babd2cf3c0b337bde3476b779c07a77

chore: remove requirement for FORGE_ACCESS_TOKEN

2595bf8

chore: typo

1360e56

fix(test): go-libp2p v0.37

78c3ae0

achingbrain reviewed Oct 29, 2024

View reviewed changes

README.md Outdated Show resolved Hide resolved

achingbrain reviewed Oct 29, 2024

View reviewed changes

README.md Outdated Show resolved Hide resolved

chore(lint): redundant return statement (S1023)

688ddf7

achingbrain reviewed Oct 29, 2024

View reviewed changes

README.md Outdated Show resolved Hide resolved

achingbrain reviewed Oct 29, 2024

View reviewed changes

docs(readme): apply suggestions from code review

4468ad5

Co-authored-by: Alex Potsides <[email protected]>

lidel approved these changes Oct 30, 2024

View reviewed changes

lidel merged commit c5971b0 into main Oct 30, 2024
4 checks passed

lidel deleted the feat/initial-implementation branch November 22, 2024 23:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: initial implementation #1

feat: initial implementation #1

aschmahmann commented Aug 16, 2024

aschmahmann Aug 16, 2024

MarcoPolo Aug 30, 2024

aschmahmann Aug 16, 2024

SgtPooki Aug 19, 2024

aschmahmann Aug 20, 2024

MarcoPolo Aug 20, 2024

MarcoPolo Aug 20, 2024

aschmahmann Aug 16, 2024

aschmahmann Aug 16, 2024

aschmahmann Aug 20, 2024

aschmahmann Aug 16, 2024

aschmahmann Aug 16, 2024

MarcoPolo Aug 30, 2024

aschmahmann Aug 16, 2024

aschmahmann Aug 16, 2024

SgtPooki left a comment

SgtPooki Aug 19, 2024

aschmahmann Aug 22, 2024

aschmahmann Aug 29, 2024

MarcoPolo left a comment

MarcoPolo Aug 30, 2024

MarcoPolo Aug 30, 2024

MarcoPolo Aug 30, 2024

aschmahmann Sep 4, 2024

MarcoPolo Aug 30, 2024

aschmahmann Aug 30, 2024

MarcoPolo Aug 31, 2024

MarcoPolo Aug 30, 2024

MarcoPolo Aug 30, 2024

MarcoPolo Aug 30, 2024

achingbrain Oct 29, 2024

lidel left a comment

		curl -X POST "https://registration.libp2p.direct/v1/<peerID>/_acme-challenge" \
		-H "Authorization: Bearer <signature>.<public_key>"

		// TODO: Do we need to listen on the identify event instead?
		// TODO: Where do we want to record this information if anywhere?


		var libp2pDirectWssComponent = multiaddr.StringCast("/tls/sni/*.libp2p.direct/ws")

		func NewHostWithP2PForge(forgeDomain string, forgeRegistrationEndpoint string, opts ...libp2p.Option) (host.Host, error) {

feat: initial implementation #1

feat: initial implementation #1

Conversation

aschmahmann commented Aug 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SgtPooki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarcoPolo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lidel left a comment

Choose a reason for hiding this comment