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

Add .onstore scripts #27

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Add .onstore scripts #27

wants to merge 2 commits into from

Conversation

habnabit
Copy link
Member

@habnabit habnabit commented Apr 26, 2016

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

This lets you run some arbitrary code after something's been stored, so
you can rehash nginx or whatever.
@habnabit
Copy link
Member Author

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
Copy link
Member Author

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.

@mithrandi
Copy link
Contributor

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.

@jonathanj
Copy link
Contributor

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.

@glyph
Copy link
Member

glyph commented Apr 26, 2016

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?

@habnabit
Copy link
Member Author

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.

@glyph
Copy link
Member

glyph commented Apr 26, 2016

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

@habnabit
Copy link
Member Author

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

@mithrandi
Copy link
Contributor

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.

@glyph
Copy link
Member

glyph commented Apr 26, 2016

Sounds like a good strategy to me.

@codecov-io
Copy link

codecov-io commented Apr 27, 2016

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

@JayH5
Copy link
Contributor

JayH5 commented Oct 21, 2016

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).

@adiroiban
Copy link
Member

+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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants