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

(maint) Read system custom facts when running as a user #2231

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

smortex
Copy link
Contributor

@smortex smortex commented Dec 11, 2020

When Facter is run as a non-privileged user, the system custom facts are not available.

Always take system custom facts into account, and if Facter is running as a user who is not root, prepend this user custom facts directories to the list of search directories.

@puppetlabs-jenkins
Copy link
Collaborator

Can one of the admins verify this patch?

@smortex smortex force-pushed the system-custom-facts-as-user branch 2 times, most recently from 1d74af0 to 9a4ce75 Compare December 12, 2020 00:01
@smortex smortex changed the title (maint) Read system custom facts when running as user (maint) Read system custom facts when running as a user Dec 12, 2020
@smortex smortex force-pushed the system-custom-facts-as-user branch from 9a4ce75 to af00fe2 Compare December 12, 2020 00:09
@smortex smortex marked this pull request as ready for review December 12, 2020 00:29
@smortex smortex requested review from a team December 12, 2020 00:29
@BogdanIrimie
Copy link
Contributor

Hi @smortex
The changes suggested in this PR would make Facter 4 incompatible with Facter 3, this is the main reason we have not merged the PR.

@smortex
Copy link
Contributor Author

smortex commented Jan 5, 2021

I understand. The current behavior is quite annoying when working with bolt as a regular user:

romain@zappy ~/Blogreen/dotfiles % bolt apply -e 'warning("country=${facts['country']}: os=${facts['os']['family']}")' -t marvin,agrajag,zappy                                                                                                                  
Starting: install puppet and gather facts on marvin, agrajag, zappy
Finished: install puppet and gather facts with 0 failures in 27.01 sec
Starting: apply catalog on marvin, agrajag, zappy
agrajag: Scope(Class[main]): country=: os=FreeBSD
marvin: Scope(Class[main]): country=: os=FreeBSD
zappy: Scope(Class[main]): country=: os=FreeBSD
Started on agrajag...
Started on marvin...
Started on zappy...
Finished on zappy:
  changed: 0, failed: 0, unchanged: 0 skipped: 0, noop: 0
Finished on marvin:
  changed: 0, failed: 0, unchanged: 0 skipped: 0, noop: 0
Finished on agrajag:
  changed: 0, failed: 0, unchanged: 0 skipped: 0, noop: 0
Finished: apply catalog with 0 failures in 14.55 sec
Successful on 3 targets: marvin,agrajag,zappy
Ran on 3 targets in 41.67 sec
romain@zappy ~/Blogreen/dotfiles %

I also got hit by that when checking if my custom facts where working or not (I though they where not working and I put them in the wrong place, just to discover that I had to run facter as root to see them).

Is this something that we can consider a dead-end (at least until Facter 5), or is there some traction to adjust this? I did not opened an issue on tickets.puppetlabs.com since I considered that was not a breaking change (since I don't see a way this can "break" a setup), is it worth doing so for discussing this topic?

@BogdanIrimie
Copy link
Contributor

@smortex at the moment we cannot go forward with this change, maybe we can discuss it again for the next major release.

The main reason is compatibility. When running with a regular user we only run the external facts for that user, if we make the change from this PR, we would also load the external facts for root. We can see an example of incompatibility if we try to read facts from root directories but don't have access right, facter will give an error message and will exit with 1 and this could break existing users.

bundle exec facter my_external_fact
[2021-01-22 16:26:20.742140 ] ERROR Facter - Failed to handle /etc/facter/facts.d/ldap_facts.json as LegacyFacter::Util::Parser::JsonParser facts: no implicit conversion of nil into String 
123

echo $?                            
1

@CLAassistant
Copy link

CLAassistant commented Sep 1, 2021

CLA assistant check
All committers have signed the CLA.

@smortex
Copy link
Contributor Author

smortex commented Jan 6, 2023

With Puppet 8 at the corner, maybe a major bump of Facter is scheduled and this can be considered again?

@smortex smortex force-pushed the system-custom-facts-as-user branch from af00fe2 to 8429e68 Compare January 6, 2023 19:42
@smortex smortex requested a review from a team as a code owner January 6, 2023 19:42
When Facter is run as a non-privileged user, the system custom facts are
not available.

Always take system custom facts into account, and if Facter is running
as a user who is not root, prepend this user custom facts directories to
the list of search directories.
@smortex smortex force-pushed the system-custom-facts-as-user branch from 8429e68 to c02389c Compare November 2, 2023 15:12
@joshcooper
Copy link
Contributor

Are executable and data (json, yaml, etc) custom facts typically readable by other? I could see the data ones being restricted.

I think it would need to be configurable and facter would need to check whether facts are readable (data) or executable (sh,bash,powershell,etc)

@joshcooper joshcooper added the enhancement New feature or enhancement label Apr 11, 2024
@smortex
Copy link
Contributor Author

smortex commented Apr 12, 2024

Are executable and data (json, yaml, etc) custom facts typically readable by other? I could see the data ones being restricted.

In my case, custom facts are readable by anyone. I don't have executable that dynamically produce facts.

The point of the permissions quite make sense: if some facts are "sensitive" (should not be read by users) they should have appropriate permissions (i.e. 400) and if running facter as an user who has no access to this file, we can indeed just ignore it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants