empiredan commented on code in PR #2223:
URL: https://github.com/apache/zookeeper/pull/2223#discussion_r1997890950
##########
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:
You understood it very accurately! The reason for this modification is
mainly based on the comment in
https://github.com/apache/zookeeper/blame/master/zookeeper-client/zookeeper-client-c/include/zookeeper.h#L661:
`\param password_file the name of a file whose first line is read in
response to \c SASL_CB_PASS, or NULL for none.`
Therefore, I replaced `strrchr` with `strchr`.
Do you think this modification is appropriate? If so, should I open a new
pull request as a bug fix?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
