Opened 5 months ago

Closed 4 months ago

#30512 closed enhancement (fixed)

Enable cache for ACME certificates in broker

Reported by: dcf Owned by:
Priority: Medium Milestone:
Component: Circumvention/Snowflake Version:
Severity: Normal Keywords: arlolra cohosh dcf phw
Cc: Actual Points: 1
Parent ID: Points:
Reviewer: Sponsor:

Description

The websocket server caches its automatic certificates:
https://gitweb.torproject.org/pluggable-transports/snowflake.git/tree/server/server.go?id=d865b7c252d3a7efd789a84757fc2635b1964921#n309
But the broker does not:
https://gitweb.torproject.org/pluggable-transports/snowflake.git/tree/broker/broker.go?id=d865b7c252d3a7efd789a84757fc2635b1964921#n265

In #30509 the broker exceeded the Let's Encrypt rate limits and couldn't get a new certificate. Implementing a certificate cache will prevent it from happening again.

Once implemented, remember to undo the temporary --cert and --key configuration that was set up in comment:6:ticket:30509. That certificate is good for 1 year.

Child Tickets

Change History (11)

comment:1 Changed 5 months ago by phw

Here's a simple patch: https://github.com/NullHypothesis/snowflake/tree/bug30509

It uses letsencrypt-cert-dir in the current working directory as cache.

comment:2 in reply to:  1 ; Changed 5 months ago by dcf

Status: newneeds_information

Replying to phw:

Here's a simple patch: https://github.com/NullHypothesis/snowflake/tree/bug30509

It uses letsencrypt-cert-dir in the current working directory as cache.

What happens if the directory cannot be created?

I suppose we will want at least a log message. Maybe not a fatal error.

comment:3 in reply to:  2 ; Changed 5 months ago by phw

Replying to dcf:

Replying to phw:

Here's a simple patch: https://github.com/NullHypothesis/snowflake/tree/bug30509

It uses letsencrypt-cert-dir in the current working directory as cache.

What happens if the directory cannot be created?

I suppose we will want at least a log message. Maybe not a fatal error.

Yes, good point. I amended my patch: https://github.com/NullHypothesis/snowflake/commit/0744b2930e88daf02b039e636e989d60c2467913

comment:4 Changed 5 months ago by phw

Status: needs_informationneeds_review

comment:5 in reply to:  3 ; Changed 5 months ago by dcf

Replying to phw:

Yes, good point. I amended my patch: https://github.com/NullHypothesis/snowflake/commit/0744b2930e88daf02b039e636e989d60c2467913

I'm fine with the general approach. I was wondering if autocert.DirCache would log itself in that situation or something, but it looks like it doesn't provide any function like that.

I think createCertCacheDir doesn't need to distinguish between the directory already existing and being newly created. It only needs to report an error if any.

I would prefer if the logging happened at the top level. Have createCertCacheDir only return an error and not log, and log the error message in main.

comment:6 in reply to:  5 Changed 5 months ago by phw

Replying to dcf:

Replying to phw:

Yes, good point. I amended my patch: https://github.com/NullHypothesis/snowflake/commit/0744b2930e88daf02b039e636e989d60c2467913

I'm fine with the general approach. I was wondering if autocert.DirCache would log itself in that situation or something, but it looks like it doesn't provide any function like that.

I think createCertCacheDir doesn't need to distinguish between the directory already existing and being newly created. It only needs to report an error if any.

I would prefer if the logging happened at the top level. Have createCertCacheDir only return an error and not log, and log the error message in main.

I incorporated your suggestions and amended the patch:
https://github.com/NullHypothesis/snowflake/commit/d647285ed4c5dc8567e2a258bfd094c4b572d081

comment:7 Changed 5 months ago by dcf

Status: needs_reviewneeds_revision

This looks good to me now. I would suggest one further change: change letsencrypt-cert-cache to acme-cert-cache for uniformity with other existing options.

And do we care or should there be a way to disable the cert cache, if running on a read-only filesystem for example? Maybe -acme-cert-cache ""? Or maybe just logging the failure and continuing to run (what the patch does now) is the best way.

comment:8 in reply to:  7 Changed 5 months ago by phw

Replying to dcf:

This looks good to me now. I would suggest one further change: change letsencrypt-cert-cache to acme-cert-cache for uniformity with other existing options.

Good point, here you go: https://github.com/NullHypothesis/snowflake/commit/8cd16ab9cc8db3e646fd09a28c3fbed9791c3b15

And do we care or should there be a way to disable the cert cache, if running on a read-only filesystem for example? Maybe -acme-cert-cache ""? Or maybe just logging the failure and continuing to run (what the patch does now) is the best way.

I think not having a certificate cache is worth a warning in any case, so I'm fine with the current behaviour.

comment:9 Changed 5 months ago by phw

Status: needs_revisionneeds_review

comment:10 Changed 5 months ago by dcf

Status: needs_reviewmerge_ready

I'm fine with it. Shall we close this ticket when the new code is deployed and we are using a Let's Encrypt cert again?

comment:11 Changed 4 months ago by phw

Actual Points: 1
Resolution: fixed
Status: merge_readyclosed

I merged the patch into master in 11efa42, deployed the new binary on the broker, and updated the runit script so it uses Let's Encrypt again.

Note: See TracTickets for help on using tickets.