docs: DISA STIG docs review and updates #2011
Conversation
louiseschmidtgen
left a comment
louiseschmidtgen
left a comment
There was a problem hiding this comment.
Thank you for the improvements Kos, I've left a few suggestions:
| You can check whether the current account has an empty password by running | ||
| `passwd --status` and looking for "NP" in the second field of the output. | ||
| To add a password to your account use `passwd` and follow the prompts. |
There was a problem hiding this comment.
Ideally, we should keep the install guide short and to the point. Could you combine this into the previous sentence.
I am thinking something along the lines of "and if the output shows "NP" set a password using passwd.
Wdyt?
There was a problem hiding this comment.
I'm not sure it's a good idea to make this recommendation at all because it's a potential security issue. Accounts that don't have passwords are often missing them for a reason and adding a password changes the security posture (it can be used for things that would have otherwise been blocked). And potentially there's different/better ways to address the issue depending on the environment. Our main goal here is to alert them to the problem; I don't think we need to solve it int his case.
There was a problem hiding this comment.
As a reader, I am reading this note and I do not know what do do about it:
You can check whether the current account has an empty password by running
passwd --statusand looking for "NP" in the second field of the output.
Sure I can check this but what do I need to do if I see a "NP". "NP" does not even tell me if this is a good thing or not. We do say these instructions work for accounts that have passwords so if my account is paswordless what should I do? Does my journey end here?
There was a problem hiding this comment.
I removed the suggestion to add a password.
There was a problem hiding this comment.
I think both points are valid.
I do think "how-to's" are intended to be more actionable.
We could phrase it as more of a suggestion than a definite approach. "Consider setting a password via passwd if this alligns with your security posture."
There was a problem hiding this comment.
I agree it would be better to have a suggestion, but if we're going to make one, we should make a good one that is solid from a security perspective. By setting a password, we're basically enabling password authentication for the account which is generally weaker and not always desirable. Instead, we should find a way to keep an empty password, keep the STIG recommendation to remove nullok support, and still allow SSH key based login. Searching "pam_unix.so allow ssh key login with no password without using nullok" yielded some promising results, but I haven't tried them yet. I'd be very surprised if there's no way to make it work. We have to test it if we're going to suggest it though.
| meaning all user pods must be in user specific namespaces rather than system | ||
| namespaces | ||
|
|
||
| ## Appendix |
There was a problem hiding this comment.
The templates should be moved to a reference page
There was a problem hiding this comment.
Are you saying just the "Templates Information" section or also the alternative/tuning sections? I would just do the former I think, but maybe we need to discuss deeper with @nhennigan. In either case, I would do it under a separate card/PR.
There was a problem hiding this comment.
I like how this page is self contained so I would not put this into a separate reference page. We already have one DISA page for auditing.
I think the title "Appendix" seems too vague/generic. How would you feel about "Advance Configuration"?
There was a problem hiding this comment.
In general, forward references within the same page as we have in this case are candidates for separate pages. It is best we avoid forward references.
There was a problem hiding this comment.
Let's wait on @nhennigan to make the final call. I see the value of having all information in one places but on the other hand the information fits better in a reference from a Diataxis standpoint.
There was a problem hiding this comment.
Agreed we can defer to Niamh. I wouldn't just remove the forward references though.
| to align with the following recommendations: | ||
| bootstrap configuration file for [joining additional control plane | ||
| nodes](#join-control-plane-nodes). Both of these bootstrap files | ||
| apply configuration to align with the following recommendations: |
There was a problem hiding this comment.
| apply configuration to align with the following recommendations: | |
| apply configurations to align with the following recommendations: |
| ### Bootstrap Configuration Template Files | ||
|
|
||
| #### Control plane templates | ||
| #### Control plane bootstrap configuration template file |
There was a problem hiding this comment.
| #### Control plane bootstrap configuration template file | |
| #### Control plane bootstrap template |
This is a bit long for a header
| | [V-254800] | Kubernetes must have a Pod Security Admission control file configured | | ||
|
|
||
| #### Worker node templates | ||
| #### Worker node bootstrap configuration template file |
There was a problem hiding this comment.
| #### Worker node bootstrap configuration template file | |
| #### Worker node bootstrap template |
There was a problem hiding this comment.
We should remove bootstrap too since this is for joining.
| `/var/snap/k8s/common/etc/templates/disa-stig/worker.yaml` is the template | ||
| `/var/snap/k8s/common/etc/templates/disa-stig/worker.yaml` is the | ||
| bootstrap configuration file template |
There was a problem hiding this comment.
I think we can keep the original here, the bootstrap configuration file template is quite the mouth-full.
There was a problem hiding this comment.
Agreed. I think this is another search and replace error. And we're not bootstrapping here either.
There was a problem hiding this comment.
Thanks for the suggestions. I made a few comments/suggestions. Also, I don't understand why the below change is being applied across the board. Was it was search and replace error or am I misunderstanding something?
use the term of "bootstrap configuration file" instead of introducing a new term "template"
https://documentation.ubuntu.com/canonical-kubernetes/latest/snap/reference/config-files/control-plane-join-config/ doesn't call it this, so using "bootstrap" everywhere is too much. Also, the file path already has the term "template" so we're inroducing the term either way. I agree that it's a bit messy to add a new term, but I like that "template" implies that it's a starting point. We could say "example" instead, but then people might think it's not production worthy. We could remove it from the path altogether but that would be more then a doc change and might make people think it's used by default.
| ## Apply Kubernetes STIG | ||
|
|
||
| {{product}} provides [templates](#templates-information) to apply additional | ||
| {{product}} provides [bootstrap configuration template files] |
There was a problem hiding this comment.
These are not just for bootstrap, they're also for joining.
| [bootstrap configuration template | ||
| files](#control-plane-bootstrap-configuration-template-file) | ||
| and [alternative configurations](#alternative-control-plane-configurations). | ||
| Once a node is bootstrapped, changing certain settings is more difficult |
There was a problem hiding this comment.
Again these are not just for bootstrap, but also for joining.
| [bootstrap configuration template | ||
| files](#control-plane-bootstrap-configuration-template-file) | ||
| and [alternative configurations](#alternative-control-plane-configurations). | ||
| Once a node is bootstrapped, changing certain settings is more difficult |
There was a problem hiding this comment.
I suppose "once a node is bootstrapped" may also need clarified to include the idea of joining.
| ``` | ||
|
|
||
| Then join the new worker node using the template: | ||
| Then join the new worker node using the bootstrap configuration template file: |
There was a problem hiding this comment.
Looks like a search and replace mistake. This is the wrong name here.
|
|
||
| This policy may be insufficient or impractical in some situations, in which | ||
| case the templates would need to be adjusted by doing one of the following: | ||
| case the bootstrap configuration file templates |
There was a problem hiding this comment.
Another search and replace error...
|
|
||
| 1. Adjust the `--admission-control-config-file` path in the templates to | ||
| 1. Adjust the `--admission-control-config-file` path in | ||
| the bootstrap configuration file to |
There was a problem hiding this comment.
Another search and replace error...
| 3. Create your own audit policy based on the [upstream instructions] and | ||
| adjust the `--admission-control-config-file` path used in the templates. | ||
| adjust the `--admission-control-config-file` path used in the | ||
| bootstrap configuration file templates. |
There was a problem hiding this comment.
Another search and replace error...
| To comply with rules [V-242402], [V-242403], [V-242461], [V-242462], | ||
| [V-242463],[V-242464] and [V-242465] you must configure the Kubernetes API | ||
| Server audit log. The STIG templates we provide to bootstrap/join control | ||
| Server audit log. The STIG bootstrap configuration file |
There was a problem hiding this comment.
Another search and replace error...
There was a problem hiding this comment.
No these are not search and replace @deuberger please see the comment #2011 (comment)
|
@deuberger as you correctly pointed out the term we are using in our docs is "bootstrap configuration file". You bootstrap a control plane or worker node when you call the bootstrap command or when you prepare a node to join a cluster. If we introduce the new term "template" we have to discuss what is the syntax of this "template". How can a user know how to edit this template? What fields are allowed? What is the syntax? If we say this is a bootstrap configuration then it is obvious what the user can do with this file.
The path has the word template because under this path you may find other files that are not bootstrap configurations. |
|
Apologies @ktsakalozos, I should not have assumed these were search and replace errors. I misunderstood what you were trying to do. Right now per https://documentation.ubuntu.com/canonical-kubernetes/latest/snap/reference/config-files/, we are using 3 terms;
So I don't think we can call it "bootstrap configuration file" everywhere without causing confusion. I haven't seen anywhere in our docs that establishes that "node join" is a type of bootstrapping even though I agree that could make sense. Maybe we should just say "configuration file" everywhere? Or maybe spell out the full name in each case?
Would "example" or "sample" be better? Or maybe we just need to add a note explaining that the templates are sample bootstrap/join config files with the necessary config for DISA STIG?
Like what though? I thought we were just trying to distinguish from the configurations folder in https://github.com/canonical/k8s-snap/tree/main/k8s/resources. Maybe we should just put everything in the configurations folder though? If we introduce the word "template" in the path, but don't use it anywhere, it could lead to confusion. I think it's too late to solve that bit of the problem in 1.34 though. |
|
@deuberger you are right, there are three types of configuration files, not just bootstrap. I followed your suggestion to spell out the full type name in most cases and use "configuration files" when the type is not essential. It is unfortunate that we used the word "template" for the directory name especially since we ended up having the final configuration files there and not the templates used to generate the configuration files. The idea was that we would have templates for different setups (eg CIS, DISA) and based on these templates we would produce the configurations. @louiseschmidtgen @deuberger I tried to address your comments, please have a look. |
| ### Configuration Files | ||
|
|
||
| #### Control plane templates | ||
| #### Control plane & bootstrap configuration files |
There was a problem hiding this comment.
That header sounds a bit weird, e.g. what are control plane files?
Maybe just "Control plane configuration files"?
There was a problem hiding this comment.
Pull Request Overview
This PR improves the DISA STIG documentation by fixing broken links, removing uncertainty from language, and standardizing terminology by replacing "template" with "bootstrap configuration file" and "configuration file" throughout.
- Fixes broken link to Kubernetes STIG viewer
- Standardizes terminology from "template" to "configuration file" terminology
- Removes uncertain language ("Strictly speaking", "probably acceptable") to make documentation more authoritative
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| docs/canonicalk8s/snap/howto/install/fips.md | Removes qualifying language to make FIPS compliance requirements more definitive |
| docs/canonicalk8s/snap/howto/install/disa-stig.md | Replaces "template" terminology with "configuration file" throughout, updates section titles and references, and removes uncertain language |
| docs/canonicalk8s/snap/explanation/security.md | Fixes broken link to Kubernetes STIG viewer by updating URL structure |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
nhennigan
left a comment
nhennigan
left a comment
There was a problem hiding this comment.
There are a few minor things that I would like to change on this page but I have created a card for next pulse and will follow up there. For now, thanks for greatly improving this page!
deuberger
left a comment
deuberger
left a comment
There was a problem hiding this comment.
Thanks @ktsakalozos for additional improvements. I made some minor comments/suggestions. I think there's two other things we still need to address:
-
The way we talk reference the config files in the guide often feels like we're talking about default config files that can be edited in place rather than examples of config files that can be used as input. I think we need to resolve this and be more explicit. The best idea I have is to use the word "example" to make this more clear.
-
Since we haven't implemented an actual "template" feature and we're not using the term in our docs, I think we need to fix the files themselves. Of course 1.34 is already released so we have to worry about compatibility. I would move them and add a symlink from the old location. And then remove the symlinks in main/1.35. This could be done in a separate PR and then this one could be rebased on top of it.
| You can check whether the current account has an empty password by running | ||
| `passwd --status` and looking for "NP" in the second field of the output. | ||
| To add a password to your account use `passwd` and follow the prompts. |
There was a problem hiding this comment.
I agree it would be better to have a suggestion, but if we're going to make one, we should make a good one that is solid from a security perspective. By setting a password, we're basically enabling password authentication for the account which is generally weaker and not always desirable. Instead, we should find a way to keep an empty password, keep the STIG recommendation to remove nullok support, and still allow SSH key based login. Searching "pam_unix.so allow ssh key login with no password without using nullok" yielded some promising results, but I haven't tried them yet. I'd be very surprised if there's no way to make it work. We have to test it if we're going to suggest it though.
| meaning all user pods must be in user specific namespaces rather than system | ||
| namespaces | ||
|
|
||
| ## Appendix |
There was a problem hiding this comment.
Agreed we can defer to Niamh. I wouldn't just remove the forward references though.
| However, in practice, it is common to mix different certified kernel and user space | ||
| library versions, as long as all components are FIPS-certified. |
There was a problem hiding this comment.
| However, in practice, it is common to mix different certified kernel and user space | |
| library versions, as long as all components are FIPS-certified. | |
| However, mixing different kernel and user space versions is supported from a technical perspective and may be acceptable to auditors in some situations. |
There was a problem hiding this comment.
The goal here is to make it clear that it's technically supported, but officially not compliant. On the other hand some users do want to do this and we're trying to clue them in that it might be acceptable though ultimately that's between them and their auditor.
There was a problem hiding this comment.
As @omar-selo pointed out, we know what this means but it is confusing for the user.
We need something simpler: fips is supported on 22.04 and (anticipated) on 24.04. The mix and match and likely the auditors won't mind is a bit unclear.
This may be out of scope for this PR.
There was a problem hiding this comment.
I see there is no consensus over this change. I am reverting it to the original wording. We can improve this part in a future PR as Louise suggested.
| | [V-254800] | Kubernetes must have a Pod Security Admission control file configured | | ||
|
|
||
| #### Worker node templates | ||
| #### Worker node join configuration file |
There was a problem hiding this comment.
Let's please be consistent either we use node-join configuration file everywhere or node join
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 2. Edit | ||
| `/var/snap/k8s/common/etc/configurations/pod-security-admission-baseline.yaml` | ||
| to suit your needs based on the [upstream instructions]. | ||
| 3. Create your own audit policy based on the [upstream instructions] and |
There was a problem hiding this comment.
The text mentions "Create your own audit policy" but this section is about Pod Security Admission control files, not audit policies. This should say "Create your own Pod Security Admission control file" or similar to match the section context.
| 3. Create your own audit policy based on the [upstream instructions] and | |
| 3. Create your own Pod Security Admission control file based on the [upstream instructions] and |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: deuberger <[email protected]>
Co-authored-by: Louise K. Schmidtgen <[email protected]>
ktsakalozos
force-pushed
the
disa-review
branch
from
75b084c to
28c9c52
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| [alternative configurations](#alternative-control-plane-configurations). | ||
| Once a node is bootstrapped, changing certain settings is more difficult | ||
| Before bootstrapping or joining control plane nodes, review the | ||
| example [configuration files](#configuration-files) as well as the |
There was a problem hiding this comment.
The internal link reference #configuration-files does not match the section heading "Configuration Example Files". The anchor should be #configuration-example-files to correctly link to this section.
| example [configuration files](#configuration-files) as well as the | |
| example [configuration files](#configuration-example-files) as well as the |
|
@deuberger here what I have for your last two comments:
Followed your suggestion and called the configuration files examples in key places.
As this would require some changes in the code/testing/validation I created a card for one of our next pulses: https://warthogs.atlassian.net/browse/KU-4838 |
Description
Backport
Should this PR be backported? If so, to which release?
1.34
Checklist
type: title