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

Security focused alternative image #2526

Merged
merged 11 commits into from
Apr 22, 2024
Merged

Conversation

spunkedy
Copy link
Contributor

I finally had time to come back around to getting a hardened version.

Let me know if we would like a different pattern, I tried to follow the conventions in place as best as possible.

This enables a ubi8 image which currently has 0 vulnerabilities

image

  • ubi8
  • FIPS
  • User change

ubi8

Use ubi8 instead of 9 since it is still pending for FIPS

Redhat universal base image allows for a maintained upstrean docker image with security in mind

Fips mode

This does NOT mean that everything that is run here is "FIPS compliant" you still have to configure hackney/req/ssl/ etc to run in these modes, but I figured that was out of scope for this. These changes to the runtime are what is required to be able to enable the others.

Startup fips mode

This introduces a LIVEBOOK_FIPS flag that will show a log and raise an error if there is an issue setting the mode.

User Change

nobody as the user for the execution environment, by default allow for this.

Possible improvements

Possible improvements I see:

  • I do not see any pre built fips compliant images to build the binaries so I put it into the docker itself.
  • Maybe some more documentation around:
    • giving example on how to extend this alternative image for adding system packages
    • using a fips compliant live books
    • how to create dedicated livebook pre built images

@CLAassistant
Copy link

CLAassistant commented Mar 26, 2024

CLA assistant check
All committers have signed the CLA.

@josevalim
Copy link
Contributor

Thank you! A pull request with the LIVEBOOK_FIPS flag will be welcome but the additional Docker images is probably best handled by the community for now. :)

@spunkedy
Copy link
Contributor Author

Thank you! A pull request with the LIVEBOOK_FIPS flag will be welcome but the additional Docker images is probably best handled by the community for now. :)

Should this also be a place for the cuda/rocm versions with the changes to the cuda upstream docker images that have been recently?

Sounds good, are we ok with having the readme for the livebook fips give links to the community versions so that people can understand the constraints of how to be able to use it?

@josevalim
Copy link
Contributor

Should this also be a place for the cuda/rocm versions with the changes to the cuda upstream docker images that have been recently?

I am not sure I get it.

Sounds good, are we ok with having the readme for the livebook fips give links to the community versions so that people can understand the constraints of how to be able to use it?

That's fine by me. Perhaps it is more important to link to the Dockerfile themselves then to the pre-built images because I assume people will want to double check those anyway?

@spunkedy
Copy link
Contributor Author

spunkedy commented Mar 26, 2024

Should this also be a place for the cuda/rocm versions with the changes to the cuda upstream docker images that have been recently?

I am not sure I get it.

I thought the nvidia docker image pipeline wouldn't be supported long term. I could be wrong

https://github.com/NVIDIA/nvidia-docker?tab=readme-ov-file#deprecation-notice

I wasn't sure if you were suggesting that all non debian/hex based livebook images should go to a community version instead of them being maintained as release packaged targets here.

Since XLA recently added ROCm I figured if there was going to be a community maintained downstream artifacts for docker images, to do it in such a way that might look like:

  graph TD;
    livebook_version_release-->community_docker_images;
    community_docker_images-->cuda;
    community_docker_images-->secured;
    community_docker_images-->rocm;
    community_docker_images-->alpine;
Loading

Sounds good, are we ok with having the readme for the livebook fips give links to the community versions so that people can understand the constraints of how to be able to use it?

That's fine by me. Perhaps it is more important to link to the Dockerfile themselves then to the pre-built images because I assume people will want to double check those anyway?

Sounds good, they probably would, not only that they might want to extend. The images might be just a baseline for extending

@josevalim
Copy link
Contributor

Great, so please ping us once this is ready to go from your side. :) Thanks!

@jonatanklosko
Copy link
Member

I thought the nvidia docker image pipeline wouldn't be supported long term. I could be wrong

@spunkedy I think the container toolkit is separate from the Docker images themselves. The images are built from https://gitlab.com/nvidia/container-images/cuda and that is not deprecated.

For ROCm we still don't have precompiled XLA, because there aren't many users and it is maintenance cost to keep the right version matrix and test the builds. If it becomes more stable and popular in the future, then we can consider having Livebook images as well.

@spunkedy
Copy link
Contributor Author

