[pgpool-hackers: 3552] Re: [PATCH] Feature: Add support for an user/password input file to pg_enc

[Tatsuo Ishii]

Hi Umar,

> Any feedback ?

I have looked into this.

(1) I think error message style is a little bit different from
the standard of ours (or PostgreSQL, we follow PostgreSQL style).

+		fprintf(stderr, "input_file \"%s\" cannot be opened\n\n", input_file);
According to our style this will be something like this. Also it's recommended to add %m to show the cause of the errro.
fprintf(stderr, "failed to open input_file \"%s\" (%m)\n\n", input_file);

(2) It's not recommended to start an error message with upper case letter.
+			fprintf(stdout, "Input exceeds maximum username length!\n\n");
Also I think it's better to inform the max username length. I propose something like:
+			fprintf(stdout, "input exceeds maximum username length %d\n\n", MAX_USER_NAME_LEN);

(3) Following message style seems to be inconsistent the message above.

+			fprintf(stdout, "Input exceeds maximum password length given:%d max allowed:%d!\n", (int)strlen(pch), MAX_PGPASS_LEN);
What about this?
+			fprintf(stdout, "input exceeds maximum password length %d\n", MAX_PGPASS_LEN);

(4) if username+password exceeds the buffer for fgets(), pg_enc
errored out but read rest of the line which fgets() did not read and
will be confused.

LINE#01: USER: username1
LINE#02: USER: username2
LINE#03: Input exceeds maximum username length!

LINE#04: Invalid username:password pair
LINE#05: USER: username4

Actually there's no line#05. See attached test input.

(5) I am not sure if we need to check the input by using stat():

+	if (stat(input_file, &stat_buf) != 0)
+	{
+		fprintf(stderr, "input_file \"%s\" cannot be opened. Check file permissions\n\n", input_file);
+		exit(EXIT_FAILURE);
+	}
 
-	return EXIT_SUCCESS;
+	if (!S_ISREG(stat_buf.st_mode))
+	{
+		fprintf(stderr, "input_file \"%s\" is not a plain file\n\n", input_file);
+		exit(EXIT_FAILURE);
+	}

I think it's overkill. Usually we (and PostgreSQL) just try to open a
file and complain if it fails.

(6) 
+			buf[len - 1] = 0;
It's better to use '\0' rather than 0 here.

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp

> On Tue, Mar 10, 2020 at 5:07 PM Umar Hayat <m.umarkiani at gmail.com> wrote:
> 
>> Hi Hackers,
>> Attached patch address mentisbt-422
>> <https://www.pgpool.net/mantisbt/view.php?id=422> for pg_enc utility.
>> User can provide user name , password pair in a file to encrypt and add
>> them to pool_passwd file.
>> A new --input-file option is added in pg_enc. Using this pg_enc will parse
>> the username:password pairs from provide input file and it will add
>> username:AESxxxxx value in pool_passwod file.
>>
>> Attached contains:
>> 1. Implementation of feature in pg_enc tool
>> 2. Documentation update
>>
>> Regards
>> Umar Hayat
>> Principle Software Engineer
>> EnterpriseDB: https://www.enterpriseDB.com
>>
>>
>>
-------------- next part --------------
username1:secretpassword1
username2:secretpassword2
111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111:111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111
username4:secretpassword3