Thread
Commits
-
Install properly fe-auth-sasl.h
- 6c9c28316683 15.0 landed
-
Add more sanity checks in SASL exchanges
- 44bd0126c70b 15.0 landed
-
Refactor SASL code with a generic interface for its mechanisms
- 9fd85570d179 15.0 landed
-
[PATCH] Pull general SASL framework out of SCRAM
Jacob Champion <pchampion@vmware.com> — 2021-06-22T22:37:29Z
(This is split off from my work on OAUTHBEARER [1].) Currently, the SASL logic is tightly coupled to the SCRAM implementation. This patch untangles the two, by introducing callback structs for both the frontend and backend. In the original thread, Michael Paquier commented: > + /* TODO: SASL_EXCHANGE_FAILURE with output is forbidden in SASL */ > if (result == SASL_EXCHANGE_SUCCESS) > sendAuthRequest(port, > AUTH_REQ_SASL_FIN, > output, > outputlen); > Perhaps that's an issue we need to worry on its own? I didn't recall > this part.. Yeah, it was non-obvious to me on the first read too. It's a consequence of a couple parts of the SASL spec [2]: > The protocol may include an optional additional data field in this > outcome message. This field can only include additional data when > the outcome is successful. and > SASL mechanism specifications MUST supply the following information: > > [...] > > b) An indication of whether the server is expected to provide > additional data when indicating a successful outcome. If so, > if the server sends the additional data as a challenge, the > specification MUST indicate that the response to this challenge > is an empty response. There isn't a corresponding provision for data for a *failed* outcome, so any such data would have to be sent as a separate, mechanism- specific, challenge. This is what OAUTHBEARER has to do, with an annoying "failure handshake". (Note that our protocol implementation provides an "additional data" field for the initial client response, but *not* for the authentication outcome. That seems odd to me, but it is what it is, I suppose.) Regarding that specific TODO -- I think it'd be good for the framework to fail hard if a mechanism tries to send data during a failure outcome, as it probably means the mechanism isn't implemented to spec. --Jacob [1] https://www.postgresql.org/message-id/d1b467a78e0e36ed85a09adf979d04cf124a9d4b.camel@vmware.com [2] https://datatracker.ietf.org/doc/html/rfc4422
-
Re: [PATCH] Pull general SASL framework out of SCRAM
Michael Paquier <michael@paquier.xyz> — 2021-06-23T07:38:46Z
On Tue, Jun 22, 2021 at 10:37:29PM +0000, Jacob Champion wrote: > Currently, the SASL logic is tightly coupled to the SCRAM > implementation. This patch untangles the two, by introducing callback > structs for both the frontend and backend. The approach to define and have a set callbacks feels natural. +/* Status codes for message exchange */ +#define SASL_EXCHANGE_CONTINUE 0 +#define SASL_EXCHANGE_SUCCESS 1 +#define SASL_EXCHANGE_FAILURE 2 It may be better to prefix those with PG_ as they begin to be published. +/* Backend mechanism API */ +typedef void (*pg_be_sasl_mechanism_func)(Port *, StringInfo); +typedef void *(*pg_be_sasl_init_func)(Port *, const char *, const char *); +typedef int (*pg_be_sasl_exchange_func)(void *, const char *, int, char **, int *, char **); + +typedef struct +{ + pg_be_sasl_mechanism_func get_mechanisms; + pg_be_sasl_init_func init; + pg_be_sasl_exchange_func exchange; +} pg_be_sasl_mech; All this is going to require much more documentation to explain what is the role of those callbacks and what they are here for. Another thing that is not tackled by this patch is the format of the messages exchanged which is something only in SCRAM now. Perhaps it would be better to extract the small-ish routines currently in fe-auth-scram.c and auth-scram.c that we use to grab values associated to an attribute in an exchange message and put them in a central place like an auth-sasl.c and fe-auth-sasl.c. This move could also make sense for the exising init and continue routines for SASL in fe-auth.c. +static int +CheckSCRAMAuth(Port *port, char *shadow_pass, char **logdetail) +{ + return SASL_exchange(&pg_be_scram_mech, port, shadow_pass, logdetail); +} It may be cleaner to live without this thin wrapper. It is a bit strange to have a SCRAM API in a file where we want mostly SASL things (Okay, uaScram does not count as this is assigned after the HBA lookup). Moving any SASL-related things into a separate file may be a cleaner option, especially considering that we have a bit more than the exchange itself, like message handling. +typedef void *(*pg_sasl_init_func)(PGconn *, const char *, const char *); +typedef void (*pg_sasl_exchange_func)(void *, char *, int, char **, int *, bool *, bool *); +typedef bool (*pg_sasl_channel_bound_func)(void *); +typedef void (*pg_sasl_free_func)(void *); + +typedef struct +{ + pg_sasl_init_func init; + pg_sasl_exchange_func exchange; + pg_sasl_channel_bound_func channel_bound; + pg_sasl_free_func free; +} pg_sasl_mech; These would be better into a separate header, with more documentation. It may be more consistent with the backend to name that pg_fe_sasl_mech? It looks like there is enough material for a callback able to handle channel binding. In the main patch for OAUTHBEARER, I can see for example that the handling of OAUTHBEARER-PLUS copied from its SCRAM sibling. That does not need to be tackled in the same patch. Just noting it on the way. > (Note that our protocol implementation provides an "additional data" > field for the initial client response, but *not* for the authentication > outcome. That seems odd to me, but it is what it is, I suppose.) You are referring to the protocol implementation as of AuthenticationSASLFinal, right? > Regarding that specific TODO -- I think it'd be good for the framework > to fail hard if a mechanism tries to send data during a failure > outcome, as it probably means the mechanism isn't implemented to spec. Agreed. That would mean patching libpq to add more safeguards in pg_SASL_continue() if I am following correctly. -- Michael -
Re: [PATCH] Pull general SASL framework out of SCRAM
Jacob Champion <pchampion@vmware.com> — 2021-06-25T23:40:33Z
On Wed, 2021-06-23 at 16:38 +0900, Michael Paquier wrote: > On Tue, Jun 22, 2021 at 10:37:29PM +0000, Jacob Champion wrote: > > Currently, the SASL logic is tightly coupled to the SCRAM > > implementation. This patch untangles the two, by introducing callback > > structs for both the frontend and backend. > > The approach to define and have a set callbacks feels natural. Good, thanks! > +/* Status codes for message exchange */ > +#define SASL_EXCHANGE_CONTINUE 0 > +#define SASL_EXCHANGE_SUCCESS 1 > +#define SASL_EXCHANGE_FAILURE 2 > > It may be better to prefix those with PG_ as they begin to be > published. Added in v2. > +/* Backend mechanism API */ > +typedef void (*pg_be_sasl_mechanism_func)(Port *, StringInfo); > +typedef void *(*pg_be_sasl_init_func)(Port *, const char *, const > char *); > +typedef int (*pg_be_sasl_exchange_func)(void *, const char *, int, > char **, int *, char **); > + > +typedef struct > +{ > + pg_be_sasl_mechanism_func get_mechanisms; > + pg_be_sasl_init_func init; > + pg_be_sasl_exchange_func exchange; > +} pg_be_sasl_mech; > > All this is going to require much more documentation to explain what > is the role of those callbacks and what they are here for. Yes, definitely. If the current approach seems generally workable, I'll get started on that. > Another thing that is not tackled by this patch is the format of the > messages exchanged which is something only in SCRAM now. Perhaps it > would be better to extract the small-ish routines currently in > fe-auth-scram.c and auth-scram.c that we use to grab values associated > to an attribute in an exchange message and put them in a central place > like an auth-sasl.c and fe-auth-sasl.c. This move could also make > sense for the exising init and continue routines for SASL in > fe-auth.c. We can. I recommend waiting for another GS2 mechanism implementation, though. The attribute/value encoding is not part of core SASL (see [1] for that RFC), and OAUTHBEARER is not technically a GS2 mechanism -- though it makes use of a vestigal GS2 header block, apparently in the hopes that one day it might become one. So we could pull out the similarities now, but I'd hate to extract the wrong abstractions and make someone else untangle it later. > +static int > +CheckSCRAMAuth(Port *port, char *shadow_pass, char **logdetail) > +{ > + return SASL_exchange(&pg_be_scram_mech, port, shadow_pass, logdetail); > +} > It may be cleaner to live without this thin wrapper. It is a bit > strange to have a SCRAM API in a file where we want mostly SASL things > (Okay, uaScram does not count as this is assigned after the HBA > lookup). Moving any SASL-related things into a separate file may be a > cleaner option, especially considering that we have a bit more than > the exchange itself, like message handling. Heh, I figured that at ~3500 lines, you all just really wanted the Check* implementations to live in auth.c. :D I can definitely move it (into, say, auth-sasl.c?). I'll probably do that in a second commit, though, since keeping it in place during the refactor makes the review easier IMO. > +typedef void *(*pg_sasl_init_func)(PGconn *, const char *, const char > *); > +typedef void (*pg_sasl_exchange_func)(void *, char *, int, char **, > int *, bool *, bool *); > +typedef bool (*pg_sasl_channel_bound_func)(void *); > +typedef void (*pg_sasl_free_func)(void *); > + > +typedef struct > +{ > + pg_sasl_init_func init; > + pg_sasl_exchange_func exchange; > + pg_sasl_channel_bound_func channel_bound; > + pg_sasl_free_func free; > +} pg_sasl_mech; > These would be better into a separate header, with more > documentation. Can do. Does libpq-int-sasl.h work as a filename? This should not be exported to applications. > It may be more consistent with the backend to name > that pg_fe_sasl_mech? Done in v2. > It looks like there is enough material for a callback able to handle > channel binding. In the main patch for OAUTHBEARER, I can see for > example that the handling of OAUTHBEARER-PLUS copied from its SCRAM > sibling. That does not need to be tackled in the same patch. Just > noting it on the way. OAUTHBEARER doesn't support channel binding -- there's no OAUTHBEARER- PLUS, and there probably won't ever be, given the mechanism's simplicity -- so I'd recommend that this wait for a second GS2 mechanism implementation, as well. > > (Note that our protocol implementation provides an "additional data" > > field for the initial client response, but *not* for the authentication > > outcome. That seems odd to me, but it is what it is, I suppose.) > > You are referring to the protocol implementation as of > AuthenticationSASLFinal, right? Yes, but I misremembered. My statement was wrong -- we do allow for additional data in the authentication outcome from the server. For AuthenticationSASLFinal, we don't distinguish between "no additional data" and "additional data of length zero", which IIRC is a violation of the SASL protocol. That may cause problems with a theoretical future mechanism implementation, but I don't think it affects SCRAM. I believe we *do* distinguish between those cases correctly for the initial client response packet. Sorry for the confusion; let me double-check again when I have fresh eyes at the start of the week, before sending you on a goose chase. > > Regarding that specific TODO -- I think it'd be good for the framework > > to fail hard if a mechanism tries to send data during a failure > > outcome, as it probably means the mechanism isn't implemented to spec. > > Agreed. That would mean patching libpq to add more safeguards in > pg_SASL_continue() if I am following correctly. Right. Thanks for the review! --Jacob [1] https://datatracker.ietf.org/doc/html/rfc5801 -
Re: [PATCH] Pull general SASL framework out of SCRAM
Michael Paquier <michael@paquier.xyz> — 2021-06-26T00:47:02Z
On Fri, Jun 25, 2021 at 11:40:33PM +0000, Jacob Champion wrote: > I can definitely move it (into, say, auth-sasl.c?). I'll probably do > that in a second commit, though, since keeping it in place during the > refactor makes the review easier IMO. auth-sasl.c is a name consistent with the existing practice. > Can do. Does libpq-int-sasl.h work as a filename? This should not be > exported to applications. I would still with the existing naming used by fe-gssapi-common.h, so that would be fe-auth-sasl.c and fe-auth-sasl.h, with the header remaining internal. Not strongly wedded to this name, of course, that just seems consistent. -- Michael
-
Re: [PATCH] Pull general SASL framework out of SCRAM
Jacob Champion <pchampion@vmware.com> — 2021-06-30T22:30:12Z
On Sat, 2021-06-26 at 09:47 +0900, Michael Paquier wrote: > On Fri, Jun 25, 2021 at 11:40:33PM +0000, Jacob Champion wrote: > > I can definitely move it (into, say, auth-sasl.c?). I'll probably do > > that in a second commit, though, since keeping it in place during the > > refactor makes the review easier IMO. > > auth-sasl.c is a name consistent with the existing practice. > > > Can do. Does libpq-int-sasl.h work as a filename? This should not be > > exported to applications. > > I would still with the existing naming used by fe-gssapi-common.h, so > that would be fe-auth-sasl.c and fe-auth-sasl.h, with the header > remaining internal. Not strongly wedded to this name, of course, that > just seems consistent. Done in v3, with a second patch for the code motion. I added a first pass at API documentation as well. This exposed some additional front-end TODOs that I added inline, but they should probably be dealt with independently of the refactor: - Zero-length client responses are legal in the SASL framework; currently we use zero as a sentinel for "don't send a response". - I don't think it's legal for a client to refuse a challenge from the server without aborting the exchange, so we should probably check to make sure that client responses are non-NULL in the success case. --Jacob
-
Re: [PATCH] Pull general SASL framework out of SCRAM
Michael Paquier <michael@paquier.xyz> — 2021-07-05T08:17:38Z
On Wed, Jun 30, 2021 at 10:30:12PM +0000, Jacob Champion wrote: > Done in v3, with a second patch for the code motion. I have gone through that, tweaking the documentation you have added as that's the meat of the patch, reworking a bit the declarations of the callbacks (no need for several typedef gere) and doing some small format changes to make the indentation happy. And that looks pretty good. It is a bit sad that the SCRAM part cannot be completely unplugged from the auth part, because of the call to the free function and the HBA checks, but adding more wrappers to accomodate with that is not really worth it. So I'd like to apply that to clarify this code layer, without the TODOs. - pg_be_scram_get_mechanisms(port, &sasl_mechs); - /* Put another '\0' to mark that list is finished. */ - appendStringInfoChar(&sasl_mechs, '\0'); I was wondering for a couple of seconds if it would not be better to let the last '\0' being set within the callback, but what you have here looks better. - if (!pg_fe_scram_channel_bound(conn->sasl_state)) + if (!conn->sasl || !conn->sasl->channel_bound(conn->sasl_state)) conn->sasl should be set in this code path. This style is safer. The top comment of scram_init() still mentioned pg_be_scram_get_mechanisms(), while it should be scram_get_mechanisms(). PG_MAX_SASL_MESSAGE_LENGTH can stay within auth-sasl.c. > I added a first pass at API documentation as well. This exposed some > additional front-end TODOs that I added inline, but they should > probably be dealt with independently of the refactor: > > - Zero-length client responses are legal in the SASL framework; > currently we use zero as a sentinel for "don't send a response". Check. > - I don't think it's legal for a client to refuse a challenge from the > server without aborting the exchange, so we should probably check to > make sure that client responses are non-NULL in the success case. Hmm. Does the RFCs tell us anything about that? -- Michael
-
Re: [PATCH] Pull general SASL framework out of SCRAM
Jacob Champion <pchampion@vmware.com> — 2021-07-06T18:20:49Z
On Mon, 2021-07-05 at 17:17 +0900, Michael Paquier wrote: > On Wed, Jun 30, 2021 at 10:30:12PM +0000, Jacob Champion wrote: > > Done in v3, with a second patch for the code motion. > > I have gone through that, tweaking the documentation you have added as > that's the meat of the patch, reworking a bit the declarations of the > callbacks (no need for several typedef gere) and doing some small > format changes to make the indentation happy. And that looks pretty > good. Looks very good, thanks! A few comments on the docs changes: > + * Output parameters: > + * > + * buf: A StringInfo buffer that the callback should populate with > + * supported mechanism names. The names are appended into this > + * StringInfo, separated by '\0' bytes. Each name must be null-terminated, not just null-separated. That way the list of names ends with an empty string: name-one\0 <- added by the mechanism name-two\0 <- added by the mechanism \0 <- added by the framework The way it's worded now, I could see some implementers failing to terminate the final name because the framework adds a trailing null already -- but the framework is terminating the list, not the final name. > + * init() > + * > + * Initializes mechanism-specific state for a connection. This > + * callback must return a pointer to its allocated state, which will > + * be passed as-is as the first argument to the other callbacks. > + * free() is called to release any state resources. Maybe say "The free() callback is called" to differentiate it from standard free()? > It is a bit sad that the SCRAM part cannot be completely > unplugged from the auth part, because of the call to the free function > and the HBA checks, but adding more wrappers to accomodate with that > is not really worth it. Yeah. I think that additional improvements/refactoring here will come naturally if clients are ever allowed to negotiate SASL mechanisms in the future. Doesn't need to happen now. > - if (!pg_fe_scram_channel_bound(conn->sasl_state)) > + if (!conn->sasl || !conn->sasl->channel_bound(conn->sasl_state)) > conn->sasl should be set in this code path. This style is safer. It's possible for conn->sasl to be NULL here, say if the client has channel_binding=require but connects as a user with an MD5 secret. The SCRAM TAP tests have one such case. > The top comment of scram_init() still mentioned > pg_be_scram_get_mechanisms(), while it should be > scram_get_mechanisms(). > > PG_MAX_SASL_MESSAGE_LENGTH can stay within auth-sasl.c. Looks good to me. > > - I don't think it's legal for a client to refuse a challenge from the > > server without aborting the exchange, so we should probably check to > > make sure that client responses are non-NULL in the success case. > > Hmm. Does the RFCs tell us anything about that? Just in general terms: > Each authentication exchange consists of a message from the client to > the server requesting authentication via a particular mechanism, > followed by one or more pairs of challenges from the server and > responses from the client, followed by a message from the server > indicating the outcome of the authentication exchange. (Note: > exchanges may also be aborted as discussed in Section 3.5.) So a challenge must be met with a response, or the exchange must be aborted. (And I don't think our protocol implementation provides a client abort message; if something goes wrong, we just tear down the connection.) Thanks, --Jacob -
Re: [PATCH] Pull general SASL framework out of SCRAM
Michael Paquier <michael@paquier.xyz> — 2021-07-07T05:08:20Z
On Tue, Jul 06, 2021 at 06:20:49PM +0000, Jacob Champion wrote: > On Mon, 2021-07-05 at 17:17 +0900, Michael Paquier wrote: > Each name must be null-terminated, not just null-separated. That way > the list of names ends with an empty string: > > name-one\0 <- added by the mechanism > name-two\0 <- added by the mechanism > \0 <- added by the framework > > The way it's worded now, I could see some implementers failing to > terminate the final name because the framework adds a trailing null > already -- but the framework is terminating the list, not the final > name. Good point. I have used ending with '\0' bytes instead. >> + * init() >> + * >> + * Initializes mechanism-specific state for a connection. This >> + * callback must return a pointer to its allocated state, which will >> + * be passed as-is as the first argument to the other callbacks. >> + * free() is called to release any state resources. > > Maybe say "The free() callback is called" to differentiate it from > standard free()? Yes, that could be confusing. Switched to your wording instead. > It's possible for conn->sasl to be NULL here, say if the client has > channel_binding=require but connects as a user with an MD5 secret. The > SCRAM TAP tests have one such case. Indeed. >> Hmm. Does the RFCs tell us anything about that? > > Just in general terms: > >> Each authentication exchange consists of a message from the client to >> the server requesting authentication via a particular mechanism, >> followed by one or more pairs of challenges from the server and >> responses from the client, followed by a message from the server >> indicating the outcome of the authentication exchange. (Note: >> exchanges may also be aborted as discussed in Section 3.5.) > > So a challenge must be met with a response, or the exchange must be > aborted. (And I don't think our protocol implementation provides a > client abort message; if something goes wrong, we just tear down the > connection.) Thanks. At the same time, section 3.5 also says that the client may send a message to abort. So one can interpret that the client has also the choice to abort without sending a response back to the server? Or I am just interpreting incorrectly the use of "may" in this context? -- Michael
-
Re: [PATCH] Pull general SASL framework out of SCRAM
Jacob Champion <pchampion@vmware.com> — 2021-07-07T15:07:14Z
On Wed, 2021-07-07 at 14:08 +0900, Michael Paquier wrote: > On Tue, Jul 06, 2021 at 06:20:49PM +0000, Jacob Champion wrote: > > On Mon, 2021-07-05 at 17:17 +0900, Michael Paquier wrote: > > > > > Hmm. Does the RFCs tell us anything about that? > > > > Just in general terms: > > > > > Each authentication exchange consists of a message from the client to > > > the server requesting authentication via a particular mechanism, > > > followed by one or more pairs of challenges from the server and > > > responses from the client, followed by a message from the server > > > indicating the outcome of the authentication exchange. (Note: > > > exchanges may also be aborted as discussed in Section 3.5.) > > > > So a challenge must be met with a response, or the exchange must be > > aborted. (And I don't think our protocol implementation provides a > > client abort message; if something goes wrong, we just tear down the > > connection.) > > Thanks. At the same time, section 3.5 also says that the client may > send a message to abort. So one can interpret that the client has > also the choice to abort without sending a response back to the > server? Or I am just interpreting incorrectly the use of "may" in > this context? That's correct. But the client may not simply ignore the challenge and keep the exchange open waiting for a new one, as pg_SASL_continue() currently allows. That's what my TODO is referring to. --Jacob
-
Re: [PATCH] Pull general SASL framework out of SCRAM
Michael Paquier <michael@paquier.xyz> — 2021-07-08T07:27:19Z
On Wed, Jul 07, 2021 at 03:07:14PM +0000, Jacob Champion wrote: > That's correct. But the client may not simply ignore the challenge and > keep the exchange open waiting for a new one, as pg_SASL_continue() > currently allows. That's what my TODO is referring to. I have been looking more at your three points from upthread and feasted on the SASL RFC, as of: - Detection that no output is generated on PG_SASL_EXCHANGE_FAILURE for the backend. - Handling of zero-length messages in the frontend. The backend handles that already, and SCRAM would complain if sending such messages, but I can see why you'd want to allow that for other mechanisms. - Making sure that a mechanism generates a message in the middle of the exchange in the frontend. I agree that this looks like an improvement in terms of the expectations behind a SASL mechanism, so I have done the attached to strengthen a bit all those checks. However, I don't really see a point in back-patching any of that, as SCRAM satisfies with its implementation already all those conditions AFAIK. So that's an improvement of the current code, and it fits nicely with the SASL refactoring for the documentation of the callbacks. Thoughts? -- Michael
-
Re: [PATCH] Pull general SASL framework out of SCRAM
Jacob Champion <pchampion@vmware.com> — 2021-07-09T23:31:48Z
On Thu, 2021-07-08 at 16:27 +0900, Michael Paquier wrote: > I agree that this looks like an improvement in terms of the > expectations behind a SASL mechanism, so I have done the attached to > strengthen a bit all those checks. However, I don't really see a > point in back-patching any of that, as SCRAM satisfies with its > implementation already all those conditions AFAIK. Agreed. > Thoughts? LGTM, thanks! > + * outputlen: The length (0 or higher) of the client response buffer, > + * invalid if output is NULL. nitpick: maybe "ignored" instead of "invalid"? --Jacob
-
Re: [PATCH] Pull general SASL framework out of SCRAM
Michael Paquier <michael@paquier.xyz> — 2021-07-11T04:16:05Z
On Fri, Jul 09, 2021 at 11:31:48PM +0000, Jacob Champion wrote: > On Thu, 2021-07-08 at 16:27 +0900, Michael Paquier wrote: >> + * outputlen: The length (0 or higher) of the client response buffer, >> + * invalid if output is NULL. > > nitpick: maybe "ignored" instead of "invalid"? Thanks, applied as 44bd012 after using your suggestion. Another thing I noticed after more review is that the check in fe-auth.c to make sure that a message needs to be generated if the exchange is not completed yet has no need to depend on "success", only "done". -- Michael
-
Re: [PATCH] Pull general SASL framework out of SCRAM
Jacob Champion <pchampion@vmware.com> — 2021-07-13T00:01:46Z
On Sun, 2021-07-11 at 13:16 +0900, Michael Paquier wrote: > On Fri, Jul 09, 2021 at 11:31:48PM +0000, Jacob Champion wrote: > > On Thu, 2021-07-08 at 16:27 +0900, Michael Paquier wrote: > > > + * outputlen: The length (0 or higher) of the client response buffer, > > > + * invalid if output is NULL. > > > > nitpick: maybe "ignored" instead of "invalid"? > > Thanks, applied as 44bd012 after using your suggestion. Thanks! > Another thing I noticed after more review is that the check in > fe-auth.c to make sure that a message needs to be generated if the > exchange is not completed yet has no need to depend on "success", only > "done". Ah, right. I think the (!done && !success) case is probably indicative of an API smell, but that's probably something to clean up in a future pass. --Jacob
-
Re: [PATCH] Pull general SASL framework out of SCRAM
Michael Paquier <michael@paquier.xyz> — 2021-07-13T00:47:15Z
On Tue, Jul 13, 2021 at 12:01:46AM +0000, Jacob Champion wrote: > Ah, right. I think the (!done && !success) case is probably indicative > of an API smell, but that's probably something to clean up in a future > pass. Yeah, agreed. I feel that it would should be cleaner to replace those two booleans with a status enum or a bitmask. -- Michael
-
RE: [PATCH] Pull general SASL framework out of SCRAM
Mikhail Kulagin <m.kulagin@postgrespro.ru> — 2021-07-13T09:41:27Z
Hello, hackers! I got an error while building one of the extensions. /home/mkulagin/pg-install/postgresql-master/include/internal/libpq-int.h:44:10: fatal error: fe-auth-sasl.h: No such file or directory #include "fe-auth-sasl.h" ^~~~~~~~~~~~~~~~ I think the new fe-auth-sasl.h file should be installed too. Correction proposal in the attached file (but I'm not sure that fix of Install.pm is correct). Regards, Mikhail A. Kulagin PostgresPro -
Re: [PATCH] Pull general SASL framework out of SCRAM
Michael Paquier <michael@paquier.xyz> — 2021-07-13T10:31:56Z
On Tue, Jul 13, 2021 at 12:41:27PM +0300, Mikhail Kulagin wrote: > I got an error while building one of the extensions. > /home/mkulagin/pg-install/postgresql-master/include/internal/libpq-int.h:44:10: fatal error: fe-auth-sasl.h: No such file or directory > #include "fe-auth-sasl.h" > ^~~~~~~~~~~~~~~~ Right. I overlooked the fact that libpq-int.h is installed. > I think the new fe-auth-sasl.h file should be installed too. > Correction proposal in the attached file (but I'm not sure that fix > of Install.pm is correct). That looks correct to me. I'll check that tomorrow. -- Michael
-
Re: [PATCH] Pull general SASL framework out of SCRAM
Jacob Champion <pchampion@vmware.com> — 2021-07-13T22:41:01Z
On Tue, 2021-07-13 at 19:31 +0900, Michael Paquier wrote: > On Tue, Jul 13, 2021 at 12:41:27PM +0300, Mikhail Kulagin wrote: > > I got an error while building one of the extensions. > > /home/mkulagin/pg-install/postgresql-master/include/internal/libpq-int.h:44:10: fatal error: fe-auth-sasl.h: No such file or directory > > #include "fe-auth-sasl.h" > > ^~~~~~~~~~~~~~~~ > > Right. I overlooked the fact that libpq-int.h is installed. Thanks for catching that Mikhail. > > I think the new fe-auth-sasl.h file should be installed too. > > Correction proposal in the attached file (but I'm not sure that fix > > of Install.pm is correct). > > That looks correct to me. I'll check that tomorrow. Looks right to me too. I'm currently rebuilding my Windows dev environment so I haven't been able to double-check that piece of it. Just to make sure -- do we want to export the fe-auth-sasl.h header as opposed to forward-declaring the pg_fe_sasl_mech struct? Is the use case for libpq-int.h just "here, have at the internals, and if you break it then it's on you"? --Jacob
-
Re: [PATCH] Pull general SASL framework out of SCRAM
Jacob Champion <pchampion@vmware.com> — 2021-07-13T23:52:10Z
On Tue, 2021-07-13 at 22:41 +0000, Jacob Champion wrote: > On Tue, 2021-07-13 at 19:31 +0900, Michael Paquier wrote: > > On Tue, Jul 13, 2021 at 12:41:27PM +0300, Mikhail Kulagin wrote: > > > > > > I think the new fe-auth-sasl.h file should be installed too. > > > Correction proposal in the attached file (but I'm not sure that fix > > > of Install.pm is correct). > > > > That looks correct to me. I'll check that tomorrow. > > Looks right to me too. I'm currently rebuilding my Windows dev > environment so I haven't been able to double-check that piece of it. (Confirmed that this patch works for me on Windows.) Thanks, --Jacob
-
Re: [PATCH] Pull general SASL framework out of SCRAM
Michael Paquier <michael@paquier.xyz> — 2021-07-14T01:42:46Z
On Tue, Jul 13, 2021 at 10:41:01PM +0000, Jacob Champion wrote: > Just to make sure -- do we want to export the fe-auth-sasl.h header as > opposed to forward-declaring the pg_fe_sasl_mech struct? Installing fe-auth-sasl.h has the advantage to make the internals of the callbacks available to applications playing with the internals. For SASL, it makes things easier to define new mechanisms out of core. > Is the use > case for libpq-int.h just "here, have at the internals, and if you > break it then it's on you"? Yes, it can be useful for applications willing to use the internals of libpq, like in forks. There is no guarantee that this will not break across major version upgrades, so that's up to the user to fix things once they play with the internals. -- Michael