WIP: pgagroal-vault

[ashu3103]

Continuation of PR #378

@jesperpedersen PTAL on this PR

About this commit

1. Hosted an HTTP server (can be advanced to HTTPS) on the host and port provided in a configuration file pgagroal_vault.conf. This server only listens to HTTP requests from the client, also the address {host, port, admin_user and admin_password} of management port is provided to the vault to connect to the remote management port and forward requests to that port. The admin_user password is provided using a -u flag which is mandatory. Typical .conf looks like this -

   
   [vault]
   host = localhost
   port = 2500

[main] host = localhost port = 2347 user = admin

2. The handelling of URLs is done in the following manner :-

    - If client provide a URL `http://localhost:2500/info`, The server will repy 

HTTP/1.1 200 OK
Content-Type: text/plain

vault is good
```
- If the client provide a URL `http://localhost:2500/users/<_frontend_user_>`, The server will try to connect and request to the management port of pgagroal to fetch the frontend_user password of the `<_frontend_user_>`, If found the server will respond 

```
HTTP/1.1 200 OK
Content-Type: text/plain

Your temporary password is: <_password_>
```   
- else the server will respond

```
HTTP/1.1 200 OK
Content-Type: text/plain

No such user
```   
- In all other cases the server will give `ERROR 404`

3. Now, this server further have to connect to the management port of pgagroal to call GET_PASSWORD method and fetch the frontend user password of the user provided.
4. Meanwhile in pgagroal we have a callback function which will keep rotating frontend passwords of all the frontend_users periodically after a certain timeout (rotate_frontend_password_timeout) [defined in pgagroal.conf file].

[jesperpedersen]

You need to squash all commits.

I don't think we need the http://localhost:2500/info URL... status should be done over a management port.

For http://localhost:2500/users/<_frontend_user_> it should be a 200 response with only the password in the body.

If the user doesn't exist, or in all other cases, response should be 404.

Once this is fixed, send me a PTAL, and I'll take a deeper look...

[ashu3103]

@jesperpedersen, PTAL

[jesperpedersen]

You still have 2 commits for this... you need to properly squash against the latest master branch.

Also, the CI system has failures...

[jesperpedersen]

https://www.git-tower.com/learn/git/faq/git-squash https://github.com/progit/progit2/releases/tag/2.1.421

[ashu3103]

https://www.git-tower.com/learn/git/faq/git-squash https://github.com/progit/progit2/releases/tag/2.1.421

Thanks!!

@jesperpedersen PTAL

[jesperpedersen]

For now, I'll stop here...

Once you have force pushed your branch - look at the commit and go over every single line of change, and think about if it is needed. Is there an existing functions that I can use ? Are the changes made necessary ? And so on...

It is about making this feature work with the fewest changes as possible.

Enhancements can be made in future pull requests - for now, the main goal is to get the basic feature into in the project.

[jesperpedersen]

A few more quick comments.

So, you decided to stick to the explicit struct structure for the vault instead of inheritance ?

[ashu3103]

A few more quick comments.

So, you decided to stick to the explicit struct structure for the vault instead of inheritance ?

If we are doing inheritance, then don't we also have to explicitly provide the configuration file - pgagroal.conf to the pgagroal-vault (which is unneccessry) as there is no shared memory between pgagroal and pgagroal-vault!

So I'm not sure how we can efficiently incorporate inheritance here

[jesperpedersen]

The inheritance is about share the definition between the main_configuration and vault_configuration for the common values.

We can look at it later, for now continue with the separate definition. Lets get the basic patch ready

[jesperpedersen]

Can you run uncrustify.sh on this ?

Also, I still don't understand why there are _bind and _connect functions - the only change is the error message which is a printf

[ashu3103]

Can you run uncrustify.sh on this ?

Sure!

Also, I still don't understand why there are _bind and _connect functions - the only change is the error message which is a printf

Yeah so basically, we are using shared memory shmem for sharing the configuration details of pgagroal among the main process and its worker child processes right and in functions like pgagroal_bind and pgagroal_connect we are utilizing the same concept while binding and connecting in pgagroal main_fds, Since pgagroal_vault is in itself a seperate and independent process with its own configuration structure vault_configuration, so if we reuse the above mentioned functions, we will be getting errors. For example, If we use pgagroal_connect to connect to the pgagroal_vault, we can head into errors in the below mentioned lines as the shmem of pgagroal_vault will have configuration of type struct vault_configuration* and doesn't have fields like config->keep_alive, config->buffer_size, config->non_blocking, One way to resolve this is to add all these fields in vault_configuration also!

