Skip to content
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

Support .well-known delegation when issuing certificates through ACME #4652

Merged
merged 9 commits into from Feb 19, 2019

Conversation

Projects
None yet
6 participants
@babolivier
Copy link
Member

babolivier commented Feb 15, 2019

Fixes #4552

@babolivier babolivier requested a review from matrix-org/synapse-core Feb 15, 2019

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Feb 15, 2019

Codecov Report

Merging #4652 into develop will decrease coverage by 0.07%.
The diff coverage is 16.66%.

@@             Coverage Diff             @@
##           develop    #4652      +/-   ##
===========================================
- Coverage    75.27%   75.19%   -0.08%     
===========================================
  Files          338      340       +2     
  Lines        34626    34721      +95     
  Branches      5670     5679       +9     
===========================================
+ Hits         26064    26109      +45     
- Misses        6966     7009      +43     
- Partials      1596     1603       +7
@hawkowl

This comment has been minimized.

Copy link
Contributor

hawkowl commented Feb 15, 2019

I feel like this should be manually configured, not done automagically?

@babolivier

This comment has been minimized.

Copy link
Member Author

babolivier commented Feb 15, 2019

I'd figure it should be done "automagically", especially since the original issue was mentioning "support .well-known delegation from 1708 and ACME", which I understand as "synapse checking whether there's a .well-known in order to know which domain it should issue a cert for".

Show resolved Hide resolved changelog.d/4652.feature Outdated
Show resolved Hide resolved synapse/handlers/acme.py Outdated

anoadragon453 and others added some commits Feb 18, 2019

Typo in info log
Co-Authored-By: babolivier <contact@brendanabolivier.com>
Typo in changelog
Co-Authored-By: babolivier <contact@brendanabolivier.com>
@richvdh

This comment has been minimized.

Copy link
Member

richvdh commented Feb 18, 2019

I'm with hawkowl. It should be explicit. The .well-known check, if we do one at all, should just be a sanity-check, but I think it's overkill.

@richvdh
Copy link
Member

richvdh left a comment

config opt pls

@erikjohnston

This comment has been minimized.

Copy link
Member

erikjohnston commented Feb 18, 2019

FWIW I think it should be a config option too, otherwise weird things will happen if we start it up while .well-known is down (e.g. due to restart server at the same time). In general its good if you can always bring up a service, even when dependencies might be temporary down, otherwise restarting the service becomes a lot scarier as you don't know if it'll actually come back.

@babolivier

This comment has been minimized.

Copy link
Member Author

babolivier commented Feb 18, 2019

Roger that, making it a config parameter. Following @erikjohnston's comment I'll remove the well-known check entirely.

@babolivier babolivier requested a review from matrix-org/synapse-core Feb 18, 2019

fixed

@richvdh
Copy link
Member

richvdh left a comment

lgtm otherwise

Show resolved Hide resolved synapse/handlers/acme.py Outdated
Show resolved Hide resolved synapse/config/tls.py Outdated

@babolivier babolivier merged commit a288bdf into develop Feb 19, 2019

7 checks passed

ci/circleci: sytestpy2merged Your tests passed on CircleCI!
Details
ci/circleci: sytestpy2postgresmerged Your tests passed on CircleCI!
Details
ci/circleci: sytestpy3merged Your tests passed on CircleCI!
Details
ci/circleci: sytestpy3postgresmerged Your tests passed on CircleCI!
Details
codecov/patch 16.66% of diff hit (target 0%)
Details
codecov/project 75.19% (target 0%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@babolivier babolivier deleted the babolivier/acme-delegated branch Apr 17, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.