Create scorecard.yml (OpenSSF)

[planetf1]

trafficstars

Adds a github action to run the OpenSSF scorecard tool, and posts results to security->code scanning

Fixes #1706

This is a new CI action. It's only effect is

• small amount of resource to run the workflow
• creates items under Security->code scanning, visible to those with 'write' access in the repo

No branch protection rules are being changed to require successful completion of this action

No additional testing is required

Example output is shown in the referenced issue

• [NO] Does this PR change the input/output behaviour of a cryptographic algorithm (i.e., does it change known answer test values)? (If so, a version bump will be required from x.y.z to x.(y+1).0.)
• [NO] Does this PR change the list of algorithms available -- either adding, removing, or renaming? Does this PR otherwise change an API? (If so, PRs in fully supported downstream projects dependent on these, i.e., oqs-provider and OQS-OpenSSH will also need to be ready for review and merge by the time this is merged.)

As per https://github.com/ossf/scorecard it's possible some additional permissions may be needed (depending on branch rotection setup). These can be dealt with as a second iteration

This PR also does not attempt to address any findings from the scorecard scan. It's purely activating the tool so that we can review and act on those findings subsequently.

Draft for

• testing
• minor changes/comments cleanup

[planetf1]

@baentsch many thanks for the comments!

[planetf1]

PR updated - only generates .sarif file, and runs on both PRs and merges to main

• can review the sarif file and decide if we want to push to code scanning results in this repo
• I can squash the commits when we're ready for final review if that's preferred

[planetf1]

Example sarif file can be found as attachment on https://github.com/open-quantum-safe/liboqs/actions/runs/8066088271?pr=1708

[planetf1]

In the scan above, we have two categories of issues reported:

• Token Permissions - recommending some refinements to permissions for the github actions

To fix these carries some risk of trial/error, but ultimately should not change results, it's just being more explicit about what we expect an action to do

• Pinned Dependencies - recommending pinning the versions of actions we use by hash.

This requires making the version of github actions we use explicit, so that a release can always be regenerated. The downside is maintenance to update even within minor versions. I see the reasoning for it, it may work better alongside an automated tool like dependabot (I know it works on major minor versions, not sure on commit hash)

I didn't notice other issues in the sarif (viewed with the sarif viewer in vscode, from ms dev labs)

[planetf1]

Rebased, to review current findings and correct.

[planetf1]

Update on this PR - good things

• added pinned SHAs for github actions - this seems to work, and pass the scorecard tests
• added explicit permissions for github actions - this again seems to work & pass tests
• updated the scorecard yaml itself as I noticed 2 deprecations (one was in checkout... looking at that across the repo should be a distinct action)

There are however still a few errors relating to freezing of pip dependencies

