Quick comment which unfortunately is going to make this more complicated: We are going to need to fix config_test to work with some type of mock loader that doesn't read files from disk. Optimally it would verify everything else in SSL and possibly just use fake valid cert data. The reason this is going to be needed is we run at Lyft config tests for configs where the certs are not available on the box where the tests are run.

Ultimately I think we know this is working when none of the example config stuff here changed as part of this diff.

I have mixed feelings about validity of this approach, but I'll add this if you need it.

I'm sorry, but we definitely need it. I would just figure out whatever is easiest in that test, even if you have to mock out more stuff.

Sorry, I think I might have misunderstood what you were saying.

I assumed that you meant letting //test/config_test:example_configs_test pass without reading certificates from disk, but after some digging, I've noticed that //test/tools/config_load_check:config_load_check_tool uses the same code, so you probably meant that you're using this tool to verify production configs and want to test them without access to the real certificates? Is that correct?

We actually use both a private version of the config test as well as the config load check tool (in different places). We need both to run in a mode that doesn't require the actual cert files to exist (because yes we test prod configs where the certs don't live). IMO it's fine to just have them both work the same way for now (don't load) which is not a regression from what happened before, so any kind of mock is fine. The optimal solution would be to not use a full mock TLS context loader so we can check other config options, but to only mock the actual cert loading part. I have no idea if that is easy or not.

We can open an issue to do what you describe, but if we want to merge this sooner we need to not break existing behavior. IMO having a flag on whether to check the cert or not makes a lot of sense.

I agree with @ggreenway, it's one thing to mock things in tests, but faking certificates when testing production config sounds like a really bad idea, and if you're testing those on a machine without access to real certificates, then you should provide self-signed certs. Otherwise, you might refer to certificates that don't exist (e.g. typo).

Furthermore, we'd need to add same hacks to --mode validate, which sounds even worse.

Yes, it's change of current behavior, but I don't see how this justifies adding error-prone hacks to the code.

I hear what you are saying, but, that's the way it works today, and this PR will break us. If we want to hold this PR until we fix the way we test things, we can do that, but it will take 1-2 weeks since we will need to schedule the work.

I'd prefer to wait, then.

Alright I will make an internal ticket and get someone to work on cutting us over to fake certs for testing next week.

@PiotrSikora as long as you are in here, can you take a look at the rest of the asserts in this file? I think some of them could potentially happen/crash with native v2 config. I might be wrong, but could you take a look and potentially fix to throw/add tests? (Follow up is fine if that is better).

Sure, I'll do in a separate PR.

Per above we need this to still work with cert files that can't be read. Sorry. :(

Is there a reasonable way to make the initializors here a bit cleaner, e.g. with anonymous namespace functions etc.

FWIW I prefer to have it all inline; having to track down a bunch of functions to understand how everything is initialized makes it harder to read, IMO.

While I agree that this isn't the prettiest code, I'm with Greg on this, it's much more readable here.

Do we have full test coverage for all these branches and exception cases?

Unlikely, I'll add some.

Not familiar enough with BoringSSL to know if this is the best we can do. Is there no way to do a non-filesystem version of things like SSL_CTX_load_verify_locations other than reimplementing bits of BoringSSL? Seems like the kind of place tmpfs could be useful but this isn't portable or even easy to manage at user privilege levels. @davidben for thoughts as well.

It's unfortunate that the openssl/boringssl has nice APIs for files, but not an analogous API for a BIO. But since that seems to be the way it is for now, I think this is the best we can do. I think a tmpfs is a huge can of worms to open up, with security and operational issues.

Unfortunately, there are no non-filesystem equivalents of the APIs we use, and even if I could add them today, we're on the chromium-stable branch, which is ~2 months behind the master branch, so it wouldn't help anyway.

I have a mid-term plan to migrate away from X509 APIs to BoringSSL-specific CRYPTO_BUFFERs and associated APIs, but that's further down the line, and it requires custom certificate verification logic.

Would it make more sense to return std::vector<uint8_t> or something? Although protobuf uses std::string for non-string buffers, so we might be stuck with this.

Maybe? I'm fine with either, but as you noted, protobuf uses std::string for the bytes inline field, so we might as well stick to it.

Agreed; let's stick with what you had.

The only place this value is used is in ContextImpl::getCaCertInformation() (just to print the path of the cert, which is output by an admin handler). Can we just get rid of it entirely? It made sense when all config pointed to a path on disk; it makes less sense now and adds complication.

It's also used in the error messages.

It's unfortunate that the openssl/boringssl has nice APIs for files, but not an analogous API for a BIO. But since that seems to be the way it is for now, I think this is the best we can do. I think a tmpfs is a huge can of worms to open up, with security and operational issues.

For each of the *Path() functions, please document that it will return an empty string in the case of inline material.

Done.

Please add a helper for getting the path in the case it is kFilename, since this code is repeated several times.

Done.

From an interface point of view, I'd rather have all these functions return empty string if they're inline, and then change that to this human-readable string right before it is presented to a user. But I don't feel strongly about it.

From an interface point of view, I think it's better to have this logic self-contained here, instead of intermixed with the code. I don't feel too strongly about it, but I'm going to leave it as-is.

That's fine; if someone ever needs it different, it's easy enough to change then.

"while (true)" is the convention used in this codebase

Done.

This returns "<inline>" not "" for inline data.

Sorry, I made my original comment before I'd gotten to the implementation of the interface.

Sigh, I should have known better! Thanks.

Out of curiosity, is there a reason this recursive flag was flipped to false?

Unit test certificates are in the certs sub-directory, and we don't want to load configuration from them.

See: PiotrSikora@5e68ded#diff-4133d0a7b4e910fd0a9861deab153595

Thanks! Missed that.