I thought the nvidia docker image pipeline wouldn't be supported long term. I could be wrong

@spunkedy I think the container toolkit is separate from the Docker images themselves. The images are built from https://gitlab.com/nvidia/container-images/cuda and that is not deprecated.

For ROCm we still don't have precompiled XLA, because there aren't many users and it is maintenance cost to keep the right version matrix and test the builds. If it becomes more stable and popular in the future, then we can consider having Livebook images as well.

Thanks! I don't use the CUDA containers, happy to be wrong here!

@spunkedy
Copy link
Contributor Author

@josevalim

Based upon the feedback, I added the documentation, examples, and more descriptions.

Specifically used hex docs for this. I went back and forth between having it be just a sub section of docs/deployment/docker.md and docs/deployment/fips.md but since it isn't technically a docker specific thing I put it in docs/deployment/fips.md

lib/livebook.ex Outdated
if :crypto.enable_fips_mode(true) do
IO.puts("[Livebook] FIPS mode set")
else
raise "Could not set FIPS mode but was asked to"
Copy link
Member

@jonatanklosko jonatanklosko Apr 2, 2024

Choose a reason for hiding this comment

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

Suggested change
raise "Could not set FIPS mode but was asked to"
Livebook.Config.abort!(
"requested FIPS mode via LIVEBOOK_FIPS, but this Erlang installation was compiled without FIPS support"
)

lib/livebook.ex Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Comment on lines 8 to 19
### Error on startup

```bash
export LIVEBOOK_FIPS=true
_build/prod/rel/livebook/bin/livebook start_iex

ERROR! Config provider Config.Reader failed with:
** (RuntimeError) Could not set FIPS mode but was asked to
(livebook 0.13.0-dev) lib/livebook.ex:242: Livebook.config_runtime/0
...

```
Copy link
Member

Choose a reason for hiding this comment

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

Instead of showing the error, I would just tell them that FIPS is enabled with LIVEBOOK_FIPS. When it fails, the error should be clear enough to tell them what's going on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤔

Here are some of my thoughts around each

Hard error:

  • Garuntees the environment in which it is run from the container perspective
  • Going through security audits. showing configuration at the environment level makes it easier

Soft warning:

  • to garuntee the environment, someone would have to do something like a supervisor script that makes sure the environment is that way on startup
  • could give people a false sense of it being turned on if it gets restarted in a node that doesn't support it for some reason or another

Would this be better?

  1. LIVEBOOK_FIPS true/false - IO puts information for logging
  2. LIVEBOOK_FIPS_ENFORCE true/false - Hard error and explain what needs to be true to allow for it

Copy link
Member

Choose a reason for hiding this comment

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

Oh sorry, I wasn't clear. We should definitely exit with clear error. My points was to remove the example from the docs and make the error itself clear enough in case they run into it.

spunkedy and others added 2 commits April 2, 2024 08:32
Co-authored-by: Jonatan Kłosko <jonatanklosko@gmail.com>
Co-authored-by: Jonatan Kłosko <jonatanklosko@gmail.com>
Copy link

github-actions bot commented Apr 2, 2024

Uffizzi Ephemeral Environment deployment-49304

☁️ https://app.uffizzi.com/github.com/livebook-dev/livebook/pull/2526

📄 View Application Logs etc.

What is Uffizzi? Learn more!

Copy link

github-actions bot commented Apr 2, 2024

Uffizzi Preview deployment-49304 was deleted.

@spunkedy
Copy link
Contributor Author

spunkedy commented Apr 2, 2024 via email

@spunkedy
Copy link
Contributor Author

spunkedy commented Apr 2, 2024 via email

@spunkedy
Copy link
Contributor Author

Ugh, the rebase added everything, let me see if I can adjust that

@spunkedy
Copy link
Contributor Author

@jonatanklosko I think I have applied everything you were asking, let me know otherwise.

Copy link
Member

@jonatanklosko jonatanklosko left a comment

Choose a reason for hiding this comment

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

@josevalim feel free to merge!

@jonatanklosko
Copy link
Member

@spunkedy thanks!

@josevalim josevalim merged commit 9eb5cbd into livebook-dev:main Apr 22, 2024
5 of 6 checks passed
@josevalim
Copy link
Contributor

💚 💙 💜 💛 ❤️

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.

4 participants