- 
                Notifications
    You must be signed in to change notification settings 
- Fork 169
Add FIPS support to build process and update .gitignore #2049
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've got a few questions, mostly for my own understanding since I know very little about FIPS compliance
|  | ||
| - name: ci/ensure-build-on-all-platforms | ||
| run: make dist | ||
| - name: ci/setup-node | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to set up the web app's dependencies now when we didn't have to before? I'm actually kind of surprised that that wasn't part of this pipeline before
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's already there - L34-41 :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that up there, but that's in the lint job instead of the dist one. Since this one sets up its own workspace, it looks like we've previously just linted the web app and that we haven't been compiling it.
From what I can tell, this container must already come with Node by default, so we haven't needed to set up a specific version of Node before, and we haven't had to install the NPM dependencies manually because the Makefile is set up to do that if they're missing
| # Build directly in the FIPS container without external script | ||
| # Create local cache directory for CI/ACT compatibility | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For my own understanding, the FIPS build requires us to build the server code in the provided Docker image without running any external scripts, is that right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should any of these changes make it back into the starter template so that any new plugins we make will have FIPS available? I get that we probably don't want this turned on for community plugins and such, but it seems like it would help share it among our plugin builds
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
feel free to inherit them there!
|  | ||
| - name: ci/ensure-build-on-all-platforms | ||
| run: make dist | ||
| - name: ci/setup-node | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's already there - L34-41 :)
        
          
                Makefile
              
                Outdated
          
        
      | GO_FIPS ?= docker run --rm -v $(PWD):/plugin -v $(HOME)/.cache:/root/.cache -w /plugin -e GOFLAGS -e GO111MODULE $(FIPS_IMAGE) go | ||
| GO_BUILD_TAGS_FIPS = fips | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where are these GO_FIPS and GO_BUILD_TAGS_FIPS variables used?
        
          
                Makefile
              
                Outdated
          
        
      | $(FIPS_IMAGE) \ | ||
| /bin/sh -c "\ | ||
| echo 'Go already available: ' && go version && \ | ||
| export GO111MODULE=on && \ | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can remove this line. It's not needed anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a minor comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Stavro!
ff012de    to
    6227f4e      
    Compare
  
    ae56996    to
    4d2173d      
    Compare
  
    - Introduced FIPS compliance build options in the Makefile, allowing for the creation of FIPS-compliant plugins using Docker. - Added new targets for building and bundling FIPS plugins. - Updated the manifest generation to support outputting to a FIPS-specific directory. - Modified .gitignore to include FIPS-related directories and build cache.
- Introduced a new `dist-fips` job in the CI workflow to support building FIPS-compliant plugins. - Added steps for checking out the repository, setting up Go and Node.js environments, caching Node modules, and logging into the Chainguard registry. - Implemented artifact upload for FIPS build outputs, enhancing the CI process for FIPS compliance.
- Modified the FIPS plugin build command to include the `-buildvcs=false` flag, ensuring that version control information is not included in the build output.
- Updated the Go setup in multiple GitHub Actions to enable caching, improving build performance by reusing dependencies across runs.
…S builds in parallel, improving efficiency.
Signed-off-by: Stavros Foteinopoulos <[email protected]>
- Changed the FIPS image to a new version for improved compatibility. - Modified the build command to enable CGO and streamline the Go installation process, ensuring a more efficient build for FIPS-compliant plugins.
Signed-off-by: Stavros Foteinopoulos <[email protected]>
Signed-off-by: Stavros Foteinopoulos <[email protected]>
Signed-off-by: Stavros Foteinopoulos <[email protected]>
Signed-off-by: Stavros Foteinopoulos <[email protected]>
Signed-off-by: Stavros Foteinopoulos <[email protected]>
Signed-off-by: Stavros Foteinopoulos <[email protected]>
Signed-off-by: Stavros Foteinopoulos <[email protected]>
Signed-off-by: Stavros Foteinopoulos <[email protected]>
Signed-off-by: Stavros Foteinopoulos <[email protected]>
Signed-off-by: Stavros Foteinopoulos <[email protected]>
49c0411    to
    29cc4d0      
    Compare
  
    Signed-off-by: Stavros Foteinopoulos <[email protected]>
