Skip to content
This repository has been archived by the owner on Mar 22, 2024. It is now read-only.

Allow for SPIRE Agent to run as non root user #209

Merged
merged 23 commits into from
Jun 17, 2023

Conversation

kfox1111
Copy link
Contributor

This patch implements a locked down spire-agent along with updating the production example to use it.

fixes: #177

This patch implements a locked down spire-agent along with
updating the production example to use it.

Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
@faisal-memon
Copy link
Contributor

whats the bash container for?

@kfox1111
Copy link
Contributor Author

whats the bash container for?

runs as root:
https://github.com/spiffe/helm-charts/pull/209/files#diff-e860a3dbff565712efc8045d2a42937c0f58f6994939846799346e253a182e75R59-R60

Then chowns the socket dir so the nonroot user the agent runs as can get to it:
https://github.com/spiffe/helm-charts/pull/209/files#diff-e860a3dbff565712efc8045d2a42937c0f58f6994939846799346e253a182e75R52

This limits the amount of root needed to a second or less at the startup time of the agent.

@kfox1111 kfox1111 requested a review from edwbuck as a code owner May 18, 2023 13:47
Co-authored-by: Marco Franssen <marco.franssen@gmail.com>
Signed-off-by: kfox1111 <Kevin.Fox@pnnl.gov>
Copy link
Contributor

@marcofranssen marcofranssen left a comment

Choose a reason for hiding this comment

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

Can't we do this without this without the init container?

This was a pretty recent change on the spire containers, see this commit spiffe/spire@0b91ce2

I think we should be able to do without the bash container now.
We just need to ensure we mount the volumes on these default paths in the container as well keep the sockets in those default locations and ensure the volumemounts use the same UID and GID when mounting the volumes.

https://github.com/spiffe/spire/blob/main/Dockerfile#L28-L54

@kfox1111
Copy link
Contributor Author

kfox1111 commented Jun 1, 2023

The dir permissions are owned by the hostpath and set by kubelet. nothing in the container helps with that I think. The uid/gid is the same as is in the container. I tried anyway and still can't make it work without the bash container.

Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
Co-authored-by: Faisal Memon <fymemon@yahoo.com>
Signed-off-by: kfox1111 <Kevin.Fox@pnnl.gov>
@faisal-memon
Copy link
Contributor

Can we change the PR title to something like "Allow for SPIRE Agent to run as non root user". I think that will look better in release notes and closer to what we are accomplishing here.

@kfox1111 kfox1111 changed the title Enable locked down spire-agent usecase. Allow for SPIRE Agent to run as non root user Jun 6, 2023
@kfox1111
Copy link
Contributor Author

kfox1111 commented Jun 8, 2023

It is defaulting to root when you don't include the production example values.

marcofranssen added a commit that referenced this pull request Jun 16, 2023
This should also give the correct permissions to run rootless.

See https://github.com/spiffe/spire/blob/main/Dockerfile\#L37

More finetuning on the image might probably be better e.g.:

- ensure spire-agent owns /tmp/spire-agent/private
- spire-server owns /tmp/spire-server/private
- spire-server uses a different uid and gid then spire-agent
- the spire-server default-socket path is
  `/tmp/spire-server/private/api.sock`. See the Dockerfile that is
  having 777 permissions.

This is an attempt to implement #345 to prevent we need init container
like we do in #209

Signed-off-by: Marco Franssen <marco.franssen@gmail.com>
Copy link
Contributor

@marcofranssen marcofranssen left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@marcofranssen marcofranssen enabled auto-merge (squash) June 17, 2023 08:32
@marcofranssen marcofranssen merged commit a6dcf26 into main Jun 17, 2023
@marcofranssen marcofranssen deleted the production-agent-rootless branch June 17, 2023 08:32
marcofranssen added a commit that referenced this pull request Jun 19, 2023
This should also give the correct permissions to run rootless.

See https://github.com/spiffe/spire/blob/main/Dockerfile\#L37

More finetuning on the image might probably be better e.g.:

- ensure spire-agent owns /tmp/spire-agent/private
- spire-server owns /tmp/spire-server/private
- spire-server uses a different uid and gid then spire-agent
- the spire-server default-socket path is
  `/tmp/spire-server/private/api.sock`. See the Dockerfile that is
  having 777 permissions.

