-
Notifications
You must be signed in to change notification settings - Fork 59
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adding filename to master key generation Issue #419 #435
base: master
Are you sure you want to change the base?
Conversation
Please, use the same pull request, and force push changes instead of opening new ones... |
Okk I will continue in this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need some time to test this, but at glance it seems there is extra work to setup the configuration in the pgagroal-admin
.
src/admin.c
Outdated
{ | ||
if (!remote_connection) | ||
{ | ||
errx(1, "Host (-h) and port (-p) must be specified to connect to the remote host"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pgagroal-admin is local only, pgagroal-cli can do remote
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
src/admin.c
Outdated
|
||
while (1) | ||
{ | ||
static struct option long_options[] = | ||
{ | ||
{"config", required_argument, 0, 'c'}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't m
less confusing than c
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
c
is for main configuration file, otherwise it is another letter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have used main configuration file to add the master_key_file_location.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think for admin.c
we just need the explicit -m
option e.g. no -c
src/admin.c
Outdated
memcpy(&config->common.log_path[0], logfile, MIN(MISC_LENGTH - 1, strlen(logfile))); | ||
} | ||
|
||
if (pgagroal_start_logging()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is pgagroal-admin
using the logging subsystem?
{ | ||
warnx("No home directory for user \'%s\'", username); | ||
mkdir(&buf[0], S_IRWXU); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we catch a failure here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@palak-chaturvedi please be patient, let's go back one step: add the |
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 . |
It is ok for the |
Ok so I am working on the -m flag it would change all the configuration.c functions because master key is required in those codes. |
I'm wondering if this is rally worth, since it seems to be fairly complex (as of design). However, mu opinion is that |
The The only binary that you have to change are |
b62b785
to
6eeff18
Compare
heyy @fluca1978 I made the changes can you please check once again? |
Looks fine. A few comments:
|
Remove remote connection part from the admin.c Added -m flag Added multiple error types for master key
d0c16c3
to
411a0fa
Compare
@palak-chaturvedi in commit 411a0fa you fixed the first problem, good!
and this is related to issue three: I would remove the master key file from the configuration and add the |
Updated with respect to the new configuration.c changes.
conf-file
to the master-key generationpgagroal-admin -c conf-file -g master-key
@jesperpedersen @fluca1978