sip/transp: add client certificate to all TLS transports #1173
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
alfredh
reviewed
Aug 3, 2024
src/sip/transp.c
Outdated
| @@ -47,6 +47,12 @@ struct sip_ccert { | |||
| }; | |||
|
|
|||
|
|
|||
| struct ccert_data { | |||
There was a problem hiding this comment.
Perhaps add sip_ prefix to match the name of the module ?
alfredh
reviewed
Aug 3, 2024
src/sip/transp.c
Outdated
| static struct le *transp_apply_all(struct sip *sip, enum sip_transp tp, int af, | ||
| list_apply_h ah, void *arg) | ||
| { | ||
| struct le *le; |
There was a problem hiding this comment.
this can be moved to for-loop
alfredh
reviewed
Aug 3, 2024
src/sip/transp.c
Outdated
| @@ -1401,6 +1434,27 @@ int sip_transp_add_websock(struct sip *sip, enum sip_transp tp, | |||
| } | |||
|
|
|||
|
|
|||
| static bool add_ccert(struct le *le, void *arg) | |||
alfredh
reviewed
Aug 3, 2024
| (void)transp_apply_all(sip, SIP_TRANSP_TLS, AF_INET, add_ccert, | ||
| &cc_data); | ||
| (void)transp_apply_all(sip, SIP_TRANSP_TLS, AF_INET6, add_ccert, | ||
| &cc_data); |
There was a problem hiding this comment.
I tried to change the behavior as little as possible. Before this change, the sip_transp_add_ccert function did not consider WSS either and when adding the WSS transport with sip_transp_add_websock, the hash table for the certs is not even allocated.
It would probably make sense, but maybe add that support in a different PR.
Currently, when a client certificate is added to a SIP transport, it is only added to the first matching transport in the transport list. Then, if multiple SIP transports exist (e.g if there are multiple network interfaces), the certificate might not be present in the transport when it is needed. Now, the certificate is added to all matching transports.
maximilianfridrich
force-pushed
the
transp_ccert_fix
branch
from
aa8fad4 to
353dd54
Compare
|
Please merge to main if you agree @sreimers |
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.
Currently, when a client certificate is added to a SIP transport, it is only added to the first matching transport in the transport list. Then, if multiple SIP transports exist (e.g if there are multiple network interfaces), the certificate might not be present in the transport when it is needed.
Now, the certificate is added to all matching transports.
This BareSIP PR tests the fix: