cnauroth commented on code in PR #2223:
URL: https://github.com/apache/zookeeper/pull/2223#discussion_r1989809880
##########
zookeeper-client/zookeeper-client-c/src/zk_sasl.c:
##########
@@ -451,41 +453,87 @@ struct zsasl_secret_ctx {
static int _zsasl_getsecret(sasl_conn_t *conn, void *context, int id,
sasl_secret_t **psecret)
{
+ const size_t MAX_PASSWORD_LEN = 1023;
struct zsasl_secret_ctx *secret_ctx = (struct zsasl_secret_ctx *)context;
- char buf[1024];
- char *password;
- size_t len;
+ char buf[MAX_PASSWORD_LEN + 1];
+ char *password = NULL;
+ size_t len = 0;
+ int res = 0;
+ char new_passwd[MAX_PASSWORD_LEN + 1];
+ char *p = NULL;
sasl_secret_t *x;
/* paranoia check */
- if (!conn || !psecret || id != SASL_CB_PASS)
+ if (!conn || !psecret || id != SASL_CB_PASS) {
return SASL_BADPARAM;
+ }
if (secret_ctx->password_file) {
- char *p;
FILE *fh = fopen(secret_ctx->password_file, "rt");
- if (!fh)
+ if (!fh) {
return SASL_FAIL;
+ }
- if (!fgets(buf, sizeof(buf), fh)) {
+ /*
+ * The file's content may be the encrypted password with binary
characters,
+ * thus use fread().
+ */
+ len = fread(buf, sizeof(buf[0]), MAX_PASSWORD_LEN, fh);
+ if (ferror(fh)) {
fclose(fh);
return SASL_FAIL;
}
fclose(fh);
+ fh = NULL;
+
+ /*
+ * Write the null terminator immediately after the last character of
the
+ * content since it would be used as a null-terminated string once it
is
+ * the actual password.
+ */
+ buf[len] = '\0';
+ password = buf;
+ }
- p = strrchr(buf, '\n');
- if (p)
- *p = '\0';
+ if (secret_ctx->callback) {
+ if (!password) {
+ /*
+ * The callback takes effect only when password_file is provided.
+ */
+ return SASL_BADPARAM;
+ }
- password = buf;
+ res = secret_ctx->callback(password, len, secret_ctx->context,
+ new_passwd, sizeof(new_passwd));
+ if (res != SASL_OK) {
+ return res;
+ }
+
+ memset(password, 0, len);
+
+ password = new_passwd;
+ } else if (secret_ctx->password_file) {
+ /*
+ * The file's content is the actual password, which must consist only
of
+ * text characters (i.e., without null terminator). The first line
would
+ * be read as the password once there are multiple lines in the file.
+ */
+ p = strchr(password, '\n');
Review Comment:
I think there is a subtle change in behavior here from `strrchr` (find last
occurrence) to `strchr` (find first occurrence). Is my understanding correct?
This doesn't seem to be directly related to the new callback functionality, but
it seems like a good bug fix.
##########
zookeeper-client/zookeeper-client-c/CMakeLists.txt:
##########
@@ -249,6 +249,7 @@ set(test_sources
tests/ThreadingUtil.cc
tests/TestZookeeperInit.cc
tests/TestZookeeperClose.cc
+ tests/TestSASLAuth.cc
Review Comment:
Great catch! This test suite has not been running regularly in our CI.
##########
zookeeper-client/zookeeper-client-c/src/zk_sasl.c:
##########
@@ -451,41 +453,87 @@ struct zsasl_secret_ctx {
static int _zsasl_getsecret(sasl_conn_t *conn, void *context, int id,
sasl_secret_t **psecret)
{
+ const size_t MAX_PASSWORD_LEN = 1023;
struct zsasl_secret_ctx *secret_ctx = (struct zsasl_secret_ctx *)context;
- char buf[1024];
- char *password;
- size_t len;
+ char buf[MAX_PASSWORD_LEN + 1];
+ char *password = NULL;
+ size_t len = 0;
+ int res = 0;
+ char new_passwd[MAX_PASSWORD_LEN + 1];
+ char *p = NULL;
sasl_secret_t *x;
/* paranoia check */
- if (!conn || !psecret || id != SASL_CB_PASS)
+ if (!conn || !psecret || id != SASL_CB_PASS) {
return SASL_BADPARAM;
+ }
if (secret_ctx->password_file) {
- char *p;
FILE *fh = fopen(secret_ctx->password_file, "rt");
- if (!fh)
+ if (!fh) {
return SASL_FAIL;
+ }
- if (!fgets(buf, sizeof(buf), fh)) {
+ /*
+ * The file's content may be the encrypted password with binary
characters,
+ * thus use fread().
+ */
+ len = fread(buf, sizeof(buf[0]), MAX_PASSWORD_LEN, fh);
+ if (ferror(fh)) {
fclose(fh);
return SASL_FAIL;
}
fclose(fh);
+ fh = NULL;
Review Comment:
Is this line necessary? It seems like `fh` is unused after this and will go
out of scope.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
