[sssd PR#197][comment] KCM responder
by mzidek-rh
URL: https://github.com/SSSD/sssd/pull/197
Title: #197: KCM responder
mzidek-rh commented:
"""
Some nitpicks mostly regarding the lack of DEBUG messages.
src/responder/kcm/kcmsrv_cmd.c
164 /* First 16 bits are 8 bit major and 8bit major protocol version */
^^^^^ Should be minor
-----------------------------------
src/responder/kcm/kcmsrv_cmd.c - this module could use more debug messages. First because it is tevent code and second because the debug messages server as "natural" comments. For example:
311 static void kcm_send_reply(struct cli_ctx *cctx,
312 struct kcm_op_io *op_io,
313 struct kcm_repbuf *repbuf)
314 {
315 errno_t ret;
316
317 ret = kcm_output_construct(op_io, repbuf);
318 if (ret != EOK) {
It would be good to print DEBUG message here with the error code.
319 kcm_reply_error(cctx, ret, repbuf);
320 return;
321 }
-----------------------------------
This function:
509 static int kcm_send_data(struct cli_ctx *cctx)
Could have a debug a message after each kcm_write_iovec operation saying what it failed to write.
-----------------------------------
369 static void kcm_cmd_request_done(struct tevent_req *req)
389 static errno_t kcm_recv_data(int fd, struct kcm_reqbuf *reqbuf)
IMO the above functions should also print DEBUG messages on errors.
-----------------------------------
The function
bool kcm_cc_access(struct kcm_ccache *cc
Should IMO include TRACE_FUNC level debug messages at least for all cases when the result is false. The root cause is lost in the above calling functions and it ends up later in some async code.
-----------------------------------
1166 static errno_t sec_list_recv(struct tevent_req *req,
1167 TALLOC_CTX *mem_ctx,
1168 const char ***_sec_key_list,
1169 size_t *_sec_key_list_len)
1170
1171 {
1172 struct sec_list_state *state = tevent_req_data(req,
1173 struct sec_list_state);
1174
1175 TEVENT_REQ_RETURN_ON_ERROR(req);
1176
1177 if (_sec_key_list != NULL) {
1178 *_sec_key_list = talloc_steal(mem_ctx, state->sec_key_list);
1179 }
1180 if (_sec_key_list_len) {
^^^^^^ use != NULL here too
1181 *_sec_key_list_len = state->sec_key_list_len;
1182 }
1183 return EOK;
1184 }
-----------------------------------
1247 static void sec_get_done(struct tevent_req *subreq)
1248 {
1249 errno_t ret;
1250 struct tevent_req *req = tevent_req_callback_data(subreq, struct tevent_req);
1251 struct sec_get_state *state = tevent_req_data(req,
1252 struct sec_get_state);
1253 struct sss_iobuf *outbuf;
1254 const char *sec_value;
1255 int http_code;
1256
1257 ret = tcurl_http_recv(state, subreq, &http_code, &outbuf);
1258 talloc_zfree(subreq);
1259 if (ret != EOK) {
1260 DEBUG(SSSDBG_OP_FAILURE,
1261 "GET HTTP request failed [%d]: %s\n", ret, sss_strerror(ret));
1262 tevent_req_error(req, ret);
1263 return;
1264 }
1265
1266 if (http_code != 200) {
1267 ret = http2errno(http_code);
Add DEBUG message here
1268 tevent_req_error(req, ret);
1269 return;
1270 }
"""
See the full comment at https://github.com/SSSD/sssd/pull/197#issuecomment-288409105
6 years, 2 months
[sssd PR#197][comment] KCM responder
by mzidek-rh
URL: https://github.com/SSSD/sssd/pull/197
Title: #197: KCM responder
mzidek-rh commented:
"""
Some nitpicks mostly regarding the lack of DEBUG messages.
src/responder/kcm/kcmsrv_cmd.c
164 /* First 16 bits are 8 bit major and 8bit major protocol version */
^^^^^ Should be minor
-----------------------------------
src/responder/kcm/kcmsrv_cmd.c - this module could use more debug messages. First because it is tevent code and second because the debug messages server as "natural" comments. For example:
311 static void kcm_send_reply(struct cli_ctx *cctx,
312 struct kcm_op_io *op_io,
313 struct kcm_repbuf *repbuf)
314 {
315 errno_t ret;
316
317 ret = kcm_output_construct(op_io, repbuf);
318 if (ret != EOK) {
It would be good to print DEBUG message here with the error code.
319 kcm_reply_error(cctx, ret, repbuf);
320 return;
321 }
-----------------------------------
This function:
509 static int kcm_send_data(struct cli_ctx *cctx)
Could have a debug a message after each kcm_write_iovec operation saying what it failed to write.
-----------------------------------
369 static void kcm_cmd_request_done(struct tevent_req *req)
389 static errno_t kcm_recv_data(int fd, struct kcm_reqbuf *reqbuf)
IMO the above functions should also print DEBUG messages on errors.
-----------------------------------
The function
bool kcm_cc_access(struct kcm_ccache *cc
Should IMO include TRACE_FUNC level debug messages at least for all cases when the result is false. The root cause is lost in the above calling functions and it ends up later in some async code.
-----------------------------------
1166 static errno_t sec_list_recv(struct tevent_req *req,
1167 TALLOC_CTX *mem_ctx,
1168 const char ***_sec_key_list,
1169 size_t *_sec_key_list_len)
1170
1171 {
1172 struct sec_list_state *state = tevent_req_data(req,
1173 struct sec_list_state);
1174
1175 TEVENT_REQ_RETURN_ON_ERROR(req);
1176
1177 if (_sec_key_list != NULL) {
1178 *_sec_key_list = talloc_steal(mem_ctx, state->sec_key_list);
1179 }
1180 if (_sec_key_list_len) {
^^^^^^ use != NULL here too
1181 *_sec_key_list_len = state->sec_key_list_len;
1182 }
1183 return EOK;
1184 }
-----------------------------------
1247 static void sec_get_done(struct tevent_req *subreq)
1248 {
1249 errno_t ret;
1250 struct tevent_req *req = tevent_req_callback_data(subreq, struct tevent_req);
1251 struct sec_get_state *state = tevent_req_data(req,
1252 struct sec_get_state);
1253 struct sss_iobuf *outbuf;
1254 const char *sec_value;
1255 int http_code;
1256
1257 ret = tcurl_http_recv(state, subreq, &http_code, &outbuf);
1258 talloc_zfree(subreq);
1259 if (ret != EOK) {
1260 DEBUG(SSSDBG_OP_FAILURE,
1261 "GET HTTP request failed [%d]: %s\n", ret, sss_strerror(ret));
1262 tevent_req_error(req, ret);
1263 return;
1264 }
1265
1266 if (http_code != 200) {
1267 ret = http2errno(http_code);
Add DEBUG message here
1268 tevent_req_error(req, ret);
1269 return;
1270 }
"""
See the full comment at https://github.com/SSSD/sssd/pull/197#issuecomment-288409105
6 years, 2 months
[sssd PR#197][comment] KCM responder
by jhrozek
URL: https://github.com/SSSD/sssd/pull/197
Title: #197: KCM responder
jhrozek commented:
"""
I made two fixes related to documentation and packaging. They are already squashed, but to ease the review, the documentation fix was:
```
diff --git a/src/man/sssd-kcm.8.xml b/src/man/sssd-kcm.8.xml
index b28b3bf..5dc9383 100644
--- a/src/man/sssd-kcm.8.xml
+++ b/src/man/sssd-kcm.8.xml
@@ -119,6 +119,24 @@ systemctl enable sssd-kcm.service
</para>
</refsect1>
+ <refsect1 id='storage'>
+ <title>THE CREDENTIAL CACHE STORAGE</title>
+ <para>
+ The credential caches are stored in the SSSD secrets service (see
+ <citerefentry>
+ <refentrytitle>sssd-secrets</refentrytitle><manvolnum>5</manvolnum>
+ </citerefentry>
+ for more details). Therefore it is important that also the sssd-secrets
+ service is enabled and its socket is started:
+ <programlisting>
+systemctl start sssd-secrets.socket
+systemctl enable sssd-secrets.socket
+systemctl enable sssd-secrets.service
+ </programlisting>
+ Your distribution should already set the dependencies between the services.
+ </para>
+ </refsect1>
+
<refsect1 id='options'>
<title>CONFIGURATION OPTIONS</title>
<para>
```
And the packaging fix was about pointing to the right socket -- I didn't notice the wrong socket name because I had the configuration on the sssd side overriden in the same manner:
```
index 45d4853..ac88fca 100644
--- a/contrib/kcm_default_ccache
+++ b/contrib/kcm_default_ccache
@@ -2,8 +2,11 @@
# directory that is included from the Kerberos configuration file (/etc/krb5.conf)
# On Fedora/RHEL/CentOS, this is /etc/krb5.conf.d/
#
-# To enable the KCM credential cache, uncomment the two following lines and enable the KCM socket:
+# To enable the KCM credential cache, uncomment the following lines and
+# enable the KCM socket and the service:
# systemctl enable sssd-kcm.socket
+# systemctl start sssd-kcm.socket
+# systemctl enable sssd-kcm.service
#[libdefaults]
# default_ccache_name = KCM:
diff --git a/src/sysv/systemd/sssd-kcm.socket.in b/src/sysv/systemd/sssd-kcm.socket.in
index b115b9a..80ec1c0 100644
--- a/src/sysv/systemd/sssd-kcm.socket.in
+++ b/src/sysv/systemd/sssd-kcm.socket.in
@@ -1,9 +1,10 @@
[Unit]
Description=SSSD Secrets Service responder socket
-Documentation=man:sssd-kcm(5)
+Documentation=man:sssd-kcm(8)
+Requires=sssd-secrets.socket
[Socket]
-ListenStream=@localstatedir(a)/run/kcm.socket
+ListenStream=@localstatedir(a)/run/.heim_org.h5l.kcm-socket
[Install]
WantedBy=sockets.target
```
"""
See the full comment at https://github.com/SSSD/sssd/pull/197#issuecomment-288329922
6 years, 2 months