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

build: enable PAM in make deb #166

Merged
merged 5 commits into from
May 10, 2023

Conversation

grondo
Copy link
Contributor

@grondo grondo commented May 4, 2023

The debian package created by make deb was accidentally missing --enable-pam so had no PAM support.

Furthermore, the IMP silently ignores the pam-support = true option in the config when built without PAM.

Add PAM development packages to the suggested requirements for this project and in the debian/control file, and add --enable-pam to the debian package build by default. Add an error if pam-support=true is specified but the IMP was not built with PAM enabled, and add a test for this error.

grondo added 4 commits May 4, 2023 07:22
Problem: The PAM development package is missing from the list
of requirements in the README.

Add the missing requirement. Add a new column to the table to note

that PAM is only required for --enable-pam support.
Problem: The IMP silently ignores `exec.pam-support = true` in the
config file when flux-security was not built with --enable-pam. This
can lead to confusion or worse if an admin erroneously thinks PAM
support is working when it is not.

Generate a fatal error if `exec.pam-support` is enabled when the IMP
is not built with --enable-pam.
Problem: The debian package generated by `make deb` does not include
support for PAM.

Add the --enable-pam option to configure and add libpam0g-dev as a
requesite package so that the packaged flux-security has support
for PAM.
Problem: No test in the testsuite ensures that the IMP fails with
an expected error if pam-support=true is specified in the config,
but flux-security was built without PAM support.

Add a test to t2000-imp-exec.t to ensure this error occurs.
@grondo
Copy link
Contributor Author

grondo commented May 4, 2023

readthedocs builds are failing again with

ImportError: urllib3 v2.0 only supports OpenSSL 1.1.1+, currently the 'ssl' module is compiled with OpenSSL 1.0.2n  7 Dec 2017. See: https://github.com/urllib3/urllib3/issues/2168

I'm not exactly sure how we work around this, seems to be a readthedocs issue. Maybe we can pin urllib<2 in doc/requirements.txt? Or maybe it will fix itself later today.

Problem: ReadTheDocs CI build currently fails with:

 ImportError: urllib3 v2.0 only supports OpenSSL 1.1.1+,
  currently the 'ssl' module is compiled with OpenSSL 1.0.2n  7 Dec 2017
  See: urllib3/urllib3#2168

Workaround the issue until the builders update their OpenSSL
by pinning urllib<2.
@grondo grondo force-pushed the debian-enable-pam branch from a33af49 to d4f48cc Compare May 4, 2023 15:30
@grondo
Copy link
Contributor Author

grondo commented May 4, 2023

For now I've pinned urllib3<2 to get past the build failures.

@codecov
Copy link

codecov bot commented May 4, 2023

Codecov Report

Merging #166 (d4f48cc) into master (4df3d8b) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #166   +/-   ##
=======================================
  Coverage   87.08%   87.08%           
=======================================
  Files          33       33           
  Lines        4079     4082    +3     
=======================================
+ Hits         3552     3555    +3     
  Misses        527      527           
Impacted Files Coverage Δ
src/imp/exec/exec.c 80.76% <100.00%> (+0.37%) ⬆️

Copy link
Member

@garlick garlick left a comment

Choose a reason for hiding this comment

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

Looks good! I tested the deb build on my system and the rlimit warnings went away.

@mergify mergify bot merged commit 2c4115b into flux-framework:master May 10, 2023
@grondo grondo deleted the debian-enable-pam branch May 10, 2023 18:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants