Skip to content
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

WIP: vault implemention #378

Closed
wants to merge 4 commits into from
Closed

WIP: vault implemention #378

wants to merge 4 commits into from

Conversation

solokyo
Copy link

@solokyo solokyo commented Sep 5, 2023

WORKING IN PROGRESS, DO NOT MERGE OR USE IN PRODUCTION

About this coomit

  1. An HTTP server that supports http and https. However, bind port 443 require sudo setcap CAP_NET_BIND_SERVICE=+eip path/to/pgagroal. vault.c currently is an executable for debugging convenience, and will convert to non-executable if needed.

  2. generate_password function moved to security.c and enhanced.

  3. New message added for vault - pgagroal communication.

  4. rotate_password_cb added for changing passwords over time.

  5. accpet_vault_cb and handle_vault_cb added for async io between vault and pgagroal

  6. Lots of places are hard coded cause the config is not implemented yet.

Bug I dealing with:

I want to implement an async listener and handler in main.c, there should be a long-lived connection between pgagroal and vault, and handle_vault_cb should read-write messages whenever it received message from vault.

But this is what I got from gdb:

Starting program: /home/pgmoneta/pgagroal/build/src/pgagroal -c /etc/pgagroal/pgagroal.conf -a /etc/pgagroal/pgagroal_hba.conf -u /etc/pgagroal/pgagroal_users.conf -F /etc/pgagroal/pgagroal_frontend_users.conf -v

