OAuth Support on Postgres #112
Conversation
2. libpq : Callback hook to send the discovery uri and scope.
|
Thanks for your Pull Request! 😄 This repo on GitHub is just a mirror of our real git repositories though, and can't really handle PRs. 😦 Hopefully you can redo the PR, and direct it to the git.postgresql.org repos? We have a developer guide, if that helps: https://wiki.postgresql.org/wiki/So,_you_want_to_be_a_developer%3F. |
| { | ||
| oauth_state state; | ||
| Port *port; | ||
| const char *discovery_uri; |
There was a problem hiding this comment.
I think we need to keep "issuer" here too. with discover_url optionally provided by the extension
There was a problem hiding this comment.
discovery_uri is provided by extension using the issuer (hba file) in the following manner:
appendStringInfo(&buf, "%s/.well-known/openid-configuration", port->hba->oauth_issuer);
| oauth_exchange(void *opaq, const char *input, int inputlen, | ||
| char **output, int *outputlen, const char **logdetail) | ||
| { | ||
| char *p; |
There was a problem hiding this comment.
A more descriptive name can be helpful here.
| @@ -119,7 +119,7 @@ void | |||
| json_parse_manifest(JsonManifestParseContext *context, char *buffer, | |||
| size_t size) | |||
| { | |||
There was a problem hiding this comment.
are changes to this file here intentional or accidental?
There was a problem hiding this comment.
yes, json changes are required.
| #ifdef FRONTEND | ||
| #include "pqexpbuffer.h" | ||
| #else | ||
| #include "lib/stringinfo.h" | ||
| #include "miscadmin.h" |
There was a problem hiding this comment.
Same question. Are changes to this file intentional? Not clear why we need to change json manipulation for our change here
There was a problem hiding this comment.
SASL exchange between client and server(for sending discovery_uri and scope) uses json object. Few changes are required for now( since it is on master branch). Will need to figure out the optimal changes.
| "OAuth-Issuer", "", 40, | ||
| offsetof(struct pg_conn, oauth_issuer)}, | ||
|
|
||
| {"oauth_client_id", NULL, NULL, NULL, |
There was a problem hiding this comment.
Shouldn't we remove those for now?
| @@ -4034,6 +4067,12 @@ freePGconn(PGconn *conn) | |||
| free(conn->inBuffer); | |||
| free(conn->outBuffer); | |||
| free(conn->rowBuf); | |||
| free(conn->oauth_issuer); | |||
| free(conn->oauth_discovery_uri); | |||
| free(conn->oauth_client_id); | |||
There was a problem hiding this comment.
Is client_id and client_secret needed here? Since the token process is completely delegated to the client
There was a problem hiding this comment.
not required. Will remove this.
Currently tupple iv calculatino is based only on the CTID, which means that when postgres reuses the same address, it also reuses the same IV. This makes our encryption theoretically weaker, in case of some unlikely but not impossible attack scenarios. As an improvement this commit also adds the command id into the calculation. As the (ctid, cid) pair will be basically always unique, this solves the problem. Fixes postgres#112
No description provided.