-
Notifications
You must be signed in to change notification settings - Fork 294
Add AutoTLS client spec #682
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
base: master
Are you sure you want to change the base?
Conversation
Thanks for writing this up! pinging @aschmahmann and @lidel for a heads up. I'll also take a look at this soon™ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just some basic errors found while skimming
There has not been much feedback in a while. Imo, this doc ready to be merged as the first version of the AutoTLS spec. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gmelodie @kaiserd Do we have a third-party implementation of this spec other than the two created by Shipyard team?
- JS: https://www.npmjs.com/package/@libp2p/auto-tls
- GO: https://github.com/ipshipyard/p2p-forge/tree/main/client
Personally I would prefer to wait until someone dogfoods this spec (Shipyard wrote things without this PR):
- (bare minimum) someone actually tries to implement AutoTLS client independently, by only following this document, providing feedback if spec is complete
- (ideally, but since this is client-only spec, not mandatory) there is at least one extra server implementation with clear licensing situation (https://github.com/ipshipyard/p2p-forge has no license, only client library has https://github.com/ipshipyard/p2p-forge/blob/main/client/LICENSE.md)
As for the spec proposed in this document, some missing things:
- guidance on sane timeouts and exponential backoff
- IPv6
(details inline)
ps. AutoTLS is not using DANE (spec does not claim that, but the PR description mentions it).
tls/autotls-client.md
Outdated
|
||
**Note:** `varint` is a protobuf [varint](https://protobuf.dev/programming-guides/encoding/#varints) field that encodes the length of each of the `key=value` string. | ||
|
||
**Note:** The node SHOULD include only multiaddresses containing public IPv4 addresses in `multiaddrs`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
iirc there is more this (IPv6, filtering):
- The node MUST include only publicly reachable addresses
- For IPv4: exclude private ranges
- For IPv6: exclude private ranges and NAT64 translated addresses
- Exclude relay addresses (containing
/p2p-circuit
)
tls/autotls-client.md
Outdated
|
||
**Note:** The node SHOULD NOT send more than `max_dns_retries` DNS requests. | ||
After `max_dns_timeout`, the communication is considered failed. | ||
What to do after `max_dns_timeout` has passed is left as an implementation decision. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: imo the spec should clearly say that at this point the certificate request flow SHOULD be aborted and retried later.
(Do not attempt to ask ACME for cert if DNS TXT record is not confirmed to exist – we don't want to hammer Let's Encrypt with obviously failing requests)
tls/autotls-client.md
Outdated
|
||
**Note:** The node SHOULD NOT send more than `max_acme_poll_retries` poll requests to the ACME server. | ||
After `max_acme_timeout`, the communication has failed. | ||
What to do after `max_acme_timeout` has passed is left as an implementation decision. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar ask: spec should clearly note that there should be exponential backoff for retry attempts.
tls/autotls-client.md
Outdated
|
||
| Parameter | Description | Reasonable Default | | ||
|--------------------------|------------------------------------------------------------------|--------------| | ||
| `max_dns_retries` | The maximum number of DNS queries that the node SHOULD make before giving up | ??? | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
p2p-forge/client
uses time-based timeout (3 minutes) with exponential backoff rather than fixed retry counts
tls/autotls-client.md
Outdated
| Parameter | Description | Reasonable Default | | ||
|--------------------------|------------------------------------------------------------------|--------------| | ||
| `max_dns_retries` | The maximum number of DNS queries that the node SHOULD make before giving up | ??? | | ||
| `max_dns_timeout` | The maximum number of seconds a node SHOULD wait for DNS records to be set | ??? | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
iirc p2p-forge/client
gives up waiting for TXT record after 3min I think -- at this point something went wrong and the entire registration needs to be resumed
tls/autotls-client.md
Outdated
|--------------------------|------------------------------------------------------------------|--------------| | ||
| `max_dns_retries` | The maximum number of DNS queries that the node SHOULD make before giving up | ??? | | ||
| `max_dns_timeout` | The maximum number of seconds a node SHOULD wait for DNS records to be set | ??? | | ||
| `max_acme_poll_retries` | The maximum number of GET requests that the node SHOULD issue to ACME server before giving up | ??? | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACME specs recommend polling until timeout rather than fixed retry counts
tls/autotls-client.md
Outdated
| `max_dns_retries` | The maximum number of DNS queries that the node SHOULD make before giving up | ??? | | ||
| `max_dns_timeout` | The maximum number of seconds a node SHOULD wait for DNS records to be set | ??? | | ||
| `max_acme_poll_retries` | The maximum number of GET requests that the node SHOULD issue to ACME server before giving up | ??? | | ||
| `max_acme_timeout` | The maximum number of seconds a node SHOULD wait for an ACME resource status to change | ??? | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's Encrypt recommendation for challenge completion is ~10minutes, p2p-forge client aborts sooner, around 3 minutes I think
Co-authored-by: Marcin Rataj <[email protected]>
Co-authored-by: Marcin Rataj <[email protected]>
Co-authored-by: Marcin Rataj <[email protected]>
Yes! There's the
We actually wrote the document based on the implementation that we did.
Thanks for clarifying! @lidel can you check again please? |
This is an attempt to formalize the spec for AutoTLS client as announced in https://blog.libp2p.io/autotls/
Note: @kaiserd noted that AutoTLS might be using DANE, but I need some input since I couldn't find any work explicitly linking the two.