Thread: Windows: openssl & gssapi dislike each other

I was able to reproduce the gssapi & openssl error on windows. I tried
on PG16 with msvc build system and on PG17 with meson build system.
The error was reproducible when enabling both openssl and gssapi from
the configurations. Turns out that it was due to the conflicting
macros.


"be-secure-openssl.c" tries to prevent this conflict here [1]. But the
error again appears when gssapi is enabled. The file
"be-secure-openssl.c" fails to compile because it has a similar
scenario as explained here [2]. The header libpq.h is indirectly
including libpq-be.h which has a wrong order of including openssl
headers. Header "gssapi.h" indirectly includes "wincrypt.h" and
openssl header should be defined after gssapi includes.

Now this can either be solved by just just undefine the macro defined
by wincrypt.h as done here [3]
```
#ifdef X509_NAME
#undef X509_NAME
#endif
```

Or we should rearrange our headers. Openssl header should be at the
bottom (after the gssapi includes).


I am attaching the patch here in which I rearranged the openssl header
in libpq-be.h


[1]:
https://github.com/postgres/postgres/blob/8ba34c698d19450ccae9a5aea59a6d0bc8b75c0e/src/backend/libpq/be-secure-openssl.c#L46
[2]: https://github.com/openssl/openssl/issues/10307#issuecomment-964155382
[3]: https://github.com/postgres/postgres/blob/00ac25a3c365004821e819653c3307acd3294818/contrib/sslinfo/sslinfo.c#L29


Thanks
Imran Zaheer
Bitnine

On 2024-06-08 Sa 06:22, Imran Zaheer wrote:
> I was able to reproduce the gssapi & openssl error on windows. I tried
> on PG16 with msvc build system and on PG17 with meson build system.
> The error was reproducible when enabling both openssl and gssapi from
> the configurations. Turns out that it was due to the conflicting
> macros.
>
>
> "be-secure-openssl.c" tries to prevent this conflict here [1]. But the
> error again appears when gssapi is enabled. The file
> "be-secure-openssl.c" fails to compile because it has a similar
> scenario as explained here [2]. The header libpq.h is indirectly
> including libpq-be.h which has a wrong order of including openssl
> headers. Header "gssapi.h" indirectly includes "wincrypt.h" and
> openssl header should be defined after gssapi includes.
>
> Now this can either be solved by just just undefine the macro defined
> by wincrypt.h as done here [3]
> ```
> #ifdef X509_NAME
> #undef X509_NAME
> #endif
> ```
>
> Or we should rearrange our headers. Openssl header should be at the
> bottom (after the gssapi includes).
>
>
> I am attaching the patch here in which I rearranged the openssl header
> in libpq-be.h
>
>
> [1]:
https://github.com/postgres/postgres/blob/8ba34c698d19450ccae9a5aea59a6d0bc8b75c0e/src/backend/libpq/be-secure-openssl.c#L46
> [2]: https://github.com/openssl/openssl/issues/10307#issuecomment-964155382
> [3]:
https://github.com/postgres/postgres/blob/00ac25a3c365004821e819653c3307acd3294818/contrib/sslinfo/sslinfo.c#L29
>
>

Let's be consistent and use the #undef from [3]. I did find the comment 
in sslinfo.c slightly confusing until I understood that this was a 
#define clashing with a typedef.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Andrew Dunstan <andrew@dunslane.net> writes:
> On 2024-06-08 Sa 06:22, Imran Zaheer wrote:
>> Now this can either be solved by just just undefine the macro defined
>> by wincrypt.h as done here [3]
>> Or we should rearrange our headers. Openssl header should be at the
>> bottom (after the gssapi includes).

> Let's be consistent and use the #undef from [3].

+1.  Depending on header order is not great, especially when you have
to make it depend on an order that is directly contradictory to
project conventions [0].

            regards, tom lane

[0] https://wiki.postgresql.org/wiki/Committing_checklist#Policies

Hi

I am submitting two new patches. We can undefine the macro at two locations

1). As be-secure-openssl.c [1] was the actual
file where the conflict happened so I undefined the macro here before
the ssl includes. I changed the comment a little to make it understandable.
I am also attaching the error generated with ninja build.

OR

2). Right after the gssapi includes in libpq-be.h


Thanks
Imran Zaheer
Bitnine

[1]:
https://github.com/postgres/postgres/blob/00ac25a3c365004821e819653c3307acd3294818/src/backend/libpq/be-secure-openssl.c#L46

On Sun, Jun 9, 2024 at 7:21 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Andrew Dunstan <andrew@dunslane.net> writes:
> > On 2024-06-08 Sa 06:22, Imran Zaheer wrote:
> >> Now this can either be solved by just just undefine the macro defined
> >> by wincrypt.h as done here [3]
> >> Or we should rearrange our headers. Openssl header should be at the
> >> bottom (after the gssapi includes).
>
> > Let's be consistent and use the #undef from [3].
>
> +1.  Depending on header order is not great, especially when you have
> to make it depend on an order that is directly contradictory to
> project conventions [0].
>
>                         regards, tom lane
>
> [0] https://wiki.postgresql.org/wiki/Committing_checklist#Policies

