krb5: write kdcinfo.* file with port configuration #5920
Conversation
|
@thalman since you were the last person touching this part of the code I think it would be sensible that you took a look at the changes. |
|
Debian and RHEL8 failures aren't related with this PR. I'm trying to fix them in #5922 |
When writing the 'kdcinfo.*' file take into account all the information set in the 'krb5_server' option, including the port. This wasn't taken into account and that's why the kerberos child only used the address part, thus being unable to contact the service in the server. Resolves: SSSD#5919 Signed-off-by: Iker Pedrosa <[email protected]>
ikerexxe
force-pushed
the
kerberos_port
branch
from
0ed9a94 to
7f76353
Compare
| @@ -730,6 +731,16 @@ errno_t write_krb5info_file_from_fo_server(struct krb5_service *krb5_service, | |||
| if (filter == NULL || filter(server) == false) { | |||
| address = fo_server_address_or_name(tmp_ctx, server); | |||
| if (address) { | |||
| port = fo_get_server_port(server); | |||
| if (port != 0) { | |||
| address = talloc_asprintf(tmp_ctx, "%s:%d", address, port); | |||
There was a problem hiding this comment.
it does not matter much but talloc_asprintf_append looks more appropriate (as well as in the second case bellow).
What do you think?
Otherwise the PR looks good to me.
There was a problem hiding this comment.
I'd like to do that but if I do that I get a compilation warning:
warning: assignment discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers].
And if I tackle that by casting to (char *) talloc_asprintf_append() then I get another warning:
warning: cast discards ‘const’ qualifier from pointer target type [-Wcast-qual].
Taking into account that we'll switch all warnings into errors I don't think it's a good idea to leave a warning in the code. So, do you have any idea on how to talloc_asprintf_append() without having any compilation warning?
There was a problem hiding this comment.
I see the point. Using _append may complicate the more common usecase where default port is used.
Thank you for the explanation
ACK
There was a problem hiding this comment.
Taking into account that we'll switch all warnings into errors
Depends on definition of "all", but in general I'm not sure this is a really good idea.
From my point of view, it would be much better to introduce a policy to ensure new patches doesn't introduce new warnings, but I'm not convinced "zero warnings code base" goal would pay off.
There was a problem hiding this comment.
While I agree on the introduce a policy to ensure new patches doesn't introduce new warnings, it isn't feasible for this type of check as the compiler doesn't know if the warning existed before the patch. That is to say, either you enable the policy for the whole code base or you don't.
With that said, I think it's best if we have this discussion in January when everybody is back from vacation.
There was a problem hiding this comment.
It is feasible: you do a run with and without patches and compute a diff.
Covscan is already doing this for us.
| @@ -730,6 +731,16 @@ errno_t write_krb5info_file_from_fo_server(struct krb5_service *krb5_service, | |||
| if (filter == NULL || filter(server) == false) { | |||
| address = fo_server_address_or_name(tmp_ctx, server); | |||
| if (address) { | |||
| port = fo_get_server_port(server); | |||
| if (port != 0) { | |||
| address = talloc_asprintf(tmp_ctx, "%s:%d", address, port); | |||
There was a problem hiding this comment.
I see the point. Using _append may complicate the more common usecase where default port is used.
Thank you for the explanation
ACK
|
Debian CI failure seems to be non-relevant. |
When writing the 'kdcinfo.*' file take into account all the information
set in the 'krb5_server' option, including the port. This wasn't taken
into account and that's why the kerberos child only used the address
part, thus being unable to contact the service in the server.
Resolves: #5919