Changes from upstream #2
Conversation
merge tanya/more-target-tests to master
.goreleaser.yml
Outdated
| homepage: https://packetframe.com | ||
| maintainer: Nate Sales <[email protected]> | ||
| description: Oblivious DNS over HTTPS server | ||
| license: GNU GPL-3.0 |
There was a problem hiding this comment.
given the use of MIT code from cloudflare, the easiest is to continue to mark this overall as an MIT package, if that's acceptable to you.
There was a problem hiding this comment.
Good catch, that's a mistake on my part. Fixed in 9606a68
There was a problem hiding this comment.
And the hompage there is wrong too. Remnants from an old package. Fixed in 4f4ccc5
main.go
Outdated
| log.Fatal(err) | ||
| } | ||
|
|
||
| keyPair, err := odoh.CreateKeyPairFromSeed(hpke.DHKEM_X25519, hpke.KDF_HKDF_SHA256, hpke.AEAD_AESGCM128, seed) |
There was a problem hiding this comment.
Is it okay to have the keypair only rotate on process restart? is it worth having the seed periodically roll over within the lifetime of the process?
There was a problem hiding this comment.
I'll have to look deeper into the IETF draft to see if there's an explicit policy about this. From an initial look I'm not seeing any reference to automated key rollover, so if we were to add this what interval would you recommend? Should it be a configurable parameter?
Also, one thing that we may add later is configuration for long lived TLS sessions that are set up when the process starts. We'd need to keep this in mind during key rollover.
There was a problem hiding this comment.
i imagine this would be needed for resuming sessions more efficiently, or something of that nature? having it rotate once a day seems like a reasonable default there.
Based on how it's passed in / used, I would expect that long-lived connections would continue to use whatever key existed when they were initiated, right?
There was a problem hiding this comment.
I'll have to test to make sure, but it seems logical that we would keep any long lived sessions with their original key unless it's required by the protocol to keep it current.
main.go
Outdated
| client: &http.Client{ | ||
| Transport: &http.Transport{ | ||
| MaxIdleConnsPerHost: 1024, | ||
| TLSHandshakeTimeout: 0 * time.Second, |
There was a problem hiding this comment.
shouldn't we force some sort of timeout here?
There was a problem hiding this comment.
As per #2 (comment), timeouts are now configurable in 5e1a4dc
Creating a PR over local changes as a place to do code review