Hi

On Sun, 9 Jun 2024 at 08:29, Imran Zaheer <imran.zhir@gmail.com> wrote:

Hi

I am submitting two new patches. We can undefine the macro at two locations

1). As be-secure-openssl.c [1] was the actual
file where the conflict happened so I undefined the macro here before
the ssl includes. I changed the comment a little to make it understandable.
I am also attaching the error generated with ninja build.

OR

2). Right after the gssapi includes in libpq-be.h

Thank you for working on this. I can confirm the undef version compiles and passes tests with 16.3.

 

Thanks for testing.

I think I prefer approach 2, which should also allow us to remove the #undef in sslinfo.c so we only need to do this in one place.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Hi

I removed the macro from the sslinfo.c as suggested by Andrew. Then I
was thinking maybe we can undo some other similar code.

I rearranged the headers to their previous position in
be-secure-openssl.c and in fe-secure-openssl.c. I was able to compile
with gssapi and openssl enabled. You can look into the original commits. [1,
2]
Is it ok if we undo the changes from these commits?

I am attaching two new patches.
One with macro guards removed from ssinfo.c.
Second patch will additionally rearrange headers for
be-secure-openssl.c and in fe-secure-openssl.c to their previous
position.

Thanks
Imran Zaheer
Bitnine

[1]: https://github.com/postgres/postgres/commit/1241fcbd7e649414f09f9858ba73e63975dcff64
[2]: https://github.com/postgres/postgres/commit/568620dfd6912351b4127435eca5309f823abde8

On Wed, Jun 12, 2024 at 12:34 AM Dave Page <dpage@pgadmin.org> wrote:
>
>
>
> On Tue, 11 Jun 2024 at 12:22, Andrew Dunstan <andrew@dunslane.net> wrote:
>>
>>
>> On 2024-06-11 Tu 05:19, Dave Page wrote:
>>
>> Hi
>>
>> On Sun, 9 Jun 2024 at 08:29, Imran Zaheer <imran.zhir@gmail.com> wrote:
>>>
>>> Hi
>>>
>>> I am submitting two new patches. We can undefine the macro at two locations
>>>
>>> 1). As be-secure-openssl.c [1] was the actual
>>> file where the conflict happened so I undefined the macro here before
>>> the ssl includes. I changed the comment a little to make it understandable.
>>> I am also attaching the error generated with ninja build.
>>>
>>> OR
>>>
>>> 2). Right after the gssapi includes in libpq-be.h
>>
>>
>> Thank you for working on this. I can confirm the undef version compiles and passes tests with 16.3.
>>
>>
>>
>> Thanks for testing.
>>
>> I think I prefer approach 2, which should also allow us to remove the #undef in sslinfo.c so we only need to do this
inone place. 
>
> OK, well that version compiles and passes tests as well :-)
>
> Thanks!
>
> --
> Dave Page
> pgAdmin: https://www.pgadmin.org
> PostgreSQL: https://www.postgresql.org
> EDB: https://www.enterprisedb.com
>

Hi,


On 2024-06-13 00:12:51 +0900, Imran Zaheer wrote:
> I removed the macro from the sslinfo.c as suggested by Andrew. Then I
> was thinking maybe we can undo some other similar code.

What precisely do you mean by that? Just getting rid of the "ordered include"
of openssl headers in {fe,be}-secure-openssl.h?


> I rearranged the headers to their previous position in
> be-secure-openssl.c and in fe-secure-openssl.c. I was able to compile
> with gssapi and openssl enabled. You can look into the original commits. [1,
> 2]
> Is it ok if we undo the changes from these commits?
>
> I am attaching two new patches.
> One with macro guards removed from ssinfo.c.
> Second patch will additionally rearrange headers for
> be-secure-openssl.c and in fe-secure-openssl.c to their previous
> position.

One thing that concerns me with this is that there are other includes of
gssapi/gssapi.h (e.g. in , which haven't been changed here. ISTM we ought to do apply
the same change to all of those, otherwise we're just waiting for the problem
to re-appear.

I wonder if we should add a src/include/libpq/pg-gssapi.h or such, which could
wrap the entire ifdeferry for gss support. Something like


#ifdef ENABLE_GSS

#if defined(HAVE_GSSAPI_H)
#include <gssapi.h>
#include <gssapi_ext.h>
#else
#include <gssapi/gssapi.h>
#include <gssapi/gssapi_ext.h>
#endif

/*
 * On Windows, <wincrypt.h> includes a #define for X509_NAME, which breaks our
 * ability to use OpenSSL's version of that symbol if <wincrypt.h> is pulled
 * in after <openssl/ssl.h> ... and, at least on some builds, it is.  We
 * can't reliably fix that by re-ordering #includes, because libpq/libpq-be.h
 * #includes <openssl/ssl.h>.  Instead, just zap the #define again here.
 */
#ifdef X509_NAME
#undef X509_NAME
#endif

#endif                                                 /* ENABLE_GSS */

Which'd allow the various places using gss (libpq-be.h, be-gssapi-common.h,
libpq-int.h) to just include pg-gssapi.h and get all of the above without
redundancy?


