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

postgresql: link against MIT Kerberos #47494

Closed
wants to merge 1 commit into from

Conversation

cbandy
Copy link
Contributor

@cbandy cbandy commented Dec 4, 2019

  • Have you followed the guidelines for contributing?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing brew install <formula>)?

The GSS libraries provided in macOS segfault when used in applications that fork(). This happens more often in languages like Ruby and Python. Notably, rails console breaks almost immediately.

Discussion: https://postgr.es/m/16041-b44f9931ad91fc3d%40postgresql.org
Related: ged/ruby-pg#311

The libpq formula is unaffected because it does not use --with-gssapi.

@cbandy
Copy link
Contributor Author

cbandy commented Dec 4, 2019

Catalina build failed with the following but it doesn't appear to be related to this change. See also #47458.

running bootstrap script ... 2019-12-04 21:11:05.950 GMT [92394] FATAL:  could not create shared memory segment: No space left on device
2019-12-04 21:11:05.950 GMT [92394] DETAIL:  Failed system call was shmget(key=5432003, size=56, 03600).
2019-12-04 21:11:05.950 GMT [92394] HINT:  This error does *not* mean that you have run out of disk space.  It occurs either if all available shared memory IDs have been taken, in which case you need to raise the SHMMNI parameter in your kernel, or because the system's overall limit for shared memory has been reached.
	The PostgreSQL documentation contains more information about shared memory configuration.
child process exited with exit code 1
initdb: removing contents of data directory "/usr/local/var/postgres"
Warning: The post-install step did not complete successfully
You can try again using `brew postinstall postgresql`

@cbandy
Copy link
Contributor Author

cbandy commented Dec 5, 2019

@BrewTestBot test this please

@Bo98
Copy link
Member

Bo98 commented Dec 6, 2019

On the pull request specifically: if we're going to go this route, would it be easy enough to add a test case for this (unless it's too complex) and add a brief comment describing why krb5 is used over the system library (even just a link to here is fine)?

On the wider issue: the core of the matter is that macOS Kerberos uses libdispatch which should not be used with fork. This isn't the only place where this happens - libsqlite3 is another known problem. The issues have lead to Python recently adding a recommendation to their docs to not use fork on macOS where possible and to instead use spawn methods. exec inside a fork is also an alternative.

It's been an issue for years as far as I know.

@cbandy
Copy link
Contributor Author

cbandy commented Dec 6, 2019

On the pull request specifically: if we're going to go this route, would it be easy enough to add a test case for this (unless it's too complex) and add a brief comment describing why krb5 is used over the system library (even just a link to here is fine)?

The included utilities, e.g. psql, don't fork, so a test case would involve a custom C program or Ruby/Python plus an extra gem/module. Even then the code would appear rather contrived. I can add a comment. Is there a typical phrasing to refer to ... the effectiveness of macOS shims?

On the wider issue: the core of the matter is that macOS Kerberos uses libdispatch which should not be used with fork. This isn't the only place where this happens - libsqlite3 is another known problem. The issues have lead to Python recently adding a recommendation to their docs to not use fork on macOS where possible and to instead use spawn methods. exec inside a fork is also an alternative.

The conclusion thus far on the linked mailing list thread is that libpq is using GSSAPI correctly. From that perspective, this is a macOS bug.

Unfortunately, I don't have the time or energy to pursue this with Apple. If someone from Homebrew is willing to do so, there are small C programs in that thread that work on Linux (with krb5) but fail on macOS (with Kerberos.framework.)

Finally, as a pg gem contributor, I was specifically testing clients and libpq. There was also a concern in that thread about how the server might behave when linked to Kerberos.framework, but I did not try to set up a full Kerberos environment to verify anything on that end. Their recommendation for that was also to use krb5.

@Bo98
Copy link
Member

Bo98 commented Dec 6, 2019

Is there a typical phrasing to refer to ... the effectiveness of macOS shims?

Not really. I'd just say that the system Kerberos.framework crashes when forked and link this pull request. It's probably the easiest way to avoid a paragraph.

I don't have the time or energy to pursue this with Apple

I don't expect you to either. This isn't a new bug.

Finally, as a pg gem contributor, there was also a concern in that thread about how the server might behave when linked to Kerberos.framework

I hope you'll understand when I say that reason is a little irrelevant for homebrew-core without any degree of certainty other than "might". The general policy has been to always use system libraries unless such libraries are proven to be insufficient or problematic. I understand your concern from a downstream POV though.

I do want to focus a bit on this though:

The conclusion thus far on the linked mailing list thread is that libpq is using GSSAPI correctly. From that perspective, this is a macOS bug.

Are they going to take any steps on their side about that? Their official binaries also use Kerberos.framework. This doesn't seem to be Homebrew-specific so if krb5 was preferred by upstream, why aren't they using it for their own builds? Or do they comple it in a way that it is not a problem?

The way Homebrew builds PostgreSQL is very similar to the official binaries at the moment.

@cbandy
Copy link
Contributor Author

cbandy commented Dec 6, 2019

The conclusion thus far on the linked mailing list thread is that libpq is using GSSAPI correctly. From that perspective, this is a macOS bug.

Are they going to take any steps on their side about that? Their official binaries also use Kerberos.framework. This doesn't seem to be Homebrew-specific so if krb5 was preferred by upstream, why aren't they using it for their own builds? Or do they comple it in a way that it is not a problem?

The way Homebrew builds PostgreSQL is very similar to the official binaries at the moment.

I'm afraid I don't know what "official binaries" are. All the links on this page lead away from postgresql.org. It might be worth asking on the mailing list.

@cbandy
Copy link
Contributor Author

cbandy commented Dec 9, 2019

Is there a typical phrasing to refer to ... the effectiveness of macOS shims?

Not really. I'd just say that the system Kerberos.framework crashes when forked and link this pull request. It's probably the easiest way to avoid a paragraph.

Added in amended commit 48e541cb7c5704639c2a65d49e9f1a75055c4ffd.

@cbandy
Copy link
Contributor Author

cbandy commented Dec 10, 2019

@BrewTestBot test this please

@stale
Copy link

stale bot commented Jan 2, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added the stale No recent activity label Jan 2, 2020
@crazymykl
Copy link
Contributor

This breakage is at runtime and widespread. I'd really like to see this merged.

@stale stale bot removed the stale No recent activity label Jan 3, 2020
@SMillerDev
Copy link
Member

CI build is failing though. That has to be fixed first

@cbandy
Copy link
Contributor Author

cbandy commented Jan 4, 2020

@BrewTestBot test this please

The GSS libraries provided in macOS segfault when used in applications
that fork(). This happens more often in languages like Ruby and Python.
Notably, `rails console` breaks almost immediately.

Discussion: https://postgr.es/m/16041-b44f9931ad91fc3d%40postgresql.org
@zbeekman zbeekman added the in progress Stale bot should stay away label Jan 4, 2020
Copy link
Contributor

@zbeekman zbeekman left a comment

Choose a reason for hiding this comment

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

LGTM

I'll give it a few more hours before merging to make sure other maintainers (esp. @Bo98) have a chance to get eyes on this.

@fxcoudert fxcoudert removed the in progress Stale bot should stay away label Jan 6, 2020
@fxcoudert fxcoudert closed this in b5c3775 Jan 6, 2020
@cbandy cbandy deleted the postgresql-kerberos branch January 8, 2020 16:04
@lock lock bot added the outdated PR was locked due to age label Feb 8, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Feb 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants