txacme icon indicating copy to clipboard operation
txacme copied to clipboard

Add .onstore scripts

Open habnabit opened this issue 8 years ago • 13 comments

This lets you run some arbitrary code after something's been stored, so you can rehash nginx or whatever.

This was the major blocker for me adopting txacme: some services need to be told to reload keys/certs from disk.


This change is Reviewable

habnabit avatar Apr 26 '16 03:04 habnabit

I should say this was mostly a proof of concept; I don't expect this to get merged as-is because it has no docs. But, how's it look?

habnabit avatar Apr 26 '16 03:04 habnabit

Oops yeah I just remembered this is also the bit with the super crappy attempt at getting hypothesis and testtools to work together. That should definitely be split off into a PR for hypothesis.

habnabit avatar Apr 26 '16 03:04 habnabit

This seems like a generally useful feature. Adding it to the endpoint as a kwarg is a bit tricky, since any kwargs we use for ourselves will mask kwargs in the endpoint we're wrapping... maybe @glyph has some thought about the Right Thing To Do here since I haven't thought about it too much myself yet.

We should add it to the docs, of course, and I see the tests are failing on Python 3 because of str/bytes issues (to nobody's surprise), but those should be easy things to fix.

mithrandi avatar Apr 26 '16 12:04 mithrandi

A thought: Make the "on stored callback" (maybe this could be implemented with the same mechanism that Request.notifyFinish uses, for multiple "observers" if necessary) a plain Python function, this would be a more generic interface with all the benefits that come with being able to invoke functions in the same process.

For the "run arbitrary scripts" idea you could always provide a Python function (it may even be in txacme itself) that implements this behaviour on top of the "on stored callback" function.

jonathanj avatar Apr 26 '16 13:04 jonathanj

We already have a directory with certain enforced naming conventions. Perhaps just put a script in there, on-store, which is executed as a hook, no need for an additional command line argument?

glyph avatar Apr 26 '16 17:04 glyph

I was hoping to avoid checking argv[1] in the script in general, but a single on-store script does probably make the most sense, given the naming convention point.

habnabit avatar Apr 26 '16 17:04 habnabit

No, I mean, put it in the certificates directory.

glyph avatar Apr 26 '16 17:04 glyph

Right. Have a single {cert-dir}/on-store script instead of {cert-dir}/{hostname}.onstore.

habnabit avatar Apr 26 '16 17:04 habnabit

Okay, so the hook script behaviour will be hardcoded for the endpoint parser, and "always on". I think @jonathanj is right that the underlying interface should just be DirectoryStore taking a callable (let's say a 1-arg callable that takes the servername?), so If you need different hooks for some hosts, you can either just use different directories, or you can write some Python code.

mithrandi avatar Apr 26 '16 18:04 mithrandi

Sounds like a good strategy to me.

glyph avatar Apr 26 '16 21:04 glyph

Current coverage is 99.75%

Merging #27 into master will increase coverage by -0.24%

  1. 5 files (not in diff) in src/txacme/test were modified. more
    • Misses +1
    • Partials +1
    • Hits -2
  2. 2 files (not in diff) in src/txacme were modified. more
@@            master        #27   diff @@
=========================================
  Files           20         20          
  Lines         1535       1579    +44   
  Methods          0          0          
  Messages         0          0          
  Branches       139        128    -11   
=========================================
+ Hits          1535       1575    +40   
- Misses           0          2     +2   
- Partials         0          2     +2   

Powered by Codecov. Last updated by 8c1a3cd...e73bd8e

codecov-io avatar Apr 27 '16 11:04 codecov-io

I'm +1 on @jonathanj's suggestion that there should be a callback for certificate changes. For now I've implemented a wrapper for ICertificateStore that triggers a reload of certificates in an external system when .store() is called. Which I think should work fine, but it's not explicit anywhere that .store() is only called when a certificate is issued/renewed, although this is probably a reasonable assumption to make if I expect you to have written this software well (this is maybe #35?).

Another problem with having .store() as the point where things are notified of certificate changes is that it means it's tricky to add new domains/certificates to the store, as this is usually done by storing an empty certificate in the store and the empty certificate is likely to break whatever external thing using the certificates (#76).

JayH5 avatar Oct 21 '16 09:10 JayH5

+1 for callback on certificate updates for the default CertificateStore implementation.

I have implemented by own ICertificateStore which will call/forward the server_name, pem_objects each time something is stored in it.

For the file based CertificateStore I am expecting the callback to also include the full path to the updated file.

adiroiban avatar Oct 30 '18 17:10 adiroiban