Another thing that concerns me about this approach is that it seems to assume
that the only source of such conflicting includes is gssapi. What if some
other header pulls in wincrypt.h?  But I can't really see a way out of that...

Greetings,

Andres Freund

On Tue, Jul 9, 2024 at 2:32 AM Andres Freund <andres@anarazel.de> wrote:
>
> Hi,
>
>
> On 2024-06-13 00:12:51 +0900, Imran Zaheer wrote:
> > I removed the macro from the sslinfo.c as suggested by Andrew. Then I
> > was thinking maybe we can undo some other similar code.
>
> What precisely do you mean by that? Just getting rid of the "ordered include"
> of openssl headers in {fe,be}-secure-openssl.h?
>
Hi

I reordered the includes in {fe,be}-secure-openssl.h as they were also placed
there to resolve similar errors and also were contradicting the
project conventions [1].
But looks like it's better to not touch those as they were for future proofing.

>
> > I rearranged the headers to their previous position in
> > be-secure-openssl.c and in fe-secure-openssl.c. I was able to compile
> > with gssapi and openssl enabled. You can look into the original commits. [1,
> > 2]
> > Is it ok if we undo the changes from these commits?
> >
> > I am attaching two new patches.
> > One with macro guards removed from ssinfo.c.
> > Second patch will additionally rearrange headers for
> > be-secure-openssl.c and in fe-secure-openssl.c to their previous
> > position.
>
> One thing that concerns me with this is that there are other includes of
> gssapi/gssapi.h (e.g. in , which haven't been changed here. ISTM we ought to do apply
> the same change to all of those, otherwise we're just waiting for the problem
> to re-appear.
>

Yes this should be better.

> I wonder if we should add a src/include/libpq/pg-gssapi.h or such, which could
> wrap the entire ifdeferry for gss support. Something like
>
>
> #ifdef ENABLE_GSS
>
> #if defined(HAVE_GSSAPI_H)
> #include <gssapi.h>
> #include <gssapi_ext.h>
> #else
> #include <gssapi/gssapi.h>
> #include <gssapi/gssapi_ext.h>
> #endif
>
> /*
>  * On Windows, <wincrypt.h> includes a #define for X509_NAME, which breaks our
>  * ability to use OpenSSL's version of that symbol if <wincrypt.h> is pulled
>  * in after <openssl/ssl.h> ... and, at least on some builds, it is.  We
>  * can't reliably fix that by re-ordering #includes, because libpq/libpq-be.h
>  * #includes <openssl/ssl.h>.  Instead, just zap the #define again here.
>  */
> #ifdef X509_NAME
> #undef X509_NAME
> #endif
>
> #endif                                                 /* ENABLE_GSS */
>
> Which'd allow the various places using gss (libpq-be.h, be-gssapi-common.h,
> libpq-int.h) to just include pg-gssapi.h and get all of the above without
> redundancy?
>
>
> Another thing that concerns me about this approach is that it seems to assume
> that the only source of such conflicting includes is gssapi. What if some
> other header pulls in wincrypt.h?  But I can't really see a way out of that...
>
> Greetings,
>
> Andres Freund

Creating src/include/libpq/pg-gssapi.h can be another great way of
handling these includes. I compiled successfully but couldn't do
proper testing as there is something wrong with my windows env.

And you are right, the approach we are going with right now only
assumes that it's due to the
gssapi as the bug also appeared when building with gssapi (openssl &
gssapi build). What if openssl clashes with
some other lib too which indirectly includes wincrypt.h

For now maybe we can do the future proofing for gssapi & openssl includes
and do testing if openssl clashes with some other lib too.

Thanks
Imran Zaheer

[1]: https://wiki.postgresql.org/wiki/Committing_checklist#Policies
(3rd last point)

> On 24 Jan 2025, at 21:07, Andrew Dunstan <andrew@dunslane.net> wrote:
> On 2025-01-22 We 4:25 AM, Dave Page wrote:

>>  Anyway, no fix was committed as far as I know. I would suggest it should be back-patched as well.
>
> I'm quite partial to the approach suggested upthread by Andres (a separate pg_gssapi.h file). If there's agreement on
thatI'm prepared to go and make it happen, unless Daniel beats me to it. Backpatching also seems reasonable. 

Thanks for the reminder, I also agree that Andres' suggestion is the best
option.  I hacked up a patch but got distracted by the pgcrypto GUC patch for a
bit.  I'll share what I have once I've done a little testing.

--
Daniel Gustafsson