• a build failure due to some dependencies in copy_from_upstream being pinned, but not all (? may need recursive definition?) - a warning from scorecard where the unix build explicitly does a 'pip install'. this needs to add SHAs (I'd only done requirements.txt)
  • a warning from scorecard with requirements.txt explicit SHAs' Either a bug, or may relate to the build failure - ie I only pinned what was specified, but not the full resolved list of dependencies.....

[planetf1]

Pip requirements.txt install was failing in the build since once shas are used, ALL dependencies need to be listed (resursively) and a few were missing.

fixed by

• installing the existing dependencies into a clear virtual environment (running python -m venv /tmp/env2; . /tmp/env2/bin/activate; pip install -r requirements.txt`)
• freezing the dependencies to get a seed list pip freeze > requirements.txt2
• running the following script to get the actual shas

cat requirements.txt | while read line                                                                                                                                            <<<
do
hashin $line
done

• finally discarding the original & using the new requirements.txt2

Also tested loading the new requirements.txt into a clean pip venv which works fine.

[planetf1]

@baentsch the checks are nearly clean, with the only OSSF negative points (score 8/10) caused by:

        run: env HOMEBREW_NO_AUTO_UPDATE=1 brew install ninja && pip3 install --break-system-packages pytest pytest-xdist pyyaml

We have the same in Windows. It's not detected, but should fix in the same way

Aside: ossf scorecard doesn't know/check brew packages, which of course can also change the results!

To address the ossf report, we would need to specify hashes. I did this for our import script (see above) which uses requirements.txt

I'm think this is the cleanest way to do here too - and in future may be easier to maintain if we use dependabot?

I could create the required 'requirements.txt' somewhere and refactor the above. The big question would be where in the source tree. it could go in the .github/workflows directory, but is this clutter? somewhere else? What do you think?

[planetf1]

The mac builds are using Python 3.12.x whilst the Windows build is using 3.9.x -- this is why the dependencies resolve differently. These versions are the standard, supplied, python versions for the respective runners

I initially used a common file, then a slight variation from a test machine running windows but with python 3.12

Additional consideration - for consistent results, having a managed, specific version of python may be advisable.

[planetf1]

Changed approach to build requirements txt to

pip install pip-tools
pip-compile requirements.txt --generate-hashes -o requirements_new.txt

This is done in a clean venv, and has been tested on macos (3.12) and windows (3.9)

[planetf1]

This is now ready for review:

• Scorecard results are clean with all reported warnings clean
• results are only reported in sarif file attached to action

Follow-on activity (after merge)

• Open a new PR to reset the schedule (ie 'on:' to OSSF recommendations) - but get the main PR in first.
• Add information about scorecard & tools to aid in pinning dependencies to documentation (discuss where)
• report-back to dev call on change
• roll out similar changes to other repositories

[ryjones]

this is really cool! thanks for doing the heavy lift

[baentsch]

This is now ready for review:

* Scorecard results are clean with all reported warnings clean

Very good, thanks. Also good to know that no software changes are/were required to get to this "green state".

* results are only reported in sarif file attached to action

Follow-on activity (after merge)

* Open a new PR to reset the schedule (ie 'on:' to OSSF recommendations) - but get the main PR in first.

Before doing that, I'd like to

• get confirmation about which (downstream) project or user needs this; this would not be required if you can guarantee that this tooling will never generate alarms that may cause work to us (i.e., be 100% automated). Looking at the current changes I have doubts about this (i.e., that this has the potential to require manual interventions) but would like to hear your thoughts on this, @planetf1
• how to ascertain that any warnings this tool may generate will not be considered blockers for use or progress, i.e.,
• how people can themselves fix any errors this tooling may highlight (aka documentation; see below)

* Add information about scorecard & tools to aid in pinning dependencies to documentation (discuss where)

Such documentation seems prudent to have in this PR already (also see my single questions) to avoid a situation in which things fail for users and they don't know where to look for documentation to "dig themselves" out of the proverbial hole this may have plunged them into. My suggestion would be to place this in a file "PROCEDURES.md" in the "docs" directory. It already should contain three sub items:

• DCO (why needed/benefits, e.g., with reference to project charter), e.g., to reference in PRs failing because of this
• CBOM (why needed/benefits with documentation how to handle CI errors if so triggered, avoiding future instances of for example this).
• Scorecard (why needed/benefits with documentation how to handle CI or local execution errors if so triggered)

* report-back to dev call on change

Good proposal, but this feature must be understandable (by reading documentation) by anyone not attending dev calls too.

* roll out similar changes to other repositories

Suggest to wait with this until a) this feature is proven to not cause any procedural complications to technical progress (quick-and-easy PRs, etc.) b) we have finally decided which projects to carry forward at which support/maintenance level

[dstebila]

Thanks Nigel for getting this started! And thanks Michael for a thorough analysis. We will definitely need some hand-holding as this lands and the first few times it pops up during a PR.

[planetf1]

To answer the remaining comments:

• It's not possible to guarantee 100% that there will be no work from this change for the following reasons

• If we want to add an additional python module, or change the versions we are using, we have to do more than list the dependency or the version, but now also need to add the sha, and list the entire set of dependencies after transitive resolution. Mitigation: document process
• If the python version supplied by github were to upgrade python we could find a transitive dependency changes, leading to a build break until above was done. Mitigation: use the github-python plugin to use a relatively fixed version of python (much much smaller change with minor fixes) << as in earlier comment

Both of the above seem reasonable to me given the intent of the scorecard recommended approach is to avoid unknown changes to what is being produced - for example not just a new, but a modified/hacked/illicit version of a library.

• We have two things

• a) The scorecard check - this is passive - it is only giving us a score, so will not progress, though we should monitor & plan action if we regress
• b) the change to the pip install - this is within the PR - but see previous bullet point

• How people can fix? - documentation

PROCEDURES.md - I can add changes there, but would suggest within this PR it focusses on scorecard, and we raise additional issue/issues to handle DCO and SBOM (which I can also help with). Agree with the point about making sure the change is understandable from the contents of the repo/docs (I could also add a pointer/info in the requirements file itself for extra clarity)

Rolling out to repo - makes sense we allow a little time (a few weeks?) after the change is merged into liboqs (suggest a checkpoint at the TSC?) before working on the other repos -- and of course they should each go through their own pr approval). As to which repos, ultimately any that has any supported code within.

[planetf1]

So in summary my plan (if reviewers agree) is to:

• [x] Add setup-python into each build script (using 3.12)
• [x] remove/rename the additional requirements file
• [x] Add a docs/PROCEDURES.md describing how the requirements file can be built with hashes etc, and link back from requirements file or script as makes sense
• [x] Raise issue to document DCO process
• [x] Raise issue to document CBOM process

Will return to TSC/rollout once this PR is merged.

[baentsch]

Please go ahead with your plan, @planetf1 . But please also keep in mind that this must be as easy-to-use as possible for developers as well as users, e.g., with any python version and any CI system (and/or add suitable documentation catering for all possible problems you may already have heard about).

[baentsch]

We will definitely need some hand-holding as this lands and the first few times it pops up during a PR.

This should never pop up during a PR as I understand it, but it may pop up during development (prior to PR) and in weekly tests (if some hashes are wrong), right @planetf1 ?

The latter doesn't concern me as it's just "decoration" for now. The former is what I'd like to avoid at all cost: Making developers scratch their heads why the "requirements.txt" file fails to properly load or having to install a specific python version just to satisfy this tool: Things like this should be running in the background raising their head only when something's really wrong and otherwise get out of the way of development progress.

Yes, this "progress-over-process" priority will change eventually as and when and if OQS takes on specific product properties. But we have still not decided which components and which properties those are, let alone their priorities. For example, I'd consider a serious code review for critical liboqs parts or #1540 or #1474 or #1366 or #167 or #1215 much more urgent than this OSSF badge (don't get me wrong, @planetf1 , it's nice to have and addresses a concern, but it's ascertaining properties for sub-components of scripts that we don't even have tests for.....)

I guess this argues for closure on https://github.com/open-quantum-safe/tsc/issues/1 before all else, @dstebila.

[planetf1]

We use python dependencies in several areas during a pr & merge build. Specifically in both the unix (macos/linux) and windows builds:

1. copy_from_upstream - this script uses python to copy some implementations across from pqclean etc.
2. for macOS/windows only, prior to running tests (see below)

So the code is used then - but I think the important point is why anything would change ie break it. As I see it this would be

• where a developer needs additional dependencies in these places (or others). They would need to add the right sha, and may need to recalculate the dependency chain. Docs should cover this
• where a particular dependency gets removed, perhaps replaced. I think this is unlikely as usually libraries just add new versions (which we'd only use when we choose to)
• where we decide to review/update our dependencies. either manually, or using a tool like dependabot - but it should just be part of the process, so answered by documentation

For the docs, making sure the developer knows where to look is important - I suggested above adding a pointer within the actual requirements.txt. Could also add a link within the relevant script, or even print output into the build logs if that would be useful?

There are lots of steps to move to a trusted production environment - this is just a very small step, and I completely agree those other issues need reviewing too, as does the decision around prod vs research. We can't do it all at once. I hope that this change also gives more confidence to other projects looking at oqs (and other pqca projects) as a source, that we have a good handle on supply chain security (I know there's many other parts, a lot already done, and this checklist is simplistic) - reputation.

We then have the pinning of github actions

We wouldn't expect versions to be withdrawn, so this is more a maint task when we want to upgrade our build action versions.

OSSF themselves say:

_Pinned dependencies reduce several security risks:

They ensure that checking and deployment are all done with the same software, reducing deployment risks, simplifying debugging, and enabling reproducibility.

They can help mitigate compromised dependencies from undermining the security of the project (in the case where you've evaluated the pinned dependency, you are confident it's not compromised, and a later version is released that is compromised).

They are one way to counter dependency confusion (aka substitution) attacks, in which an application uses multiple feeds to acquire software packages (a "hybrid configuration"), and attackers fool the user into using a malicious package via a feed that was not expected for that package.

However, pinning dependencies can inhibit software updates, either because of a security vulnerability or because the pinned version is compromised. Mitigate this risk by:

using automated tools to notify applications when their dependencies are outdated; quickly updating applications that do pin dependencies._

The last section is relevant -- A follow-on action would ideally be to look at automatic dependency updates/recommendations. For example dependabot manages this well for github actions . It should also manage python. I'll add:

• [x] open issue on dependency management tools

Finally - I note the linux build uses the openquantumsafe/ci-debian-buster-amd64 image - where does this get created?

[planetf1]

Added

• [x] open issue on ci image

Perhaps we could build within our repos? Or maybe use a standard image? (I don't know enough about it to comment)

[baentsch]

Perhaps we could build within our repos?

We do: https://github.com/open-quantum-safe/ci-containers

Or maybe use a standard image?

That would most likely run counter to https://github.com/open-quantum-safe/tsc/issues/5

[planetf1]

Added explicit python setup using 'setup-python' github action where used in workflow.

In testing this fix, I hit the doxygen deprecation issue which has been addressed in https://github.com/open-quantum-safe/liboqs/pull/1775/files

Will redo these changes back once that PR has been merged

[ryjones]

interesting

[baentsch]

interesting

Thanks for this easy-to-digest pointer! Indeed, finally a reference maybe now triggering interest/motivation to work on #1215 and #1366. What is a bit surprising is the lack of a constant (run-)time checkbox in this list, though. The remaining checkboxes seem to be fixable by re-configuring the github setup, right?

[planetf1]

interesting

That scorecard was NOT generated by the definitions here. For example ours is still running the v4 scorecard, whilst that is for v5 (prerelease) where they have added more checks.

I suspect (but can't be sure) this was run by the openssf team directly, perhaps as a precursor to the PQCA tac meeting. Not something I was aware of. I know they do proactively run on 'top' projects, but when I started these changes we were not included. Note that much of this PR is the mitigation not the actual scheduling of the check.

There's now a few things to think of:

• We may wish to merge the changes to date so that we are in control of the runs & any reviewers can see gradually improving metrics (including some of the fixes here). Outstanding changes (v5 additions, docs) can be added through a further PR
• We should probably move to the v5 scorecard as soon as it is GA
• There will be more potential aspects to check - there won't be quick fixes for all of these, and even scorecard itself doesn't get 10/10

[planetf1]

interesting

Thanks for this easy-to-digest pointer! Indeed, finally a reference maybe now triggering interest/motivation to work on #1215 and #1366. What is a bit surprising is the lack of a constant (run-)time checkbox in this list, though. The remaining checkboxes seem to be fixable by re-configuring the github setup, right?

Some, not all. Varying amounts of work. This PR already has fixes to some (pinned dependencies, token permissions) - albeit not verified with v5.

Others may require more work -- for example CII best Practices is a significant effort in it's own right

[ryjones]

yes, OpenSSF scans the top million repos automatically, which is why it's there.

[planetf1]

Added PROCEDURES.md & a template to document & make it easier to maintain pinned versions

[planetf1]

Hopefully I've addressed the review issues. No findings are reported. I propose

• [ ] This PR is merged (once final review is completed). This will continue running the scorecard check within the PR and not publish.
• [ ] A new PR will be opened to move the execution to a scheduled check as per openSSF recommendations,
• [ ] A new PR will be opened to migrate from scorecard v4 to scorecard v5
• [ ] Further PRs will be opened if/when any fixes are needed.

Trying to maintain a PR with build changes (albeit minor) is a little tricky in merging as there is other activity in this area. I hope the changes are sufficient know to close this one out. In addition proactive scans are already being done by the openssf team, and getting our mitigations in place will improve our already public score.

[ryjones]

@planetf1 could you do a squash commit or similar?