[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib64/libthread_db.so.1".
[Detaching after fork from child process 32673]
creating thread
/home/pgmoneta/pgagroal/src/libpgagroal/memory.c:111: pgagroal_memory_message: Assertion `message != NULL' failed.

Program received signal SIGABRT, Aborted.
0x00007ffff782abec in __pthread_kill_implementation () from /lib64/libc.so.6
(gdb) bt
#0  0x00007ffff782abec in __pthread_kill_implementation () from /lib64/libc.so.6
#1  0x00007ffff77da956 in raise () from /lib64/libc.so.6
#2  0x00007ffff77c47f4 in abort () from /lib64/libc.so.6
#3  0x00007ffff77c471b in __assert_fail_base.cold () from /lib64/libc.so.6
#4  0x00007ffff77d3536 in __assert_fail () from /lib64/libc.so.6
#5  0x00007ffff79b9ce8 in pgagroal_memory_message () at /home/pgmoneta/pgagroal/src/libpgagroal/memory.c:111
#6  0x00007ffff79bd6ab in read_message (socket=13, block=false, timeout=0, msg=0x7fffffffd558)
    at /home/pgmoneta/pgagroal/src/libpgagroal/message.c:1291
#7  0x00007ffff79b9f53 in pgagroal_read_socket_message (socket=13, msg=0x7fffffffd558)
    at /home/pgmoneta/pgagroal/src/libpgagroal/message.c:87
#8  0x000055555555d679 in handle_vault_cb (loop=0x7ffff7fba060, watcher=0x5555555ad3a0, revents=1)
    at /home/pgmoneta/pgagroal/src/main.c:1978
#9  0x00007ffff7faed0b in ev_invoke_pending () from /lib64/libev.so.4
#10 0x00007ffff7fb28e8 in ev_run () from /lib64/libev.so.4
#11 0x00005555555578f9 in ev_loop (loop=0x7ffff7fba060, flags=0) at /usr/include/ev.h:841
#12 0x000055555555b4c1 in main (argc=10, argv=0x7fffffffdcd8) at /home/pgmoneta/pgagroal/src/main.c:1210
(gdb) 

I don't know why handle_vault_cb is called, I didn't send anything from vault to pgagroal.

Also, I am unsure if I am doing the message read-write right.

**WORKING IN PROGRESS, DO NOT MERGE OR USE IN PRODUCTION**
@jesperpedersen
Copy link
Collaborator

Let it go to a separate process.

We just need to update the frontend passwords and then serve them to the vault binary

src/vault.c Outdated

client_socket = socket(AF_INET, SOCK_STREAM, 0);
if (client_socket == -1) {
perror("Socket creation error");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

errx

src/vault.c Outdated
serverAddress.sin_addr.s_addr = inet_addr("127.0.0.1");

if (connect(client_socket, (struct sockaddr *)&serverAddress, sizeof(serverAddress)) == -1) {
perror("Connection error");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

errx

src/vault.c Outdated

int result = pthread_create(&http_thread, NULL, http_thread_func, NULL);
if(result != 0) {
printf("something wrong\n");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warnx

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your feedback. I am aware that there are many flaws in the code I submitted in the PR. I opened this PR to discuss my implementation with you all. Once we have finalized how to implement these features, I will close this PR and submit a clean one.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need a new pull request.

Just squash and force push

@fluca1978
Copy link
Collaborator

I do agree in not using thread right here.
Also, as a minor note, you should run crustify against the code.

@solokyo
Copy link
Author

solokyo commented Sep 6, 2023

Let it go to a separate process.

What do you mean by "it"?

We just need to update the frontend passwords and then serve them to the vault binary

Are you suggesting pgagroal keep sending updated passwords to the vault and vault will keep a cache of it? When client comes and ask for password, vault don't need to talk to pgagroal.

@jesperpedersen
Copy link
Collaborator

No, let pgagroal update the front-end passwords. And then, the vault process ask for the current password whenever there is a client request for it

src/vault.c Outdated
if(result != 0) {
printf("something wrong\n");
}
pthread_create(&https_thread, NULL, https_thread_func, NULL);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should follow the process model instead of using threads

ev_periodic_init(&rotate_frontend_password, rotate_frontend_password_cb, 0., 60, 0);
ev_periodic_start (main_loop, &rotate_frontend_password);

unix_vault_socket = socket(AF_INET, SOCK_STREAM, 0);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this ? The request for the current frontend password can go over the management port...

fix bug that cause generate same password for all users
fix bug that cause cursh on querying non-exist users
@ashu3103
Copy link
Collaborator

@jesperpedersen, I have gone through the task and the corresponding files and I wish to continue this task, kindly assign me.

Things to be fixed as per discussions

pgagroal

  • Inside pgagroal, the valut's requests will be directed to management port.
  • Incorporate some way in pgagroal main.c to start pgagroal-vault server (by running pgagroal-vault executable in a seperate child process).
  • The logic to trigger rotate_frontend_password_cb is run periodically after X seconds (uisng ev_periodic watcher) where X is to be taken from configuration.

pgagroal-vault

  • The host can be same as the host of pgagroal and can be accessed from configuration and the port is defined 80 for HTTP and 443 for HTTPS.

Doubts

  • Suppose user_a and user_b are two frontend users and user_a sends an HTTP request http://XXX:YYY/user_b to the vault so will it get the password of user_b? I mean how are we gonna authenticate the users in case of HTTP.
  • Still not sure where to implement the command to change the frontend_user passwords, in vault or in pgagroal?

Any other suggestions...

@jesperpedersen
Copy link
Collaborator

In pgagroal there should be a GET_PASSWORD management command. pgagroal should not "talk" to pgagroal-vault in any way. pgagroal should handle the changing of the frontend end user passwords.

pgagroal-vault is its own binary, and is independent of pgagroal.

The port configuration should be defined in pgagroal-vault.conf, and for the initial implementation there is no need for HTTPS support nor user authentication.

Think of pgagroal-vault as a "proxy" that speaks HTTP to get the current front end password for a specific user.

@jesperpedersen jesperpedersen assigned ashu3103 and unassigned solokyo Feb 18, 2024
@ashu3103
Copy link
Collaborator

In pgagroal there should be a GET_PASSWORD management command. pgagroal should not "talk" to pgagroal-vault in any way. pgagroal should handle the changing of the frontend end user passwords.

Right, so pgagroal-vault should connect to management port of pgagroal which then handles the GET_PASSWORD request using messages exchanges.

pgagroal-vault is its own binary, and is independent of pgagroal.

Yeah, but it has to be spawned by pgagroal in the initialization stage of pgagroal if enable_vault (configuration parameter) is true.

The port configuration should be defined in pgagroal-vault.conf

Can we have a more specific format of pgagroal-vault.conf.

and for the initial implementation there is no need for HTTPS support nor user authentication.
Think of pgagroal-vault as a "proxy" that speaks HTTP to get the current front end password for a specific user.

Got it!

@jesperpedersen
Copy link
Collaborator

There is no enable_vault - it is a separate process managed on its own.

pgagroal-vault.conf will just have the configuration parameters needed for that process.

@ashu3103
Copy link
Collaborator

ashu3103 commented Feb 18, 2024

pgagroal-vault.conf will just have the configuration parameters needed for that process.

I am thinking pgagroal-vault.conf, should be like this...

host
pgagroal-host
pgagroal-management-port  # of pgagroal
max_connections
tls
tls_cert_file
tls_key_file
tls_ca_file

@jesperpedersen
Copy link
Collaborator

Yeah, something like

[pgagroal-vault]
host = localhost
port = 2500

[main]
host = localhost
port = 2345
user = admin
#tls
#tls_cert_file
#tls_key_file
#tls_ca_file

For now, 1 server connection definition is enough...

@ashu3103
Copy link
Collaborator

I have one confusion here, regarding connecting pgagroal-vault with pgagroal, according to the above .conf file we are directing the GET_PASSWORD requests from pgagroal-vault (port 2500) to the 2345 port of pgagroal (which is the default pgagroal port) which would call accept_main_cb (internally) that handles the database requests of the client. So, to handle the get password requests from the vault --
either
-> We need a seperate socket (in pgagroal) to handle this request
or
-> We need to have some information about the vault's address/port from which the requests are coming so as to handle them seperately inside the accept_main_cb.

@jesperpedersen, any suggestions on this.

Also what is the purpose of user in the pgagroal-vault.conf, aren't we passing username in the path of the URL of HTTP request?

@jesperpedersen
Copy link
Collaborator

jesperpedersen commented Feb 21, 2024

pgagroal-vault connects to the management port of pgagroal and uses the GET_PASSWORD command to get the password for the user in question.

The user that is specified in the configuration file is an admin user in pgagroal.

Follow accept_management_cb

@ashu3103 ashu3103 mentioned this pull request Mar 2, 2024
ashu3103 pushed a commit to ashu3103/pgagroal that referenced this pull request Mar 2, 2024
ashu3103 pushed a commit to ashu3103/pgagroal that referenced this pull request Mar 2, 2024
ashu3103 pushed a commit to ashu3103/pgagroal that referenced this pull request Mar 2, 2024
This defines how long a connection will live in seconds
- Add a `max_connection_age` member to `struct configuration`. It will be checked upon returned to the pool, or during idle timeout.
- Add new STATE, TRACKER, and Prometheus metric for `max_connection_age`
- Add documentation for `max_connection_age`
- Add a `start_time` member to `struct connection`. Its implementation is similar to `timestamp`

[agroal#378] Vault Implementaion

[agroal#253][agroal#209] Refactor commands in `pgagroal-cli` and `pgagroal-admin`

Now `pgagroal-cli` has a set of "logically" grouped commands and
subcommands. For example, all the commands related to shutting down
the pooler are under the `shutdown` command, that can operate with
subcommands like `gracefully`, `immediate` or `cancel`.

In order to provide this capability, new functions have been
introduced as utilities:
- `parse_command()` accepts the command line and seek for a command,
possibly its subcommand, and an optional "value" (often the database
or server name).
- `parse_command_simple()` is a wrapper around the above
`parse_command` that shorten the function call line because it does
not require to specify the key and the value (and their defaults).
- `parse_deprecated_command()` does pretty much the same thing but
against the old command. Thanks to this, old commands can still work
and the user will be warned about their deprecation, but the interface
of `pgagroal-cli` is not broken.

All the above functions require to know the offset at which start seeking for
a command, and that depends on the number of options already parsed
via `getopt_long()`. Since the `&option_index` is valued only for long
options, I decided to use the `optind` global value, see
getopt_long(3).
This value is initialized with the "next thing" to seek on the command
line, i.e., the next index on `argv`.

In the case the command accepts an optional database name, the
database value is automatically set to '*' (all databases) in case the
database name is not found on the command line.
Therefore:
   pgagroal-cli flush idle
is equivalent to
   pgagroal-cli flush idle '*'

On the other hand, commands that require a server name get the value
automatically set to "\0" (an invalid server name) in order to "block"
other pieces of code. Moroever, if the server has not been specified,
the command is automatically set to "unknown" so that the help screen
is shown.

The `pgagroal-cli` has a set of `pgagroal_log_debug()` calls whenever
a command is "parsed", so that it is possible to quickly follow the
command line parsing.

Also, since the `pgagroal-cli` exists if no command line arguments
have been specified, the safety check aboutt `argc > 0` around the
command line parsing has been removed.

In the case the user specified an unknown command, she is warned on
stdout before printing the `usage()` help screen.

Deprecated commands are notified to the user via a warning message,
printed on stderr, that provides some hints about the correct usage of
the new command. The warning about deprecated commands is shown only
if the currently running version of the software is greater than the
version the command has been deprecated onto. In particular these
commands have been deprecated since 1.6.

This commit also introduces the command refactoring for `pgagroal-admin` in
a way similar to the work done for `pgagroal-cli`.
New commands are available:
- user <what>
with <what> being <add>, <del>, <edit>, <ls>.

Updated:
- documentation
- shell completions
- help screens
- examples

Close agroal#290 agroal#253

[agroal#381] Changes to `pgagroal-cli` commands

This commit changes two commands in `pgagroal-cli`.

The `is-alive` command is deprecated by means of the `ping`
command. Documentation has been modified accordingly.

The `details` command is now deprecated by the `status details`
one. To achieve this, the `status details` is parsed _before_ the
`status` one (that has not changed at all). In order to better reflect
this change, the internal constant `ACTION_DETAILS` has been renamed
to `ACTION_STATUS_DETAIL`.

Documentation updated accordingly.
Shell completions updated accordingly.

Close agroal#381

[agroal#378] Vault Implementation
ashu3103 added a commit to ashu3103/pgagroal that referenced this pull request Mar 2, 2024
ashu3103 added a commit to ashu3103/pgagroal that referenced this pull request Mar 2, 2024
ashu3103 added a commit to ashu3103/pgagroal that referenced this pull request Mar 2, 2024
ashu3103 added a commit to ashu3103/pgagroal that referenced this pull request Mar 2, 2024
ashu3103 added a commit to ashu3103/pgagroal that referenced this pull request Mar 3, 2024
ashu3103 added a commit to ashu3103/pgagroal that referenced this pull request Mar 4, 2024
ashu3103 added a commit to ashu3103/pgagroal that referenced this pull request Mar 5, 2024
ashu3103 added a commit to ashu3103/pgagroal that referenced this pull request Mar 9, 2024
ashu3103 added a commit to ashu3103/pgagroal that referenced this pull request Mar 14, 2024
ashu3103 added a commit to ashu3103/pgagroal that referenced this pull request Mar 14, 2024
@jesperpedersen
Copy link
Collaborator

Work is being done on #407 now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants