Fix TLS context not being updated on signed certificate messages on agents #7654
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This makes the SSL context in ApiListener::SpawnCoroutine non-const to address an issue when an agent recieves an signed certificate from the master and tries to update the SSL context. This also uses the class member variable which is captured by `this` in the lamda expression. fixes #7650
Turns out that the switch to the class member variable inside the lambda expression is sufficient to fix the issue.
dnsmichi
changed the title
|
@Al2Klimov This also affects icinga2/lib/remote/apilistener.cpp Line 465 in 8431ea5 |
Al2Klimov
approved these changes
Nov 26, 2019
| @@ -416,7 +416,7 @@ bool ApiListener::AddListener(const String& node, const String& service) | |||
| Log(LogInformation, "ApiListener") | |||
| << "Started new listener on '[" << localEndpoint.address() << "]:" << localEndpoint.port() << "'"; | |||
|
|
|||
| IoEngine::SpawnCoroutine(io, [this, acceptor, sslContext](asio::yield_context yc) { ListenerCoroutineProc(yc, acceptor, sslContext); }); | |||
| IoEngine::SpawnCoroutine(io, [this, acceptor](asio::yield_context yc) { ListenerCoroutineProc(yc, acceptor, m_SSLContext); }); | |||
There was a problem hiding this comment.
Please have a look where the previously explicitly captured variable comes from and what it's a copy of. IMO this change is a no-op...
|
And yes, |
|
@Al2Klimov Thanks a lot 👍 @mcktr Can you please take care of this in a new PR? I'll approve and merge here. |
dnsmichi
approved these changes
Nov 27, 2019
mcktr
added a commit
that referenced
this pull request
Nov 28, 2019
This avoids copying the TLS context in the ApiListener class and removes the obsolete variable. This is a follow-up from #7654
dnsmichi
pushed a commit
that referenced
this pull request
Nov 29, 2019
This avoids copying the TLS context in the ApiListener class and removes the obsolete variable. This is a follow-up from #7654
N-o-X
pushed a commit
that referenced
this pull request
Sep 11, 2020
This avoids copying the TLS context in the ApiListener class and removes the obsolete variable. This is a follow-up from #7654
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This uses the class member variable for the SSL context inside the lambda expression which is captured by
this.The problem occurs with the following setup:
Master:
Agent
hostattribute for the masterzone.confAnd (re)start the master.
hostattribute from agentszones.conffileNow restart the agent to apply the configuration changes
Sign the CSR on the master
Notice that the agents always updates the certificate. The agent is in a loop. A restart of the agent ends the loop and the master can successfully connect and performs no further certificate updates.
While the agent was stuck in the loop I connected to the agent with
openssl s_client. The used certificate was not updated, the issuer of the certificate was the agent and not the Icinga CA. After the agent restarts the correct certificate was used (issuer: Icinga CA).fixes #7650