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

Docker user directives #895

Merged
merged 28 commits into from
Jan 5, 2024
Merged

Conversation

mourya-33
Copy link
Contributor

Feature or Bugfix

  • Bugfix

Detail

This PR is to add USER directive to all the Docker files

Relates

#864

Security

Please answer the questions below briefly where applicable, or write N/A. Based on
OWASP 10.

  • Does this PR introduce or modify any input fields or queries - this includes
    fetching data from storage outside the application (e.g. a database, an S3 bucket)? No
    • Is the input sanitized? Yes
    • What precautions are you taking before deserializing the data you consume? N/A
    • Is injection prevented by parametrizing queries? N/A
    • Have you ensured no eval or similar functions are used? N/A
  • Does this PR introduce any functionality or component that requires authorization? N/A
    • How have you ensured it respects the existing AuthN/AuthZ mechanisms? N/A
    • Are you logging failed auth attempts? N/A
  • Are you using or adding any cryptographic features? N/A
    • Do you use a standard proven implementations? Yes
    • Are the used keys controlled by the customer? Where are they stored? N/A
  • Are you introducing any new policies/roles/users? No
    • Have you used the least-privilege principle? How? Yes, we are disabling the containers to run as root user. This change will create a system user without root privileges to run the containers.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

mourya-33 and others added 3 commits December 10, 2023 02:45
# Conflicts:
#	backend/docker/dev/Dockerfile
#	backend/docker/prod/ecs/Dockerfile
#	backend/docker/prod/lambda/Dockerfile
@dlpzx
Copy link
Contributor

dlpzx commented Dec 12, 2023

@dbalintx I have seen your deployment in AWS with the latest changes. I went ahead and tested some items there:

  • CICD succeeds
  • frontend views appear as expected
  • dataset creation --> it fails with the following error 'Error: EACCES: permission denied, mkdtemp '/tmp/jsii-kernel-XXXXXX''
  • ML Studio user creation --> same error

image
image

@anmolsgandhi anmolsgandhi added this to the v2.3.0 milestone Dec 12, 2023
@dbalintx
Copy link
Contributor

dbalintx commented Dec 12, 2023

@dlpzx thank you for checking it

@mourya-33 I think you need to provide permissions to /tmp for the new user as these processes (and probably also further ones) are writting to it

@mourya-33
Copy link
Contributor Author

@dbalintx I added permissions for tmp directory for all the docker files

@noah-paige noah-paige linked an issue Dec 18, 2023 that may be closed by this pull request
@noah-paige
Copy link
Contributor

Tested the latest changes to this PR in an AWS internet facing deployment:

  • CICD succeeds
  • frontend views appear as expected
  • dataset creation --> it fails with the following error 'Error: EACCES: permission denied, mkdtemp '/tmp/jsii-kernel-XXXXXX''
  • ML Studio user creation --> same error

Getting the same errors as before with permission issues @mourya-33

@mourya-33
Copy link
Contributor Author

@noah-paige i updated the permissions for temp dir and tested the dataset and environment creation as well.

@dlpzx
Copy link
Contributor

dlpzx commented Jan 5, 2024

Hi @mourya-33 thanks for the changes, we are almost there!
I have tested it in AWS:

  • CICD succeeds
  • frontend views appear as expected
  • dataset creation succeeds
  • CodePipeline trunk pipeline creation succeeds
  • CDK trunk pipeline creation succeeds

I also tested locally:

  • Docker images create successfully when running docker-compose up

@zsaltys
Copy link
Contributor

zsaltys commented Jan 5, 2024

@dlpzx is this ready to be merged or more testing is needed?

Copy link
Contributor

@dlpzx dlpzx 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! Ready to be merged

@dlpzx dlpzx merged commit 5cd8647 into data-dot-all:main Jan 5, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Docker containers run as root
6 participants