Adding filename to master key generation Issue #419 #435

palak-chaturvedi · 2024-04-01T14:33:35Z

Updated with respect to the new configuration.c changes.

Can add -c conf-file to the master-key generation
pgagroal-admin -c conf-file -g master-key
The pgagroal_get_master_key() is also updated to access the master_key_file_location from the conf file.

@jesperpedersen @fluca1978

jesperpedersen · 2024-04-01T15:08:03Z

Please, use the same pull request, and force push changes instead of opening new ones...

palak-chaturvedi · 2024-04-01T18:44:55Z

Okk I will continue in this
There were some merge conflicts in it. I was not able to pull all the new commits from the master branch. So I thought I would make it much cleaner in this one.

fluca1978

I need some time to test this, but at glance it seems there is extra work to setup the configuration in the pgagroal-admin.

fluca1978 · 2024-04-02T06:48:35Z

src/admin.c

+      {
+         if (!remote_connection)
+         {
+            errx(1, "Host (-h) and port (-p) must be specified to connect to the remote host");


Is pgagroal-admin supposed to be able to connect to the remote host? It doesn't even contain the arguments the message is claiming to use. I suspect there is extra copy and paste here.

pgagroal-admin is local only, pgagroal-cli can do remote

I was not sure how to handle the error when conf file is not specified. I copied the cli one.
I will remove the remote connection thing.

fluca1978 · 2024-04-02T06:49:04Z

src/admin.c


   while (1)
   {
      static struct option long_options[] =
      {
+         {"config", required_argument, 0, 'c'},


isn't m less confusing than c?

c is for main configuration file, otherwise it is another letter

I have used main configuration file to add the master_key_file_location.

I think for admin.c we just need the explicit -m option e.g. no -c

fluca1978 · 2024-04-02T06:49:35Z

src/admin.c

+            memcpy(&config->common.log_path[0], logfile, MIN(MISC_LENGTH - 1, strlen(logfile)));
+         }
+
+         if (pgagroal_start_logging())


is pgagroal-admin using the logging subsystem?

fluca1978 · 2024-04-02T06:51:50Z

src/admin.c

      {
-         warnx("No home directory for user \'%s\'", username);
+         mkdir(&buf[0], S_IRWXU);


should we catch a failure here?

I haven't made any modifications to this part. This is already written in the master branch. I just indented it for those cases where filename is not provided.
I can look into it if there is some error?

I haven't made any modifications to this part. This is already written in the master branch. I just indented it for those cases where filename is not provided. I can look into it if there is some error?

Ok, if you will, please check for possible errors, or will postpone.

fluca1978 · 2024-04-04T13:34:30Z

@palak-chaturvedi please be patient, let's go back one step: add the -m flag to both pagroal- and pgagroal-admin, so that if specified, the executables can find out the correct location of the master key file and other related stuff. Then we will decide if and how to handle it within the main configuration file.

palak-chaturvedi · 2024-04-06T20:01:39Z

If we add the -m flag then we will have to change lots of functions including reloading the configuration files and all the functions in configuration.c .
Is this better option than conf file changing?

jesperpedersen · 2024-04-06T23:15:41Z

It is ok for the pgagroal.conf file, but pgagroal-admin shouldn't depend on that file

palak-chaturvedi · 2024-04-07T12:33:55Z

Ok so
If the conf file is not given then all the functions other than those that require master-key should not throw error.

I am working on the -m flag it would change all the configuration.c functions because master key is required in those codes.

fluca1978 · 2024-04-08T07:37:14Z

I'm wondering if this is rally worth, since it seems to be fairly complex (as of design).

However, mu opinion is that pgagroal.conf should have a setting for the master key file location, and pgagroal-admin should have a -m flag that reflects the same setting. In this way, the configuration of main should be straightforward, while the pgagroal-admin can use the same location specified on the command line.
However, this could cause some confusion, since pgagroal-admin does not ask for a master key location, and will write on the default path, so the user can end up with pgagroal-admin writing in a location and pgagroal looking into another without any error or warning. Is it worth?

jesperpedersen · 2024-04-08T12:05:46Z

The master_key_path is in pgagroal.conf, so it will be used for pgagroal. pgagroal-cli also uses pgagroal.conf.

The only binary that you have to change are pgagroal-admin with the -m switch, and pgagroal-vault for the pgagroal-vault.conf configuration

palak-chaturvedi · 2024-04-21T13:08:30Z

heyy @fluca1978 I made the changes can you please check once again?

fluca1978 · 2024-04-22T07:06:39Z

Looks fine. A few comments:

if I try to use a command that do not require a master key file with a wrong master key file, everything works fine. Maybe we should check for the -m usage early in pgagroal-admin:

% pgagroal-admin user ls --master-key-file /this/does/not/exists
luca

if pgagroal does not find the master key file, it warns with the wrong error message:

% pgagroal
pgagroal: Invalid master key file (file </etc/pgagroal/pgagroal_users.conf>)

I would remove master_key_file_location from the configuration and add the -m flag to the pgagroal tool itself.

Remove remote connection part from the admin.c Added -m flag Added multiple error types for master key

fluca1978 · 2024-04-29T11:37:54Z

@palak-chaturvedi in commit 411a0fa you fixed the first problem, good!
Still issue two is there.

% pgagroal                                   
pgagroal: Invalid master key file (file </etc/pgagroal/pgagroal_users.conf>)

and this is related to issue three: I would remove the master key file from the configuration and add the -m option to main.c. Otherwise, if you believe it is better to keep such file in the configuration file, can you please argument your idea?

jesperpedersen requested review from jesperpedersen and fluca1978 April 1, 2024 18:54

jesperpedersen assigned palak-chaturvedi Apr 1, 2024

jesperpedersen added the feature New feature label Apr 1, 2024

fluca1978 suggested changes Apr 2, 2024

View reviewed changes

palak-chaturvedi requested a review from fluca1978 April 3, 2024 18:01

palak-chaturvedi force-pushed the Issue419 branch from b62b785 to 6eeff18 Compare April 16, 2024 08:32

Adding filename to master key generation Issue agroal#419

411a0fa

Remove remote connection part from the admin.c Added -m flag Added multiple error types for master key

palak-chaturvedi force-pushed the Issue419 branch from d0c16c3 to 411a0fa Compare April 27, 2024 12:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding filename to master key generation Issue #419 #435

Adding filename to master key generation Issue #419 #435

palak-chaturvedi commented Apr 1, 2024

jesperpedersen commented Apr 1, 2024

palak-chaturvedi commented Apr 1, 2024

fluca1978 left a comment

fluca1978 Apr 2, 2024

jesperpedersen Apr 2, 2024

palak-chaturvedi Apr 3, 2024

fluca1978 Apr 2, 2024

jesperpedersen Apr 2, 2024

palak-chaturvedi Apr 3, 2024

jesperpedersen Apr 3, 2024

fluca1978 Apr 2, 2024

fluca1978 Apr 2, 2024

palak-chaturvedi Apr 3, 2024

fluca1978 Apr 4, 2024

fluca1978 commented Apr 4, 2024

palak-chaturvedi commented Apr 6, 2024

jesperpedersen commented Apr 6, 2024 •

edited

palak-chaturvedi commented Apr 7, 2024

fluca1978 commented Apr 8, 2024

jesperpedersen commented Apr 8, 2024

palak-chaturvedi commented Apr 21, 2024

fluca1978 commented Apr 22, 2024

fluca1978 commented Apr 29, 2024

Adding filename to master key generation Issue #419 #435

Are you sure you want to change the base?

Adding filename to master key generation Issue #419 #435

Conversation

palak-chaturvedi commented Apr 1, 2024

jesperpedersen commented Apr 1, 2024

palak-chaturvedi commented Apr 1, 2024

fluca1978 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fluca1978 commented Apr 4, 2024

palak-chaturvedi commented Apr 6, 2024

jesperpedersen commented Apr 6, 2024 • edited

palak-chaturvedi commented Apr 7, 2024

fluca1978 commented Apr 8, 2024

jesperpedersen commented Apr 8, 2024

palak-chaturvedi commented Apr 21, 2024

fluca1978 commented Apr 22, 2024

fluca1978 commented Apr 29, 2024

jesperpedersen commented Apr 6, 2024 •

edited