Skip to content

Comments

Update client certificates automatically#1730

Merged
pcapriotti merged 31 commits intodevelopfrom
pcapriotti/fed-cert-update
Sep 9, 2021
Merged

Update client certificates automatically#1730
pcapriotti merged 31 commits intodevelopfrom
pcapriotti/fed-cert-update

Conversation

@pcapriotti
Copy link
Contributor

@pcapriotti pcapriotti commented Aug 31, 2021

This makes federator update its cached TLS settings when changes to client certificates or the CA store are detected.

Most of the functionality is contained in Federator.Monitor.Internal. There is some complicated logic to make sure we get inotify events when the paths containing the credentials change in any possible way, including being modified directly, or overwritten with other files, or their parent directories being replaced with other ones.

Details

A "monitor" is started when an environment is created. At this point, all the files that need to be watched are collected, and inotify watches are created for each one of them and each of their parent directories up to the filesystem root. Whenever we detect a relevant change, we reload the TLSSettings from the filesystem, and update an IORef.

For file watches, we listen for the Close event, which is sent when a file descriptor in write mode for the corresponding inode is closed. For directory watches, things are more complicated: we listen for Create and MoveIn events, which happen when a new file appear in the directory, and as soon as it happens (before reloading the settings) we replace the existing watch on the file with a new one (since the inode might have changed).

There is a tiny PR to hs-certificate associated to this PR: haskell-tls/hs-certificate#125. This was needed to make sure we are able to catch and log exceptions in inotify handlers. Using error is problematic because the exception can be (and indeed is, in my tests) thrown outside of IO code we control, and results in the handler thread dying silently. I have pointed stack.yaml to the patched version, but it is not essential. Without the patch, we might just lose some logging errors in some cases.

Testing

In tests, we create a setup using temporary files or directories, then we set up a channel (using Control.Concurrent.Chan) to keep track of asynchronous reloads or exceptions thrown within a reload.

TODO

  • Graceful handling of missing inotify support
    This is probably not needed, since the inotify library cannot even be built for targets that do not support inotify.
  • Test updating a grandparent directory
  • Test modifying a file after a parent directory has been replaced
  • Integration tests
    This is not trivial, because it needs an outward service integration test. I'll skip it for now.
  • Unit test directory traversal

Checklist

  • The PR Title explains the impact of the change.
  • The PR description provides context as to why the change should occur and what the code contributes to that effect. This could also be a link to a JIRA ticket or a Github issue, if there is one.
  • Section Unreleased of CHANGELOG-draft.md contains the following bits of information:
    • A line with the title and number of the PR in one or more suitable sub-sections.

@pcapriotti pcapriotti force-pushed the pcapriotti/fed-cert-update branch 4 times, most recently from 13a95fc to c36d886 Compare September 2, 2021 13:22
@pcapriotti pcapriotti marked this pull request as ready for review September 2, 2021 14:02
@pcapriotti pcapriotti force-pushed the pcapriotti/fed-cert-update branch 4 times, most recently from c26a3ba to 3cf2ae7 Compare September 7, 2021 09:07
@pcapriotti
Copy link
Contributor Author

With the last few fixes, certificate autoreload works nicely on kubernetes.

Copy link
Member

@akshaymankar akshaymankar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we shouldn't expose IORef TLSSettings to the app.

@pcapriotti pcapriotti force-pushed the pcapriotti/fed-cert-update branch from 1f9737f to 51923d1 Compare September 8, 2021 13:52
Copy link
Member

@akshaymankar akshaymankar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

@pcapriotti pcapriotti force-pushed the pcapriotti/fed-cert-update branch from 51923d1 to 4c1bc91 Compare September 9, 2021 05:27
@pcapriotti pcapriotti merged commit 39894a6 into develop Sep 9, 2021
@pcapriotti pcapriotti deleted the pcapriotti/fed-cert-update branch September 9, 2021 06:55
@jschaul jschaul mentioned this pull request Sep 13, 2021
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.

3 participants