int
pgagroal_connect(const char* hostname, int port, int* fd)
{
   ...
   struct configuration* config;

   config = (struct configuration*)shmem;
   ...

         if (config != NULL && config->keep_alive)
   ...

         if (config != NULL && config->nodelay)
   ...
         if (config != NULL)
         {
            if (setsockopt(*fd, SOL_SOCKET, SO_RCVBUF, &config->buffer_size, optlen) == -1)
    ...
            if (setsockopt(*fd, SOL_SOCKET, SO_SNDBUF, &config->buffer_size, optlen) == -1)   
    ...

   /* Set O_NONBLOCK on the socket */
   if (config != NULL && config->non_blocking)
   {
      pgagroal_socket_nonblocking(*fd, true);
   }
   ...

error:
   pgagroal_log_debug("pgagroal_connect: %s", strerror(error));
   ...
}

[jesperpedersen]

It is better to change the function signature of _bind and _connect to pass in the needed parameters instead of making new functions.

I don't see the need for uthash.h - so remove all that too.

Also, change all printf() with proper logging.

[jesperpedersen]

Just leave stuff like keepalive, ... out of the vault configuration for now - and hard code the values in function call

[jesperpedersen]

... There are still uncrustify changes too ...

[jesperpedersen]

You have 2 commits again...

[ashu3103]

You have 2 commits again...

Yeah, I am fixing things

I’ll ping you once done

[ashu3103]

You have 2 commits again...

@jesperpedersen PTAL

[jesperpedersen]

./pgagroal-vault -c pgagroal-vault.conf -u pgagroal_admins.conf 
pgagroal-vault: pgagroal-vault: USERS configuration file not found (file </etc/pgagroal/pgagroal_vault_users.conf>)

[ashu3103]

./pgagroal-vault -c pgagroal-vault.conf -u pgagroal_admins.conf 
pgagroal-vault: pgagroal-vault: USERS configuration file not found (file </etc/pgagroal/pgagroal_vault_users.conf>)

Done!

[jesperpedersen]

You forgot the header for vault.c

[jesperpedersen]

And, then I'll start to dig deeper into the patch

[jesperpedersen]

I'm still gettting

./pgagroal-vault -c pgagroal-vault.conf 
pgagroal-vault: pgagroal-vault: USERS configuration file not found (file </etc/pgagroal/pgagroal_vault_users.conf>)

and you need to update the pgagroal.spec file, plus new files in doc/man

[ashu3103]

I'm still gettting

./pgagroal-vault -c pgagroal-vault.conf 
pgagroal-vault: pgagroal-vault: USERS configuration file not found (file </etc/pgagroal/pgagroal_vault_users.conf>)

Yes you should get this error since -u flag is required.

and you need to update the `pgagroal.spec` file, plus new files in `doc/man`

Okk

[jesperpedersen]

Using

curl http://localhost:8080/trust
curl http://localhost:8080/trust
curl http://localhost:8080/users/trust

./pgagroal-vault -c pgagroal-vault.conf -u pgagroal_admins.conf 
2024-03-19 12:58:08 INFO  vault.c:432 pgagroal-vault 1.7.0: Started on localhost:8080
2024-03-19 12:58:44 TRACE vault.c:517 accept_vault_cb: client address: ::1

2024-03-19 12:58:50 TRACE vault.c:517 accept_vault_cb: client address: ::1

2024-03-19 13:02:44 TRACE vault.c:517 accept_vault_cb: client address: ::1

2024-03-19 13:02:44 DEBUG vault.c:193 connect_pgagroal: Authenticating the remote management access to localhost:2347

2024-03-19 13:02:44 DEBUG vault.c:216 pgagroal-vault: Bad credentials for admin
2024-03-19 13:02:44 FATAL vault.c:145 pgagroal-vault: Couldn't connect to localhost:2347

Only the first 2 URLs should be valid

[jesperpedersen]

There is no output on the client side

[jesperpedersen]

Could be an idea to add a

doc/tutorial/06_vault.md

file...

[ashu3103]

Using

curl http://localhost:8080/trust
curl http://localhost:8080/trust
curl http://localhost:8080/users/trust

./pgagroal-vault -c pgagroal-vault.conf -u pgagroal_admins.conf 
2024-03-19 12:58:08 INFO  vault.c:432 pgagroal-vault 1.7.0: Started on localhost:8080
2024-03-19 12:58:44 TRACE vault.c:517 accept_vault_cb: client address: ::1

2024-03-19 12:58:50 TRACE vault.c:517 accept_vault_cb: client address: ::1

2024-03-19 13:02:44 TRACE vault.c:517 accept_vault_cb: client address: ::1

2024-03-19 13:02:44 DEBUG vault.c:193 connect_pgagroal: Authenticating the remote management access to localhost:2347

2024-03-19 13:02:44 DEBUG vault.c:216 pgagroal-vault: Bad credentials for admin
2024-03-19 13:02:44 FATAL vault.c:145 pgagroal-vault: Couldn't connect to localhost:2347

Check if the management port is enabled

Only the first 2 URLs should be valid

The way I have coded it, only the 3rd should work as it contains ‘/users’.

Also, can you please send the ‘.conf’ files

Thanks

[fluca1978]

Could be an idea to add a

doc/tutorial/06_vault.md

file...

Quite frankly, I think it is a mandatory step!

[jesperpedersen]

Can you rebase ?

[jesperpedersen]

Merged.

Thanks for your contribution !