trafficserver icon indicating copy to clipboard operation
trafficserver copied to clipboard

Fix an error on SSL config reload (plus some cleanup).

Open ywkaras opened this issue 2 years ago • 7 comments

This seems to have eliminated some ERROR diags we were seeing in Yahoo Prod when doing config reloads.

The SSLSecret public functions no longer return pointers into the unorded_map of secrets, they return a copy of the secret data. This seemed thread unsafe. A periodic poll running in the background can update the secret data for an entry for a secret name in the map.

To avoid exporting pointers, I had to change the prototype of TSSslSecretGet(). Hopefully there are no existing plugins that are already using this TS API function, so breaking this rule will be moot. I added a new TS API handle, TSHeapBuf, in order to be able to cleanly return a copy of the secret data.

ywkaras avatar Apr 12 '22 15:04 ywkaras

I'll undraft this after letting it soak in Yahoo Prod for a while.

ywkaras avatar Apr 12 '22 16:04 ywkaras

Sorry, I accidentally made the PR branch in the Apache repo and not my fork. I'll delete the branch once this PR is closed.

ywkaras avatar May 25 '22 20:05 ywkaras

I disagree this is incompatible, since, as Masakazu found, it's only incompatible with commits which have not been released yet.

ywkaras avatar Jun 07 '22 20:06 ywkaras

This has been running in Yahoo Prod for more than 2 weeks, with no issues/problems. These changes fixed errors that were happening when doing a config reload.

ywkaras avatar Jul 27 '22 19:07 ywkaras

"I would not call this general cleanup."

What title do you prefer then?

ywkaras avatar Aug 30 '22 19:08 ywkaras

What title do you prefer then?

I don't know what happened exactly, so I can only say "Fix an error on SSL config reload".

maskit avatar Aug 30 '22 19:08 maskit

As mentioned already, at a minimum this should be split into two PRs, and a discussion should be opened on the mailing list about adding the new set of APIs.

zwoop avatar Sep 01 '22 23:09 zwoop

We're seeing a deadlock with our plugin that uses SSL_SECRET_HOOK, so making this a draft for now.

ywkaras avatar Sep 30 '22 21:09 ywkaras

Brian Neradt has a fix for the deadlock problem. We're going to let it ripen internally for a bit, then I'll pull it into this PR.

ywkaras avatar Oct 03 '22 18:10 ywkaras

[approve ci autest]

ywkaras avatar Nov 01 '22 16:11 ywkaras

I still think we should fix the API without introducing additional data structure to avoid the unfortunate release that has an unusable API.

Even if we are going to have TSAllocatedVarLenData, using it just for TSSslSecretGet causes inconsistency. TSUrlStringGet could use it too. We can continue the discussion about the data structure on the list and have it later. Let's focus on the issue on this PR.

maskit avatar Jan 03 '23 18:01 maskit

We have this in Yahoo internal TS. If anyone else sees errors like:

[Jan 20 00:01:47.669] [ET_TASK 1] ERROR: SSL::139889882617600:error:0909006C:PEM routines:get_name:no start line:crypto/pem/pem_lib.c:745:Expecting: TRUSTED CERTIFICATE
[Jan 20 00:01:47.669] [ET_TASK 1] ERROR: SSL::139889882617600:error:0909006C:PEM routines:get_name:no start line:crypto/pem/pem_lib.c:745:Expecting: ANY PRIVATE KEY
[Jan 20 00:01:47.669] [ET_TASK 1] ERROR: failed to load server private key from /home/y/conf/trafficserver/ssl/ycpi.tls.yahooinc.com.ec.key

we should reopen this PR.

ywkaras avatar Jan 20 '23 20:01 ywkaras

Weren't you going to update this PR with the simplest interface? I think this PR was closed because the branch was on the upstream repo.

maskit avatar Jan 20 '23 20:01 maskit

I was planning to, but it was delayed by production issues. Leif didn't comment on why he was closing it.

ywkaras avatar Jan 20 '23 21:01 ywkaras