This is an attempt to implement #345 to prevent we need init container
like we do in #209

Signed-off-by: Marco Franssen <marco.franssen@gmail.com>
marcofranssen added a commit that referenced this pull request Jun 19, 2023
* 57a9320 Add SPIRE 1.7.0 to main readme (#357)
* af36f7c Align the bash image version with other instances for spire-agent (#356)
* c11a8c0 Implement pre-delete hook for graceful delete of spiffe-oidc-discovery-provider (#353)
* a6dcf26 Allow for SPIRE Agent to run as non root user (#209)
* 9cf6049 Allow contributors to run linting easily on local
* e88f7f6 Add configmap annotation to spire-bundle configmap (#351)
* 020bde8 Add support to create a issuer and CA via cert-manager (#342)
* 9d504de Ignore .DS_Store files
* e6b608c Bump spire images to 1.7.0 (#348)
* c97a788 Fix bundle role/rolebinding naming conflict (#333)
* b66077e Bump peter-evans/create-pull-request from 5.0.1 to 5.0.2 (#349)
* d0da864 Add missing metadata to subcharts (#347)
* 4c0a1d5 Allow overriding test images (#186)
* 250fd5d Add missing global values to charts (#311)
* 5d8c907 Dropping k8s versions in CI older than 3, as per readme (#344)
* 8748933 Update upstream-ca-secret.yaml (#341)
* 4e07450 Fix ingress annotations for federation (#337)
* ea09199 Bump actions/checkout from 3.5.0 to 3.5.3
* 87fe198 Merge pull request #331 from edwbuck/key_conventions
* ddc0166 Fix line wrapping.
* 0cae9ce Update project/conventions.md
* cb18255 Update project/conventions.md
* 52e5c24 Upgrade Tornjak to image v1.2.2 (#328)
* 28e2abf Choose a different example for dotted Acronyms.
* d60d68c Added accidentally clipped explicit name guidelines.
* abe9fde Merge branch 'main' into key_conventions
* f6a7b62 Update project/conventions.md
* c4d19db Update project/conventions.md
* cfa9f78 Bump test chart dependencies (#332)
* c3213ab Initial submission of Helm Chart key naming conventions.
* 28c0824 Bump test chart dependencies (#322)
* d333154 Add Makefile for local testing (#327)
* 9fa1ec2 Improve Tornjak backend test (#321)
* 5b779dc Improve Tornjak frontend test (#320)

Signed-off-by: Marco Franssen <marco.franssen@gmail.com>
marcofranssen added a commit that referenced this pull request Jun 19, 2023
This should also give the correct permissions to run rootless.

See https://github.com/spiffe/spire/blob/main/Dockerfile\#L37

More finetuning on the image might probably be better e.g.:

- ensure spire-agent owns /tmp/spire-agent/private
- spire-server owns /tmp/spire-server/private
- spire-server uses a different uid and gid then spire-agent
- the spire-server default-socket path is
  `/tmp/spire-server/private/api.sock`. See the Dockerfile that is
  having 777 permissions.

This is an attempt to implement #345 to prevent we need init container
like we do in #209

Signed-off-by: Marco Franssen <marco.franssen@gmail.com>
marcofranssen added a commit that referenced this pull request Jun 19, 2023
* 57a9320 Add SPIRE 1.7.0 to main readme (#357)
* af36f7c Align the bash image version with other instances for spire-agent (#356)
* c11a8c0 Implement pre-delete hook for graceful delete of spiffe-oidc-discovery-provider (#353)
* a6dcf26 Allow for SPIRE Agent to run as non root user (#209)
* 9cf6049 Allow contributors to run linting easily on local
* e88f7f6 Add configmap annotation to spire-bundle configmap (#351)
* 020bde8 Add support to create a issuer and CA via cert-manager (#342)
* 9d504de Ignore .DS_Store files
* e6b608c Bump spire images to 1.7.0 (#348)
* c97a788 Fix bundle role/rolebinding naming conflict (#333)
* b66077e Bump peter-evans/create-pull-request from 5.0.1 to 5.0.2 (#349)
* d0da864 Add missing metadata to subcharts (#347)
* 4c0a1d5 Allow overriding test images (#186)
* 250fd5d Add missing global values to charts (#311)
* 5d8c907 Dropping k8s versions in CI older than 3, as per readme (#344)
* 8748933 Update upstream-ca-secret.yaml (#341)
* 4e07450 Fix ingress annotations for federation (#337)
* ea09199 Bump actions/checkout from 3.5.0 to 3.5.3
* 87fe198 Merge pull request #331 from edwbuck/key_conventions
* ddc0166 Fix line wrapping.
* 0cae9ce Update project/conventions.md
* cb18255 Update project/conventions.md
* 52e5c24 Upgrade Tornjak to image v1.2.2 (#328)
* 28e2abf Choose a different example for dotted Acronyms.
* d60d68c Added accidentally clipped explicit name guidelines.
* abe9fde Merge branch 'main' into key_conventions
* f6a7b62 Update project/conventions.md
* c4d19db Update project/conventions.md
* cfa9f78 Bump test chart dependencies (#332)
* c3213ab Initial submission of Helm Chart key naming conventions.
* 28c0824 Bump test chart dependencies (#322)
* d333154 Add Makefile for local testing (#327)
* 9fa1ec2 Improve Tornjak backend test (#321)
* 5b779dc Improve Tornjak frontend test (#320)

Signed-off-by: Marco Franssen <marco.franssen@gmail.com>
marcofranssen added a commit that referenced this pull request Jul 19, 2023
This should also give the correct permissions to run rootless.

See https://github.com/spiffe/spire/blob/main/Dockerfile\#L37

More finetuning on the image might probably be better e.g.:

- ensure spire-agent owns /tmp/spire-agent/private
- spire-server owns /tmp/spire-server/private
- spire-server uses a different uid and gid then spire-agent
- the spire-server default-socket path is
  `/tmp/spire-server/private/api.sock`. See the Dockerfile that is
  having 777 permissions.

This is an attempt to implement #345 to prevent we need init container
like we do in #209

Signed-off-by: Marco Franssen <marco.franssen@gmail.com>
marcofranssen added a commit that referenced this pull request Sep 8, 2023
This should also give the correct permissions to run rootless.

See https://github.com/spiffe/spire/blob/main/Dockerfile\#L37

More finetuning on the image might probably be better e.g.:

- ensure spire-agent owns /tmp/spire-agent/private
- spire-server owns /tmp/spire-server/private
- spire-server uses a different uid and gid then spire-agent
- the spire-server default-socket path is
  `/tmp/spire-server/private/api.sock`. See the Dockerfile that is
  having 777 permissions.

This is an attempt to implement #345 to prevent we need init container
like we do in #209

Signed-off-by: Marco Franssen <marco.franssen@gmail.com>
marcofranssen added a commit that referenced this pull request Sep 8, 2023
This should also give the correct permissions to run rootless.

See https://github.com/spiffe/spire/blob/main/Dockerfile\#L37

More finetuning on the image might probably be better e.g.:

- ensure spire-agent owns /tmp/spire-agent/private
- spire-server owns /tmp/spire-server/private
- spire-server uses a different uid and gid then spire-agent
- the spire-server default-socket path is
  `/tmp/spire-server/private/api.sock`. See the Dockerfile that is
  having 777 permissions.

This is an attempt to implement #345 to prevent we need init container
like we do in #209

Signed-off-by: Marco Franssen <marco.franssen@gmail.com>
marcofranssen added a commit that referenced this pull request Sep 15, 2023
This should also give the correct permissions to run rootless.

See https://github.com/spiffe/spire/blob/main/Dockerfile\#L37

More finetuning on the image might probably be better e.g.:

- ensure spire-agent owns /tmp/spire-agent/private
- spire-server owns /tmp/spire-server/private
- spire-server uses a different uid and gid then spire-agent
- the spire-server default-socket path is
  `/tmp/spire-server/private/api.sock`. See the Dockerfile that is
  having 777 permissions.

This is an attempt to implement #345 to prevent we need init container
like we do in #209

Signed-off-by: Marco Franssen <marco.franssen@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Charts don't support configuring to not run as root
4 participants