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

Fix certificate reloading issues #717

Merged
merged 4 commits into from Aug 22, 2018

Conversation

Projects
None yet
4 participants
@alexmiller-apple
Copy link
Contributor

alexmiller-apple commented Aug 21, 2018

Of which two were raised: certificates were required to be read-write, and there were FDBLibTLSOutOfMemory errors when certificates were actually reloaded.

The former is an easy fix of adding OPEN_UNCACHED.

The latter is an easy fix of actually initializing the cached copy of certificate data, which I had assumed were initialized anyway. Oops.

alexmiller-apple added some commits Aug 21, 2018

Open certificates as uncached, which means they can be write-protected.
OPEN_READONLY still opens the file as read-write.  To actually be
read-only, one needs to open the file as READONLY and UNCACHED.
Initialize the cached copy of ca/cert/key data.
This was just purely an accidental oversight from before.  The variables
were there and handled like they were actually initilized with the
contents of the various certificate files at start-up, but never
actually were.

And add a few trace events to make it easy to see when the system
noticed and tried to reload certificate data.

@alexmiller-apple alexmiller-apple requested a review from satherton Aug 21, 2018

alexmiller-apple added some commits Aug 21, 2018

Loading a mismatching cert shouldn't prevent TLS connections.
set_{cert,key,ca}_data returns pass/fail and not throw.  The existing
code wrongly assumed that they threw.
Improve TLS cert refresh logging.
Explicitly call out failure/success, and surface repeated cert
mismatches.
@alexmiller-apple

This comment has been minimized.

Copy link
Contributor

alexmiller-apple commented Aug 22, 2018

In its current state, there's not much that I can do about the fact that any time we load a mismatched key,cert pair during a roll, a SevError event will be logged. A lot of the FDBLibTLS code needs to change from C-style code of goto err; err: return false; to c++-with-exceptions style code, that allows us to make decisions about what is a failure and know why.

@ajbeamon

This comment has been minimized.

Copy link
Contributor

ajbeamon commented Aug 22, 2018

This should be made against release-6.0

@alexmiller-apple

This comment has been minimized.

Copy link
Contributor

alexmiller-apple commented Aug 22, 2018

I'm inclined at this point to just get the approval from Steve to merge, and then cherrypick the merge back to 6.0

@satherton satherton merged commit 365fe99 into apple:master Aug 22, 2018

alexmiller-apple added a commit to alexmiller-apple/foundationdb that referenced this pull request Aug 23, 2018

Cherry-pick pull request apple#717 to release-6.0
Which contains:
* Improve TLS cert refresh logging.
* Loading a mismatching cert shouldn't prevent TLS connections.
* Initialize the cached copy of ca/cert/key data.
* Open certificates as uncached, which means they can be write-protected.

richardalow added a commit that referenced this pull request Aug 24, 2018

Merge pull request #729 from alexmiller-apple/tls-refresh-6.0
Cherry-pick pull request #717 to release-6.0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment