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

Enforce FIPS compatible Sprig functions for template rendering #2708

Merged
merged 9 commits into from
Mar 4, 2024

Conversation

sukhil-suresh
Copy link
Contributor

@sukhil-suresh sukhil-suresh commented Feb 29, 2024

Change Overview

This is part of the effort to enable a FIPS-compatible Kanister. The github.com/Masterminds/sprig library is used by
Kanister for rendering blueprint templates. Some of the available sprig template functions are not FIPS-compatible and
therefore their usage has been disallowed.

  • Added a FIPS-compatible TxtFuncMap()
  • Updated usages of TxtFuncMap() to use the FIPS-compatible version
  • Updated docs to indicate unsupported functions

Pull request type

  • 🌈 Refactoring
  • 🌻 Feature

Test Plan

  • ⚡ Unit test

Verified the newly added unit tests for template rendering pass:

=== RUN   TestFipsOnlySprigSuite
OK: 2 passed
--- PASS: TestFipsOnlySprigSuite (1.63s)
PASS

@sukhil-suresh sukhil-suresh changed the title Allow only FIPS compatible Sprig functions for template rendering Enforce FIPS compatible Sprig functions for template rendering Feb 29, 2024
@sukhil-suresh sukhil-suresh force-pushed the fipsonly-sprig branch 5 times, most recently from d39d79a to 2332c86 Compare February 29, 2024 17:35
@sukhil-suresh sukhil-suresh marked this pull request as ready for review February 29, 2024 17:36
docs/templates.rst Outdated Show resolved Hide resolved
pkg/ksprig/fipsonly_sprig.go Show resolved Hide resolved
pkg/ksprig/fipsonly_sprig_test.go Show resolved Hide resolved
pkg/ksprig/fipsonly_sprig.go Outdated Show resolved Hide resolved
}

// fipsNonCompliantFuncs is a map of sprig function name to its replacement function.
// Functions identified for Sprig v3.2.3.
Copy link
Contributor

Choose a reason for hiding this comment

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

How are we ensuring that future bumps of sprig are analyzed to see whether anything here needs to be added?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The kanister project uses an older v2.22.0 Sprig version but we have vetted against v3.2.3. I left the comment for easy tracking of when the replacement functions were added but it seems to have triggered fair upgrade concerns :)
Having a runtime error for a mismatch doesn't seem like a great user experience. Wondering if a custom linter rule is possible.

Copy link
Contributor

@kidgilson kidgilson Feb 29, 2024

Choose a reason for hiding this comment

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

Yea, maybe we need to add a file like fips.txt where we have the name of the package from go.mod and the version it's vetted too, and if go.mod != fips.txt then the CI checks throw an error that those packages need to be looked at and the fips.txt version should be updated. A little like the .tool-versions file.

Copy link
Contributor

Choose a reason for hiding this comment

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

How do we ensure we use this file in place of sprig in the future?

Copy link
Contributor Author

@sukhil-suresh sukhil-suresh Feb 29, 2024

Choose a reason for hiding this comment

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

I can only think of exploring a custom linter rule for this purpose. We need an agreement on the solution and if it needs to be addressed in this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yea, I don't think this comment or #2708 (comment) should block this PR. I brought it up as a concern but if we agree to address it moving forward then that's fine.

@sukhil-suresh sukhil-suresh force-pushed the fipsonly-sprig branch 2 times, most recently from 4c953ec to b8e8f39 Compare March 1, 2024 19:32
sukhil-suresh and others added 9 commits March 4, 2024 17:57
Co-authored-by: Pavan Navarathna <6504783+pavannd1@users.noreply.github.com>
Co-authored-by: Pavan Navarathna <6504783+pavannd1@users.noreply.github.com>
Co-authored-by: Pavan Navarathna <6504783+pavannd1@users.noreply.github.com>
@mergify mergify bot merged commit af11989 into kanisterio:master Mar 4, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants