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

2021 11 27/libsubid symbols #449

Merged
merged 3 commits into from
Dec 7, 2021

Conversation

hallyn
Copy link
Member

@hallyn hallyn commented Nov 27, 2021

No description provided.

Signed-off-by: Serge Hallyn <serge@hallyn.com>
Rename libsubid symbols to all be prefixed with subid_.

Don't export anything but the subid_*.

Closes shadow-maint#443

Signed-off-by: Serge Hallyn <serge@hallyn.com>
@hallyn
Copy link
Member Author

hallyn commented Nov 28, 2021

@brauner @rbalint @ikerexxe So this does change the names of the exported variables. Are we ok with that? Do we maybe want #defines in subid.h from the old to the new names?

@jubalh
Copy link
Member

jubalh commented Nov 28, 2021

I hope it's ok to ask a quick question here: what does the --enable-shared actually do?

@hallyn
Copy link
Member Author

hallyn commented Nov 28, 2021

I hope it's ok to ask a quick question here: what does the --enable-shared actually do?

It sets ENABLE_SHARED which is hooked to libsubid/Makefile.am:

2 if ENABLE_SHARED
3 libsubid_la_LDFLAGS = -Wl,-soname,libsubid.so.@LIBSUBID_ABI@
4 -shared -version-info @LIBSUBID_ABI_MAJOR@
5 endif

It pre-exists libsubid, but I don't believe it's doing anything else.
configure.ac always does both AM_ENABLE_STATIC and AM_ENABLE_SHARED.

@rbalint
Copy link
Contributor

rbalint commented Nov 28, 2021

@hallyn IMO it is perfectly fine to rename the symbols together with the major version bump. Since the library is not widely used yet IMO there is no need to provide the mapping #define-s. From Debian's POV the library has never been even in experimental, thus it is not known in the official archive.

@ikerexxe
Copy link
Collaborator

I don't think that configuring the API names with #defines is a good idea and I prefer to keep them "hardcoded". @alexey-tikhonov, what do you think about this change? Will it affect Fedora or RHEL?

@alexey-tikhonov
Copy link

Podman is already using existing API upstream and in Fedora/RHEL.
@giuseppe, what do you think?

@hallyn, what is the motivation to rename functions?

@giuseppe
Copy link
Contributor

Podman is already using existing API upstream and in Fedora/RHEL.
@giuseppe, what do you think?

yes, Podman is already using these functions so it would be a breaking change: https://github.com/containers/storage/blob/main/pkg/idtools/idtools_supported.go#L34-L38

@hallyn
Copy link
Member Author

hallyn commented Nov 29, 2021

Podman is already using existing API upstream and in Fedora/RHEL. @giuseppe, what do you think?

@hallyn, what is the motivation to rename functions?

The motivation is to have a prefix for all the functions it exports, to avoid conflicts.

@hallyn
Copy link
Member Author

hallyn commented Nov 29, 2021

Podman is already using existing API upstream and in Fedora/RHEL.
@giuseppe, what do you think?

yes, Podman is already using these functions so it would be a breaking change: https://github.com/containers/storage/blob/main/pkg/idtools/idtools_supported.go#L34-L38

Ok, thanks. So perhaps I could add them as aliases for now, with the intent of removing the old names "eventually"?

@giuseppe
Copy link
Contributor

Ok, thanks. So perhaps I could add them as aliases for now, with the intent of removing the old names "eventually"?

sure thanks, that would help.

Thinking more of it: would it be possible to export the ABI major number as part of subid.h? In this way, I could address the problem directly in the Cgo code with some macros (not sure if there is a smarter way in Go to have the equivalent of AC_CHECK_FUNCS) so you won't have to worry about carrying the alias. What do you think?

@alexey-tikhonov
Copy link

yes, Podman is already using these functions so it would be a breaking change: https://github.com/containers/storage/blob/main/pkg/idtools/idtools_supported.go#L34-L38

Ok, thanks. So perhaps I could add them as aliases for now, with the intent of removing the old names "eventually"?

When do you plan next upstream release?

@rbalint
Copy link
Contributor

rbalint commented Nov 30, 2021

Ok, thanks. So perhaps I could add them as aliases for now, with the intent of removing the old names "eventually"?

sure thanks, that would help.

Thinking more of it: would it be possible to export the ABI major number as part of subid.h? In this way, I could address the problem directly in the Cgo code with some macros (not sure if there is a smarter way in Go to have the equivalent of AC_CHECK_FUNCS) so you won't have to worry about carrying the alias. What do you think?

This is redundant and I have not seen this anywhere. I'm sure there is something in Go that helps with major shared library version bumps. If you need something at run time, you can certainly use dlsym from Go.

@giuseppe
Copy link
Contributor

I have not seen this anywhere

there are a bunch of .*_ABI_VERSION definitions under /usr/include.

Said so, if it looks redundant, I will find some other way to deal with the renaming.

@rbalint
Copy link
Contributor

rbalint commented Dec 1, 2021

I have not seen this anywhere

there are a bunch of .*_ABI_VERSION definitions under /usr/include.

Said so, if it looks redundant, I will find some other way to deal with the renaming.

I'm sorry, you are righ and I did not get what you meant.
I suggest setting not only the major version, but minor and micro to be able to check for it like it is done for glib: GLIB_CHECK_VERSION(2, 68, 0).

@hallyn
Copy link
Member Author

hallyn commented Dec 1, 2021