Signed-off-by: Stavros Foteinopoulos <[email protected]>
Signed-off-by: Stavros Foteinopoulos <[email protected]>
| No need to rebase this branch on top of anything else, this is already branched off the  | 
* Translated using Weblate (German) Currently translated at 100.0% (46 of 46 strings) Translation: Playbooks/server Translate-URL: https://translate.mattermost.com/projects/playbooks/server/de/ * Translated using Weblate (Polish) Currently translated at 100.0% (46 of 46 strings) Translation: Playbooks/server Translate-URL: https://translate.mattermost.com/projects/playbooks/server/pl/ --------- Co-authored-by: jprusch <[email protected]> Co-authored-by: master7 <[email protected]>
* Translated using Weblate (Dutch) Currently translated at 100.0% (46 of 46 strings) Translation: Playbooks/server Translate-URL: https://translate.mattermost.com/projects/playbooks/server/nl/ * Translated using Weblate (Swedish) Currently translated at 100.0% (46 of 46 strings) Translation: Playbooks/server Translate-URL: https://translate.mattermost.com/projects/playbooks/server/sv/ --------- Co-authored-by: Tom De Moor <[email protected]> Co-authored-by: MArtin Johnson <[email protected]>
Co-authored-by: unified-ci-app[bot] <121569378+unified-ci-app[bot]@users.noreply.github.com>
| I just cherry-picked all commits in the v2.4.1...v2.4.2 diff except for a commit that modified the CI pipeline and had conflicts with the changes in this PR. | 
| the reason for not running are the conflicts that need to be resolved @agarciamontoro | 
| I was able to merge master, including the previously ignored commit. Let's see if this works. | 
| It worked! Thank you, @stafot <3 | 
| This PR has been automatically labelled "stale" because it hasn't had recent activity. | 
Bumping mattermost-plugin-playbooks to version 2.5.0 # -----BEGIN PGP SIGNATURE----- # # iQIzBAABCAAdFiEEXpuxF1ERdvMJE+HprmJVCBCXQzQFAmjtaCMACgkQrmJVCBCX # QzSaxxAAvOeeqmYxMETaxOP3EG9kB3+EpYGGVMQZd5NTHtx8bHp1GKjREIgEx/Sp # XEyDDpftrtpvPVogoyuBjbQMhA32Nlo7CLN3884cWpJvgGRiKuqKxAmn5RtOgCV8 # DjjuLOm5llK7TaA1Mi4UC255OTRB/SGsYwmL2pStX9j2NTt8CQEVbz9kVwT5TQJx # olEbHj2GzWceUPtPuB6VMuJTXC/lX8rtDiDeTPhfrxgeyfWAybs2vjgWPdevSHva # KhnramUlRufuI7EuQGmrc7LTti+hOXIVuTfMv7kIKU+mWWIG8cb2flz3hGeZaBP3 # toXbYgItXrZC07NyxEaIcfk4Azo1O/S/4kHe60H7Jj/OiKvX8ufAUHNVttA1Poug # 3w2kQm3cIxYb8uMQB+nNVkWSfIllXwn7gS5JD9ImF3wCHMHkPlCByGjH3UbUNt2Q # WobjpTGbwFZ3H/C3B8jRTWRI00Ctp3bjMkVUdD4TfO312eVpkNrkDutwtws3EdwD # 0VWNY/s0yUB0tApRcmnCba73qjI85MwhHJCjtRMUegDYHYGE10vEhKMcg768T7Y2 # +5UtcU9a/bHjRIq8ihYDB3NZt20TwhCcbj+VhacPPmhStSPmu/4WghivoMJOm1ge # IuHeG3s++GUcW+CoZX6SLFMGqr4mJ1qajhqfnY0T7WS+5+WN9e8= # =fK6n # -----END PGP SIGNATURE----- # gpg: Signature made lun 13 oct 2025 22:59:15 CEST # gpg: using RSA key 5E9BB117511176F30913E1E9AE62550810974334 # gpg: Can't check signature: No public key
| This pull request grants a job id-token: write and runs make dist-all, which could allow an attacker who can inject code via a pull request to execute arbitrary commands and exfiltrate the OIDC token (especially since the workflow uses a secret-based identity input). It also pipes CHAINGUARD_DEV_TOKEN to docker login via stdin in the Makefile, which increases the risk of accidental credential leakage in build logs compared with using a purpose-built, secret-safe action. 
Potential OIDC Token Theft in  | 
| Vulnerability | Potential OIDC Token Theft | 
|---|---|
| Description | The distjob (referred to asbuildin the plan) is grantedid-token: writepermission, which is necessary for OIDC authentication with Chainguard. While thesetup-chainctlaction itself is a trusted third-party action, theidentityinput is sourced fromsecrets.CHAINGUARD_IDENTITY. If this secret were compromised, or if a prior step in the workflow (e.g.,make dist-allwhich executes a Makefile) could be manipulated by an attacker via a pull request to inject malicious commands, the OIDC token could be exfiltrated. Thedisplay-signing-parametersstep also processesgithub.shaandgithub.event.numberwhich are attacker-controlled in a pull request, but the output is only to the workflow log, not directly used in a way that would compromise the OIDC token. The primary risk lies in the execution ofmake dist-allwhich could be influenced by malicious code in a pull request, potentially leading to arbitrary code execution within the job and subsequent OIDC token theft. | 
mattermost-plugin-playbooks/.github/workflows/ci.yaml
Lines 102 to 105 in c140653
| id-token: write # Required for OIDC authentication with Chainguard identity | |
| steps: | |
| - name: ci/checkout-repo | |
| uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 | 
Potential for Information Disclosure of Credentials in Build Logs in Makefile
| Vulnerability | Potential for Information Disclosure of Credentials in Build Logs | 
|---|---|
| Description | The Makefile directly pipes the CHAINGUARD_DEV_TOKENsecret todocker loginvia standard input. While GitHub Actions attempts to mask secrets in logs, relying on this mechanism for secrets passed viastdinin shell commands is not explicitly documented as a secure practice and carries a higher risk of accidental exposure compared to using dedicated actions designed for secure credential handling. If the masking fails or the command output is inadvertently captured, the token could be exposed in build logs. | 
mattermost-plugin-playbooks/Makefile
Lines 266 to 269 in c140653
| echo "$(CHAINGUARD_DEV_TOKEN)" | docker login cgr.dev --username "$(CHAINGUARD_DEV_USERNAME)" --password-stdin; \ | |
| else \ | |
| echo "Warning: No authentication available. FIPS build may fail."; \ | |
| fi; \ | 
All finding details can be found in the DryRun Security Dashboard.
| Merged up to the  | 
Summary
Ticket Link
https://mattermost.atlassian.net/browse/CLD-9414
Checklist