Skip to content

Make default pki directory configurable #61453

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

Merged
merged 1 commit into from
Sep 29, 2022

Conversation

bdrung
Copy link
Contributor

@bdrung bdrung commented Jan 13, 2022

The files in /etc/salt/pki are not configuration files in the sense of the FHS ("local file used to control the operation of a program"). Debian wants to change the default location to /var/lib/salt/pki (to properly follow FHS and to allow setting StateDirectory in the salt master systemd configuration).

Therefore introduce a LIB_STATE_DIR syspaths variable which defaults to CONFIG_DIR, but can be individually customized.

This merge proposal is a follow-up of merge proposal #46277 since that MR was closed.

fixes #3396
Bug-Debian: https://bugs.debian.org/698898

@bdrung bdrung requested a review from a team as a code owner January 13, 2022 20:26
@bdrung bdrung requested review from krionbsd and removed request for a team January 13, 2022 20:26
@bdrung bdrung force-pushed the configurable-pki-dir branch from b222db6 to 266f486 Compare February 3, 2022 13:59
bdrung added a commit to bdrung/salt that referenced this pull request Apr 15, 2022
The files in `/etc/salt/pki` are not configuration files in the sense of
the FHS ("local file used to control the operation of a program").
Debian wants to change the default location to `/var/lib/salt/pki` (to
properly follow FHS and to allow setting StateDirectory in the salt
master systemd configuration).

Therefore introduce a `STATE_DIR` syspaths variable which defaults to
`CONFIG_DIR`, but can be individually customized.

fixes saltstack#3396
Bug-Debian: https://bugs.debian.org/698898
Forwarded: saltstack#61453
Signed-off-by: Benjamin Drung <benjamin.drung@ionos.com>
bdrung added a commit to bdrung/salt that referenced this pull request Apr 15, 2022
The files in `/etc/salt/pki` are not configuration files in the sense of
the FHS ("local file used to control the operation of a program").
Debian wants to change the default location to `/var/lib/salt/pki` (to
properly follow FHS and to allow setting StateDirectory in the salt
master systemd configuration).

Therefore introduce a `STATE_DIR` syspaths variable which defaults to
`CONFIG_DIR`, but can be individually customized.

fixes saltstack#3396
Bug-Debian: https://bugs.debian.org/698898
Forwarded: saltstack#61453
Signed-off-by: Benjamin Drung <benjamin.drung@ionos.com>
bdrung added a commit to bdrung/salt that referenced this pull request Apr 16, 2022
The files in `/etc/salt/pki` are not configuration files in the sense of
the FHS ("local file used to control the operation of a program").
Debian wants to change the default location to `/var/lib/salt/pki` (to
properly follow FHS and to allow setting StateDirectory in the salt
master systemd configuration).

Therefore introduce a `STATE_DIR` syspaths variable which defaults to
`CONFIG_DIR`, but can be individually customized.

fixes saltstack#3396
Bug-Debian: https://bugs.debian.org/698898
Forwarded: saltstack#61453
Signed-off-by: Benjamin Drung <benjamin.drung@ionos.com>
@Ch3LL Ch3LL added the P1 Priority 1 label Sep 19, 2022
twangboy
twangboy previously approved these changes Sep 21, 2022
Copy link
Contributor

@dmurphy18 dmurphy18 left a comment

Choose a reason for hiding this comment

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

@bdrung Given that the default for Salt State files is /srv/salt, wondering if the name STATE_DIR is confusing here since my first reading is of STATE_DIR makes me think of /srv/salt and not anything related to PKI directory.

Would prefer a better name and I know a prime problem with programming is naming. I'd even at first glance would go with PKI_DIR, but would be up for something better.

@bdrung
Copy link
Contributor Author

bdrung commented Sep 22, 2022

I named it STATE_DIR following the naming convention from the FHS: https://refspecs.linuxfoundation.org/FHS_3.0/fhs/ch05s08.html. The PKI is just one example for the "variable state information". Do you still prefer to call it PKI_DIR? In that case only the PKI information can go there.

@dmurphy18
Copy link
Contributor

dmurphy18 commented Sep 23, 2022

@bdrung Only so many words in English :) , understand the issue of STATE, and it's getting used for many contexts. In the case of Salt, STATE usually implies Salt States and trying to prevent confusion for users between the 5.8 definition of state in FHS and Salt state. I had high hopes for FHS back in the late 90s/early 2000s, but other distributions ignored it and here we are today :(.

So PKI_DIR was a suggestion, and I would be fine with it signifying only pki information there, but I see /var/lib/salt/pki is already in use (Ubuntu 22.04 with 3004.1+dfsg-2), so perhaps there may be other uses intended for this of which I am unaware.

I understand that this PR is essentially moving Make-default-pki-directory-configurable.patch into Salt from the Debian fork and you want to not have to change code and operation that you already have, however, I think the use of STATE_DIR while accurate, would be confusing to users, esp after

123 -        "pki_dir": os.path.join(salt.syspaths.CONFIG_DIR, "pki", "minion"),
124 +        "pki_dir": os.path.join(salt.syspaths.STATE_DIR, "pki", "minion"),

I dislike differentiating by case since some OS's don't (burned when porting to Windows from UNIX in a different life), hence wondering about LIB_STATE_DIR | PKI_LIB_STATE_DIR | LIB_PKI_STATE_DIR, since one of those reflects accurately STATE in LIB directory [for PKI], and nobody should confuse that with anything to do with Salt State.

This would imply changes to Debian's fork, but I believe that was a poor confusing choice of term, I would not want to propagate it moving forward.

@bdrung
Copy link
Contributor Author

bdrung commented Sep 27, 2022

Okay. What do you prefer?

  • Just rename STATE_DIR to LIB_STATE_DIR (pki will be in <LIB_STATE_DIR>/pki)
  • or use PKI_DIR which specifies the full PKI directory (pki will be in <PKI_DIR>)?

@dmurphy18
Copy link
Contributor

@bdrung Lets go with LIB_STATE_DIR

@bdrung
Copy link
Contributor Author

bdrung commented Sep 27, 2022

Okay. Changed this merge request to use LIB_STATE_DIR.

dmurphy18
dmurphy18 previously approved these changes Sep 27, 2022
Copy link
Contributor

@Ch3LL Ch3LL left a comment

Choose a reason for hiding this comment

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

just a small doc request

The files in `/etc/salt/pki` are not configuration files in the sense of
the FHS ("local file used to control the operation of a program").
Debian wants to change the default location to `/var/lib/salt/pki` (to
properly follow FHS and to allow setting StateDirectory in the salt
master systemd configuration).

Therefore introduce a `LIB_STATE_DIR` syspaths variable which defaults
to `CONFIG_DIR`, but can be individually customized.

fixes saltstack#3396
Bug-Debian: https://bugs.debian.org/698898
Signed-off-by: Benjamin Drung <benjamin.drung@ionos.com>
@Ch3LL Ch3LL merged commit ad95c4f into saltstack:master Sep 29, 2022
@bdrung bdrung deleted the configurable-pki-dir branch September 29, 2022 19:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P1 Priority 1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PKI files should live in /var/lib
4 participants