When do you plan next upstream release?

Possibly Dec 11.

@hallyn
Copy link
Member Author

hallyn commented Dec 5, 2021

@giuseppe how do you feel about hallyn@0c9f641 ?

I can still add the symbols if you like, but arguably it makes things uglier by forcing us to deal with that again later.

@hallyn hallyn force-pushed the 2021-11-27/libsubid-symbols branch from 52302da to f81708a Compare December 5, 2021 14:02
Signed-off-by: Serge Hallyn <serge@hallyn.com>
@hallyn hallyn force-pushed the 2021-11-27/libsubid-symbols branch from f81708a to 0c9f641 Compare December 5, 2021 14:03
@giuseppe
Copy link
Contributor

giuseppe commented Dec 5, 2021

@giuseppe how do you feel about hallyn@0c9f641 ?

@hallyn thanks for amending the patch! That works for me and I think there is no need to keep the symbol name.

@hallyn
Copy link
Member Author

hallyn commented Dec 7, 2021

Merging now. Hoping to do a release this weekend. Please let me know if you run into any problems, or need me to wait (to do a release) on more testing.

@hallyn hallyn merged commit abb879f into shadow-maint:master Dec 7, 2021
@giuseppe
Copy link
Contributor

giuseppe commented Dec 9, 2021

Please let me know if you run into any problems, or need me to wait (to do a release) on more testing.

no problems from my side, I've tested the current master version with containers/storage#1086 and it works well.

debarshiray pushed a commit to debarshiray/toolbox that referenced this pull request Jan 28, 2023
On enterprise FreeIPA set-ups, the subordinate user and group IDs are
provided by SSSD's sss plugin for the GNU Name Service Switch (or NSS)
functionality of the GNU C Library.  They are not listed in /etc/subuid
and /etc/subgid.  Therefore, its necessary to use libsubid.so to check
the subordinate ID ranges.

The CGO interaction with libsubid.so is loosely based on 'readSubid' in
github.com/containers/storage/pkg/idtools [1].

However, unlike 'readSubid', this code considers the absence of any
range (ie., nRanges == 0) to be an error as well.

More importantly, this code uses dlopen(3) and friends to dynamically
load the symbols from libsubid.so, instead of linking to libsubid.so at
build-time and having the dependency noted in the /usr/bin/toolbox
binary.  This is done because libsubid.so itself depends on several
other shared libraries, and indirect dependencies can't be influenced
by the RUNPATH [2] embedded in the /usr/bin/toolbox binary [3].  Hence,
when the binary is used inside Toolbx containers (eg., as the entry
point), those indirect dependencies won't be picked from the host's
runtime against which the binary was built.  This can render the binary
useless due to ABI compatibility issues.  Using dlopen(3) avoids this
problem, especially because libsubid.so is only used when running on the
host.

Care was taken to not load and link libsubid.so twice to separately
validate the subordinate ID ranges for the user and the group.  Note
that libsubid_init() must be passed a FILE pointer for logging.
Otherwise, it will create it's own for logging, and there's no way to
close it during dlclose(3).

Version 4 of the libsubid.so API/ABI [4] was released in Shadow 4.10,
which is newer than the versions shipped on RHEL 8 and Debian 10 [5],
and even that newer version had some problems [6].  Therefore, support
for older versions, with the relevant workarounds, is necessary.
Fortunately, the oldest that needs to be support is Shadow 4.9 because
that's when libsubid.so was introduced [7].

Note that SUBID_ABI_VERSION was only introduced with version 4 of the
libsubid.so API/ABI released in Shadow 4.10 [8].  The first release of
libsubid.so in Shadow 4.9 already had an ABI version of 3.0.0 [9], since
it was bumped a few times during development, so that's what's assumed
when SUBID_ABI_VERSION is absent.

This code doesn't set the public variables Prog and shadow_logfd that
older Shadow versions used to expect for logging, because from Shadow
4.9 onwards there's a separate function [4,10] to specify these.  This
can be changed if there are libsubid.so versions in the wild that really
do need those public variables to be set.

Finally, ISO C99 is required because of the use of <stdbool.h> in the
libsubid.so API.

Some changes by Debarshi Ray.

[1] https://github.com/containers/storage/blob/main/pkg/idtools/idtools_supported.go

[2] https://man7.org/linux/man-pages/man8/ld.so.8.html

[3] Commit 6063eb2
    containers#821

[4] Shadow commit 32f641b207f6ddff
    shadow-maint/shadow@32f641b207f6ddff
    shadow-maint/shadow#443

[5] https://packages.debian.org/source/buster/shadow

[6] Shadow commit 79157cbad87f42cd
    shadow-maint/shadow@79157cbad87f42cd
    shadow-maint/shadow#465

[7] Shadow commit 0a7888b1fad613a0
    shadow-maint/shadow@0a7888b1fad613a0
    shadow-maint/shadow#154

[8] Shadow commit 0c9f64140852e8d5
    shadow-maint/shadow@0c9f64140852e8d5
    shadow-maint/shadow#449

[9] Shadow commit 3d670ba7ed58f910
    shadow-maint/shadow@3d670ba7ed58f910
    shadow-maint/shadow#339

[10] Shadow commit 2b22a6909dba60d
     shadow-maint/shadow@2b22a6909dba60d
     shadow-maint/shadow#325

containers#1074

Signed-off-by: Martin Jackson <martjack@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants